Implement Symbolic Macros #19922

brandjon · 2023-10-23T15:19:44Z

This issue tracks the implementation of the Symbolic Macros feature (#19921).

Currently we have an unsubmitted, rough, partial prototype. When the code matures we'll begin submitting it, tagging this issue.

Targeting EoQ4 for the prototype.

A Metadata consists only of information that can be determined by the end of evaluating a BUILD file, but not any symbolic macros it may contain. The idea is to use Metadata objects wherever the full Package is not required -- basically anywhere we're not iterating over the targets. This can enable lazy symbolic macro evaluation, so that we don't run macros for targets that are not requested. Work toward #19922. PiperOrigin-RevId: 591271790 Change-Id: I28916eef9cef66fb4ea52ef985963e233044b9cd

- Make initialization of important fields a little more uniform, by having the builder construct a Metadata object to pass to the Package() constructor. - Delete unnecessary repositoryMapping builder field, in favor of the Metadata field. Move its initialization to the Builder() constructor. - Fix theoretical bug in use of ruleLabels cache in beforeBuild(). - Deletion of mainRepositoryMapping param will come in a follow-up that changes the Builder() constructor signature. - Update javadocs and add TODOs. In particular, remove outdated verbiage on the steps of Package initialization. - Reorder Metadata fields and accessors to fix grouping (buildFile and sourceRoot are set after BUILD eval completes). - Add note in LabelConverter's javadoc that it's not thread-safe. This came up when deciding whether or not the converter field should be moved from the package builder to the Metadata. Work toward #19922. PiperOrigin-RevId: 592317902 Change-Id: Ic87a8504442c3eea1c0fcf246f8413777ae0f2c9

Each of the three affected fields is initialized when the builder is constructed, so they may as well go alongside the other fields that are determined prior to BUILD evaluation. The affected fields are also conceivably useful to symbolic macros as well, so they probably belong on Metadata for that reason too. Work toward #19922. PiperOrigin-RevId: 592602808 Change-Id: Ia171bb67ff6d386cc9687ad46e05e8c6aeade1f8

The two Package.Builder factory methods take a StarlarkSemantics just to extract a single boolean flag. This CL has the callers do it instead, to make the Package.Builder API more about individual features than Starlark environments. By contrast, the wrapper methods in PackageFactory are left unchanged, because PackageFactory is already knee-deep in Starlark logic thanks to executeBuildFile[Impl](). Minor change spun out of unknown commit. Work toward #19922. PiperOrigin-RevId: 594115666 Change-Id: I7af4388f35b6393a3d8debd829452fe5fd633d54

This is to prepare for eliminating the PackageContext class itself, in favor of Package.Builder. PackageContext's getLabel() and setMakeVariable() accessors are replaced by the corresponding methods of the underlying Builder. The globber and eventHandler fields are migrated to the Builder as well. Since we don't have a PackageContext in all circumstances (e.g., when a Package.Builder is instantiated for bzlmod logic or unit tests), these fields are @nullable, and the caller must beware. The eventHandler field in particular is just a direct migration from PackageContext; a follow-up CL will further refactor it so that eventHandler is not set by the caller but rather owned outright by the builder, and can never be null. Minor notes: - Since globber is now stored on the builder, it no longer needs to be threaded as a separate arg to executeBuildFile[Impl](). - Callers who would've read the eventHandler off the PackageContext now retrieve it from the Builder, so we now need to assign the eventHandler to the Builder via the new setLocalEventHandler() in PackageFactory and WorkspaceFactory. Again, this is temporary. Continued from unknown commit. Work toward #19922. PiperOrigin-RevId: 594123106 Change-Id: I2bd898c57755a4d7bd687aff526305168478ce94

Its fields were already merged into Package.Builder in cc9b647. This CL just deletes the class and makes Package.Builder get stored directly as a threadlocal in the StarlarkThread. Continued from unknown commit. Work toward #19922. PiperOrigin-RevId: 594142401 Change-Id: I235c2ccbdcaad6de478776e528ab42f3827a34f9

Previously there were three mechanisms for reporting events during package building: 1) a list of events/posts on the Package.Builder, which get accumulated until the package is constructed, at which they're replayed on the skyframe environment's eventHandler 2) a separate StoredEventHandler managed by PackageFactory, used for the odd message here and there, but ultimately merged into the Builder's events/posts 3) the StarlarkThread's print() handler, which points to the handler in (2) unknown commit moved the handler in (2) from PackageContext to a field of Package.Builder, but left the initialization and merging of this handler in PackageFactory#executeBuildFileImpl(). This CL closes the loop by making Package.Builder responsible for initializing this handler, and uses it to replace the separate events/posts list. Package.java: - events/posts fields on Builder go away, localEventHandler becomes non-nullable. addEvent[s]/addPosts/getEvents/getPosts accessors also go away, there were very few uses and they can all be replaced using getLocalEventHandler(). PackageFactory.java / WorkspaceFactory.java: - no need to create a StoredEventHandler and associate it with the Builder, or merge it into events/posts when done Event handling machinery: - add replayPostsOn(), for symmetry with replayEventsOn() This CL is *probably* a no-op, but there may be slight differences in error reporting at the margins, e.g. whether a certain type of error triggers Builder.setContainsErrors(). Work toward #19922. PiperOrigin-RevId: 595397832 Change-Id: I09d29dcc58870581a59a8275eff7533fa739be42

