Allow macros to have attributes

This adds an `attrs` param to the `macro()` callable, allowing users to specify an attribute schema for symbolic macros. As for rules, the `name` attribute is implied and should not be included in `attrs`, while certain other names are reserved and can never be defined on macros. Macros support default values and implicit defaults, but not computed defaults or late-bound defaults.

StarlarkRuleClassFunctions.java
- Add attr schema validation to `macro()`.
- Factor instantiation logic from `MacroFunction#call()` to `MacroClass#instantiateAndAddMacro()`.
- Replace ad hoc `name` attr validation with standard RuleClass#NAME_ATTRIBUTE logic.

BuildType.java
- In `copyAndLiftStarlarkValue`, generalize `ruleClass` param to apply to macros as well, and update stringification in `AttributeConversionContext` accordingly.

MacroClass.java
- Add significant new logic to instantiate a macro by processing and validating its attribute values.

BazelEvaluationTestCase.java
- Add ability to inject config fragments into the Starlark environment. Used by one new test case that can't take advantage of `BuildViewTestCase`'s existing fragment setup.

New test cases are added to StarlarkRuleClassFunctionsTest.java and SymbolicMacroTest.java, with the loose rationale that the former is for cases that don't require an implementation function to run and the latter is for everything else. But it may be more sensible to just move all the symbolic macro tests to the latter file in the future.

Work toward #19922.

PiperOrigin-RevId: 626042528
Change-Id: Ie1c09cfdf2ca2168467035b2fa0ccd75cbf68dfd

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

@@ -127,6 +127,7 @@
 
 /** A helper class to provide an easier API for Starlark rule definitions. */
 public class StarlarkRuleClassFunctions implements StarlarkRuleFunctionsApi {
+
   // A cache for base rule classes (especially tests).
   private static final LoadingCache<String, Label> labelCache =
       Caffeine.newBuilder().build(Label::parseCanonical);
@@ -316,7 +317,8 @@ private static void failIf(boolean condition, String message, Object... args)
   }
 
   @Override
-  public StarlarkCallable macro(StarlarkFunction implementation, Object doc, StarlarkThread thread)
+  public StarlarkCallable macro(
+      StarlarkFunction implementation, Dict<?, ?> attrs, Object doc, StarlarkThread thread)
       throws EvalException {
     // Ordinarily we would use StarlarkMethod#enableOnlyWithFlag, but this doesn't work for
     // top-level symbols (due to StarlarkGlobalsImpl relying on the Starlark#addMethods overload
@@ -328,6 +330,26 @@ public StarlarkCallable macro(StarlarkFunction implementation, Object doc, Starl
     }
 
     MacroClass.Builder builder = new MacroClass.Builder(implementation);
+    builder.addAttribute(RuleClass.NAME_ATTRIBUTE);
+    for (Map.Entry<String, Descriptor> descriptorEntry :
+        Dict.cast(attrs, String.class, Descriptor.class, "attrs").entrySet()) {
+      String attrName = descriptorEntry.getKey();
+      Descriptor descriptor = descriptorEntry.getValue();
+
+      if (MacroClass.RESERVED_MACRO_ATTR_NAMES.contains(attrName)) {
+        throw Starlark.errorf("Cannot declare a macro attribute named '%s'", attrName);
+      }
+
+      if (!descriptor.getValueSource().equals(AttributeValueSource.DIRECT)) {
+        throw Starlark.errorf(
+            "In macro attribute '%s': Macros do not support computed defaults or late-bound"
+                + " defaults",
+            attrName);
+      }
+
+      Attribute attr = descriptor.build(attrName);
+      builder.addAttribute(attr);
+    }
     return new MacroFunction(
         builder, Starlark.toJavaOptional(doc, String.class).map(Starlark::trimDocString));
   }
@@ -1084,22 +1106,7 @@ public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> kwarg
         throw Starlark.errorf("unexpected positional arguments");
       }
 
