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) { * *