From 6062db14b775360ecd97e43b7bdb9049ac6674a1 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 18 Apr 2024 08:27:27 -0700 Subject: [PATCH] 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 --- .../starlark/StarlarkRuleClassFunctions.java | 41 +-- .../build/lib/packages/BuildType.java | 31 ++- .../build/lib/packages/MacroClass.java | 152 ++++++++++- .../build/lib/packages/MacroInstance.java | 26 +- .../build/lib/packages/RuleClass.java | 4 +- .../StarlarkRuleFunctionsApi.java | 36 ++- .../FakeStarlarkRuleFunctionsApi.java | 2 +- .../build/lib/packages/RuleClassTest.java | 23 +- .../build/lib/packages/SymbolicMacroTest.java | 241 ++++++++++++++++++ .../query2/common/QueryPreloadingTest.java | 2 +- .../StarlarkRuleClassFunctionsTest.java | 209 ++++++++++++++- .../util/BazelEvaluationTestCase.java | 17 +- 12 files changed, 719 insertions(+), 65 deletions(-) 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());