[8.2.0] Set generator_name, generator_function, generator_location, full Starlark stack for rules in symbolic macros (#25772)

Before, we did not set generator_* pseudo-attributes for rules generated
in a symbolic macro. However, it turns out that there are existing tools
which expect to be able to filter rule targets by generator_name in
query expressions; and such tools get broken when legacy macros are
migrated to symbolic. The easiest solution is to set generator_name and
friends consistently for both legacy and symbolic macro rule targets.

In the long term, we wish to deprecate and remove generator_*
pseudo-attrs, since the API has numerous problems and doesn't expose the
information users really need:

But for now, however, we ought to support the broken API symmetrically
for all varieties of macro.

In addition, take the opportunity to concatenate the Starlark stacks of
BUILD and symbolic macro threads for rule targets in symbolic macros,
because (1) the generator_* psudo-attributes are derived from the BUILD
thread's stack, and (2) the stack is shown in some query output, and
truncating it at symbolic macro boundaries is likely to confuse users
who rely on it for debugging.

RELNOTES: Set generator_name, generator_function, generator_location,
and the full Starlark stack for rule targets instantiated in a symbolic
macro.

PiperOrigin-RevId: 744110163
Change-Id: I5216b05f22ee0663343bd4468f4f299d095bb739

Commit
d6b07eb

Author: tetromino

src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java

@@ -1467,7 +1467,8 @@ public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> kwarg
         throw Starlark.errorf("unexpected positional arguments");
       }
 
-      MacroInstance macroInstance = macroClass.instantiateAndAddMacro(pkgBuilder, kwargs);
+      MacroInstance macroInstance =
+          macroClass.instantiateAndAddMacro(pkgBuilder, kwargs, thread.getCallStack());
 
       // Evaluate the macro now, if it's not a finalizer. Finalizer evaluation will be deferred to
       // the end of the BUILD file evaluation.

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

@@ -18,6 +18,8 @@
 import com.google.devtools.build.lib.skyframe.serialization.VisibleForSerialization;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.lib.util.HashCodes;
+import java.util.ArrayDeque;
+import java.util.Deque;
 import java.util.List;
 import java.util.Objects;
 import javax.annotation.Nullable;
