Fix implicit input file creation for symbolic macros

Previously we weren't creating input files for labels used in the attribute of a top-level symbolic macro. This meant that if you wrote

```
my_macro(
    name = "foo",
    srcs = ["input.txt"],
)
```

you'd get an error message claiming that input.txt is not declared and requires an `exports_files()`. (The usages of input.txt by targets declared inside of foo don't count, because implicit input file creation only works for usages of labels outside of symbolic macro bodies.)

This CL fixes this behavior, refactors the relevant logic in Package.java, and adds more test coverage.

Package.java:
- Factor the big for loop from `beforeBuild()` to `createAssumedInputFiles()`. Leave the original for loop in place for test suite accumulation, and pull the `if (discover...)` out of the new call.
- Factor the meat of the logic into a helper, `maybeCreateAssumedInputFile()`, so we can reuse it for both rules and macros (previously there was no loop over references in macros).
- Add tedious javadoc to this tedious logic.

SymbolicMacroTest.java
- Add explanatory comment to `macroCanReferToInputFile()`.
- Expand `macroCannotForceCreationOfImplicitInputFileOnItsOwn()` to check that the target is still not created when the inner usage is in an attr of a MacroInstance rather than a Rule.

PackageFactoryTest.java
- Expand comment in `testSymbolicMacro_macroPreventsImplicitCreationOfInputFilesUnderItsNamespace()`, include coverage for when the reference comes from within a target or submacro inside the symbolic macro itself.
- New test case for the behavior of this CL, `..._macroInstantiationCanForceImplicitCreationOfInputFile()`.
- Drive-by: Add a test case for when a badly named target in a symbolic macro clashes with an implicitly created input file. This behavior may be affected by lazy macro evaluation in the future.

Confirmed that this CL works for the case I originally discovered it on while writing examples documentation.

Fixes bazelbuild#24007.

PiperOrigin-RevId: 686919555
Change-Id: I6313070f76456c0b0b5d5458ca35c89d1d6da33b

src/main/java/com/google/devtools/build/lib/packages/Package.java

@@ -1525,6 +1525,103 @@ public void expandAllRemainingMacros(StarlarkSemantics semantics) throws Interru
       }
     }
 
