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 c35318f1804b63..db6c14f97e35d0 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 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 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 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 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 c56822abbcd044..ded74784dafbff 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 @@ private static Object copyAndLiftSelectorList( /** * Copies a Starlark value to immutable ones and converts label strings to Label objects. * + *

{@code attrOwner} is the name of the rule or macro on which the attribute is defined, e.g. + * "cc_library". + * *

All Starlark values are also type checked. * *

In comparison to {@link #convertFromBuildLangType} unordered attributes are not @@ -278,7 +281,7 @@ private static 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. + * + *

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 d70e2b82af615d..fd1c76f56b2b57 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()}. + * + *

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 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 attributes; - public MacroClass(String name, StarlarkFunction implementation) { + public MacroClass( + String name, StarlarkFunction implementation, ImmutableMap 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 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 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}. + * + *

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 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 attrValues = new LinkedHashMap<>(); + + // For each given attr value, validate that the attr exists and can be set. + for (Map.Entry 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 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 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() 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 ab92f6c401abce..40b682f906ab1a 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 attrValues; - public MacroInstance(MacroClass macroClass, String name) { + public MacroInstance(MacroClass macroClass, Map 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 @@ public MacroClass getMacroClass() { * 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. + * + *

Contains all attributes, as seen after processing by {@link + * MacroClass#instantiateAndAddMacro}. + */ + public ImmutableMap 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 f263e8f317fdc3..caa6ef441f78f5 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 509333b4474460..5f65f2033326bd 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 @@ -201,6 +201,33 @@ Object provider(Object doc, Object fields, Object init, StarlarkThread thread) named = true, 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 rule.attrs +. Keys are attribute names, and values are attribute objects like attr.label_list(...) + (see the attr module). + +

The special name attribute is predeclared and must not be included in the +dictionary. There are also reserved attribute names that must not be included: +visibility, deprecation, tags, testonly, and +features. + +

Attributes whose names start with _ are private -- they cannot be passed at the call +site of the rule. Such attributes can be assigned a default value (as in +attr.label(default="//pkg:foo")) to create an implicit dependency on a label. + +

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, @@ -212,11 +239,11 @@ Object provider(Object doc, Object fields, Object init, StarlarkThread thread) 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 @@ StarlarkCallable macro(StarlarkFunction implementation, Object doc, StarlarkThre + " visibility, deprecation, tags," + " testonly, and features 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 9850eaf67f6cf4..4294852f0923b7 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 @@ public ProviderInfoWrapper forProviderInfo( @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 25be24b9ee2919..ced3bf236d95a8 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 @@ public void testCreateRule() throws Exception { Rule rule = createRule(ruleClassA, TEST_RULE_NAME, attributeValues); // TODO(blaze-team): (2009) refactor to use assertContainsEvent - Iterator 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 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 799b6a29118bb1..0d569fdf81f27b 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 @@ def _impl(name): Package pkg = getPackage("pkg"); assertPackageNotInError(pkg); + // TODO(#19922): change naming convention to not use "$"" assertThat(pkg.getTargets()).containsKey("abc$lib"); } @@ -216,6 +217,246 @@ def query(): 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 4a0846860bf5af..66335bae0bfcbc 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 @@ public void testDoubleSlashInPackageName() throws Exception { 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 00bf41de906f8a..2ef8babbf33dd7 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 Package getPackage(String pkgName) throws InterruptedException { } } + 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 @@ def _impl(name): 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 @@ def _impl(name): 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 @@ def _impl(ctx): 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 @@ def impl(ctx): 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 cad1c62e7ce0d3..178be3d2c51455 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 @@ public final class BazelEvaluationTestCase { private StarlarkThread thread = null; // created lazily by getStarlarkThread private Module module = null; // created lazily by getModule + private ImmutableMap> fragmentNameToClass = ImmutableMap.of(); + public BazelEvaluationTestCase() { this(DEFAULT_LABEL); } @@ -146,7 +149,7 @@ public final void execAndExport(String... lines) throws Exception { 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 @@ private static void newThread(StarlarkThread thread) { /* 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. + * + *

Must be called prior to any evaluation calls. + */ + public void setFragmentNameToClass(ImmutableMap> fragmentNameToClass) { + Preconditions.checkState(this.thread == null, "Call this method before getStarlarkThread()"); + this.fragmentNameToClass = fragmentNameToClass; + } + private Object newModule(ImmutableMap.Builder predeclared) { predeclared.putAll(StarlarkGlobalsImpl.INSTANCE.getFixedBzlToplevels()); predeclared.put("platform_common", new PlatformCommon());