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
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
index c35318f..db6c14f 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
+++ b/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 @@
}
@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 @@
}
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 @@
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
diff --git a/src/main/java/com/google/devtools/build/lib/packages/BuildType.java b/src/main/java/com/google/devtools/build/lib/packages/BuildType.java
index c56822a..ded7478 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/BuildType.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/BuildType.java
@@ -266,6 +266,9 @@
/**
* 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 @@
* 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 @@
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);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java b/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java
index d70e2b8..fd1c76f 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java
+++ b/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 @@
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,13 +97,126 @@
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;
+ }
+
+ /**
* Executes a symbolic macro's implementation function, in a new Starlark thread, mutating the
* given package under construction.
*/
@@ -108,11 +248,11 @@
// 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()
diff --git a/src/main/java/com/google/devtools/build/lib/packages/MacroInstance.java b/src/main/java/com/google/devtools/build/lib/packages/MacroInstance.java
index ab92f6c..40b682f 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/MacroInstance.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/MacroInstance.java
@@ -14,6 +14,10 @@
package com.google.devtools.build.lib.packages;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
+
/**
* Represents a use of a symbolic macro in a package.
*
@@ -25,11 +29,14 @@
public final class MacroInstance {
private final MacroClass macroClass;
- private final String name;
+ // TODO(#19922): Consider switching to more optimized, indexed representation, as in Rule.
+ // Order isn't guaranteed, sort before dumping.
+ private final ImmutableMap<String, Object> attrValues;
- public MacroInstance(MacroClass macroClass, String name) {
+ public MacroInstance(MacroClass macroClass, Map<String, Object> attrValues) {
this.macroClass = macroClass;
- this.name = name;
+ this.attrValues = ImmutableMap.copyOf(attrValues);
+ Preconditions.checkArgument(macroClass.getAttributes().keySet().equals(attrValues.keySet()));
}
/** Returns the {@link MacroClass} (i.e. schema info) that this instance parameterizes. */
@@ -42,6 +49,17 @@
* BUILD file or macro.
*/
public String getName() {
- return name;
+ // Type enforced by RuleClass.NAME_ATTRIBUTE.
+ return (String) Preconditions.checkNotNull(attrValues.get("name"));
+ }
+
+ /**
+ * Dictionary of attributes for this instance.
+ *
+ * <p>Contains all attributes, as seen after processing by {@link
+ * MacroClass#instantiateAndAddMacro}.
+ */
+ public ImmutableMap<String, Object> getAttrValues() {
+ return attrValues;
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
index f263e8f..caa6ef44 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -122,8 +122,8 @@
@Immutable
public class RuleClass implements RuleClassData {
- /** The name attribute, present for all rules at index 0. */
- static final Attribute NAME_ATTRIBUTE =
+ /** The name attribute, present for all rules at index 0. Also defined for all symbolic macros. */
+ public static final Attribute NAME_ATTRIBUTE =
attr("name", STRING_NO_INTERN)
.nonconfigurable("All rules have a non-customizable \"name\" attribute")
.mandatory()
diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java
index 509333b..5f65f20 100644
--- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java
+++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java
@@ -202,6 +202,33 @@
documented = false // TODO(#19922): Document
),
@Param(
+ name = "attrs",
+ allowedTypes = {
+ @ParamType(type = Dict.class),
+ },
+ named = true,
+ positional = false,
+ defaultValue = "{}",
+ doc =
+ """
+A dictionary of the attributes this macro supports, analogous to <a href="#rule.attrs">rule.attrs
+</a>. Keys are attribute names, and values are attribute objects like <code>attr.label_list(...)
+</code> (see the <a href=\"../toplevel/attr.html\">attr</a> module).
+
+<p>The special <code>name</code> attribute is predeclared and must not be included in the
+dictionary. There are also reserved attribute names that must not be included:
+<code>visibility</code>, <code>deprecation</code>, <code>tags</code>, <code>testonly</code>, and
+<code>features</code>.
+
+<p>Attributes whose names start with <code>_</code> are private -- they cannot be passed at the call
+site of the rule. Such attributes can be assigned a default value (as in
+<code>attr.label(default="//pkg:foo")</code>) to create an implicit dependency on a label.
+
+<p>To limit memory usage, there is a cap on the number of attributes that may be declared.
+"""),
+ // TODO(#19922): Make good on the above threat of enforcing a cap on the number of
+ // attributes.
+ @Param(
name = "doc",
positional = false,
named = true,
@@ -212,11 +239,11 @@
defaultValue = "None",
doc =
"A description of the macro that can be extracted by documentation generating "
- + "tools."),
- // TODO(#19922): Take attrs dict
+ + "tools.")
},
useStarlarkThread = true)
- StarlarkCallable macro(StarlarkFunction implementation, Object doc, StarlarkThread thread)
+ StarlarkCallable macro(
+ StarlarkFunction implementation, Dict<?, ?> attrs, Object doc, StarlarkThread thread)
throws EvalException;
@StarlarkMethod(
@@ -271,7 +298,8 @@
+ " <code>visibility</code>, <code>deprecation</code>, <code>tags</code>,"
+ " <code>testonly</code>, and <code>features</code> are implicitly added and"
+ " cannot be overridden. Most rules need only a handful of attributes. To"
- + " limit memory usage, the rule function imposes a cap on the size of attrs."),
+ + " limit memory usage, there is a cap on the number of attributes that may be"
+ + " declared."),
// TODO(bazel-team): need to give the types of these builtin attributes
@Param(
name = "outputs",
diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java
index 9850eaf..4294852 100644
--- a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java
+++ b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java
@@ -130,7 +130,7 @@
@Override
public StarlarkCallable macro(
- StarlarkFunction implementation, Object doc, StarlarkThread thread) {
+ StarlarkFunction implementation, Dict<?, ?> attrs, Object doc, StarlarkThread thread) {
// We don't support documenting symbolic macros in legacy Stardoc. Return a dummy.
return new StarlarkCallable() {
@Override
diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
index 25be24b..ced3bf2 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
@@ -538,18 +538,17 @@
Rule rule = createRule(ruleClassA, TEST_RULE_NAME, attributeValues);
// TODO(blaze-team): (2009) refactor to use assertContainsEvent
- Iterator<String> expectedMessages = Arrays.asList(
- "expected value of type 'list(label)' for attribute 'my-labellist-attr' "
- + "in 'ruleA' rule, but got \"foobar\" (string)",
- "no such attribute 'bogus-attr' in 'ruleA' rule",
- "missing value for mandatory "
- + "attribute 'my-string-attr' in 'ruleA' rule",
- "missing value for mandatory attribute 'my-label-attr' in 'ruleA' rule",
- "missing value for mandatory "
- + "attribute 'my-labellist-attr' in 'ruleA' rule",
- "missing value for mandatory "
- + "attribute 'my-string-attr2' in 'ruleA' rule"
- ).iterator();
+ Iterator<String> expectedMessages =
+ Arrays.asList(
+ """
+ expected value of type 'list(label)' for attribute 'my-labellist-attr' \
+ of 'ruleA', but got \"foobar\" (string)""",
+ "no such attribute 'bogus-attr' in 'ruleA' rule",
+ "missing value for mandatory attribute 'my-string-attr' in 'ruleA' rule",
+ "missing value for mandatory attribute 'my-label-attr' in 'ruleA' rule",
+ "missing value for mandatory attribute 'my-labellist-attr' in 'ruleA' rule",
+ "missing value for mandatory attribute 'my-string-attr2' in 'ruleA' rule")
+ .iterator();
for (Event event : collector) {
assertThat(event.getLocation().line()).isEqualTo(TEST_RULE_DEFINED_AT_LINE);
diff --git a/src/test/java/com/google/devtools/build/lib/packages/SymbolicMacroTest.java b/src/test/java/com/google/devtools/build/lib/packages/SymbolicMacroTest.java
index 799b6a2..0d569fd 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/SymbolicMacroTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/SymbolicMacroTest.java
@@ -115,6 +115,7 @@
Package pkg = getPackage("pkg");
assertPackageNotInError(pkg);
+ // TODO(#19922): change naming convention to not use "$""
assertThat(pkg.getTargets()).containsKey("abc$lib");
}
@@ -216,6 +217,246 @@
assertContainsEvent("existing_rules() keys: [\"outer_target\", \"abc$lib\"]");
}
+ @Test
+ public void defaultAttrValue_isUsedWhenNotOverridden() throws Exception {
+ scratch.file(
+ "pkg/foo.bzl",
+ """
+ def _impl(name, xyz):
+ print("xyz is %s" % xyz)
+ my_macro = macro(
+ implementation=_impl,
+ attrs = {
+ "xyz": attr.string(default="DEFAULT")
+ },
+ )
+ """);
+ scratch.file(
+ "pkg/BUILD",
+ """
+ load(":foo.bzl", "my_macro")
+ my_macro(name="abc")
+ """);
+
+ Package pkg = getPackage("pkg");
+ assertPackageNotInError(pkg);
+ assertContainsEvent("xyz is DEFAULT");
+ }
+
+ @Test
+ public void defaultAttrValue_canBeOverridden() throws Exception {
+ scratch.file(
+ "pkg/foo.bzl",
+ """
+ def _impl(name, xyz):
+ print("xyz is %s" % xyz)
+ my_macro = macro(
+ implementation=_impl,
+ attrs = {
+ "xyz": attr.string(default="DEFAULT")
+ },
+ )
+ """);
+ scratch.file(
+ "pkg/BUILD",
+ """
+ load(":foo.bzl", "my_macro")
+ my_macro(
+ name = "abc",
+ xyz = "OVERRIDDEN",
+ )
+ """);
+
+ Package pkg = getPackage("pkg");
+ assertPackageNotInError(pkg);
+ assertContainsEvent("xyz is OVERRIDDEN");
+ }
+
+ @Test
+ public void defaultAttrValue_isUsed_whenAttrIsImplicit() throws Exception {
+ scratch.file(
+ "pkg/foo.bzl",
+ """
+ def _impl(name, _xyz):
+ print("xyz is %s" % _xyz)
+ my_macro = macro(
+ implementation=_impl,
+ attrs = {
+ "_xyz": attr.string(default="IMPLICIT")
+ },
+ )
+ """);
+ scratch.file(
+ "pkg/BUILD",
+ """
+ load(":foo.bzl", "my_macro")
+ my_macro(name="abc")
+ """);
+
+ Package pkg = getPackage("pkg");
+ assertPackageNotInError(pkg);
+ assertContainsEvent("xyz is IMPLICIT");
+ }
+
+ @Test
+ public void noneAttrValue_doesNotOverrideDefault() throws Exception {
+ scratch.file(
+ "pkg/foo.bzl",
+ """
+ def _impl(name, xyz):
+ print("xyz is %s" % xyz)
+ my_macro = macro(
+ implementation=_impl,
+ attrs = {
+ "xyz": attr.string(default="DEFAULT")
+ },
+ )
+ """);
+ scratch.file(
+ "pkg/BUILD",
+ """
+ load(":foo.bzl", "my_macro")
+ my_macro(
+ name = "abc",
+ xyz = None,
+ )
+ """);
+
+ Package pkg = getPackage("pkg");
+ assertPackageNotInError(pkg);
+ assertContainsEvent("xyz is DEFAULT");
+ }
+
+ @Test
+ public void noneAttrValue_doesNotSatisfyMandatoryRequirement() throws Exception {
+ setBuildLanguageOptions("--experimental_enable_first_class_macros");
+
+ scratch.file(
+ "pkg/foo.bzl",
+ """
+ def _impl(name):
+ pass
+ my_macro = macro(
+ implementation = _impl,
+ attrs = {
+ "xyz": attr.string(mandatory=True),
+ },
+ )
+ """);
+ scratch.file(
+ "pkg/BUILD",
+ """
+ load(":foo.bzl", "my_macro")
+ my_macro(
+ name = "abc",
+ xyz = None,
+ )
+ """);
+
+ reporter.removeHandler(failFastHandler);
+ Package pkg = getPackage("pkg");
+ assertThat(pkg).isNotNull();
+ assertThat(pkg.containsErrors()).isTrue();
+ assertContainsEvent("missing value for mandatory attribute 'xyz' in 'my_macro' macro");
+ }
+
+ @Test
+ public void noneAttrValue_disallowedWhenAttrDoesNotExist() throws Exception {
+ setBuildLanguageOptions("--experimental_enable_first_class_macros");
+
+ scratch.file(
+ "pkg/foo.bzl",
+ """
+ def _impl(name):
+ pass
+ my_macro = macro(
+ implementation = _impl,
+ attrs = {
+ "xzz": attr.string(doc="This attr is public"),
+ },
+ )
+ """);
+ scratch.file(
+ "pkg/BUILD",
+ """
+ load(":foo.bzl", "my_macro")
+ my_macro(
+ name = "abc",
+ xyz = None,
+ )
+ """);
+
+ reporter.removeHandler(failFastHandler);
+ Package pkg = getPackage("pkg");
+ assertThat(pkg).isNotNull();
+ assertThat(pkg.containsErrors()).isTrue();
+ assertContainsEvent("no such attribute 'xyz' in 'my_macro' macro (did you mean 'xzz'?)");
+ }
+
+ @Test
+ public void stringAttrsAreConvertedToLabelsAndInRightContext() throws Exception {
+ scratch.file("lib/BUILD");
+ scratch.file(
+ "lib/foo.bzl",
+ """
+ def _impl(name, xyz, _xyz):
+ print("xyz is %s" % xyz)
+ print("_xyz is %s" % _xyz)
+ my_macro = macro(
+ implementation=_impl,
+ attrs = {
+ "xyz": attr.label(),
+ "_xyz": attr.label(default=":BUILD")
+ },
+ )
+ """);
+ scratch.file(
+ "pkg/BUILD",
+ """
+ load("//lib:foo.bzl", "my_macro")
+ my_macro(
+ name = "abc",
+ xyz = ":BUILD", # Should be parsed relative to //pkg, not //lib
+ )
+ """);
+
+ Package pkg = getPackage("pkg");
+ assertPackageNotInError(pkg);
+ assertContainsEvent("xyz is @@//pkg:BUILD");
+ assertContainsEvent("_xyz is @@//lib:BUILD");
+ }
+
+ @Test
+ public void cannotMutateAttrValues() throws Exception {
+ scratch.file(
+ "pkg/foo.bzl",
+ """
+ def _impl(name, xyz):
+ xyz.append(4)
+ my_macro = macro(
+ implementation=_impl,
+ attrs = {
+ "xyz": attr.int_list(),
+ },
+ )
+ """);
+ scratch.file(
+ "pkg/BUILD",
+ """
+ load(":foo.bzl", "my_macro")
+ my_macro(
+ name = "abc",
+ xyz = [1, 2, 3],
+ )
+ """);
+
+ reporter.removeHandler(failFastHandler);
+ Package pkg = getPackage("pkg");
+ assertThat(pkg).isNotNull();
+ assertThat(pkg.containsErrors()).isTrue();
+ assertContainsEvent("Error in append: trying to mutate a frozen list value");
+ }
+
// TODO: #19922 - Add more test cases for interaction between macros and environment_group,
// package_group, implicit/explicit input files, and the package() function. But all of these
// behaviors are about to change (from allowed to prohibited).
diff --git a/src/test/java/com/google/devtools/build/lib/query2/common/QueryPreloadingTest.java b/src/test/java/com/google/devtools/build/lib/query2/common/QueryPreloadingTest.java
index 4a08468..66335ba 100644
--- a/src/test/java/com/google/devtools/build/lib/query2/common/QueryPreloadingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/query2/common/QueryPreloadingTest.java
@@ -354,7 +354,7 @@
assertLabelsVisitedWithErrors(ImmutableSet.of("//foo:x"), ImmutableSet.of("//foo:x"));
assertContainsEvent(
"//foo:x: invalid label '//foo//y' in element 0 of attribute "
- + "'deps' in 'sh_library' rule: invalid package name 'foo//y': "
+ + "'deps' of 'sh_library': invalid package name 'foo//y': "
+ "package names may not contain '//' path separators");
}
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java
index 00bf41d..2ef8bab 100644
--- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java
@@ -74,6 +74,7 @@
import com.google.devtools.build.lib.packages.StructProvider;
import com.google.devtools.build.lib.packages.Type;
import com.google.devtools.build.lib.packages.Types;
+import com.google.devtools.build.lib.rules.cpp.CppConfiguration;
import com.google.devtools.build.lib.starlark.util.BazelEvaluationTestCase;
import com.google.devtools.build.lib.testutil.MoreAsserts;
import com.google.devtools.build.lib.testutil.TestConstants;
@@ -247,6 +248,11 @@
}
}
+ private void assertPackageNotInError(@Nullable Package pkg) {
+ assertThat(pkg).isNotNull();
+ assertThat(pkg.containsErrors()).isFalse();
+ }
+
@Test
public void testSymbolicMacro_failsWithoutFlag() throws Exception {
setBuildLanguageOptions("--experimental_enable_first_class_macros=false");
@@ -372,7 +378,7 @@
Package pkg = getPackage("pkg");
assertThat(pkg).isNotNull();
assertThat(pkg.containsErrors()).isTrue();
- assertContainsEvent("macro requires a `name` attribute");
+ assertContainsEvent("missing value for mandatory attribute 'name' in 'my_macro' macro");
}
@Test
@@ -400,6 +406,199 @@
assertContainsEvent("unexpected positional arguments");
}
+ // TODO(#19922): Migrate away from using "$" as separator in these test cases.
+ @Test
+ public void testSymbolicMacroCanAcceptAttributes() throws Exception {
+ setBuildLanguageOptions("--experimental_enable_first_class_macros");
+
+ scratch.file(
+ "pkg/foo.bzl",
+ """
+ def _impl(name, target_suffix):
+ native.cc_library(name = name + "$" + target_suffix)
+ my_macro = macro(
+ implementation=_impl,
+ attrs = {
+ "target_suffix": attr.string(),
+ },
+ )
+ """);
+ scratch.file(
+ "pkg/BUILD",
+ """
+ load(":foo.bzl", "my_macro")
+ my_macro(
+ name = "abc",
+ target_suffix = "xyz"
+ )
+ """);
+
+ Package pkg = getPackage("pkg");
+ assertPackageNotInError(pkg);
+ assertThat(pkg.getTargets()).containsKey("abc$xyz");
+ }
+
+ @Test
+ public void testSymbolicMacro_rejectsUnknownAttribute() throws Exception {
+ setBuildLanguageOptions("--experimental_enable_first_class_macros");
+
+ scratch.file(
+ "pkg/foo.bzl",
+ """
+ def _impl(name):
+ pass
+ my_macro = macro(
+ implementation = _impl,
+ attrs = {
+ "xzz": attr.string(doc="This attr is public"),
+ },
+ )
+ """);
+ scratch.file(
+ "pkg/BUILD",
+ """
+ load(":foo.bzl", "my_macro")
+ my_macro(
+ name = "abc",
+ xyz = "UNKNOWN",
+ )
+ """);
+
+ reporter.removeHandler(failFastHandler);
+ Package pkg = getPackage("pkg");
+ assertThat(pkg).isNotNull();
+ assertThat(pkg.containsErrors()).isTrue();
+ assertContainsEvent("no such attribute 'xyz' in 'my_macro' macro (did you mean 'xzz'?)");
+ }
+
+ @Test
+ public void testSymbolicMacro_rejectsReservedAttributeName() throws Exception {
+ ev.setSemantics("--experimental_enable_first_class_macros");
+
+ ev.setFailFast(false);
+ evalAndExport(
+ ev,
+ """
+ def _impl(name):
+ pass
+ my_macro = macro(
+ implementation = _impl,
+ attrs = {
+ "visibility": attr.string(),
+ },
+ )
+ """);
+
+ ev.assertContainsError("Cannot declare a macro attribute named 'visibility'");
+ }
+
+ @Test
+ public void testSymbolicMacro_requiresMandatoryAttribute() throws Exception {
+ setBuildLanguageOptions("--experimental_enable_first_class_macros");
+
+ scratch.file(
+ "pkg/foo.bzl",
+ """
+ def _impl(name):
+ pass
+ my_macro = macro(
+ implementation = _impl,
+ attrs = {
+ "xyz": attr.string(mandatory=True),
+ },
+ )
+ """);
+ scratch.file(
+ "pkg/BUILD",
+ """
+ load(":foo.bzl", "my_macro")
+ my_macro(name="abc")
+ """);
+
+ reporter.removeHandler(failFastHandler);
+ Package pkg = getPackage("pkg");
+ assertThat(pkg).isNotNull();
+ assertThat(pkg.containsErrors()).isTrue();
+ assertContainsEvent("missing value for mandatory attribute 'xyz' in 'my_macro' macro");
+ }
+
+ @Test
+ public void testSymbolicMacro_cannotOverrideImplicitAttribute() throws Exception {
+ setBuildLanguageOptions("--experimental_enable_first_class_macros");
+
+ scratch.file(
+ "pkg/foo.bzl",
+ """
+ def _impl(name, _xyz):
+ print("_xyz is %s" % _xyz)
+ my_macro = macro(
+ implementation=_impl,
+ attrs = {
+ "_xyz": attr.string(default="IMPLICIT")
+ },
+ )
+ """);
+ scratch.file(
+ "pkg/BUILD",
+ """
+ load(":foo.bzl", "my_macro")
+ my_macro(
+ name = "abc",
+ _xyz = "CAN'T SET THIS",
+ )
+ """);
+
+ reporter.removeHandler(failFastHandler);
+ Package pkg = getPackage("pkg");
+ assertThat(pkg.containsErrors()).isTrue();
+ assertContainsEvent("cannot set value of implicit attribute '_xyz'");
+ }
+
+ @Test
+ public void testSymbolicMacro_doesNotSupportComputedDefaults() throws Exception {
+ ev.setSemantics("--experimental_enable_first_class_macros");
+
+ ev.checkEvalErrorContains(
+ "In macro attribute 'xyz': Macros do not support computed defaults or late-bound defaults",
+ """
+ def _impl(name, xyz): pass
+ def _computed_default(): return "DEFAULT"
+ my_macro = macro(
+ implementation=_impl,
+ attrs = {
+ "xyz": attr.label(default=_computed_default)
+ },
+ )
+ """);
+ }
+
+ @Test
+ public void testSymbolicMacro_doesNotSupportLateBoundDefaults() throws Exception {
+ // We need to ensure there's a fragment registered on the BazelEvaluationTestCase for
+ // `configuration_field()` to retrieve.
+ //
+ // (Ordinarily we would use the BuildViewTestCase machinery (scratch + getPackage()) and rely on
+ // the analysis mock to register the fragment. But since our expected failure occurs during
+ // .bzl loading, our test machinery doesn't process the error correctly, and instead
+ // getPackage() returns null and no events are emitted.)
+ ev.setFragmentNameToClass(ImmutableMap.of("cpp", CppConfiguration.class));
+
+ ev.setSemantics("--experimental_enable_first_class_macros");
+
+ ev.checkEvalErrorContains(
+ "In macro attribute 'xyz': Macros do not support computed defaults or late-bound defaults",
+ """
+ def _impl(name, xyz): pass
+ _latebound_default = configuration_field(fragment = "cpp", name = "cc_toolchain")
+ my_macro = macro(
+ implementation=_impl,
+ attrs = {
+ "xyz": attr.label(default=_latebound_default)
+ },
+ )
+ """);
+ }
+
@Test
public void testSymbolicMacro_macroFunctionApi() throws Exception {
ev.setSemantics("--experimental_enable_first_class_macros");
@@ -1464,8 +1663,7 @@
AssertionError expected = assertThrows(AssertionError.class, () -> createRuleContext("//p"));
assertThat(expected)
.hasMessageThat()
- .contains(
- "for attribute 'i' in 'r' rule, got 4294967296, want value in signed 32-bit range");
+ .contains("for attribute 'i' of 'r', got 4294967296, want value in signed 32-bit range");
}
@Test
@@ -3417,8 +3615,9 @@
getConfiguredTarget("//initializer_testing:my_target");
ev.assertContainsError(
- "expected value of type 'list(label)' for attribute 'srcs' in 'my_rule' rule, but got"
- + " \"default_files\" (string)");
+ """
+ expected value of type 'list(label)' for attribute 'srcs' of 'my_rule', but got \
+ "default_files" (string)""");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/util/BazelEvaluationTestCase.java b/src/test/java/com/google/devtools/build/lib/starlark/util/BazelEvaluationTestCase.java
index cad1c62..178be3d 100644
--- a/src/test/java/com/google/devtools/build/lib/starlark/util/BazelEvaluationTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/starlark/util/BazelEvaluationTestCase.java
@@ -16,6 +16,7 @@
import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.fail;
+import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.analysis.starlark.StarlarkGlobalsImpl;
@@ -73,6 +74,8 @@
private StarlarkThread thread = null; // created lazily by getStarlarkThread
private Module module = null; // created lazily by getModule
+ private ImmutableMap<String, Class<?>> fragmentNameToClass = ImmutableMap.of();
+
public BazelEvaluationTestCase() {
this(DEFAULT_LABEL);
}
@@ -146,7 +149,7 @@
execAndExport(this.label, lines);
}
- private static void newThread(StarlarkThread thread) {
+ private void newThread(StarlarkThread thread) {
// This StarlarkThread has no PackageContext, so attempts to create a rule will fail.
// Rule creation is tested by StarlarkIntegrationTest.
@@ -159,10 +162,20 @@
/* transitiveDigest= */ new byte[0], // dummy value for tests
TestConstants.TOOLS_REPOSITORY,
/* networkAllowlistForTests= */ Optional.empty(),
- /* fragmentNameToClass= */ ImmutableMap.of())
+ fragmentNameToClass)
.storeInThread(thread);
}
+ /**
+ * Allows for subclasses to inject custom fragments into the environment.
+ *
+ * <p>Must be called prior to any evaluation calls.
+ */
+ public void setFragmentNameToClass(ImmutableMap<String, Class<?>> fragmentNameToClass) {
+ Preconditions.checkState(this.thread == null, "Call this method before getStarlarkThread()");
+ this.fragmentNameToClass = fragmentNameToClass;
+ }
+
private Object newModule(ImmutableMap.Builder<String, Object> predeclared) {
predeclared.putAll(StarlarkGlobalsImpl.INSTANCE.getFixedBzlToplevels());
predeclared.put("platform_common", new PlatformCommon());
