Allow implicit creation of input files within a macro's namespace

Recent experience with symbolic macro migration showed that we need to relax the restrictions on the implicit creation of input files.

Previously it was an error to refer to a non-existent target `"foo_input"` if there happened to be a symbolic macro named `"foo"` in the package. The rationale was that we don't want to have to evaluate `foo` just to decide whether or not `foo_input` is in fact an `InputFile`.

But this restriction means that the common idiom:

```starlark
bzl_library(
    name = "foo",
    srcs = ["foo.bzl"],
)
```

blocks `bzl_library` from being a symbolic macro (e.g. wrapping the underlying rule). We believe this will be a common blocker well beyond this one example.

Therefore, we will now only block the implicit creation of input files when they collide exactly with a target or macro, and not when they merely fall within a macro's namespace.

A consequence of this design change is that it complicates how input files are handled when we have lazy macro evaluation. But we'll deal with that when the time comes.

Package.java
- Update and clarify javadoc for `createAssumedInputFiles()` and `maybeCreateAssumedInput()`. Update check in the latter to only care about exact matches of macro names.

TargetRecorder.java
- Factor out an accessor for checking whether a macro with the given name exists (insulating the caller from having to worry about the detail of how macro ids are constructed).

PackageFactoryTest.java
- Make implicit-input-file-creation tests more orthogonal and uniformly named, and update for the semantics change in this CL. (It may be easier to review this file by ignoring the diff and just reviewing the new test content.)

Fixes #24064.

PiperOrigin-RevId: 689374256
Change-Id: I218338af9f9638e1ec03c34e57e7c9f6e10beaaa

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

@@ -1529,22 +1529,22 @@ 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:
+     * <p>Precisely: For each label L appearing in one or more label-typed attributes of one or more
+     * declarations D (either of a target or a symbolic macro), we create an {@code InputFile} for L
+     * and return it in the map (keyed by its name) if all of the following are true:
      *
      * <ol>