This makes the generatorMap, configSettingVisibilityPolicy, and globber into Builder() constructor args, instead of setter mutations. These kinds of settings are always known before BUILD file evaluation begins, so it's best to bake this guarantee into the builder's API. This makes it slightly easier to follow the rest of the builder's logic. setFilename() will be done in a follow-up. We may or may not push setLoads() up in this manner. This does make the builder's constructor signature longer, but I think that's preferable to mixing initial setup mutations with actual package construction mutations. One way to address this might be to have the caller supply the Package.Metadata object to the builder constructor, but first we'd have to migrate off Metadata the fields that are determined through BUILD file evaluation. Work toward #19922. PiperOrigin-RevId: 595407792 Change-Id: I5ed2b07b631fdbbf393a2095acd342a6e53d3c25

Follow-up to unknown commit. The initialization of the metadata fields is moved to the top of the builder constructor. The creation of the InputFile corresponding to the BUILD file is moved to the bottom of the builder constructor. Had to replace a use of Builder#createLabel with Label#create to avoid a circular init problem where pkg was not yet initialized. Work toward #19922. PiperOrigin-RevId: 595426821 Change-Id: I1e48dae33231d6ca3eb3feebbf9e202a1d1b2dec

This is made possible by the change in d8d9078 to have Package.Builder own its own eventHandler. Also delete an unused mainRepositoryMapping param of Package.Builder's constructor, which was made obsolete by ef98ef9. RuleClass.java - createRule(): delete eventHandler param, always use the Builder's eventHandler - createRuleUnchecked(): possibly write events to the Builder's eventHandler that were previously discarded -- maybe not a no-op, though since this is an internal method hopefully it's not being used in ways where this would matter. - update various signatures to not take the separate handler Rule.java - Narrow arg type from Builder to PackageIdentifier BzlmodRepoRuleCreator.java - event handler might now get events from Builder.build(), not just from createAndAddRule(). I'm not expecting this to matter. WorkspaceFactoryHelper.java - addBindRule() no longer discards events Work toward #19922. PiperOrigin-RevId: 595462762 Change-Id: I5bd3209aaeb3030a6e8d18d193294e7d3c356b23

outputFilePrefixes is a map used to help check whether one OutputFile's name clashes with a directory prefix of another. Since it's updated with info from every OutputFile of a rule passed to checkForConflicts(), there's no need to also update it in addRuleUnchecked(). This is a no-op assuming that no user of Package.Builder calls both addRule() and addRuleUnchecked() -- which is currently the case, and ought to be enforced, perhaps in a follow-up CL. (Note that the alternative approach, of deleting the map update in checkForConflicts() while leaving it intact in addRuleUnchecked(), is not only less readable, but also incorrect since it doesn't catch conflicts between OutputFiles of the same generating rule.) Work toward #19922. PiperOrigin-RevId: 595683434 Change-Id: I8b660807edfa1eef5101fe362ddaed4352e3f684

Previously, it was possible in theory, but not in practice, to call both addRule() and addRuleUnchecked() on the same Package.Builder. This CL removes that possibility, and in doing so makes it easier to reason about whether the ruleLabels map will be null. It also makes the outputFilePrefixes map nullable along the same lines as ruleLabels. Both maps are only used for validation, and so are irrelevant to Builders that use addRuleUnchecked. For completeness/correctness, replaceTarget is also updated to mention its interaction with this invariant. Work toward #19922. PiperOrigin-RevId: 595696336 Change-Id: I3965033784025e3dd6cfc85ec0b591a21c24de8c

