Fix implementation of ComputedDefault attributes on aspects, not to throw ClassCast exception.
Before the fix as soon as a ComputedDefault attribute was added to an aspect ClassCast exception was thrown.
RELNOTES: None.
PiperOrigin-RevId: 314797027
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
index b81feff..c9612dd 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
@@ -42,6 +42,7 @@
import com.google.devtools.build.lib.packages.AspectClass;
import com.google.devtools.build.lib.packages.AspectDescriptor;
import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.Attribute.ComputedDefault;
import com.google.devtools.build.lib.packages.Attribute.LateBoundDefault;
import com.google.devtools.build.lib.packages.AttributeMap;
import com.google.devtools.build.lib.packages.AttributeTransitionData;
@@ -504,6 +505,9 @@
dependencyKind.getOwningAspect() == null
? attributeMap.get(attribute.getName(), attribute.getType())
: attribute.getDefaultValue(rule);
+ if (attributeValue instanceof ComputedDefault) {
+ attributeValue = ((ComputedDefault) attributeValue).getDefault(attributeMap);
+ }
} else if (attribute.isLateBound()) {
attributeValue =
resolveLateBoundDefault(rule, attributeMap, attribute, ruleConfig, hostConfig);
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkRuleClassFunctions.java
index 4687d90..8087f53 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkRuleClassFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkRuleClassFunctions.java
@@ -47,6 +47,7 @@
import com.google.devtools.build.lib.cmdline.LabelValidator;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.Attribute.StarlarkComputedDefaultTemplate;
import com.google.devtools.build.lib.packages.AttributeContainer;
import com.google.devtools.build.lib.packages.AttributeMap;
import com.google.devtools.build.lib.packages.AttributeValueSource;
@@ -549,6 +550,20 @@
throw new EvalException(
null, String.format("Aspect attribute '%s' has no default value.", starlarkName));
}
+ if (attribute.getDefaultValueUnchecked() instanceof StarlarkComputedDefaultTemplate) {
+ // Attributes specifying dependencies using computed value are currently not supported.
+ // The limitation is in place because:
+ // - blaze query requires that all possible values are knowable without BuildConguration
+ // - aspects can attach to any rule
+ // Current logic in StarlarkComputedDefault is not enough,
+ // however {Conservative,Precise}AspectResolver can probably be improved to make that work.
+ String starlarkName = "_" + nativeName.substring(1);
+ throw new EvalException(
+ null,
+ String.format(
+ "Aspect attribute '%s' (%s) with computed default value is unsupported.",
+ starlarkName, attribute.getType()));
+ }
attributes.add(attribute);
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java b/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java
index abf181e..5aa17b3 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java
@@ -25,6 +25,7 @@
import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
+import com.google.devtools.build.lib.packages.Attribute.ComputedDefault;
import com.google.devtools.build.lib.packages.ConfigurationFragmentPolicy.MissingFragmentPolicy;
import com.google.devtools.build.lib.packages.Type.LabelClass;
import com.google.devtools.build.lib.packages.Type.LabelVisitor;
@@ -370,16 +371,42 @@
/**
* Adds an attribute to the aspect.
*
- * <p>Since aspects do not appear in BUILD files, the attribute must be either implicit
- * (not available in the BUILD file, starting with '$') or late-bound (determined after the
+ * <p>Since aspects do not appear in BUILD files, the attribute must be either implicit (not
+ * available in the BUILD file, starting with '$') or late-bound (determined after the
* configuration is available, starting with ':')
+ *
+ * <p>Aspect definition currently cannot handle {@link ComputedDefault} dependencies (type LABEL
+ * or LABEL_LIST), because all the dependencies are resolved from the aspect definition and the
+ * defining rule.
*/
public Builder add(Attribute attribute) {
- Preconditions.checkArgument(attribute.isImplicit() || attribute.isLateBound()
- || (attribute.getType() == Type.STRING && attribute.checkAllowedValues()),
- "Invalid attribute '%s' (%s)", attribute.getName(), attribute.getType());
- Preconditions.checkArgument(!attributes.containsKey(attribute.getName()),
- "An attribute with the name '%s' already exists.", attribute.getName());
+ Preconditions.checkArgument(
+ attribute.isImplicit()
+ || attribute.isLateBound()
+ || (attribute.getType() == Type.STRING && attribute.checkAllowedValues()),
+ "%s: Invalid attribute '%s' (%s)",
+ aspectClass.getName(),
+ attribute.getName(),
+ attribute.getType());
+
+ // Attributes specifying dependencies using ComputedDefault value are currently not supported.
+ // The limitation is in place because:
+ // - blaze query requires that all possible values are knowable without BuildConguration
+ // - aspects can attach to any rule
+ // Current logic in #forEachLabelDepFromAllAttributesOfAspect is not enough,
+ // however {Conservative,Precise}AspectResolver can probably be improved to make that work.
+ Preconditions.checkArgument(
+ !(attribute.getType().getLabelClass() == LabelClass.DEPENDENCY
+ && (attribute.getDefaultValueUnchecked() instanceof ComputedDefault)),
+ "%s: Invalid attribute '%s' (%s) with computed default dependencies",
+ aspectClass.getName(),
+ attribute.getName(),
+ attribute.getType());
+ Preconditions.checkArgument(
+ !attributes.containsKey(attribute.getName()),
+ "%s: An attribute with the name '%s' already exists.",
+ aspectClass.getName(),
+ attribute.getName());
attributes.put(attribute.getName(), attribute);
return this;
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
index 60164a6..2df81ba 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
@@ -327,6 +327,15 @@
}
@Test
+ public void aspectWithComputedAttribute() throws Exception {
+ setRulesAvailableInTests(TestAspects.BASE_RULE, TestAspects.COMPUTED_ATTRIBUTE_ASPECT_RULE);
+
+ pkg("a", "rule_with_computed_deps_aspect(name='a', foo=[':b'])", "base(name='b')");
+
+ getConfiguredTarget("//a:a");
+ }
+
+ @Test
public void ruleDependencyDeprecationWarningsAbsentDuringAspectEvaluations() throws Exception {
setRulesAvailableInTests(TestAspects.BASE_RULE, TestAspects.ASPECT_REQUIRING_RULE);
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
index d15b30b..709d697 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
@@ -623,6 +623,7 @@
TestAspects.ALL_ATTRIBUTES_WITH_TOOL_ASPECT,
TestAspects.BAR_PROVIDER_ASPECT,
TestAspects.EXTRA_ATTRIBUTE_ASPECT,
+ TestAspects.COMPUTED_ATTRIBUTE_ASPECT,
TestAspects.FOO_PROVIDER_ASPECT,
TestAspects.ASPECT_REQUIRING_PROVIDER_SETS,
TestAspects.WARNING_ASPECT,
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java b/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java
index dec5353..f324777 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java
@@ -17,6 +17,7 @@
import static com.google.devtools.build.lib.packages.BuildType.LABEL;
import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
import static com.google.devtools.build.lib.packages.Type.STRING;
+import static com.google.devtools.build.lib.packages.Type.STRING_LIST;
import com.google.common.base.Function;
import com.google.common.collect.ImmutableCollection;
@@ -45,7 +46,9 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.packages.AspectDefinition;
import com.google.devtools.build.lib.packages.AspectParameters;
+import com.google.devtools.build.lib.packages.Attribute.ComputedDefault;
import com.google.devtools.build.lib.packages.Attribute.LabelListLateBoundDefault;
+import com.google.devtools.build.lib.packages.AttributeMap;
import com.google.devtools.build.lib.packages.BuildType;
import com.google.devtools.build.lib.packages.NativeAspectClass;
import com.google.devtools.build.lib.packages.Rule;
@@ -335,6 +338,29 @@
}
}
+ public static final ComputedAttributeAspect COMPUTED_ATTRIBUTE_ASPECT =
+ new ComputedAttributeAspect();
+ private static final AspectDefinition COMPUTED_ATTRIBUTE_ASPECT_DEFINITION =
+ new AspectDefinition.Builder(COMPUTED_ATTRIBUTE_ASPECT)
+ .add(
+ attr("$default_copts", STRING_LIST)
+ .value(
+ new ComputedDefault() {
+ @Override
+ public Object getDefault(AttributeMap rule) {
+ return rule.getPackageDefaultCopts();
+ }
+ }))
+ .build();
+
+ /** An aspect that defines its own computed default attribute. */
+ public static class ComputedAttributeAspect extends BaseAspect {
+ @Override
+ public AspectDefinition getDefinition(AspectParameters aspectParameters) {
+ return COMPUTED_ATTRIBUTE_ASPECT_DEFINITION;
+ }
+ }
+
public static final AttributeAspect ATTRIBUTE_ASPECT = new AttributeAspect();
private static final AspectDefinition ATTRIBUTE_ASPECT_DEFINITION =
new AspectDefinition.Builder(ATTRIBUTE_ASPECT)
@@ -648,6 +674,17 @@
attr("foo", LABEL_LIST).allowedFileTypes(FileTypeSet.ANY_FILE)
.aspect(EXTRA_ATTRIBUTE_ASPECT));
+ /** A rule that defines an {@link ComputedAttributeAspect} on one of its attributes. */
+ public static final MockRule COMPUTED_ATTRIBUTE_ASPECT_RULE =
+ () ->
+ MockRule.ancestor(BASE_RULE.getClass())
+ .factory(DummyRuleFactory.class)
+ .define(
+ "rule_with_computed_deps_aspect",
+ attr("foo", LABEL_LIST)
+ .allowedFileTypes(FileTypeSet.ANY_FILE)
+ .aspect(COMPUTED_ATTRIBUTE_ASPECT));
+
/**
* A rule that defines an {@link ParametrizedDefinitionAspect} on one of its attributes.
*/
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/StarlarkDefinedAspectsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/StarlarkDefinedAspectsTest.java
index d73eaff..5530aca 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/StarlarkDefinedAspectsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/StarlarkDefinedAspectsTest.java
@@ -1358,6 +1358,60 @@
}
@Test
+ public void aspectParametersConfigurationField() throws Exception {
+ scratch.file(
+ "test/aspect.bzl",
+ "def _impl(target, ctx):",
+ " return struct()",
+ "def _rule_impl(ctx):",
+ " return struct()",
+ "MyAspect = aspect(",
+ " implementation=_impl,",
+ " attrs = { '_my_attr' : attr.label(default=",
+ " configuration_field(fragment='cpp', name = 'cc_toolchain')) },",
+ ")",
+ "my_rule = rule(",
+ " implementation=_rule_impl,",
+ " attrs = { 'deps' : attr.label_list(aspects=[MyAspect]) },",
+ ")");
+ scratch.file("test/BUILD", "load('//test:aspect.bzl', 'my_rule')", "my_rule(name = 'xxx')");
+
+ AnalysisResult result = update(ImmutableList.<String>of(), "//test:xxx");
+ assertThat(result.hasError()).isFalse();
+ }
+
+ @Test
+ public void aspectParameterComputedDefault() throws Exception {
+ scratch.file(
+ "test/aspect.bzl",
+ "def _impl(target, ctx):",
+ " return struct()",
+ "def _rule_impl(ctx):",
+ " return struct()",
+ "def _defattr():",
+ " return Label('//foo/bar:baz')",
+ "MyAspect = aspect(",
+ " implementation=_impl,",
+ " attrs = { '_extra' : attr.label(default = _defattr) }",
+ ")",
+ "my_rule = rule(",
+ " implementation=_rule_impl,",
+ " attrs = { 'deps' : attr.label_list(aspects=[MyAspect]) },",
+ ")");
+ scratch.file("test/BUILD", "load('//test:aspect.bzl', 'my_rule')", "my_rule(name = 'xxx')");
+ reporter.removeHandler(failFastHandler);
+
+ if (keepGoing()) {
+ AnalysisResult result = update("//test:xxx");
+ assertThat(result.hasError()).isTrue();
+ } else {
+ assertThrows(TargetParsingException.class, () -> update("//test:xxx"));
+ }
+ assertContainsEvent(
+ "Aspect attribute '_extra' (label) with computed default value is unsupported.");
+ }
+
+ @Test
public void aspectParametersOptional() throws Exception {
scratch.file(
"test/aspect.bzl",
