From cda9858b6cb4160243b5ceb930f7100589426312 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 18 Apr 2024 11:51:54 -0700 Subject: [PATCH] Allow macro and target names to overlap 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 --- .../devtools/build/lib/packages/Package.java | 117 ++++++------ .../lib/packages/PackageFactoryTest.java | 173 ++++++++++-------- .../build/lib/packages/SymbolicMacroTest.java | 26 +++ 3 files changed, 175 insertions(+), 141 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java index 3c8b6cf57ec065..42a5b8f8715c6d 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Package.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java @@ -122,18 +122,19 @@ public class Package { /** * The collection of all targets defined in this package, indexed by name. * - *

Invariant: This is disjoint with the set of keys in {@link #macros}. + *

Note that a target and a macro may share the same name. */ private ImmutableSortedMap targets; /** * The collection of all symbolic macro instances defined in this package, indexed by name. * - *

Invariant: This is disjoint with the set of keys in {@link #targets}. + *

Note that a target and a macro may share the same name. */ - // TODO(#19922): We'll have to strengthen this invariant. It's not just that nothing should share - // the same name as a macro, but also that nothing should be inside a macro's namespace (meaning, - // in the current design, having the macro as a prefix) unless it was defined by that macro. + // TODO(#19922): Enforce that symbolic macros may only instantiate targets whose names are + // suffixes of the macro's name. + // TODO(#19922): Enforce that macro namespaces are "exclusive", meaning that target names may only + // suffix a macro name when the target is created (transitively) within the macro. private ImmutableSortedMap macros; public PackageArgs getPackageArgs() { @@ -1494,8 +1495,8 @@ private Iterable getRules() { * @param targetName name of the input file. This must be a valid target name as defined by * {@link com.google.devtools.build.lib.cmdline.LabelValidator#validateTargetName}. * @return the newly-created {@code InputFile}, or the old one if it already existed. - * @throws NameConflictException if the name was already taken by another target/macro that is - * not an input file + * @throws NameConflictException if the name was already taken by another target that is not an + * input file * @throws IllegalArgumentException if the name is not a valid label */ InputFile createInputFile(String targetName, Location location) throws NameConflictException { @@ -1513,7 +1514,7 @@ InputFile createInputFile(String targetName, Location location) throws NameConfl "FileTarget in package " + pkg.getName() + " has illegal name: " + targetName, e); } - checkForExistingName(inputFile); + checkForExistingTargetName(inputFile); addInputFile(inputFile); return inputFile; } @@ -1575,7 +1576,7 @@ void addPackageGroup( repoRootMeansCurrentRepo, eventHandler, location); - checkForExistingName(group); + checkForExistingTargetName(group); targets.put(group.getName(), group); if (group.containsErrors()) { @@ -1625,7 +1626,7 @@ void addEnvironmentGroup( EnvironmentGroup group = new EnvironmentGroup(createLabel(name), pkg, environments, defaults, location); - checkForExistingName(group); + checkForExistingTargetName(group); targets.put(group.getName(), group); // Invariant: once group is inserted into targets, it must also: @@ -1725,7 +1726,7 @@ void addRule(Rule rule) throws NameConflictException { /** Adds a symbolic macro instance to the package. */ public void addMacro(MacroInstance macro) throws NameConflictException { - checkForExistingName(macro); + checkForExistingMacroName(macro); macros.put(macro.getName(), macro); } @@ -1790,6 +1791,13 @@ private Builder beforeBuild(boolean discoverAssumedInputFiles) throws NoSuchPack for (Label label : labels) { if (label.getPackageIdentifier().equals(pkg.getPackageIdentifier()) && !targets.containsKey(label.getName()) + // The existence of a macro by the same name blocks implicit creation of an input + // file. This is because we plan on allowing macros to be passed as inputs to other + // macros, and don't want this usage to be implicitly conflated with an unrelated + // input file by the same name (e.g., if the macro's label makes its way into a + // target definition by mistake, we want that to be treated as an unknown target + // rather than a missing input file). + // TODO(#19922): Update this comment when said behavior is implemented. && !macros.containsKey(label.getName()) && !newInputFiles.containsKey(label.getName())) { Location loc = rule.getLocation(); @@ -1890,7 +1898,7 @@ private void addInputFile(InputFile inputFile) { * *