This change is a no-op. - The idiom of checking the `targets` map for a newly added target and failing if it finds one, is factored into a new helper checkNameConflict(). - Updated javadoc to checkForConflicts(). Ensured that all existing checks are mentioned, and deleted reference to two conditions that don't appear to actually be checked: "no rule with errors is inserted into the package" -- we seem to do the opposite and propagate error bits from rules to the package; and "the generating rule of every output must itself be in the package" -- perhaps this is enforced elsewhere, but it's not part of this method. - Add TODO of me pondering whether we care if an output file prefix clashes with a non-output-file target. - Merge duplicateOutputFile() into nameConflict(). (This doesn't change the error message because nameConflict() was never called with an OutputFile as its first arg.) - Add TODO identifying that we don't actually verify that an output doesn't clash with its own generating rule, even though we check it against other previously introduced rules. - Rename conflictingOutputFile() -> overlappingOutputFilePrefixes() to distinguish from the other kinds of conflicts we're talking about. - Inline checkForInputOutputConflicts() and inputOutputNameConflict(), which each only had one call site and are more readable as part of checkForConflicts(). - Use String.format() more. Work toward #19922. PiperOrigin-RevId: 596031120 Change-Id: I02ed6bb4d0f4c1f5aa2bfa16c91835d92e4cef39

This makes the affected error message more symmetric with other related errors that do not include the package name. The package name should be added somewhere higher up in the handling logic anyway. Work toward #19922. (Drive-by fix) PiperOrigin-RevId: 596041531 Change-Id: Ibfc3731f941de3a5020d751d46ac33f77e7d2019

This alters an error message that used some obsolete verbiage (it said "generated label" when really it could be any non-input-file target). - Eliminate GeneratedLabelConflict subclass. It's unnecessary to distinguish the case of input file conflicts from other types of name conflicts. - Clarify comment about nameConflictCheckingPolicy. - Remove addInputFile() overload that did its own creation of the InputFile. This makes custody of the input file object more explicit and allows it to be passed to helper functions like nameConflict(). - In createInputFile(), reorder clauses for readability and reuse of InputFile object across two cases. Work toward #19922. PiperOrigin-RevId: 596059017 Change-Id: Ie7acc7ba7592875c8726271b01c81f4a56c42a2d

The ability to define symbolic macros in the build is guarded by --experimental_enable_first_class_macros. (For the moment, we're using "first class macros" in user documentation and "symbolic macros" or sometimes just "macros" in internal code.) Symbolic macro types follow an analogous pattern to rules: - A MacroFunction (StarlarkRuleFunction) is the Starlark callable object, returned by `macro()`, that can be invoked during package loading to instantiate the symbolic macro. MacroFunctions accept the name of the instance (like a target name) and return `None`. - A MacroClass (RuleClass) represents the information needed to actually instantiate the macro, such as the name the macro was exported with, and its implementation function. - A MacroInstance (Rule) represents the result of calling the macro during package loading, and is the object tracked by Package.Builder. Note that MacroFunction references MacroInstance references MacroClass. Package / Package.Builder gain new fields and methods to track macro instances created during BUILD evaluation. A follow-up CL will actually evaluate macro implementation functions. A different follow-up will add name conflict checking so that macro instance names are guaranteed to not clash with targets in the package. StarlarkRuleClassFunctions: - Update javadoc on StarlarkRuleFunction; add @nullable, field comments, and other comments Work toward #19922. PiperOrigin-RevId: 597571980 Change-Id: I1789d94e02c45e23b25c80990fb6ccef82e13da9

…in a package The maps of targets and macro instances are tracked separately in the Package.Builder, but we need to enforce that their keys are disjoint (since macro names will be used as the basis for deciding which macro to expand to obtain the target for a given label.) This CL accomplishes this by updating the name conflict utility methods to conveniently check both at the same time. Package.java: - createInputFile(): Rephrase to use checkNameConflict (now checkForExistingName()). This improves readability and avoids adding new complexity to this method, at the expense of an extra redundant lookup in the targets map. - beforeBuild(): Don't implicitly create an input file if it is the name of a macro. - checkForConflicts(): Rename checkRuleAndOutputs(). Construct a bespoke (and more streamlined) exception message, so we can eliminate the nameConflict() helper. - checkNameConflict(): Rename checkForExistingName(). Split into overloads for a Target and MacroInstance arg, both of which dispatch to unified logic for checking the targets/macros maps and constructing the appropriate error message. This absorbs the nameConflict() helper. OutputFileTest: - Update test case for new error message. PackageFactoryTest: - Test cases for conflicts between macros and each of: rule targets, generated outputs, environment groups, package groups, explicitly declared input files (exports_files), and other macros, all in both declaration orders (to exercise different code paths). - Check that macros *can* conflict with output prefixes, because that's a behavior we have for targets, whether or not it's a good one (I honestly don't understand it). Didn't bother doing both orders here. - Check that macros prevent implicit creation of input files by the same name. Work toward #19922. PiperOrigin-RevId: 597836620 Change-Id: Iee110b4db0dfcd09b72d43e6c83f435003a45e16

09eef85 added the ability to declare symbolic macros in packages, but did not run their implementation functions. This CL adds that feature. For now, implementation functions run synchronously with the call that instantiates the macro. The implementation runs in its own Starlark thread, but it has the side effect of mutating the original Package.Builder, so the targets that it declares are visible to native.existing_rules() in legacy macros. In the future we may make symbolic macro evaluation lazy, in which case the implementation function would run sometime after BUILD evaluation completes, and macro-generated targets would be invisible to native.existing_rules(). MacroClass.java: - add static helper executeMacroImplementation(). This is analogous to PackageFactory#executeBuildFileImpl(), but I prefer to not weigh that file down with additional code. SymbolicMacroTest.java: - tests for signature of implementation function - test for macro-generated targets. Note that the "my_macro$my_target" naming convention isn't enforced yet. - test for macro composability - test for calling existing_rules and glob in a symbolic macro. These will be inverted in a later CL when we ban them, by supplying a different class than Package.Builder to executeMacroImplementation(). Work toward #19922. PiperOrigin-RevId: 601848482 Change-Id: Ic70fdad9e7d8323c76a3f0125c518547adeee96f

Previously, when evaluating a BUILD file we would store three types of thread-locals in the StarlarkThread: 1) a BazelStarlarkContext to hold a SymbolGenerator (for reference equality semantics) and a Phase enum (should ideally be replaced by helpers like fromOrFail()); 2) a Package.Builder (or, prior to 29318bb, a PackageContext) to support defining targets and things like native.glob(); and 3) a RuleDefinitionEnvironment to support the analysis_test() feature. This CL merges (1) and (2) by having Package.Builder also be a BazelStarlarkContext. This simplifies the setup of a StarlarkThread for evaluating BUILD files and symbolic macros, and paves the way for distinguishing symbolic macro construction with its own separate builder type in a later CL. In Package.java: - Add Phase and SymbolGenerator parameters to the Builder constructor, to pass along to super(). Hide this complexity from callers by making the Builder constructor private and adding Package.newPackageBuilder(), complementing the two existing builder factory methods in Package. (This results in a lint for adding a 13-parameter method, but I'm going with the lesser of evils here. What does the lint want me to do, make a builder for the builder?) - The Phase is always LOADING and the SymbolGenerator is based on the package id, except for in newExternalPackageBuilder() where it's WORKSPACE and the SymbolGenerator uses the skykey to distinguish evaluations of different chunks of the same WORKSPACE file. This is all just movement of existing caller logic into Package.java. newExternalPackageBuilder() now takes in the whole skykey instead of just a path. - Add fromOrNull() and fromOrFail() helpers, which is more idiomatic for BazelStarlarkContext subclasses. Callers now use these in place of calling getThreadLocal() directly. Callers use storeInThread() instead of calling setThreadLocal() directly. In WorkspaceFactory, the symbol generator construction is logically moved from execute() to when the builder is first constructed in WorkspaceFileFunction. So execute() loses the key as a parameter. Some tests that call newExternalPackageBuilder() directly now pass in a key there instead of where execute() is called. Work toward #19922. PiperOrigin-RevId: 602187597 Change-Id: Id650be08f82c0b0e3baf89e8b89db7bd1f401f79

This adds an intermediate class between BazelStarlarkContext and Package.Builder, to prepare the way for a separate builder type for evaluating symbolic macros. A TargetDefinitionContext is anything that can consume the side-effect of defining a target, i.e. either a BUILD file's top-level logic, or a symbolic macro. NameConflictException is migrated from Package.Builder to TargetDefinitionContext, since in principle it can happen during either BUILD file or symbolic macro evaluation. Work toward #19922. PiperOrigin-RevId: 602445896 Change-Id: I6ea14a118480418ae1da3c69973a508261c119f2

The test cases in this file are updated to use the text block feature available now that we've upgraded to JDK 21. - Removed the empty "//" comments that were used as hints to the autoformatter to not mush all string arguments onto the same line. - Replaced single quotes with double, since that's the preferred style in actual Starlark code. Single quoting was only used for readability to avoid escaping, which is no longer necessary. In general, we shouldn't blindly migrate all tests to text blocks, since that obscures blame history. But in this case, the history of this file is relatively short and we're actively working on adding new test cases, so it seemed like a good tradeoff. Work toward #19922. PiperOrigin-RevId: 617153701 Change-Id: I1ecc70ecc47deb2386b2faf0ffd6d02621087e2f

This is a follow-up to 25f0365, for symbolic macro tests in StarlarkRuleClassFunctionsTest.java. Work toward #19922. PiperOrigin-RevId: 619567559 Change-Id: Ifaab484fae81076324ff8b4bad8ca7538d7f66ad

Update a few tests to stick in a dummy name argument if one is not present. This only applies to low-level tests of RuleClass, since other tests go through machinery that already validates the presence of `name`. Work toward #19922. PiperOrigin-RevId: 620267659 Change-Id: Ie77db99ac4c4f4234ca298654656716ff4d8546d

This CL follows 32def70 by removing Attribute.ValidityPredicate, along with its sole non-test (trivial) implementation, ANY_EDGE. It looks like Attribute.ValidityPredicate was added ten years ago to replace an earlier mechanism on RuleClass. At that time it was only used to enforce android-related constraints on srcs/deps attributes in the native java rules. Opportunistic cleanup discovered during work on #19922. PiperOrigin-RevId: 625736563 Change-Id: I1355785546549613f3982b760ca0638b0a76267a

This adds an `attrs` param to the `macro()` callable, allowing users to specify an attribute schema for symbolic macros. As for rules, the `name` attribute is implied and should not be included in `attrs`, while certain other names are reserved and can never be defined on macros. Macros support default values and implicit defaults, but not computed defaults or late-bound defaults. StarlarkRuleClassFunctions.java - Add attr schema validation to `macro()`. - Factor instantiation logic from `MacroFunction#call()` to `MacroClass#instantiateAndAddMacro()`. - Replace ad hoc `name` attr validation with standard RuleClass#NAME_ATTRIBUTE logic. BuildType.java - In `copyAndLiftStarlarkValue`, generalize `ruleClass` param to apply to macros as well, and update stringification in `AttributeConversionContext` accordingly. MacroClass.java - Add significant new logic to instantiate a macro by processing and validating its attribute values. BazelEvaluationTestCase.java - Add ability to inject config fragments into the Starlark environment. Used by one new test case that can't take advantage of `BuildViewTestCase`'s existing fragment setup. New test cases are added to StarlarkRuleClassFunctionsTest.java and SymbolicMacroTest.java, with the loose rationale that the former is for cases that don't require an implementation function to run and the latter is for everything else. But it may be more sensible to just move all the symbolic macro tests to the latter file in the future. Work toward #19922. PiperOrigin-RevId: 626042528 Change-Id: Ie1c09cfdf2ca2168467035b2fa0ccd75cbf68dfd

Previously, we had symbolic macros and targets occupy the same namespace in a package, and reported an error whenever there was a conflict between a macro and a target. This failed to consider that macros frequently (and as per style guidance, *should*) define a "main target", i.e. a target whose name is the same as the string `name` arg passed to the macro's instantiation. This CL splits the macro and target namespaces. The relevant test cases are also inverted. Note that the existence of a macro named "foo" will still prevent the implicit creation of an input file target named "foo". This is because we plan on allowing macro labels to be passed as inputs to other macros for the purpose of an undeclared inputs check, and we don't want to implicitly conflate this usage with an unrelated input file (see comment in beforeBuild()). Also update relevant test cases to use text block syntax because yay for text blocks. Work toward #19922. PiperOrigin-RevId: 626104981 Change-Id: Id65afaadb59dc49d88621f9a9abfd8029f12f557

This codifies the previous CLs' efforts to make `Package.Metadata` immutable. It also saves a few LOC. - Accessors on `Metadata` are deleted, in favor of the implicit ones generated by `record`. Updated usages to prefer these accessors over direct access to the fields. `Metadata#getPackageDirectory()` remains as an explicit accessor since it's not materialized as a field. - Add `@Nullable` for `configSettingVisibilityPolicy`. - Rename `Metadata#filename` -> `Metadata#buildFilename` for clarity Work toward #19922. PiperOrigin-RevId: 676389043 Change-Id: Id40dab239d160681be69b48d111ac8c32b475163

This field is initialized in `finishInit()`, but doesn't depend on anything discovered during BUILD file evaluation. So we may as well compute it during the constructor call and remove some complexity from this class. This CL should not cause a behavioral change. Structural changes: - `sourceRoot` is now final. - The existing method `computeSourceRoot()` is inlined and eliminated. A new static method by the same name is factored out of the relevant part of `finishInit()`. The effect is that the method boundary now encapsulates the actual job of determining the source root, whereas previously this work both spanned multiple methods and intermingled with unrelated tasks. - Validation of the consistency between `buildFilename` and `isRepoRulePackage` is moved from `computeSourceRoot()` down into `Metadata`'s constructor. `isWorkspaceFile()` and `isModuleDotBazelFile()` are inlined into their sole call sites. Changes to the body (hope this helps for reading the diff): - The `baseName` local var is no longer needed. - The trivial true branch of the outer `if` statement is turned into an early exit at the top of the method. - Rewrote the early `return`s from the inlined method as `if`-`else`s that initialize a local var `sourceRoot`. - The `throw` IAE statement is turned into a `Preconditions.checkArgument()`. - Renamed local var `packageDirectory` -> `pkgDirFragment`, factored some other local vars up top. - Added TODO about simplifying `current`'s initialization (didn't want to risk it in this CL). Some test cases were previously creating `Package.Builder`s with bad BUILD filenames but not actually constructing the final package. This was allowed because the validation was deferred until `finishInit()`, which didn't run. Fixed those test cases to pass, now that the validation of BUILD filenames happens up front. Work toward #19922. Drive-by simplification while doing other refactoring of `Package`'s fields. PiperOrigin-RevId: 676390008 Change-Id: I1c928924adea5c5caaa6d056de50cb98ce17b79a

