Remove `values` constraint from command-line aspects parameters
Aspect parameters of type string are required to have `values` constraint to specify its set of allowed values. This CL removes this requirement for command-line aspects and leave it for rule-propagated aspects.
PiperOrigin-RevId: 416246364
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 cd2b5fb..fa7c661 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
@@ -59,6 +59,7 @@
import com.google.devtools.build.lib.events.EventKind;
import com.google.devtools.build.lib.events.StoredEventHandler;
import com.google.devtools.build.lib.packages.AllowlistChecker;
+import com.google.devtools.build.lib.packages.AspectsListBuilder.AspectDetails;
import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.packages.Attribute.StarlarkComputedDefaultTemplate;
import com.google.devtools.build.lib.packages.AttributeValueSource;
@@ -634,15 +635,13 @@
hasDefault = false; // isValueSet() is always true for attr.string.
}
if (!Attribute.isImplicit(nativeName) && !Attribute.isLateBound(nativeName)) {
- if (!attribute.checkAllowedValues() || attribute.getType() != Type.STRING) {
+ if (attribute.getType() != Type.STRING) {
throw Starlark.errorf(
- "Aspect parameter attribute '%s' must have type 'string' and use the 'values'"
- + " restriction.",
- nativeName);
+ "Aspect parameter attribute '%s' must have type 'string'.", nativeName);
}
if (!hasDefault) {
requiredParams.add(nativeName);
- } else {
+ } else if (attribute.checkAllowedValues()) {
PredicateWithMessage<Object> allowed = attribute.getAllowedValues();
Object defaultVal = attribute.getDefaultValue(null);
if (!allowed.apply(defaultVal)) {
@@ -762,20 +761,7 @@
throw new EvalException("Invalid rule class hasn't been exported by a bzl file");
}
- for (Attribute attribute : ruleClass.getAttributes()) {
- // TODO(dslomov): If a Starlark parameter extractor is specified for this aspect, its
- // attributes may not be required.
- for (Map.Entry<String, ImmutableSet<String>> attrRequirements :
- attribute.getRequiredAspectParameters().entrySet()) {
- for (String required : attrRequirements.getValue()) {
- if (!ruleClass.hasAttr(required, Type.STRING)) {
- throw Starlark.errorf(
- "Aspect %s requires rule %s to specify attribute '%s' with type string.",
- attrRequirements.getKey(), ruleClass.getName(), required);
- }
- }
- }
- }
+ validateRulePropagatedAspects(ruleClass);
BuildLangTypedAttributeValuesMap attributeValues =
new BuildLangTypedAttributeValuesMap(kwargs);
@@ -799,6 +785,40 @@
return Starlark.NONE;
}
+ private static void validateRulePropagatedAspects(RuleClass ruleClass) throws EvalException {
+ for (Attribute attribute : ruleClass.getAttributes()) {
+ for (AspectDetails<?> aspect : attribute.getAspectsDetails()) {
+ ImmutableSet<String> requiredAspectParameters = aspect.getRequiredParameters();
+ for (Attribute aspectAttribute : aspect.getAspectAttributes()) {
+ String aspectAttrName = aspectAttribute.getPublicName();
+ Type<?> aspectAttrType = aspectAttribute.getType();
+
+ // When propagated from a rule, explicit aspect attributes must be of type string and
+ // they must have `values` restriction.
+ if (!aspectAttribute.isImplicit() && !aspectAttribute.isLateBound()) {
+ if ((aspectAttrType != Type.STRING && aspectAttrType != Type.INTEGER)
+ || !aspectAttribute.checkAllowedValues()) {
+ throw Starlark.errorf(
+ "Aspect %s: Aspect parameter attribute '%s' must have type 'string'"
+ + " and use the 'values' restriction.",
+ aspect.getName(), aspectAttrName);
+ }
+ }
+
+ // Required aspect parameters must be specified by the rule propagating the aspect with
+ // the same parameter type.
+ if (requiredAspectParameters.contains(aspectAttrName)) {
+ if (!ruleClass.hasAttr(aspectAttrName, aspectAttrType)) {
+ throw Starlark.errorf(
+ "Aspect %s requires rule %s to specify attribute '%s' with type %s.",
+ aspect.getName(), ruleClass.getName(), aspectAttrName, aspectAttrType);
+ }
+ }
+ }
+ }
+ }
+ }
+
/** Export a RuleFunction from a Starlark file with a given name. */
// To avoid losing event information in the case where the rule was defined with an explicit
// name= arg, all events should be created using errorf(). See the comment in rule() above for
diff --git a/src/main/java/com/google/devtools/build/lib/packages/AspectsListBuilder.java b/src/main/java/com/google/devtools/build/lib/packages/AspectsListBuilder.java
index 2863aab..04f7f31 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AspectsListBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AspectsListBuilder.java
@@ -112,7 +112,7 @@
/** Wraps the information necessary to construct an Aspect. */
@VisibleForSerialization
- abstract static class AspectDetails<C extends AspectClass> {
+ public abstract static class AspectDetails<C extends AspectClass> {
final C aspectClass;
final Function<Rule, AspectParameters> parametersExtractor;
final String baseAspectName;
@@ -132,14 +132,18 @@
this.baseAspectName = baseAspectName;
}
- String getName() {
+ public String getName() {
return this.aspectClass.getName();
}
- ImmutableSet<String> getRequiredParameters() {
+ public ImmutableSet<String> getRequiredParameters() {
return ImmutableSet.of();
}
+ public ImmutableList<Attribute> getAspectAttributes() {
+ return ImmutableList.of();
+ }
+
protected abstract Aspect getAspect(Rule rule);
protected abstract Aspect getTopLevelAspect(ImmutableMap<String, String> aspectParameters)
@@ -194,6 +198,11 @@
}
@Override
+ public ImmutableList<Attribute> getAspectAttributes() {
+ return aspect.getAttributes();
+ }
+
+ @Override
public Aspect getAspect(Rule rule) {
AspectParameters params = parametersExtractor.apply(rule);
return Aspect.forStarlark(aspectClass, aspect.getDefinition(params), params);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
index 558ab43..c4a8388 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
@@ -227,14 +227,6 @@
}
}
- public ImmutableMap<String, ImmutableSet<String>> getRequiredAspectParameters() {
- ImmutableMap.Builder<String, ImmutableSet<String>> paramBuilder = ImmutableMap.builder();
- for (AspectDetails<?> aspect : aspects) {
- paramBuilder.put(aspect.getName(), aspect.getRequiredParameters());
- }
- return paramBuilder.build();
- }
-
/**
* Creates a new attribute builder.
*
@@ -1599,7 +1591,7 @@
super(useHostConfiguration, fragmentClass, defaultValue);
}
}
-
+
@AutoCodec.VisibleForSerialization
static class AlwaysNullLateBoundDefault extends SimpleLateBoundDefault<Void, Void> {
@SerializationConstant @AutoCodec.VisibleForSerialization
@@ -2098,6 +2090,10 @@
return result.build();
}
+ public ImmutableList<AspectDetails<?>> getAspectsDetails() {
+ return aspects;
+ }
+
/**
* Returns the default value of this attribute in the context of the specified Rule. For
* attributes with a computed default, i.e. {@code hasComputedDefault()}, {@code rule} must be
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkDefinedAspect.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkDefinedAspect.java
index 279a1c6..df2d628 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkDefinedAspect.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkDefinedAspect.java
@@ -148,7 +148,21 @@
getName(),
attr.getName(),
aspectParams.getAttribute(attr.getName()).size());
- attr = attr.cloneBuilder(Type.STRING).value(value).build(attr.getName());
+
+ if (!attr.checkAllowedValues()) {
+ // The aspect attribute can have no allowed values constraint if the aspect is used from
+ // command-line. However, AspectDefinition.Builder$add requires the existance of allowed
+ // values in all aspects string attributes for both native and starlark aspects.
+ // Therefore, allowedValues list is added here with only the current value of the
+ // attribute.
+ attr =
+ attr.cloneBuilder(Type.STRING)
+ .value(value)
+ .allowedValues(new Attribute.AllowedValueSet(value))
+ .build(attr.getName());
+ } else {
+ attr = attr.cloneBuilder(Type.STRING).value(value).build(attr.getName());
+ }
}
builder.add(attr);
}
@@ -240,14 +254,15 @@
parametersValues.getOrDefault(
parameterName, (String) aspectParameter.getDefaultValue(null));
- PredicateWithMessage<Object> allowedValues = aspectParameter.getAllowedValues();
- if (allowedValues.apply(parameterValue)) {
- builder.addAttribute(parameterName, parameterValue);
- } else {
- throw Starlark.errorf(
- "%s: invalid value in '%s' attribute: %s",
- getName(), parameterName, allowedValues.getErrorReason(parameterValue));
+ if (aspectParameter.checkAllowedValues()) {
+ PredicateWithMessage<Object> allowedValues = aspectParameter.getAllowedValues();
+ if (!allowedValues.apply(parameterValue)) {
+ throw Starlark.errorf(
+ "%s: invalid value in '%s' attribute: %s",
+ getName(), parameterName, allowedValues.getErrorReason(parameterValue));
+ }
}
+ builder.addAttribute(parameterName, parameterValue);
}
return builder.build();
}
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkDefinedAspectsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkDefinedAspectsTest.java
index ef91b34b..086077e 100644
--- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkDefinedAspectsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkDefinedAspectsTest.java
@@ -7216,6 +7216,128 @@
+ " 'p_v2' instead of ''");
}
+ @Test
+ public void testTopLevelAspectsWithParameters_noNeedForAllowedValues() throws Exception {
+ scratch.file(
+ "test/defs.bzl",
+ "def _aspect_a_impl(target, ctx):",
+ " result = ['aspect_a on target {}, p = {}'.",
+ " format(target.label, ctx.attr.p)]",
+ " if ctx.rule.attr.dep:",
+ " result += ctx.rule.attr.dep.aspect_a_result",
+ " return struct(aspect_a_result = result)",
+ "",
+ "aspect_a = aspect(",
+ " implementation = _aspect_a_impl,",
+ " attr_aspects = ['dep'],",
+ " attrs = { 'p' : attr.string(default='val') },",
+ ")",
+ "",
+ "def _my_rule_impl(ctx):",
+ " pass",
+ "my_rule = rule(",
+ " implementation = _my_rule_impl,",
+ " attrs = { 'dep' : attr.label() },",
+ ")");
+ scratch.file(
+ "test/BUILD",
+ "load('//test:defs.bzl', 'my_rule')",
+ "my_rule(",
+ " name = 'main_target',",
+ " dep = ':dep_target_1',",
+ ")",
+ "my_rule(",
+ " name = 'dep_target_1',",
+ " dep = ':dep_target_2',",
+ ")",
+ "my_rule(",
+ " name = 'dep_target_2',",
+ ")");
+ useConfiguration("--experimental_allow_top_level_aspects_parameters");
+
+ AnalysisResult analysisResult =
+ update(
+ ImmutableList.of("//test:defs.bzl%aspect_a"),
+ ImmutableMap.of("p", "p_v"),
+ "//test:main_target");
+
+ ImmutableMap<AspectKey, ConfiguredAspect> configuredAspects = analysisResult.getAspectsMap();
+ ConfiguredAspect aspectA = getConfiguredAspect(configuredAspects, "aspect_a");
+ assertThat(aspectA).isNotNull();
+ StarlarkList<?> aspectAResult = (StarlarkList) aspectA.get("aspect_a_result");
+ assertThat(Starlark.toIterable(aspectAResult))
+ .containsExactly(
+ "aspect_a on target //test:main_target, p = p_v",
+ "aspect_a on target //test:dep_target_1, p = p_v",
+ "aspect_a on target //test:dep_target_2, p = p_v");
+ }
+
+ /**
+ * Aspect parameter has to require set of values only if the aspect is used in a rule attribute.
+ */
+ @Test
+ public void testAttrAspectParameterMissingRequiredValues() throws Exception {
+ scratch.file(
+ "test/defs.bzl",
+ "def _impl(target, ctx):",
+ " pass",
+ "my_aspect = aspect(_impl,",
+ " attrs = { 'param' : attr.string(default = 'c') }",
+ ")",
+ "def _rule_impl(ctx):",
+ " pass",
+ "r1 = rule(_rule_impl, attrs={'dep': attr.label(aspects = [my_aspect])})");
+ scratch.file("test/BUILD", "load('//test:defs.bzl', 'r1')", "r1(name = 'main_target')");
+ reporter.removeHandler(failFastHandler);
+
+ // This call succeeds if "--keep_going" was passed, which it does in the WithKeepGoing test
+ // suite. Otherwise, it fails and throws a TargetParsingException.
+ if (keepGoing()) {
+ AnalysisResult analysisResult = update("//test:main_target");
+ assertThat(analysisResult.hasError()).isTrue();
+ } else {
+ assertThrows(TargetParsingException.class, () -> update("//test:main_target"));
+ }
+ assertContainsEvent(
+ "Aspect //test:defs.bzl%my_aspect: Aspect parameter attribute 'param' must have type"
+ + " 'string' and use the 'values' restriction.");
+ }
+
+ /**
+ * Aspect parameter has to require set of values only if the aspect is used in a rule attribute.
+ */
+ @Test
+ public void testAttrRequiredAspectParameterMissingRequiredValues() throws Exception {
+ scratch.file(
+ "test/defs.bzl",
+ "def _impl(target, ctx):",
+ " pass",
+ "required_aspect = aspect(_impl,",
+ " attrs = { 'p1' : attr.string(default = 'b') }",
+ ")",
+ "my_aspect = aspect(_impl,",
+ " attrs = { 'p2' : attr.string(default = 'c', values = ['c']) },",
+ " requires = [required_aspect],",
+ ")",
+ "def _rule_impl(ctx):",
+ " pass",
+ "r1 = rule(_rule_impl, attrs={'dep': attr.label(aspects = [my_aspect])})");
+ scratch.file("test/BUILD", "load('//test:defs.bzl', 'r1')", "r1(name = 'main_target')");
+ reporter.removeHandler(failFastHandler);
+
+ // This call succeeds if "--keep_going" was passed, which it does in the WithKeepGoing test
+ // suite. Otherwise, it fails and throws a TargetParsingException.
+ if (keepGoing()) {
+ AnalysisResult analysisResult = update("//test:main_target");
+ assertThat(analysisResult.hasError()).isTrue();
+ } else {
+ assertThrows(TargetParsingException.class, () -> update("//test:main_target"));
+ }
+ assertContainsEvent(
+ "Aspect //test:defs.bzl%required_aspect: Aspect parameter attribute 'p1' must have type"
+ + " 'string' and use the 'values' restriction.");
+ }
+
private ConfiguredAspect getConfiguredAspect(
Map<AspectKey, ConfiguredAspect> aspectsMap, String aspectName) {
for (Map.Entry<AspectKey, ConfiguredAspect> entry : aspectsMap.entrySet()) {
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 b2f580f..16edc87 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
@@ -571,22 +571,51 @@
}
@Test
- public void testAspectParameterRequiresValues() throws Exception {
- ev.checkEvalErrorContains(
- "Aspect parameter attribute 'param' must have type 'string' and use the 'values' "
- + "restriction.",
+ public void testAspectParameterWithDefaultValue() throws Exception {
+ evalAndExport(
+ ev,
"def _impl(target, ctx):",
" pass",
"my_aspect = aspect(_impl,",
- " attrs = { 'param' : attr.string(default = 'c') }",
+ " attrs = { 'param' : attr.string(default = 'a', values=['a', 'b']) }",
")");
+ StarlarkDefinedAspect aspect = (StarlarkDefinedAspect) ev.lookup("my_aspect");
+ Attribute attribute = Iterables.getOnlyElement(aspect.getAttributes());
+ assertThat(attribute.getName()).isEqualTo("param");
+ assertThat(((String) attribute.getDefaultValueUnchecked())).isEqualTo("a");
+ }
+
+ @Test
+ public void testAspectParameterBadDefaultValue() throws Exception {
+ ev.checkEvalErrorContains(
+ "Aspect parameter attribute 'param' has a bad default value: has to be"
+ + " one of 'b' instead of 'a'",
+ "def _impl(target, ctx):",
+ " pass",
+ "my_aspect = aspect(_impl,",
+ " attrs = { 'param' : attr.string(default = 'a', values = ['b']) }",
+ ")");
+ }
+
+ @Test
+ public void testAspectParameterNotRequireValues() throws Exception {
+ evalAndExport(
+ ev,
+ "def _impl(target, ctx):",
+ " pass",
+ "my_aspect = aspect(_impl,",
+ " attrs = { 'param' : attr.string(default = 'val') }",
+ ")");
+ StarlarkDefinedAspect aspect = (StarlarkDefinedAspect) ev.lookup("my_aspect");
+ Attribute attribute = Iterables.getOnlyElement(aspect.getAttributes());
+ assertThat(attribute.getName()).isEqualTo("param");
+ assertThat(((String) attribute.getDefaultValueUnchecked())).isEqualTo("val");
}
@Test
public void testAspectParameterBadType() throws Exception {
ev.checkEvalErrorContains(
- "Aspect parameter attribute 'param' must have type 'string' and use the 'values' "
- + "restriction.",
+ "Aspect parameter attribute 'param' must have type 'string'.",
"def _impl(target, ctx):",
" pass",
"my_aspect = aspect(_impl,",
