From b9530269569d40cf8495f667c52c0746bf403762 Mon Sep 17 00:00:00 2001
From: Googler <brandjon@google.com>
Date: Fri, 12 Jan 2024 07:35:43 -0800
Subject: [PATCH] Check symbolic macro instances for name conflicts with other
 objects 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
---
 .../devtools/build/lib/packages/Package.java  | 154 +++++++++++-------
 .../build/lib/packages/OutputFileTest.java    |  11 +-
 .../lib/packages/PackageFactoryTest.java      | 153 +++++++++++++++++
 3 files changed, 256 insertions(+), 62 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 2b0d617bb56c05..5b052c075aed87 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
@@ -116,10 +116,12 @@ public class Package {
   private static final long PACKAGE_OVERHEAD_UNSET = -1;
 
   /**
-   * An exception used when a target's name conflicts with other targets in the package.
+   * An exception used when the name of a target or symbolic macro clashes with another entity
+   * defined in the package.
    *
-   * <p>This could be an exact match, or in the case of output files, a prefix clash (one output
-   * file is a prefix of another).
+   * <p>Common examples of conflicts include two targets or symbolic macros sharing the same name,
+   * and one output file being a prefix of another. See {@link #checkForExistingName} and {@link
+   * #checkRuleAndOutputs} for more details.
    */
   public static final class NameConflictException extends Exception {
     private NameConflictException(String message) {
@@ -922,19 +924,19 @@ default boolean precomputeTransitiveLoads() {
 
     private enum NameConflictCheckingPolicy {
       UNKNOWN,
-      DISABLED,
+      NOT_GUARANTEED,
       ENABLED;
     }
 
     /**
-     * Whether to do validation checks for name clashes between targets and between output file
+     * Whether to do all validation checks for name clashes among targets, macros, and output file
      * prefixes.
      *
-     * <p>This should only be disabled for data that has already been validated, e.g. in package
-     * deserialization.
+     * <p>The {@code NOT_GUARANTEED} value should only be used when the package data has already
+     * been validated, e.g. in package deserialization.
      *
-     * <p>Even disabling this does not turn off *all* checking, just some of the more expensive
-     * ones. Do not rely on being able to violate these checks.
+     * <p>Setting it to {@code NOT_GUARANTEED} does not necessarily turn off *all* checking, just
+     * some of the more expensive ones. Do not rely on being able to violate these checks.
      */
     private NameConflictCheckingPolicy nameConflictCheckingPolicy =
         NameConflictCheckingPolicy.UNKNOWN;
@@ -953,7 +955,7 @@ private enum NameConflictCheckingPolicy {
      * file having that prefix. Used for error reporting.
      *
      * <p>This field is null if name conflict checking is disabled. It is also null after the
-     * package is built. The content of the map is manipulated only in {@link #checkForConflicts}.
+     * package is built. The content of the map is manipulated only in {@link #checkRuleAndOutputs}.
      */
     @Nullable private Map<String, OutputFile> outputFilePrefixes = new HashMap<>();
 
@@ -1425,8 +1427,8 @@ private Iterable<Rule> 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 that is not an
-     *     input file
+     * @throws NameConflictException if the name was already taken by another target/macro that is
+     *     not an input file
      * @throws IllegalArgumentException if the name is not a valid label
      */
     InputFile createInputFile(String targetName, Location location) throws NameConflictException {
@@ -1444,12 +1446,9 @@ InputFile createInputFile(String targetName, Location location) throws NameConfl
               "FileTarget in package " + pkg.getName() + " has illegal name: " + targetName, e);
       }
 
-      if (existing == null) {
-        addInputFile(inputFile);
-        return inputFile;
-      } else {
-        throw nameConflict(inputFile, existing);
-      }
+      checkForExistingName(inputFile);
+      addInputFile(inputFile);
+      return inputFile;
     }
 
     /**
@@ -1509,7 +1508,7 @@ void addPackageGroup(
               repoRootMeansCurrentRepo,
               eventHandler,
               location);
-      checkNameConflict(group);
+      checkForExistingName(group);
       targets.put(group.getName(), group);
 
       if (group.containsErrors()) {
@@ -1559,7 +1558,7 @@ void addEnvironmentGroup(
 
       EnvironmentGroup group =
           new EnvironmentGroup(createLabel(name), pkg, environments, defaults, location);
-      checkNameConflict(group);
+      checkForExistingName(group);
       targets.put(group.getName(), group);
 
       // Invariant: once group is inserted into targets, it must also:
@@ -1594,8 +1593,9 @@ void addEnvironmentGroup(
     }
 
     /**
-     * Turns off conflict checking for name clashes between targets, or between output file
-     * prefixes.
+     * Turns off (some) conflict checking for name clashes between targets, macros, and output file
+     * prefixes. (It is not guaranteed to disable all checks, since it is intended as an
+     * optimization and not for semantic effect.)
      *
      * <p>This should only be done for data that has already been validated, e.g. during package
      * deserialization. Do not call this unless you know what you're doing.
@@ -1606,14 +1606,15 @@ void addEnvironmentGroup(
     @CanIgnoreReturnValue
     Builder disableNameConflictChecking() {
       Preconditions.checkState(nameConflictCheckingPolicy == NameConflictCheckingPolicy.UNKNOWN);
-      this.nameConflictCheckingPolicy = NameConflictCheckingPolicy.DISABLED;
+      this.nameConflictCheckingPolicy = NameConflictCheckingPolicy.NOT_GUARANTEED;
       this.ruleLabels = null;
       this.outputFilePrefixes = null;
       return this;
     }
 
     private void ensureNameConflictChecking() {
-      Preconditions.checkState(nameConflictCheckingPolicy != NameConflictCheckingPolicy.DISABLED);
+      Preconditions.checkState(
+          nameConflictCheckingPolicy != NameConflictCheckingPolicy.NOT_GUARANTEED);
       this.nameConflictCheckingPolicy = NameConflictCheckingPolicy.ENABLED;
     }
 
@@ -1637,7 +1638,8 @@ private void addRuleInternal(Rule rule) {
      * #disableNameConflictChecking} was already called.
      */
     void addRuleUnchecked(Rule rule) {
-      Preconditions.checkState(nameConflictCheckingPolicy == NameConflictCheckingPolicy.DISABLED);
+      Preconditions.checkState(
+          nameConflictCheckingPolicy == NameConflictCheckingPolicy.NOT_GUARANTEED);
       addRuleInternal(rule);
     }
 
@@ -1649,13 +1651,14 @@ void addRule(Rule rule) throws NameConflictException {
       ensureNameConflictChecking();
 
       List<Label> labels = rule.getLabels();
-      checkForConflicts(rule, labels);
+      checkRuleAndOutputs(rule, labels);
       addRuleInternal(rule);
       ruleLabels.put(rule, labels);
     }
 
+    /** Adds a symbolic macro instance to the package. */
     public void addMacro(MacroInstance macro) throws NameConflictException {
-      // TODO(#19922): Add name conflict checking!
+      checkForExistingName(macro);
       macros.put(macro.getName(), macro);
       // TODO(#19922): Push to a queue of unexpanded macros, read those when expanding macros as
       // part of monolithic package evaluation (but not lazy macro evaluation).
@@ -1722,6 +1725,7 @@ private Builder beforeBuild(boolean discoverAssumedInputFiles) throws NoSuchPack
           for (Label label : labels) {
             if (label.getPackageIdentifier().equals(pkg.getPackageIdentifier())
                 && !targets.containsKey(label.getName())
+                && !macros.containsKey(label.getName())
                 && !newInputFiles.containsKey(label.getName())) {
               Location loc = rule.getLocation();
               newInputFiles.put(
@@ -1816,11 +1820,12 @@ private void addInputFile(InputFile inputFile) {
     }
 
     /**
-     * Precondition check for {@link #addRule}. Verifies that:
+     * Precondition check for {@link #addRule} (to be called before the rule and its outputs are in
+     * the targets map). Verifies that:
      *
      * <ul>
      *   <li>The added rule's name, and the names of its output files, are not the same as the name
-     *       of any target already declared in the package.
+     *       of any target/macro already declared in the package.
      *   <li>The added rule's output files list does not contain the same name twice.
      *   <li>The added rule does not have an input file and an output file that share the same name.
      *   <li>For each of the added rule's output files, no directory prefix of that file matches the
@@ -1832,12 +1837,12 @@ private void addInputFile(InputFile inputFile) {
     // TODO(bazel-team): We verify that all prefixes of output files are distinct from other output
     // file names, but not that they're distinct from other target names in the package. What
     // happens if you define an input file "abc" and output file "abc/xyz"?
-    private void checkForConflicts(Rule rule, List<Label> labels) throws NameConflictException {
+    private void checkRuleAndOutputs(Rule rule, List<Label> labels) throws NameConflictException {
       Preconditions.checkNotNull(outputFilePrefixes); // ensured by addRule's precondition
 
       // Check the name of the new rule itself.
       String ruleName = rule.getName();
-      checkNameConflict(rule);
+      checkForExistingName(rule);
 
       ImmutableList<OutputFile> outputFiles = rule.getOutputFiles();
       Map<String, OutputFile> outputFilesByName =
@@ -1846,13 +1851,16 @@ private void checkForConflicts(Rule rule, List<Label> labels) throws NameConflic
       // Check the new rule's output files, both for direct conflicts and prefix conflicts.
       for (OutputFile outputFile : outputFiles) {
         String outputFileName = outputFile.getName();
-        // Check for duplicate within a single rule. (Can't use checkNameConflict since this rule's
-        // outputs aren't in the target map yet.)
+        // Check for duplicate within a single rule. (Can't use checkForExistingName since this
+        // rule's outputs aren't in the target map yet.)
         if (outputFilesByName.put(outputFileName, outputFile) != null) {
-          throw nameConflict(outputFile, outputFile); // Duplicate within a single rule.
+          throw new NameConflictException(
+              String.format(
+                  "rule '%s' has more than one generated file named '%s'",
+                  ruleName, outputFileName));
         }
-        // Check for conflict with any other already added target.
-        checkNameConflict(outputFile);
+        // Check for conflict with any other already added target/macro.
+        checkForExistingName(outputFile);
         // TODO(bazel-team): We also need to check for a conflict between an output file and its own
         // rule, which is not yet in the targets map.
 
@@ -1892,34 +1900,64 @@ private void checkForConflicts(Rule rule, List<Label> labels) throws NameConflic
     }
 
     /**
-     * Throws {@link NameConflictException} if a target with the given target's name is already
-     * present in the package.
+     * Throws {@link NameConflictException} if the given target's name matches an existing target or
+     * macro in the package.
      */
-    private void checkNameConflict(Target added) throws NameConflictException {
-      Target existing = targets.get(added.getName());
-      if (existing != null) {
-        throw nameConflict(added, existing);
-      }
+    private void checkForExistingName(Target added) throws NameConflictException {
+      checkForExistingName(added.getName(), added);
+    }
+
+    /**
+     * Throws {@link NameConflictException} if the given macro's name matches an existing target or
+     * macro in the package.
+     */
+    private void checkForExistingName(MacroInstance added) throws NameConflictException {
+      checkForExistingName(added.getName(), added);
     }
 
-    /** Returns a {@link NameConflictException} about two targets having the same name. */
-    private static NameConflictException nameConflict(Target added, Target existing) {
-      String subject = String.format("%s '%s'", added.getTargetKind(), added.getName());
-      if (added instanceof OutputFile) {
-        subject += " in rule '" + ((OutputFile) added).getGeneratingRule().getName() + "'";
+    private void checkForExistingName(String name, Object added) throws NameConflictException {
+      Object existing = targets.get(name);
+      if (existing == null) {
+        existing = macros.get(name);
+      }
+      if (existing == null) {
+        return;
       }
 
-      String object =
-          existing instanceof OutputFile
-              ? String.format(
-                  "generated file from rule '%s'",
-                  ((OutputFile) existing).getGeneratingRule().getName())
-              : existing.getTargetKind();
+      // Format error message subject and object, which are either Targets or MacroInstances.
 
-      return new NameConflictException(
-          String.format(
-              "%s conflicts with existing %s, defined at %s",
-              subject, object, existing.getLocation()));
+      String subject;
+      if (added instanceof Target) {
+        subject =
+            String.format("%s '%s'", ((Target) added).getTargetKind(), ((Target) added).getName());
+        if (added instanceof OutputFile) {
+          subject += " in rule '" + ((OutputFile) added).getGeneratingRule().getName() + "'";
+        }
+      } else if (added instanceof MacroInstance) {
+        subject = String.format("macro '%s'", ((MacroInstance) added).getName());
+      } else {
+        throw new IllegalArgumentException("Unexpected object type: " + added.getClass());
+      }
+
+      String object;
+      if (existing instanceof Target) {
+        object =
+            existing instanceof OutputFile
+                ? String.format(
+                    "generated file from rule '%s'",
+                    ((OutputFile) existing).getGeneratingRule().getName())
+                : ((Target) existing).getTargetKind();
+        object += ", defined at " + ((Target) existing).getLocation();
+      } else if (existing instanceof MacroInstance) {
+        // TODO(#19922): Add definition location info for the existing object, like we have in the
+        // case for rules.
+        object = "macro";
+      } else {
+        throw new AssertionError();
+      }
+
+      throw new NameConflictException(
+          String.format("%s conflicts with existing %s", subject, object));
     }
 
     /**
diff --git a/src/test/java/com/google/devtools/build/lib/packages/OutputFileTest.java b/src/test/java/com/google/devtools/build/lib/packages/OutputFileTest.java
index aea61f436c3ebb..2a95fe712aece6 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/OutputFileTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/OutputFileTest.java
@@ -123,12 +123,15 @@ public void testOutputFileNameConflictsWithExistingRule() throws Exception {
   @Test
   public void testDuplicateOutputFilesInSameRule() throws Exception {
     scratch.file(
-        "two_outs/BUILD", "genrule(name='a', cmd='ls >$(location out)',outs=['out', 'out'])");
+        "two_outs/BUILD",
+        "genrule(",
+        "    name='a',",
+        "    cmd='ls >$(location out)',",
+        "    outs=['out', 'out'],",
+        ")");
     reporter.removeHandler(failFastHandler);
     getTarget("//two_outs:BUILD");
-    assertContainsEvent(
-        "generated file 'out' in rule 'a' conflicts with "
-            + "existing generated file from rule 'a'");
+    assertContainsEvent("rule 'a' has more than one generated file named 'out'");
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
index 3e18c32faa77e6..d80bc59fe67df2 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
@@ -1145,6 +1145,159 @@ public void testPackageDefaultRestrictionDuplicates() throws Exception {
         "package(default_restricted_to=['//foo', '//bar', '//foo'])");
   }
 
+  /**
+   * Defines a symbolic macro "my_macro" in //pkg:my_macro.bzl, and enables the experimental flag.
+   */
+  private void defineMacroBzl() throws Exception {
+    setBuildLanguageOptions("--experimental_enable_first_class_macros");
+    scratch.file(
+        "pkg/my_macro.bzl", //
+        "def _impl(name):",
+        "    pass",
+        "my_macro = macro(implementation = _impl)");
+  }
+
+  @Test
+  public void testSymbolicMacro_duplicateMacroNames() throws Exception {
+    defineMacroBzl();
+    expectEvalError(
+        "macro 'foo' conflicts with existing macro",
+        "load(':my_macro.bzl', 'my_macro')",
+        "my_macro(name = 'foo')",
+        "my_macro(name = 'foo')");
+  }
+
+  @Test
+  public void testSymbolicMacro_ruleConflictsWithMacro() throws Exception {
+    defineMacroBzl();
+    expectEvalError(
+        "cc_library rule 'foo' conflicts with existing macro",
+        "load(':my_macro.bzl', 'my_macro')",
+        "my_macro(name = 'foo')",
+        "cc_library(name = 'foo')");
+  }
+
+  @Test
+  public void testSymbolicMacro_macroConflictsWithRule() throws Exception {
+    defineMacroBzl();
+    expectEvalError(
+        "macro 'foo' conflicts with existing cc_library rule",
+        "load(':my_macro.bzl', 'my_macro')",
+        "cc_library(name = 'foo')",
+        "my_macro(name = 'foo')");
+  }
+
+  @Test
+  public void testSymbolicMacro_outputConflictsWithMacro() throws Exception {
+    defineMacroBzl();
+    expectEvalError(
+        "generated file 'foo' in rule 'gen' conflicts with existing macro",
+        "load(':my_macro.bzl', 'my_macro')",
+        "my_macro(name = 'foo')",
+        "genrule(name = 'gen', outs = ['foo'], cmd = '')");
+  }
+
+  @Test
+  public void testSymbolicMacro_macroConflictsWithOutput() throws Exception {
+    defineMacroBzl();
+    expectEvalError(
+        "macro 'foo' conflicts with existing generated file from rule 'gen'",
+        "load(':my_macro.bzl', 'my_macro')",
+        "genrule(name = 'gen', outs = ['foo'], cmd = '')",
+        "my_macro(name = 'foo')");
+  }
+
+  @Test
+  public void testSymbolicMacro_macroMayCollideWithPrefixOfOutput() throws Exception {
+    // TODO(bazel-team): This test case verifies an existing but possibly undesirable behavior.
+    // Currently we only prevent output file prefixes from colliding with other output files, and
+    // don't check if they collide with other types of targets. If we become more restrictive in the
+    // future, we should also ensure they can't collide with macros.
+    defineMacroBzl();
+    expectEvalSuccess(
+        "load(':my_macro.bzl', 'my_macro')",
+        "my_macro(name = 'foo')",
+        "genrule(name = 'gen', outs = ['foo/bar'], cmd = '')");
+  }
+
+  @Test
+  public void testSymbolicMacro_environmentGroupConflictsWithMacro() throws Exception {
+    defineMacroBzl();
+    expectEvalError(
+        "environment group 'foo' conflicts with existing macro",
+        "load(':my_macro.bzl', 'my_macro')",
+        "my_macro(name = 'foo')",
+        "environment(name = 'env')",
+        "environment_group(name='foo', environments = [':env'], defaults = [':env'])");
+  }
+
+  @Test
+  public void testSymbolicMacro_macroConflictsWithEnvironmentGroup() throws Exception {
+    defineMacroBzl();
+    expectEvalError(
+        "macro 'foo' conflicts with existing environment group",
+        "load(':my_macro.bzl', 'my_macro')",
+        "environment(name = 'env')",
+        "environment_group(name='foo', environments = [':env'], defaults = [':env'])",
+        "my_macro(name = 'foo')");
+  }
+
+  @Test
+  public void testSymbolicMacro_packageGroupConflictsWithMacro() throws Exception {
+    defineMacroBzl();
+    expectEvalError(
+        "package group 'foo' conflicts with existing macro",
+        "load(':my_macro.bzl', 'my_macro')",
+        "my_macro(name = 'foo')",
+        "package_group(name = 'foo')");
+  }
+
+  @Test
+  public void testSymbolicMacro_macroConflictsWithPackageGroup() throws Exception {
+    defineMacroBzl();
+    expectEvalError(
+        "macro 'foo' conflicts with existing package group",
+        "load(':my_macro.bzl', 'my_macro')",
+        "package_group(name = 'foo')",
+        "my_macro(name = 'foo')");
+  }
+
+  @Test
+  public void testSymbolicMacro_inputConflictsWithMacro() throws Exception {
+    defineMacroBzl();
+    expectEvalError(
+        "source file 'foo' conflicts with existing macro",
+        "load(':my_macro.bzl', 'my_macro')",
+        "my_macro(name = 'foo')",
+        "exports_files(['foo'])");
+  }
+
+  @Test
+  public void testSymbolicMacro_macroConflictsWithInput() throws Exception {
+    defineMacroBzl();
+    expectEvalError(
+        "macro 'foo' conflicts with existing source file",
+        "load(':my_macro.bzl', 'my_macro')",
+        "exports_files(['foo'])",
+        "my_macro(name = 'foo')");
+  }
+
+  @Test
+  public void testSymbolicMacro_macroPreventsImplicitCreationOfInputFile() throws Exception {
+    defineMacroBzl();
+    scratch.file(
+        "pkg/BUILD",
+        "load(':my_macro.bzl', 'my_macro')",
+        "my_macro(name = 'foo')",
+        "cc_library(",
+        "    name = 'lib',",
+        "    srcs = ['foo', 'bar'],",
+        ")");
+    Package pkg = loadPackage("pkg");
+    assertThat(pkg.getTarget("bar")).isInstanceOf(InputFile.class);
+    assertThat(pkg.getTargets()).doesNotContainKey("foo");
+  }
+
   @Test
   public void testGlobPatternExtractor() throws Exception {
     StarlarkFile file =