This is a little more consistent and makes it easier to factor metadata-related accessors in a follow-up. Added a `Metadata#getName` convenience accessor, like `Package#getName`. Added a `Metadata` field to `Package.Builder`. Work toward #19922. PiperOrigin-RevId: 676525169 Change-Id: I9f9451695ed0a6e621d6288b25932412e48a538c

Now the three builder-factor methods construct the `Metadata` and pass it as a constructor arg. This simplifies the constructor signature and helps with passing metadata to a super() constructor in a follow-up CL. The `isRepoRulePackage` calculation is only done in one of the constructors, and there it's only needed to work around a hack in package deserialization that I don't want to address right now (and which may be obsolete after WORKSPACE logic is deleted). Work toward #19922. PiperOrigin-RevId: 676530459 Change-Id: Ieaabeb412925886a0452447f186a9bb15a3a21c5

The self-return isn't used anywhere. Anyway, that idiom is more appropriate for true builders where the information is known up-front; here it works more as a stateful mutation. Using void simplifies pulling these methods up to a base class in a follow-up CL. Work toward #19922. PiperOrigin-RevId: 676531919 Change-Id: Id13f600a167ac13183129a7e87bff97e519adf97

`Package.java` is too large and its builder class tries to do too many things. This and the following CLs are an effort to reduce its LOC and better separate concerns. The main strategy for accomplishing this is to move out the logic for registering targets/macros, validating their names, and indexing them, into a separate dedicated container class. Future work could look at minimizing `Package.Builder` down to just the things needed to construct a `Package` in the most straightforward context, deserialization. All the stuff that's needed to evaluate BUILD Starlark threads doesn't have to be there -- for instance, `glob()` and `generator_name` machinery. This CL is focused on just branching `Package.Builder` into a new superclass, `TargetRegistrationEnvironment`. To keep the diff simple I avoided other refactoring in this CL, such as reordering members and restricting field access by adding new getters. The net result is about 600 LOC moved out of a 2800 line file. In Package.java: - Inlined static methods `getTargets(BiMap) and `getTargets(Target, Class)`. - Moved the map fields that track registration of targets and macros. Also moved `currentMacroFrame`, `nameConflictCheckingPolicy`, the `ruleLabels` cache, and `containsErrors`. I did *not* move `unexpandedMacros` and `rulesSnapshotViewForFinalizers` because those are part of the evaluation model for symbolic macros, and aren't needed for their registration / conflict checking. - Moved corresponding accessors/setters for these fields. - The new base class doesn't track `pkg`, so it can't do the precondition checks that the targets it's manipulating belong to the package being constructed. For now, I kept these checks by overriding the applicable methods in the Builder class and dispatching to `super`. If these checks are worthwhile, then the better solution is to add a `pkg` field to `TargetRegistrationEnvironment`. - Methods that *create* targets (rather than merely adding them) are *not* pushed up to `TargetRegistrationEnvironment`. In TargetRegistrationEnvironment: - Fields access broadened (for now) from `private` to `protected`. (Technically, package-private would've also worked, but `protected` is a clear signal that it's intended for use in a subclass.) Same for a few methods. - Update javadoc/comment in `setContainsErrors()` to not mention `addEvent[s]()`, which hasn't existed on `Package.Builder` since d8d9078. - `disableNameConflictChecking()` now returns void. Work toward #19922. PiperOrigin-RevId: 678399563 Change-Id: I32cfd2c1d1aab142f271235abd59fd9d46cabf9d

The builder itself shouldn't be retained after package construction, so there shouldn't be a need to nullify its fields. Running the benchmark script shows this CL has no effect on memory. The line wrapping `targets` in an unmodifiable map dates back to unknown commit, and at that point was needed because Package didn't copy the map itself in `finishInit()` but just pointed to the builder's map. Eliminating these nullifications makes it easier to make these fields `private` in `TargetRegistrationEnvironment`. Work toward #19922. PiperOrigin-RevId: 678401129 Change-Id: Ica887b69fb795aaebf00d86e666b2817aa0ef659

This makes it unnecessary for `Package.Builder` to directly call `TargetRegistrationEnvironment#checkTargetName`, so it can be private. In TargetRegistrationEnvironment.java: - Don't recommend `addOrReplaceTargets()` as the sole canonical way to update the targets map. - Rename `addOrReplaceTargets()` to `putTargetInternal()` and make it private. - Replace `addInputFile()` with a more general `addTarget()`. For non-rule target's, it's just a strict version of `putTargetInternal()` that does `checkTargetName()`. For rule targets, it's equivalent to `addRule()`, which does all the extra processing associated with adding the rule's outputs. (The long-term view is that the client shouldn't really have to care so much what the type of target they're adding is, so `addTarget()` might be preferred over other mutators. This is complicated by package deserialization, which wants an optimized code path.) - Add `addInputFileUnchecked()` to add an input file specifically, asserting that there's no prior target of the same name, but without the possibility of throwing `NameConflictException`. - Similarly, add `replaceInputFileUnchecked()` for the one specific case where it's needed. - Drive-by lint fix for two `currentlyIn...()` methods. In Package.java: Replace usages with new methods as appropriate. Work toward #19922. PiperOrigin-RevId: 678407422 Change-Id: I20b774faf9808f15d34ca44e0ba2253bf9ac2df2