-      Object nameUnchecked = kwargs.get("name");
-      if (nameUnchecked == null) {
-        throw Starlark.errorf("macro requires a `name` attribute");
-      }
-      if (!(nameUnchecked instanceof String instanceName)) {
-        throw Starlark.errorf(
-            "Expected a String for attribute 'name'; got %s",
-            nameUnchecked.getClass().getSimpleName());
-      }
-
-      MacroInstance macroInstance = new MacroInstance(macroClass, instanceName);
-      try {
-        pkgBuilder.addMacro(macroInstance);
-      } catch (NameConflictException e) {
-        throw new EvalException(e);
-      }
+      MacroInstance macroInstance = macroClass.instantiateAndAddMacro(pkgBuilder, kwargs);
 
       // TODO: #19922 - Instead of evaluating macros synchronously with their declaration, evaluate
       // them lazily as the targets they declare are requested. But this would mean that targets

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

@@ -266,6 +266,9 @@ private static <T> Object copyAndLiftSelectorList(
   /**
    * Copies a Starlark value to immutable ones and converts label strings to Label objects.
    *
+   * <p>{@code attrOwner} is the name of the rule or macro on which the attribute is defined, e.g.
+   * "cc_library".
+   *
    * <p>All Starlark values are also type checked.
    *
    * <p>In comparison to {@link #convertFromBuildLangType} unordered attributes are not
@@ -278,7 +281,7 @@ private static <T> Object copyAndLiftSelectorList(
    *     false}.
    */
   public static Object copyAndLiftStarlarkValue(
-      String ruleClass, Attribute attr, Object starlarkValue, LabelConverter labelConverter)
+      String attrOwner, Attribute attr, Object starlarkValue, LabelConverter labelConverter)
       throws ConversionException {
     if (starlarkValue instanceof com.google.devtools.build.lib.packages.SelectorList) {
       if (!attr.isConfigurable()) {
@@ -288,35 +291,41 @@ public static Object copyAndLiftStarlarkValue(
       return copyAndLiftSelectorList(
           attr.getType(),
           (com.google.devtools.build.lib.packages.SelectorList) starlarkValue,
-          new AttributeConversionContext(attr.getName(), ruleClass),
+          new AttributeConversionContext(attr.getName(), attrOwner),
           labelConverter);
     } else {
       return attr.getType()
           .copyAndLiftStarlarkValue(
               starlarkValue,
-              new AttributeConversionContext(attr.getName(), ruleClass),
+              new AttributeConversionContext(attr.getName(), attrOwner),
               labelConverter);
     }
   }
 
   /**
-   * Provides a {@link #toString()} description of the attribute being converted for {@link
-   * BuildType#selectableConvert}. This is preferred over a raw string to avoid uselessly
-   * constructing strings which are never used. A separate class instead of inline to avoid
-   * accidental memory leaks.
+   * A pair of an attribute name and owner, with a toString that includes both.
+   *
+   * <p>This is used to defer stringifying this information until needed for an error message, so as
+   * to avoid generating unnecessary garbage.
    */
   private static class AttributeConversionContext {
     private final String attrName;
-    private final String ruleClass;
+    private final String attrOwner;
 
-    AttributeConversionContext(String attrName, String ruleClass) {
+    /**
+     * Constructs a new context object from a pair of strings.
+     *
+     * @param attrName an attribute name, such as "deps"
+     * @param attrOwner a rule or macro on which the attribute is defined, e.g. "cc_library"
+     */
+    AttributeConversionContext(String attrName, String attrOwner) {
       this.attrName = attrName;
-      this.ruleClass = ruleClass;
+      this.attrOwner = attrOwner;
     }
 
     @Override
     public String toString() {
-      return "attribute '" + attrName + "' in '" + ruleClass + "' rule";
+      return String.format("attribute '%s' of '%s'", attrName, attrOwner);
     }
   }
 

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

@@ -14,12 +14,20 @@
 
 package com.google.devtools.build.lib.packages;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
 import com.google.auto.value.AutoValue;
 import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.packages.TargetDefinitionContext.NameConflictException;
 import com.google.devtools.build.lib.server.FailureDetails.PackageLoading.Code;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
+import java.util.LinkedHashMap;
+import java.util.Map;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.EvalException;
 import net.starlark.java.eval.Mutability;
@@ -28,6 +36,7 @@
 import net.starlark.java.eval.StarlarkSemantics;
 import net.starlark.java.eval.StarlarkThread;
 import net.starlark.java.eval.SymbolGenerator;
+import net.starlark.java.spelling.SpellChecker;
 
 /**
  * Represents a symbolic macro, defined in a .bzl file, that may be instantiated during Package
@@ -38,12 +47,25 @@
  */
 public final class MacroClass {
 
+  /**
+   * Names that users may not pass as keys of the {@code attrs} dict when calling {@code macro()}.
+   *
+   * <p>Of these, {@code name} is special cased as an actual attribute, and the rest do not exist.
+   */
+  // Keep in sync with `macro()`'s `attrs` user documentation in StarlarkRuleFunctionsApi.
+  public static final ImmutableSet<String> RESERVED_MACRO_ATTR_NAMES =
+      ImmutableSet.of("name", "visibility", "deprecation", "tags", "testonly", "features");
+
   private final String name;
   private final StarlarkFunction implementation;
+  // Implicit attributes are stored under their given name ("_foo"), not a mangled name ("$foo").
+  private final ImmutableMap<String, Attribute> attributes;
 
-  public MacroClass(String name, StarlarkFunction implementation) {
+  public MacroClass(
+      String name, StarlarkFunction implementation, ImmutableMap<String, Attribute> attributes) {
     this.name = name;
     this.implementation = implementation;
+    this.attributes = attributes;
   }
 
   /** Returns the macro's exported name. */
@@ -55,10 +77,15 @@ public StarlarkFunction getImplementation() {
     return implementation;
   }
 
+  public ImmutableMap<String, Attribute> getAttributes() {
+    return attributes;
+  }
+
   /** Builder for {@link MacroClass}. */
   public static final class Builder {
-    private final StarlarkFunction implementation;
     @Nullable private String name = null;
+    private final StarlarkFunction implementation;
+    private final ImmutableMap.Builder<String, Attribute> attributes = ImmutableMap.builder();
 
     public Builder(StarlarkFunction implementation) {
       this.implementation = implementation;
@@ -70,10 +97,123 @@ public Builder setName(String name) {
       return this;
     }
 
+    @CanIgnoreReturnValue
+    public Builder addAttribute(Attribute attribute) {
+      attributes.put(attribute.getName(), attribute);
+      return this;
+    }
+
     public MacroClass build() {
       Preconditions.checkNotNull(name);
-      return new MacroClass(name, implementation);
+      return new MacroClass(name, implementation, attributes.buildOrThrow());
+    }
+  }
+
+  /**
+   * Constructs and returns a new {@link MacroInstance} associated with this {@code MacroClass}.
+   *
+   * <p>See {@link #instantiateAndAddMacro}.
+   */
+  // 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)
+      throws EvalException {
+    // A word on edge cases:
+    //   - If an attr is implicit but does not have a default specified, its value is just the
+    //     default value for its attr type (e.g. `[]` for `attr.label_list()`).
+    //   - If an attr is implicit but also mandatory, it's impossible to instantiate it without
+    //     error.
+    //   - If an attr is mandatory but also has a default, the default is meaningless.
+    // These behaviors align with rule attributes.
+
+    LinkedHashMap<String, Object> attrValues = new LinkedHashMap<>();
+
+    // For each given attr value, validate that the attr exists and can be set.
+    for (Map.Entry<String, Object> entry : kwargs.entrySet()) {
+      String attrName = entry.getKey();
+      Object value = entry.getValue();
+      Attribute attr = attributes.get(attrName);
+
+      // Check for unknown attr.
+      if (attr == null) {
+        throw Starlark.errorf(
+            "no such attribute '%s' in '%s' macro%s",
+            attrName,
+            name,
+            SpellChecker.didYouMean(
+                attrName,
+                attributes.values().stream()
+                    .filter(Attribute::isDocumented)
+                    .map(Attribute::getName)
+                    .collect(toImmutableList())));
+      }
+
+      // Setting an attr to None is the same as omitting it (except that it's still an error to set
+      // an unknown attr to None).
+      if (value == Starlark.NONE) {
+        continue;
+      }
+
+      // Can't set implicit default.
+      // (We don't check Attribute#isImplicit() because that assumes "_" -> "$" prefix mangling.)
+      if (attr.getName().startsWith("_")) {
+        throw Starlark.errorf("cannot set value of implicit attribute '%s'", attr.getName());
+      }
+
+      attrValues.put(attrName, value);
+    }
+
+    // Populate defaults for the rest, and validate that no mandatory attr was missed.
+    for (Attribute attr : attributes.values()) {
+      if (attrValues.containsKey(attr.getName())) {
+        continue;
+      }
+      if (attr.isMandatory()) {
+        throw Starlark.errorf(
+            "missing value for mandatory attribute '%s' in '%s' macro", attr.getName(), name);
+      } else {
+        // Already validated at schema creation time that the default is not a computed default or
+        // late-bound default
+        attrValues.put(attr.getName(), attr.getDefaultValueUnchecked());
+      }
+    }
+
+    // Normalize and validate all attr values. (E.g., convert strings to labels, fail if bool was
+    // passed instead of label, ensure values are immutable.)
+    for (Map.Entry<String, Object> entry : ImmutableMap.copyOf(attrValues).entrySet()) {
+      String attrName = entry.getKey();
+      Object normalizedValue =
+          // copyAndLiftStarlarkValue ensures immutability.
+          BuildType.copyAndLiftStarlarkValue(
+              name, attributes.get(attrName), entry.getValue(), pkgBuilder.getLabelConverter());
+      // TODO(#19922): Validate that LABEL_LIST type attributes don't contain duplicates, to match
+      // the behavior of rules. This probably requires factoring out logic from
+      // AggregatingAttributeMapper.
+      // TODO(#19922): select() promotion here
+      attrValues.put(attrName, normalizedValue);
+    }
+
+    return new MacroInstance(this, attrValues);
+  }
+
+  /**
+   * Constructs a new {@link MacroInstance} associated with this {@code MacroClass}, adds it to the
+   * package, and returns it.
+   *
+   * @param pkgBuilder The builder corresponding to the package in which this instance will live.
+   * @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).
+   */
+  public MacroInstance instantiateAndAddMacro(
+      Package.Builder pkgBuilder, Map<String, Object> kwargs) throws EvalException {
+    MacroInstance macroInstance = instantiateMacro(pkgBuilder, kwargs);
+    try {
+      pkgBuilder.addMacro(macroInstance);
+    } catch (NameConflictException e) {
+      throw new EvalException(e);
     }
+    return macroInstance;
   }
 
   /**
@@ -108,11 +248,11 @@ public static void executeMacroImplementation(
       // ConfiguredRuleClassProvider. For instance, we could put it in the builder.
 
       try {
-        Starlark.fastcall(
+        Starlark.call(
             thread,
             macro.getMacroClass().getImplementation(),
-            /* positional= */ new Object[] {},
-            /* named= */ new Object[] {"name", macro.getName()});
+            /* args= */ ImmutableList.of(),
+            /* kwargs= */ macro.getAttrValues());
       } catch (EvalException ex) {
         builder
             .getLocalEventHandler()