@@ -124,20 +126,52 @@ public int hashCode() {
   private static final Interner<Node> nodeInterner = BlazeInterners.newWeakInterner();
 
   /**
-   * Returns a compact representation of the <em>interior</em> of the given call stack.
+   * Returns a compact representation of the given call stack, optionally ignoring the outermost
+   * frame.
    *
-   * <p>The outermost frame of the call stack is not reflected in the compact representation because
-   * it is already stored as {@link Rule#getLocation}.
-   *
-   * <p>Returns {@code null} for call stacks with fewer than two frames.
+   * @param start index of frame at which to start; in other words, skip this many outermost frames.
+   *     This is useful for skipping the outermost frame in BUILD file thread stacks, since the
+   *     BUILD file location is already stored in {@link Rule#getLocation} and {@link
+   *     MacroInstance#getBuildFileLocation}.
+   * @return {@code null} for call stacks with fewer than two frames.
    */
   @Nullable
-  static Node compactInterior(List<StarlarkThread.CallStackEntry> stack) {
+  static Node compact(List<StarlarkThread.CallStackEntry> stack, int start) {
     Node node = null;
-    for (int i = stack.size() - 1; i > 0; i--) {
+    for (int i = stack.size() - 1; i >= start; i--) {
       StarlarkThread.CallStackEntry entry = stack.get(i);
       node = nodeInterner.intern(new Node(entry.name, entry.location, node));
     }
     return node;
   }
+
+  /**
+   * Returns a concatenation of two compact call stacks.
+   *
+   * <p>The result will contain {@code inner} stack appended unmodified to a new copy of the {@code
+   * outer} stack.
+   *
+   * @return {@code null} if both of the inputs are {@code null} - in other words, if both of the
+   *     inputs are empty stacks.
+   */
+  @Nullable
+  static Node concatenate(@Nullable Node outer, @Nullable Node inner) {
+    Deque<Node> outerReversed = new ArrayDeque<>();
+    while (outer != null) {
+      outerReversed.addFirst(outer);
+      outer = outer.next();
+    }
+    Node node = inner;
+    for (Node origOuterNode : outerReversed) {
+      node =
+          nodeInterner.intern(
+              new Node(
+                  origOuterNode.name,
+                  origOuterNode.file,
+                  origOuterNode.line,
+                  origOuterNode.col,
+                  node));
+    }
+    return node;
+  }
 }

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

@@ -178,7 +178,10 @@ public MacroClass build() {
    */
   // TODO(#19922): Consider reporting multiple events instead of failing on the first one. See
   // analogous implementation in RuleClass#populateDefinedRuleAttributeValues.
-  private MacroInstance instantiateMacro(Package.Builder pkgBuilder, Map<String, Object> kwargs)
+  private MacroInstance instantiateMacro(
+      Package.Builder pkgBuilder,
+      Map<String, Object> kwargs,
+      ImmutableList<StarlarkThread.CallStackEntry> parentThreadCallStack)
       throws EvalException {
     // A word on edge cases:
     //   - If an attr is implicit but does not have a default specified, its value is just the
@@ -319,7 +322,7 @@ private MacroInstance instantiateMacro(Package.Builder pkgBuilder, Map<String, O
             ? 1
             : parentMacroFrame.macroInstance.getSameNameDepth() + 1;
 
-    return pkgBuilder.createMacro(this, attrValues, sameNameDepth);
+    return pkgBuilder.createMacro(this, attrValues, sameNameDepth, parentThreadCallStack);
   }
 
   /**
@@ -346,10 +349,16 @@ private static boolean shouldForceDefaultToNone(Attribute attr) {
    * @param kwargs A map from attribute name to its given Starlark value, such as passed in a BUILD
    *     file (i.e., prior to attribute type conversion, {@code select()} promotion, default value
    *     substitution, or even validation that the attribute exists).
+   * @param parentThreadCallStack The call stack of the Starlark thread in whose context the macro
+   *     instance is being constructed. This is *not* the thread that will execute the macro's
+   *     implementation function.
    */
   public MacroInstance instantiateAndAddMacro(
-      Package.Builder pkgBuilder, Map<String, Object> kwargs) throws EvalException {
-    MacroInstance macroInstance = instantiateMacro(pkgBuilder, kwargs);
+      Package.Builder pkgBuilder,
+      Map<String, Object> kwargs,
+      ImmutableList<StarlarkThread.CallStackEntry> parentThreadCallStack)
+      throws EvalException {
+    MacroInstance macroInstance = instantiateMacro(pkgBuilder, kwargs, parentThreadCallStack);
     try {
       pkgBuilder.addMacro(macroInstance);
     } catch (NameConflictException e) {

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

@@ -16,6 +16,7 @@
 
 import static java.util.Objects.requireNonNull;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -29,6 +30,8 @@
 import javax.annotation.Nullable;
 import net.starlark.java.eval.EvalException;
 import net.starlark.java.eval.Starlark;
+import net.starlark.java.eval.StarlarkThread;
+import net.starlark.java.syntax.Location;
 
 /**
  * Represents a use of a symbolic macro in a package.
@@ -48,6 +51,10 @@ public final class MacroInstance {
 
   @Nullable private final MacroInstance parent;
 
+  private final Location buildFileLocation;
+
+  private final CallStack.Node parentCallStack;
+
   private final MacroClass macroClass;
 
   private final int sameNameDepth;
@@ -80,12 +87,16 @@ public final class MacroInstance {
   public MacroInstance(
       Package pkg,
       @Nullable MacroInstance parent,
+      Location buildFileLocation,
+      CallStack.Node parentCallStack,
       MacroClass macroClass,
       Map<String, Object> attrValues,
       int sameNameDepth)
       throws EvalException {
     this.pkg = pkg;
     this.parent = parent;
+    this.buildFileLocation = buildFileLocation;
+    this.parentCallStack = parentCallStack;
     this.macroClass = macroClass;
     this.attrValues = ImmutableMap.copyOf(attrValues);
     Preconditions.checkArgument(sameNameDepth > 0);
@@ -107,6 +118,44 @@ public MacroInstance getParent() {
     return parent;
   }
 
+  /**
+   * Returns the location in the BUILD file at which this macro was created or its outermost
+   * enclosing symbolic or legacy macro was called.
+   */
+  @VisibleForTesting
+  public Location getBuildFileLocation() {
+    return buildFileLocation;
+  }
+
+  /**
+   * Returns the call stack of the Starlark thread that created this macro instance.
+   *
+   * <p>If this macro was instantiated in a BUILD file thread (as contrasted with a symbolic macro
+   * thread), the call stack does not include the frame for the BUILD file top level, since it's
+   * redundant with {@link #getBuildFileLocation}.
+   */
+  CallStack.Node getParentCallStack() {
+    return parentCallStack;
+  }
+
+  /**
+   * Returns the call stack of the Starlark thread that created this macro instance.
+   *
+   * <p>Requires reconstructing the call stack from a compact representation, so should only be
+   * called when the full call stack is needed.
+   */
+  @VisibleForTesting
+  public ImmutableList<StarlarkThread.CallStackEntry> reconstructParentCallStack() {
+    ImmutableList.Builder<StarlarkThread.CallStackEntry> stack = ImmutableList.builder();
+    if (parent == null) {
+      stack.add(StarlarkThread.callStackEntry(StarlarkThread.TOP_LEVEL, buildFileLocation));
+    }
+    for (CallStack.Node node = parentCallStack; node != null; node = node.next()) {
+      stack.add(node.toCallStackEntry());
+    }
+    return stack.build();
+  }
+
   /** Returns the {@link MacroClass} (i.e. schema info) that this instance parameterizes. */
   public MacroClass getMacroClass() {
     return macroClass;

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

@@ -826,6 +826,7 @@ public <T> BuildType.SelectorList<T> getSelectorList(String attributeName, Type<
    * Returns whether this rule was created by a macro.
    */
   public boolean wasCreatedByMacro() {
+    // TODO(bazel-team): do we really need the `hasStringAttribute(GENERATOR_NAME)` check?
     return interiorCallStack != null || hasStringAttribute(GENERATOR_NAME);
   }
 

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

@@ -50,6 +50,10 @@ private RuleFactory() {} // uninstantiable
    *
    * <p>It is the caller's responsibility to add the rule to the package (the caller may choose not
    * to do so if, for example, the rule has errors).
+   *
+   * @param callstack the stack of the Starlark thread where the rule is created. In the case of
+   *     rules instantiated by a symbolic macro, this is the inner macro's stack, and does not
+   *     include frames for enclosing macros or the BUILD file.
    */
   public static Rule createRule(
       Package.Builder pkgBuilder,
@@ -227,26 +231,28 @@ public boolean isExplicitlySpecified(Map.Entry<String, Object> attributeAccessor
    * (and which are not also within a symbolic macro). Its value is the name argument of the
    * top-level macro on the call stack, if its value can be determined statically (see {@link
    * PackageFactory#checkBuildSyntax}), or just the name of the target otherwise.
+   *
+   * @param callstack the stack of the Starlark thread where the rule is created. In the case of
+   *     rules instantiated by a symbolic macro, this is the inner macro's stack, and does not
+   *     include frames for enclosing macros or the BUILD file.
    */
-  // TODO: #19922 - Should we set generator_name on targets created by a symbolic macro instantiated
-  // within a legacy macro? Otherwise tooling may think those targets were not created in a macro.
   @Nullable
   private static String getGeneratorName(
       Package.Builder pkgBuilder,
       BuildLangTypedAttributeValuesMap args,
-      ImmutableList<CallStackEntry> stack) {
-    // The "generator" of a rule is the function outermost in the call stack (regardless of whether
-    // or not it was passed a "name" parameter). For rules with generators, the stack must contain
-    // at least two entries:
-    //   0: the outermost function (e.g. a BUILD file),
-    //   1: the function called by it (e.g. a "macro" in a .bzl file).
-    // optionally followed by other Starlark or built-in functions, and finally the rule
-    // instantiation function.
-    if (stack.size() < 2 || !stack.get(1).location.file().endsWith(".bzl")) {
-      // Not instantiated by a legacy macro.
-      // TODO: #19922 - This stack inspection logic doesn't work for symbolic macros, where it will
-      // likely incorrectly discriminate between targets created in the implementation function
-      // directly and targets created in a helper function called from the implementation function.
+      ImmutableList<CallStackEntry> callstack) {
+    @Nullable MacroInstance macro = pkgBuilder.currentMacro();
+    // The "generator" of a rule is the function outermost in the BUILD file thread's call stack
+    // (regardless of whether or not it was passed a "name" parameter). For rules with generators,
+    // the BUILD file thread's call stack must contain at least two entries:
+    //   0: the outermost function (BUILD file top level),
+    //   1: the function called by it (e.g. a macro in a .bzl file),
+    // optionally followed by other Starlark or built-in functions, and finally - if the rule is
+    // instantiated in the BUILD file thread - the rule instantiation function.
+    if (macro == null
+        && (callstack.size() < 2 || !callstack.get(1).location.file().endsWith(".bzl"))) {
+      // We are in a BUILD file thread, and the rule is being instantiated directly at the top
+      // level of the BUILD file.
       // TODO(bazel-team): Tolerate ".scl" extension in the above if? An .scl file can instantiate a
       // rule if the rule function is passed as an argument.
       return null;
@@ -258,7 +264,9 @@ private static String getGeneratorName(
       return null;
     }
 
-    String generatorName = pkgBuilder.getGeneratorNameByLocation(stack.get(0).location);
+    String generatorName =
+        pkgBuilder.getGeneratorNameByLocation(
+            macro != null ? macro.getBuildFileLocation() : callstack.get(0).location);
     if (generatorName == null) {
       // Fall back on target name (meh).
       generatorName = (String) args.getAttributeValue("name");

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

@@ -364,13 +364,27 @@ public MacroInstance currentMacro() {
    *
    * <p>The created {@link Rule} will have no output files and therefore will be in an invalid
    * state.
+   *
+   * @param threadCallStack the call stack of the thread that created the rule. Call stacks for
+   *     threads of enclosing symbolic macros (if any) will be prepended to it automatically to form
+   *     the rule's full call stack.
    */
-  Rule createRule(Label label, RuleClass ruleClass, List<StarlarkThread.CallStackEntry> callstack) {
-    return createRule(
-        label,
-        ruleClass,
-        callstack.isEmpty() ? Location.BUILTIN : callstack.get(0).location,
-        CallStack.compactInterior(callstack));
+  Rule createRule(
+      Label label, RuleClass ruleClass, List<StarlarkThread.CallStackEntry> threadCallStack) {
+    CallStack.Node fullInteriorCallStack;
+    final Location location;
+    if (currentMacro() != null) {
+      location = currentMacro().getBuildFileLocation();
+      fullInteriorCallStack = CallStack.compact(threadCallStack, /* start= */ 0);
+      for (MacroInstance macro = currentMacro(); macro != null; macro = macro.getParent()) {
+        fullInteriorCallStack =
+            CallStack.concatenate(macro.getParentCallStack(), fullInteriorCallStack);
+      }
+    } else {
+      location = threadCallStack.isEmpty() ? Location.BUILTIN : threadCallStack.get(0).location;
+      fullInteriorCallStack = CallStack.compact(threadCallStack, /* start= */ 1);
+    }
+    return createRule(label, ruleClass, location, fullInteriorCallStack);
   }
 
   Rule createRule(
@@ -386,10 +400,23 @@ Rule createRule(
    * Package} associated with this {@link Builder}.
    */
   MacroInstance createMacro(
-      MacroClass macroClass, Map<String, Object> attrValues, int sameNameDepth)
+      MacroClass macroClass,
+      Map<String, Object> attrValues,
+      int sameNameDepth,
+      List<StarlarkThread.CallStackEntry> parentCallStack)
       throws EvalException {
     MacroInstance parent = currentMacro();
-    return new MacroInstance(pkg, parent, macroClass, attrValues, sameNameDepth);
+    final Location location;
+    final CallStack.Node compactParentCallStack;
+    if (parent != null) {
+      location = parent.getBuildFileLocation();
+      compactParentCallStack = CallStack.compact(parentCallStack, /* start= */ 0);
+    } else {
+      location = parentCallStack.isEmpty() ? Location.BUILTIN : parentCallStack.get(0).location;
+      compactParentCallStack = CallStack.compact(parentCallStack, /* start= */ 1);
+    }
+    return new MacroInstance(
+        pkg, parent, location, compactParentCallStack, macroClass, attrValues, sameNameDepth);
   }
 
   @Nullable

src/test/java/com/google/devtools/build/lib/analysis/BUILD

@@ -375,6 +375,8 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib/analysis:test/analysis_failure_info",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/packages",
+        "//src/main/java/net/starlark/java/eval",
+        "//src/main/java/net/starlark/java/syntax",
         "//src/test/java/com/google/devtools/build/lib/analysis/util",
         "//third_party:error_prone_annotations",
         "//third_party:guava",