- All fields are now private. - `targets` and `macros` are renamed `targetMap` and `macroMap` for clarity/symmetry with accessors. - New accessors for private fields, either returning the whole map or just a contains boolean, as needed. (The accessors are protected rather than public because we're using inheritance rather than composition in `Package.Builder`.) - Drive-by: made `nameIsWithinMacroNamespace()` static. - `addRuleInternal()` is made private. Its override in the builder for precondition enforcement is replaced by overrides of `addRule()` and `addRuleUnchecked()`. - Added `unwrapSnapshottableBiMap()`, which seems like a bad abstraction but is still better than allowing direct field access. Work toward #19922. PiperOrigin-RevId: 678408757 Change-Id: Ic233754e7b0e6866d1211ccfe4c362046c40321e

The only thing in TargetDefinitionContext is NameConflictException and its subclass. Moving it into TargetRegistrationEnvironment matches how the name conflict checking is now performed there anyway. A subsequent CL may reintroduce TargetDefinitionContext as a branched file from Package.java, factoring with it a number of methods and fields from Package.Builder. Work toward #19922. PiperOrigin-RevId: 678410292 Change-Id: I8e21076a5736a10d8d7053bf39e650e5c7b86a6e

…onEnvironment This gives the Builder more explicit control over its own API, at the cost of a bit of verbosity in accessing the environment as a field, and in enumerating the methods it chooses to re-export to its own clients. It also means that in the future, we can repeat the trick of factoring out a bunch of builder methods to a new base class, without having to worry about which base class supersedes the other in the hierarchy. - `Pakcage.Builder` now inherits directly from `StarlarkThreadContext`. - None of `TargetRegistrationEnvironment`'s members are protected, and only a few are package-private. The choice of which methods are public now (mostly) comes down to what API makes sense for `TargetRegistrationEnvironment`, without regard for what makes sense for `Package.Builder`'s clients. - In the builder, add `currentMacro()` convenience method. Work toward #19922. PiperOrigin-RevId: 678411561 Change-Id: I5e550fe5b582e1039fa304d42d37e882a856e742

`Package.Builder` has a utility method `copyAppendingCurrentMacroLocation`, that doesn't really need to be there. Ahead of further refactoring of this class, I figured I'd spin off this method. (The purpose of the method is to determine the munged visibility values for targets declared within symbolic macros.) The method is re-expressed as two helpers in `RuleVisibility` and one in `MacroInstance`, each of which does a narrower task more suited for its new home. Made the convenience method `Builder#currentMacro` public, since it's much terser than `getCurrentMacroFrame().macroInstance`. Added a drive-by TODO in `StarlarkNativeModule`. Work toward #19922. PiperOrigin-RevId: 678786589 Change-Id: I5cc190bd6ef908a3e67094eacd13e9c91ef43d7c

… values If the --incompatible_simplify_unconditional_selects_in_rule_attrs option is enabled, we simplify unconditional selects (in other words, selects with only the //conditions:default branch), as well as concatenations of such selects, into a non-select value. This non-select value will be reflected in various introspection results, such as query output and native.existing_rules() attribute values. The motivation for this change is symbolic macros (#19922), which promote non-select values of configurable macro attributes to unconditional selects. If these unconditional selects propagate to saved rule attribute values, they have the potential to make native.existing_rules() useless since we do not (yet) have a build language API for introspecting selects. RELNOTES: Add the --incompatible_simplify_unconditional_selects_in_rule_attrs option to simplify configurable rule attributes which contain only unconditional selects; for example, if ["a"] + select("//conditions:default", ["b"]) is assigned to a rule attribute, it is stored as ["a", "b"]. PiperOrigin-RevId: 678803898 Change-Id: I989b6a2f0cc297b8740d6854e676210ee35f37d1

And rename Package.Builder#regEnv -> Package.Builder#recorder. See discussion in 11c090f for (some of) the bikeshedding background. Work toward #19922. PiperOrigin-RevId: 679329986 Change-Id: Idcf5c6f24ab243d496b5ea826d1f00806877898e

Work toward #19922. PiperOrigin-RevId: 679364770 Change-Id: I486c4147fb0234b4d78f8ed2ab3f71dca5d5e118

This has a blast radius extending to all symbolic macros (i.e. all our symbolic macro unit tests). Also move the addition of the builtin macro attributes (just `name` and `visibility`) to `MacroClass.Builder`, and make `MacroClass`'s constructor private for good measure. Fixed a few test cases that incorrectly defined rules with implementation function's whose signatures looked like macros, but which didn't affect the test behavior. Work toward #19922. PiperOrigin-RevId: 679954938 Change-Id: I958211c92fa2f9749cd6a7672af3936961a95285

This is useful if you have several nested symbolic macros, since it avoids generating a long visibility value like `["//A:__pkg__", "//A:__pkg__", ..., "//A:__pkg__"]` when each macro's body is defined in the same package `A`. Also clarify the purpose of the label accessors on `RuleVisibility` and flip their declaration order. Work toward #19922. PiperOrigin-RevId: 679962558 Change-Id: Iebb55d0256d72d7bc0a8b575446dea83c9bbf632

Previously we did not apply `select()` promotion if the attribute value was `None`, which occurs as the Starlark representation of the default value for certain types (most notably `attr.label()`). But this is inconsistent. The same argument for general `select()` promotion -- that you fail fast and find errors sooner -- applies here. For instance, the macro author might have written code that passes when the argument is `None` but not when it is `select("//conditions:default", None)`. Fixing this exposed a weakness in `copyAndLiftStarlarkValue()`, that it did not handle Nones correctly. (This was probably why I avoided select() promotion in this case to begin with.) Work toward #19922. PiperOrigin-RevId: 680640681 Change-Id: I6983bc3e44d3cf629c40c6014a2873b6c62e8841

This implements the behavior that the `visibility` attribute of symbolic macros is whatever the user specifies concatenated with the callsite's location. It also ensures that the package's default visibility is used for top-level symbolic macros (and only top-level ones). The bulk of the work is done in the attribute value processing logic of `MacroClass#instantiateMacro`. The new code is strategically placed before most of the other attribute processing to avoid violating invariants (like "by this point, all attr values have had `copyAndLift...()` applied to them). Note that, unlike the `visibility` attribute of rules, the `visibility` attribute of macros always materializes the true visibility, even for macros that are declared at the top level. I.e., if you already have a macro's computed visibility value, you don't need to also have the package's default visibility value. `MacroInstance` gains a `getVisibility()` accessor, the value of which it validates in its constructor. This means the constructor can now throw EvalException. In `SymbolicMacroTest`, delete unused scratch files and do minor formatting fixes for symmetry with .bzl style expectations. Work toward #19922. PiperOrigin-RevId: 680661002 Change-Id: Ibaba32f458f69d3b7ad14db0afc68aaf9468e3a7

…isibility declarations ... but simplify the resulting visibility appropriately when saving the visibility attribute: $foo plus public is public; $foo plus private is $foo; and an empty visibility list is canonicalized to ["//visibility:private"]. This makes it easier for symbolic macros to grant visibilities on targets that they declare. Work toward #19922. The canonicalization of an empty visibility list to ["//visibility:private"] is technically an incompatible change affecting query output, but is very unlikely to break anything in practice. RELNOTES: Non-singleton target visibility lists can now contain "//visibility:public" and "//visibility:private" elements; the result is appropriately simplified when assigned to an attribute: ["//foo:__subpackages__", "//visibility:public"] is saved as ["//visibility:public"], ["//foo:__subpackages__", "//visibility:private"] is saved as ["//foo:__subpackages__"], and for consistency's sake, an empty target visibility list [] is saved as ["//visibility:private"]. PiperOrigin-RevId: 680761159 Change-Id: Ic2edaf13dfe48b0f0014d404ced34aa7632d33c7

This enables `--experimental_enable_first_class_macros` and `--incompatible_simplify_unconditional_selects_in_rule_attrs` to true. The former releases Symbolic Macros, and with it, Finalizers and Macro-Aware Visibility. The latter is a quality-of-life improvement for macro users that is technically a breaking change: Any rule instantiated with an attribute value of `select({"//conditions:default": foo})` instead sees just `foo`. Main milestone of #19922. RELNOTES: Symbolic Macros -- and with them, Finalizers and the new Macro-Aware Visibility model -- are now generally available (`--experimental_enable_first_class_macros` now defaults to true). Trivial `select()` values are automatically unwrapped (`--incompatible_simplify_unconditional_selects_in_rule_attrs` now defaults to true). PiperOrigin-RevId: 680779247 Change-Id: Icdbf4b47bc893ba61417eddc22c79d85b6f5d920

This CL creates a new base class of `Package.Builder`, `TargetDefinitionContext`, to hold all the API needed during Starlark evaluation of a package. It does not include operations that are only needed during setup / tear down of the package builder. For instance, `addRule()` belongs on this base class, whereas `finishBuild()` remains on the `Builder`. Factoring in this manner makes it clearer what operations need to be supported to evaluate a piece of a package corresponding to a single symbolic macro (needed for lazy macro evaluation). In any case, anything that splits the API in a reasonably systematic way and allows us to siphon off lines of code from Package.java is a win. A previous base class by the name of `TargetDefinitionContext` was deleted in 33a2bb7. I made it a point to first remove that file before working on this CL so that we could treat the new `TargetDefinitionContext.java` as a branch of Package.java for better code review and history preservation. All the stuff that's branched over is left in the same declaration order. Other changes: - Migrated fields are made `protected` if they are still directly referenced in `Package.java`. - `setMakeVariable`, `mergePackageArgsFrom` (2 overloads), and `setWorkspaceName` are made void (avoids the `Builder` return type problem). - `getRulesSnapshotView`, `getNonFinalizerInstantiatedRule`, and `addMacro` are moved but also overridden by the builder. Work toward #19922. PiperOrigin-RevId: 681006375 Change-Id: I2df816dc35e7ceea9e8704ae9df2a2ea6daeceda

Name conflict checking is disabled for package deserialization but enabled in all other contexts where we are building a `Package` object. Previously, this disabling was done by a stateful mutation on the `Package.Builder`, with associated precondition checks to confirm that this mutation did or did not occur before calling certain methods, and that this mutation was only attempted at most once. This is a bit convoluted and is the result of my original attempt to understand `Package.Builder`'s sprawling API (now part of `TargetRecorder`). It's clear though that this can be better expressed as a constructor arg, removing the need for the extra consistency checking. Also updated some javadoc to remove mention of fields being nullified after package construction -- that was removed in 8977891. The field `macroNamespaceViolatingTargets` is also now never null -- even during package deserialization it still eventually gets a value due to the `putAll...()` method, so we may as well initialize it to a collection object. With these changes, the fields in question are now able to be marked `final`. Drive-by cleanup during #19922. PiperOrigin-RevId: 681065954 Change-Id: I060ae2fbaca9589ed95a45568127538a6aafe537

brandjon · 2024-10-02T21:59:12Z

This feature is set to be released (by flipping the --experimental_enable_first_class_macros flag) in 8.0. There are some remaining issues concerning how visibility is introspected in bazel query / native.existing_rules(), but now seems like an excellent time to close this long standing bug and open smaller scoped ones for individual issues.

fmeum · 2024-12-06T12:41:01Z

Adopting symbolic macros in with_cfg.bzl helped me find existing bugs without adding new tests. After the bugs were fixed, converting the legacy macros generated by with_cfg to symbolic macros was straightforward. Thanks for the great feature! Lazy expansion will be particularly useful for this use case as the generated macros can end up declaring a large number of targets.

brandjon · 2024-12-09T17:13:05Z

Glad to hear it! Thanks for trying it out!

brandjon added type: feature request P1 I'll work on this now. (Assignee required) team-Loading-API BUILD file and macro processing: labels, package(), visibility, glob labels Oct 23, 2023

brandjon self-assigned this Oct 23, 2023

tetromino mentioned this issue Sep 25, 2024

deprecate native.existing_rule() and native.existing_rules() in non-finalizer context #23765

Open

copybara-service bot pushed a commit that referenced this issue Sep 27, 2024

Don't propagate default_visibility to within a symbolic macro

9c7d587

Work toward #19922. PiperOrigin-RevId: 679364770 Change-Id: I486c4147fb0234b4d78f8ed2ab3f71dca5d5e118

brandjon closed this as completed Oct 2, 2024

brandjon mentioned this issue Oct 2, 2024

Implement Macro-Aware Visibility #22477

Closed

tetromino mentioned this issue Oct 2, 2024

Lazy expansion of symbolic macros #23852

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Symbolic Macros #19922

Implement Symbolic Macros #19922

brandjon commented Oct 23, 2023

brandjon commented Oct 2, 2024

fmeum commented Dec 6, 2024

brandjon commented Dec 9, 2024

Implement Symbolic Macros #19922

Implement Symbolic Macros #19922

Comments

brandjon commented Oct 23, 2023

brandjon commented Oct 2, 2024

fmeum commented Dec 6, 2024

brandjon commented Dec 9, 2024