-     *   <li>The package does not otherwise declare a target for L.
+     *   <li>L points to within the current package.
+     *   <li>The package does not otherwise declare a target or macro named 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).
+     * <p>The third condition ensures that we can know all *possible* implicitly created input files
+     * without evaluating any symbolic macros. However, if the label lies within one or more
+     * symbolic macro's namespaces, then we do still need to evaluate those macros to determine
+     * whether or not the second condition is true, i.e. whether the label points to a target the
+     * macro declares (or a submacro it clashes with), or defaults to an implicitly created input
+     * file.
      */
     private static Map<String, InputFile> createAssumedInputFiles(
         Package pkg, TargetRecorder recorder, boolean noImplicitFileExport) {
@@ -1585,8 +1585,7 @@ private static Map<String, InputFile> createAssumedInputFiles(
 
     /**
      * 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.
+     * package and there is no existing target or macro for that label.
      */
     private static void maybeCreateAssumedInputFile(
         Map<String, InputFile> implicitlyCreatedInputFiles,
@@ -1600,20 +1599,10 @@ private static void maybeCreateAssumedInputFile(
         return;
       }
       if (recorder.getTargetMap().containsKey(name)
+          || recorder.hasMacroWithName(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,

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

@@ -172,6 +172,18 @@ public Map<String, MacroInstance> getMacroMap() {
     return macroMap;
   }
 
+  /**
+   * Returns whether there exists a macro with the given name.
+   *
+   * <p>There may be more than one such macro, nested in a chain of main submacros.
+   */
+  public boolean hasMacroWithName(String name) {
+    // Macros are indexed by id, not name, so we can't just use macroMap.get() directly.
+    // Instead, we reason that if at least one macro by the given name exists, then there is one
+    // with an id suffix of ":1".
+    return macroMap.containsKey(name + ":1");
+  }
+
   public List<Label> getRuleLabels(Rule rule) {
     return (ruleLabels != null) ? ruleLabels.get(rule) : rule.getLabels();
   }
@@ -651,11 +663,7 @@ private void checkMacroName(MacroInstance macro) throws NameConflictException {
    * <p>{@code what} must be either "macro" or "target".
    */
   private void checkForExistingMacroName(String name, String what) throws NameConflictException {
-    // Macros are indexed by id, not name, so we can't just use macroMap.get() directly.
-    // Instead, we reason that if at least one macro by the given name exists, then there is one
-    // with an id suffix of ":1".
-    MacroInstance existing = macroMap.get(name + ":1");
-    if (existing == null) {
+    if (!hasMacroWithName(name)) {
       return;
     }
 

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

@@ -1346,32 +1346,39 @@ public void testSymbolicMacro_macroAndInputClash_inputDeclaredFirst() throws Exc
   }
 
   @Test
-  public void testSymbolicMacro_macroPreventsImplicitCreationOfInputFilesUnderItsNamespaces()
+  public void testSymbolicMacro_implicitlyCreatedInput_isCreatedEvenInsideMacroNamespace()
       throws Exception {
-    // 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):
+            pass
+        my_macro = macro(implementation = _impl)
+        """);
+    scratch.file(
+        "pkg/BUILD",
+        """
+        load(":my_macro.bzl", "my_macro")
+        my_macro(name = "foo")
+        cc_library(
+            name = "toplevel_target",
+            srcs = ["foo_implicit"],
+        )
+        """);
+
+    Package pkg = loadPackageAndAssertSuccess("pkg");
+    assertThat(pkg.getTarget("foo_implicit")).isInstanceOf(InputFile.class);
+  }
 
+  @Test
+  public void testSymbolicMacro_implicitlyCreatedInput_isNotCreatedIfMacroDeclaresTarget()
+      throws Exception {
+    scratch.file(
+        "pkg/my_macro.bzl",
+        """
         def _impl(name, visibility):
-            native.cc_library(
-                name = name + "_target",
-                srcs = ["foo_input"],
-            )
-            my_submacro(name = name + "_submacro")
+            native.cc_library(name = name + "_declared_target")
+            native.cc_library(name = "illegally_named_target")
         my_macro = macro(implementation = _impl)
         """);
     scratch.file(
@@ -1382,124 +1389,108 @@ def _impl(name, visibility):
         cc_library(
             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
+                "foo_declared_target",
+                "illegally_named_target",
             ],
         )
         """);
 
     Package pkg = loadPackageAndAssertSuccess("pkg");
-    assertThat(pkg.getTargets()).doesNotContainKey("foo_input");
-    assertThat(pkg.getTargets()).doesNotContainKey("foo");
-    assertThat(pkg.getTarget("foo_")).isInstanceOf(InputFile.class);
-    assertThat(pkg.getTarget("baz")).isInstanceOf(InputFile.class);
+    assertThat(pkg.getTarget("foo_declared_target")).isInstanceOf(Rule.class);
+    // This target doesn't lie within the macro's namespace and so can't be analyzed, but it still
+    // exists and prevents input file creation. (Under a lazy macro evaluation model, we would
+    // potentially create an InputFile for it but later discover a name clash if the macro is
+    // evaluated.)
+    // TODO: #23852 - Test behavior under lazy macro evaluation when implemented.
+    assertThat(pkg.getTarget("illegally_named_target")).isInstanceOf(Rule.class);
   }
 
   @Test
-  public void testSymbolicMacro_macroInstantiationCanForceImplicitCreationOfInputFile()
+  public void testSymbolicMacro_implicitlyCreatedInput_isNotCreatedIfMacroNameMatchesExactly()
       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):
+        def _impl(name, visibility):
             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()},
-        )
+        my_macro = macro(implementation = _impl)
         """);
     scratch.file(
         "pkg/BUILD",
         """
         load(":my_macro.bzl", "my_macro")
-        my_macro(
-            name = "foo",
-            src = "//pkg:input",
+        my_macro(name = "foo")
+        cc_library(
+            name = "toplevel_target",
+            srcs = ["foo"],
         )
         """);
 
     Package pkg = loadPackageAndAssertSuccess("pkg");
-    assertThat(pkg.getTarget("input")).isInstanceOf(InputFile.class);
-    assertThat(pkg.getTargets()).doesNotContainKey("does_not_exist");
+    assertThat(pkg.getTargets()).doesNotContainKey("foo");
   }
 
   @Test
-  public void testSymbolicMacro_failsGracefullyWhenInputFileClashesWithMisnamedMacroTarget()
+  public void testSymbolicMacro_implicitlyCreatedInput_isCreatedByUsageInMacroAttr()
       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.
+    // A usage in a macro, provided it is top-level, is sufficient to cause an input file to be
+    // implicitly created, even if that input file is not also referred to by any actual targets.
     scratch.file(
         "pkg/my_macro.bzl",
         """
-        def _impl(name, visibility):
-            native.cc_library(name="conflicting_name")
-        my_macro = macro(implementation=_impl)
+        def _impl(name, visibility, src):
+            pass
+        my_macro = macro(
+            implementation = _impl,
+            attrs = {"src": attr.label()},
+        )
         """);
     scratch.file(
         "pkg/BUILD",
         """
         load(":my_macro.bzl", "my_macro")
-        my_macro(name="foo")
-        cc_library(
-            name = "bar",
-            srcs = [":conflicting_name"],
+        my_macro(
+            name = "foo",
+            src = "//pkg:input",
         )
         """);
+
     Package pkg = loadPackageAndAssertSuccess("pkg");
-    assertThat(pkg.getTarget("conflicting_name")).isInstanceOf(Rule.class);
+    assertThat(pkg.getTarget("input")).isInstanceOf(InputFile.class);
   }
 
   @Test
-  public void testSymbolicMacro_implicitCreationOfInputFilesIsNotTriggeredByMacros()
+  public void testSymbolicMacro_implicitlyCreatedInput_isNotCreatedByUsageInMacroBody()
       throws Exception {
+    // A usage in the body of a macro (whether the declaration is for a target or submacro), does
+    // not by itself cause an input file to be implicitly created.
     scratch.file(
         "pkg/my_macro.bzl",
         """
+        def _sub_impl(name, visibility):
+            native.cc_library(
+                name = name,
+                srcs = ["//pkg:input"],
+            )
+        my_submacro = macro(implementation = _sub_impl)
+
         def _impl(name, visibility):
             native.cc_library(
                 name = name,
-                srcs = ["//pkg:src_A.txt", "//pkg:src_B.txt"],
+                srcs = ["//pkg: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 = "bar",
-            srcs = ["src_A.txt"],
-        )
         """);
 
     Package pkg = loadPackageAndAssertSuccess("pkg");
-    assertThat(pkg.getTargets()).containsKey("src_A.txt");
-    assertThat(pkg.getTargets()).doesNotContainKey("src_B.txt");
+    assertThat(pkg.getTargets()).doesNotContainKey("input");
   }
 
   @Test