+    /**
+     * Creates and returns input files for targets that have been referenced but not explicitly
+     * declared in this package.
+     *
+     * <p>Precisely: If L is a label that points within the current package, and L appears in a
+     * label-typed attribute of some declaration (target or symbolic macro) D in this package, then
+     * we create an {@code InputFile} corresponding to L and return it in this map (keyed by its
+     * name), provided that all of the following are true:
+     *
+     * <ol>
+     *   <li>The package does not otherwise declare a target for L.
+     *   <li>D is not itself declared inside a symbolic macro.
+     *   <li>L is not within the namespace of any symbolic macro in the package.
+     * </ol>
+     *
+     * The second condition ensures that we can know all implicitly created input files without
+     * having to evaluate any symbolic macros. The third condition ensures that we don't need to
+     * expand a symbolic macro to decide whether it defines a target that conflicts with an
+     * implicitly created input file (except for the case where the target doesn't satisfy the
+     * macro's naming requirements, in which case it would be unusable anyway).
+     */
+    private static Map<String, InputFile> createAssumedInputFiles(
+        Package pkg, TargetRecorder recorder, boolean noImplicitFileExport) {
+      Map<String, InputFile> implicitlyCreatedInputFiles = new HashMap<>();
+
+      for (Rule rule : recorder.getRules()) {
+        if (!recorder.isRuleCreatedInMacro(rule)) {
+          for (Label label : recorder.getRuleLabels(rule)) {
+            maybeCreateAssumedInputFile(
+                implicitlyCreatedInputFiles,
+                pkg,
+                recorder,
+                noImplicitFileExport,
+                label,
+                rule.getLocation());
+          }
+        }
+      }
+
+      for (MacroInstance macro : recorder.getMacroMap().values()) {
+        if (macro.getParent() == null) {
+          macro.visitExplicitAttributeLabels(
+              label ->
+                  maybeCreateAssumedInputFile(
+                      implicitlyCreatedInputFiles,
+                      pkg,
+                      recorder,
+                      noImplicitFileExport,
+                      label,
+                      // TODO(bazel-team): We don't save a MacroInstance's location information yet,
+                      // but when we do, use that here.
+                      Location.BUILTIN));
+        }
+      }
+
+      return implicitlyCreatedInputFiles;
+    }
+
+    /**
+     * Adds an implicitly created input file to the given map if the label points within the current
+     * package, there is no existing target for that label, and the label does not lie within any
+     * macro's namespace.
+     */
+    private static void maybeCreateAssumedInputFile(
+        Map<String, InputFile> implicitlyCreatedInputFiles,
+        Package pkg,
+        TargetRecorder recorder,
+        boolean noImplicitFileExport,
+        Label label,
+        Location loc) {
+      String name = label.getName();
+      if (!label.getPackageIdentifier().equals(pkg.getPackageIdentifier())) {
+        return;
+      }
+      if (recorder.getTargetMap().containsKey(name)
+          || implicitlyCreatedInputFiles.containsKey(name)) {
+        return;
+      }
+      // TODO(#19922): This conflict check is quadratic complexity -- the number of candidate inputs
+      // to create times the number of macros in the package (top-level or nested). We can optimize
+      // by only checking against top-level macros, since child macro namespaces are contained
+      // within their parents' namespace. We can also use a trie data structure to zoom in on the
+      // relevant conflicting macro if it exists, since you can't be in a macro's namespace unless
+      // you suffix its name (at least, under current namespacing rules).
+      for (MacroInstance macro : recorder.getMacroMap().values()) {
+        if (TargetRecorder.nameIsWithinMacroNamespace(name, macro.getName())) {
+          return;
+        }
+      }
+
+      implicitlyCreatedInputFiles.put(
+          name,
+          noImplicitFileExport
+              ? new PrivateVisibilityInputFile(pkg, label, loc)
+              : new InputFile(pkg, label, loc));
+    }
+
     @CanIgnoreReturnValue
     private Builder beforeBuild(boolean discoverAssumedInputFiles) throws NoSuchPackageException {
       // For correct semantics, we refuse to build a package that has declared symbolic macros that
@@ -1573,58 +1670,18 @@ private Builder beforeBuild(boolean discoverAssumedInputFiles) throws NoSuchPack
       // Clear tests before discovering them again in order to keep this method idempotent -
       // otherwise we may double-count tests if we're called twice due to a skyframe restart, etc.
       testSuiteImplicitTestsAccumulator.clearAccumulatedTests();
-
-      Map<String, InputFile> newInputFiles = new HashMap<>();
       for (Rule rule : recorder.getRules()) {
-        if (discoverAssumedInputFiles) {
-          // Labels mentioned by a rule that refer to an unknown target in the current package are
-          // assumed to be InputFiles, unless they overlap a namespace owned by a macro. Create
-          // these InputFiles now. But don't do this for rules created within a symbolic macro,
-          // since we don't want the evaluation of the macro to affect the semantics of whether or
-          // not this target was created (i.e. all implicitly created files are knowable without
-          // necessarily evaluating symbolic macros).
-          if (recorder.isRuleCreatedInMacro(rule)) {
-            continue;
-          }
-          // We use a temporary map, newInputFiles, to avoid concurrent modification to this.targets
-          // while iterating (via getRules() above).
-          List<Label> labels = recorder.getRuleLabels(rule);
-          for (Label label : labels) {
-            String name = label.getName();
-            if (label.getPackageIdentifier().equals(metadata.packageIdentifier())
-                && !recorder.getTargetMap().containsKey(name)
-                && !newInputFiles.containsKey(name)) {
-              // Check for collision with a macro namespace. Currently this is a linear loop over
-              // all symbolic macros in the package.
-              // TODO(#19922): This is quadratic complexity, optimize with a trie or similar if
-              // needed.
-              boolean macroConflictsFound = false;
-              for (MacroInstance macro : recorder.getMacroMap().values()) {
-                macroConflictsFound |=
-                    TargetRecorder.nameIsWithinMacroNamespace(name, macro.getName());
-              }
-              if (!macroConflictsFound) {
-                Location loc = rule.getLocation();
-                newInputFiles.put(
-                    name,
-                    // Targets added this way are not in any macro, so
-                    // copyAppendingCurrentMacroLocation() munging isn't applicable.
-                    noImplicitFileExport
-                        ? new PrivateVisibilityInputFile(pkg, label, loc)
-                        : new InputFile(pkg, label, loc));
-              }
-            }
-          }
-        }
-
         testSuiteImplicitTestsAccumulator.processRule(rule);
       }
-
       // Make sure all accumulated values are sorted for determinism.
       testSuiteImplicitTestsAccumulator.sortTests();
 
-      for (InputFile file : newInputFiles.values()) {
-        recorder.addInputFileUnchecked(file);
+      if (discoverAssumedInputFiles) {
+        Map<String, InputFile> newInputFiles =
+            createAssumedInputFiles(pkg, recorder, noImplicitFileExport);
+        for (InputFile file : newInputFiles.values()) {
+          recorder.addInputFileUnchecked(file);
+        }
       }
 
       return this;

src/test/java/com/google/devtools/build/lib/analysis/SymbolicMacroTest.java

@@ -394,7 +394,13 @@ public void macroCanReferToInputFile() throws Exception {
         def _impl(name, visibility):
             native.cc_library(
                 name = name,
-                srcs = ["explicit_input.cc", "implicit_input.cc"],
+                srcs = [
+                    "explicit_input.cc",
+                    # This usage does not cause implicit_input.cc to be created since we're inside
+                    # a symbolic macro. We force the input's creation by referring to it from bar
+                    # in the BUILD file.
+                    "implicit_input.cc",
+                ],
             )
         my_macro = macro(implementation=_impl)
         """);
@@ -419,11 +425,20 @@ public void macroCannotForceCreationOfImplicitInputFileOnItsOwn() throws Excepti
     scratch.file(
         "pkg/foo.bzl",
         """
+        def _sub_impl(name, visibility):
+            native.cc_library(
+                name = name + "_target",
+                srcs = ["implicit_input.cc"],
+            )
+
+        my_submacro = macro(implementation=_sub_impl)
+
         def _impl(name, visibility):
             native.cc_library(
-                name = name,
+                name = name + "_target",
                 srcs = ["implicit_input.cc"],
             )
+            my_submacro(name = name + "_submacro")
         my_macro = macro(implementation=_impl)
         """);
     scratch.file(
@@ -434,10 +449,14 @@ def _impl(name, visibility):
         """);
 
     Package pkg = getPackage("pkg");
-    // Confirm that implicit_input.cc is not a target of the package.
-    // It'd be an execution time error to build :abc, but the package still loads just fine.
+    // Confirm that implicit_input.cc is not a target of the package, despite being used inside a
+    // symbolic macro (and not by anything at the top level) for both a target and a submacro.
+    //
+    // It'd be an execution time error to attempt to build the declared rule targets, but the
+    // package still loads just fine.
     assertPackageNotInError(pkg);
-    assertThat(pkg.getTargets()).containsKey("abc");
+    assertThat(pkg.getTargets()).containsKey("abc_target");
+    assertThat(pkg.getTargets()).containsKey("abc_submacro_target");
     assertThat(pkg.getTargets()).doesNotContainKey("implicit_input.cc");
   }
 

src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java

@@ -1348,26 +1348,131 @@ public void testSymbolicMacro_macroAndInputClash_inputDeclaredFirst() throws Exc
   @Test
   public void testSymbolicMacro_macroPreventsImplicitCreationOfInputFilesUnderItsNamespaces()
       throws Exception {
-    defineEmptyMacroBzl();
+    // We don't implicitly create InputFile targets whose names lie inside symbolic macros'
+    // namespaces, no matter where the file is referred to from. This avoids having to force
+    // evaluation of the macro when depending on the input file, to determine whether the macro
+    // declares a conflicting target.
+    //
+    // Create a macro instance named "foo" and try to refer to "foo_input" from various places.
+    // Ensure that "foo_input" does not in fact get created. (You could still used an
+    // exports_files() to declare it explicitly if you wanted.)
+    scratch.file(
+        "pkg/my_macro.bzl",
+        """
+        def _sub_impl(name, visibility):
+            native.cc_library(
+                name = name + "_target",
+                srcs = ["foo_input"],
+            )
+        my_submacro = macro(implementation = _sub_impl)
+
+        def _impl(name, visibility):
+            native.cc_library(
+                name = name + "_target",
+                srcs = ["foo_input"],
+            )
+            my_submacro(name = name + "_submacro")
+        my_macro = macro(implementation = _impl)
+        """);
     scratch.file(
         "pkg/BUILD",
         """
         load(":my_macro.bzl", "my_macro")
         my_macro(name = "foo")
         cc_library(
-            name = "lib",
-            srcs = ["foo", "foo_", "foo_bar", "baz"],
+            name = "toplevel_target",
+            srcs = [
+                "foo_input",
+                # Also try other name patterns.
+                "foo",   # conflicts, not created
+                "foo_",  # not in namespace, created
+                "baz",   # not in namespace, created
+            ],
         )
         """);
-    // You can't implicitly make an input file with a name that foo could've defined. (You can still
-    // have an explicit exports_files() do it.)
+
     Package pkg = loadPackageAndAssertSuccess("pkg");
+    assertThat(pkg.getTargets()).doesNotContainKey("foo_input");
     assertThat(pkg.getTargets()).doesNotContainKey("foo");
-    assertThat(pkg.getTargets()).doesNotContainKey("foo_bar");
     assertThat(pkg.getTarget("foo_")).isInstanceOf(InputFile.class);
     assertThat(pkg.getTarget("baz")).isInstanceOf(InputFile.class);
   }
 
+  @Test
+  public void testSymbolicMacro_macroInstantiationCanForceImplicitCreationOfInputFile()
+      throws Exception {
+    // Referring to an input file when instantiating a top-level symbolic macro causes it to be
+    // implicitly created, even though no targets refer to it. Referring to an input when
+    // instantiating a submacro does not by itself cause creation.
+    scratch.file(
+        "pkg/my_macro.bzl",
+        """
+        def _sub_impl(name, visibility, src):
+            pass
+        my_submacro = macro(
+            implementation = _sub_impl,
+            attrs = {"src": attr.label()},
+        )
+
+        def _impl(name, visibility, src):
+            my_submacro(
+                name = name + "_submacro",
+                src = "//pkg:does_not_exist",
+            )
+        my_macro = macro(
+            implementation = _impl,
+            attrs = {"src": attr.label()},
+        )
+        """);
+    scratch.file(
+        "pkg/BUILD",
+        """
+        load(":my_macro.bzl", "my_macro")
+        my_macro(
+            name = "foo",
+            src = "//pkg:input",
+        )
+        """);
+
+    Package pkg = loadPackageAndAssertSuccess("pkg");
+    assertThat(pkg.getTarget("input")).isInstanceOf(InputFile.class);
+    assertThat(pkg.getTargets()).doesNotContainKey("does_not_exist");
+  }
+
+  @Test
+  public void testSymbolicMacro_failsGracefullyWhenInputFileClashesWithMisnamedMacroTarget()
+      throws Exception {
+    // Symbolic macros can't define usable targets outside their namespace, and BUILD files can't
+    // implicitly create input files inside a macro's namespace. But we could still have a conflict
+    // between an *unusable* ill-named macro target and an implicitly created input file. Make sure
+    // we don't crash at least.
+    //
+    // If symbolic macros are evaluated either synchronously with their instantiation or deferred to
+    // the end of the BUILD file, the target declared by the macro wins because it happens before
+    // implicit input file creation. But under lazy macro evaluation, the implicit input file will
+    // win and we should see a conflict if the macro is expanded.
+    // TODO: #23852 - Test behavior under lazy macro evaluation when implemented.
+    scratch.file(
+        "pkg/my_macro.bzl",
+        """
+        def _impl(name, visibility):
+            native.cc_library(name="conflicting_name")
+        my_macro = macro(implementation=_impl)
+        """);
+    scratch.file(
+        "pkg/BUILD",
+        """
+        load(":my_macro.bzl", "my_macro")
+        my_macro(name="foo")
+        cc_library(
+            name = "bar",
+            srcs = [":conflicting_name"],
+        )
+        """);
+    Package pkg = loadPackageAndAssertSuccess("pkg");
+    assertThat(pkg.getTarget("conflicting_name")).isInstanceOf(Rule.class);
+  }
+
   @Test
   public void testSymbolicMacro_implicitCreationOfInputFilesIsNotTriggeredByMacros()
       throws Exception {