Allow aspects to propagate to all attributes.
--
MOS_MIGRATED_REVID=134378592
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 10703ba..895b6be 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
@@ -548,7 +548,7 @@
aspectCandidates.addAll(attribute.getAspects(originalRule));
if (aspect != null) {
for (AspectClass aspectClass :
- aspect.getDefinition().getAttributeAspects().get(attribute.getName())) {
+ aspect.getDefinition().getAttributeAspects(attribute)) {
if (aspectClass.equals(aspect.getAspectClass())) {
aspectCandidates.add(aspect);
} else if (aspectClass instanceof NativeAspectClass) {
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 20c2a6a..85843b8 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
@@ -14,6 +14,8 @@
package com.google.devtools.build.lib.packages;
+import com.google.common.collect.ImmutableCollection;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.ImmutableSet;
@@ -25,14 +27,12 @@
import com.google.devtools.build.lib.packages.ConfigurationFragmentPolicy.MissingFragmentPolicy;
import com.google.devtools.build.lib.syntax.Type;
import com.google.devtools.build.lib.util.Preconditions;
-
import java.util.Collection;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
-
import javax.annotation.Nullable;
/**
@@ -59,14 +59,18 @@
private final ImmutableSet<Class<?>> requiredProviders;
private final ImmutableSet<String> requiredProviderNames;
private final ImmutableMap<String, Attribute> attributes;
- private final ImmutableMultimap<String, AspectClass> attributeAspects;
+ private final PropagationFunction attributeAspects;
@Nullable private final ConfigurationFragmentPolicy configurationFragmentPolicy;
+ private interface PropagationFunction {
+ ImmutableCollection<AspectClass> propagate(Attribute attribute);
+ }
+
private AspectDefinition(
String name,
ImmutableSet<Class<?>> requiredProviders,
ImmutableMap<String, Attribute> attributes,
- ImmutableMultimap<String, AspectClass> attributeAspects,
+ PropagationFunction attributeAspects,
@Nullable ConfigurationFragmentPolicy configurationFragmentPolicy) {
this.name = name;
this.requiredProviders = requiredProviders;
@@ -119,10 +123,10 @@
}
/**
- * Returns the attribute -> set of required aspects map.
+ * Returns the set of required aspects for a given atribute.
*/
- public ImmutableMultimap<String, AspectClass> getAttributeAspects() {
- return attributeAspects;
+ public ImmutableCollection<AspectClass> getAttributeAspects(Attribute attribute) {
+ return attributeAspects.propagate(attribute);
}
/**
@@ -226,6 +230,7 @@
private final Map<String, Attribute> attributes = new LinkedHashMap<>();
private final Set<Class<?>> requiredProviders = new LinkedHashSet<>();
private final Multimap<String, AspectClass> attributeAspects = LinkedHashMultimap.create();
+ private ImmutableCollection<AspectClass> allAttributesAspects = null;
private final ConfigurationFragmentPolicy.Builder configurationFragmentPolicy =
new ConfigurationFragmentPolicy.Builder();
@@ -265,12 +270,23 @@
*/
public final Builder attributeAspect(String attribute, AspectClass aspectClass) {
Preconditions.checkNotNull(attribute);
+ Preconditions.checkState(this.allAttributesAspects == null,
+ "Specify either aspects for all attributes, or for specific attributes, not both");
this.attributeAspects.put(attribute, Preconditions.checkNotNull(aspectClass));
return this;
}
+ public final Builder allAttributesAspect(AspectClass... aspectClasses) {
+ Preconditions.checkState(this.attributeAspects.isEmpty(),
+ "Specify either aspects for all attributes, or for specific attributes, not both");
+ Preconditions.checkState(this.allAttributesAspects == null,
+ "Aspects for all attributes must only be specified once");
+ this.allAttributesAspects = ImmutableList.copyOf(aspectClasses);
+ return this;
+ }
+
/**
* Adds an attribute to the aspect.
*
@@ -361,6 +377,35 @@
return this;
}
+ @Immutable
+ private static final class AllAttributesPropagationFunction implements PropagationFunction {
+ private final ImmutableCollection<AspectClass> aspects;
+
+ private AllAttributesPropagationFunction(ImmutableCollection<AspectClass> aspects) {
+ this.aspects = aspects;
+ }
+
+ @Override
+ public ImmutableCollection<AspectClass> propagate(Attribute attribute) {
+ return aspects;
+ }
+ }
+
+ @Immutable
+ private static final class PerAttributePropagationFunction implements PropagationFunction {
+ ImmutableSetMultimap<String, AspectClass> aspects;
+
+ public PerAttributePropagationFunction(
+ ImmutableSetMultimap<String, AspectClass> aspects) {
+ this.aspects = aspects;
+ }
+
+ @Override
+ public ImmutableCollection<AspectClass> propagate(Attribute attribute) {
+ return aspects.get(attribute.getName());
+ }
+ }
+
/**
* Builds the aspect definition.
*
@@ -368,7 +413,10 @@
*/
public AspectDefinition build() {
return new AspectDefinition(name, ImmutableSet.copyOf(requiredProviders),
- ImmutableMap.copyOf(attributes), ImmutableSetMultimap.copyOf(attributeAspects),
+ ImmutableMap.copyOf(attributes),
+ allAttributesAspects != null
+ ? new AllAttributesPropagationFunction(allAttributesAspects)
+ : new PerAttributePropagationFunction(ImmutableSetMultimap.copyOf(attributeAspects)),
configurationFragmentPolicy.build());
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AspectDefinitionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AspectDefinitionTest.java
index 4bf2c55..667af95 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/AspectDefinitionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/AspectDefinitionTest.java
@@ -31,7 +31,6 @@
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
import com.google.devtools.build.lib.util.FileTypeSet;
-
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
@@ -121,8 +120,30 @@
.attributeAspect("srcs", TEST_ASPECT_CLASS)
.attributeAspect("deps", TEST_ASPECT_CLASS)
.build();
- assertThat(withAspects.getAttributeAspects()).containsEntry("srcs", TEST_ASPECT_CLASS);
- assertThat(withAspects.getAttributeAspects()).containsEntry("deps", TEST_ASPECT_CLASS);
+
+ assertThat(withAspects.getAttributeAspects(createLabelListAttribute("srcs")))
+ .containsExactly(TEST_ASPECT_CLASS);
+ assertThat(withAspects.getAttributeAspects(createLabelListAttribute("deps")))
+ .containsExactly(TEST_ASPECT_CLASS);
+ }
+
+ @Test
+ public void testAttributeAspect_AllAttributes() throws Exception {
+ AspectDefinition withAspects = new AspectDefinition.Builder("attribute_aspect")
+ .allAttributesAspect(TEST_ASPECT_CLASS)
+ .build();
+
+ assertThat(withAspects.getAttributeAspects(createLabelListAttribute("srcs")))
+ .containsExactly(TEST_ASPECT_CLASS);
+ assertThat(withAspects.getAttributeAspects(createLabelListAttribute("deps")))
+ .containsExactly(TEST_ASPECT_CLASS);
+ }
+
+
+ private static Attribute createLabelListAttribute(String name) {
+ return Attribute.attr(name, BuildType.LABEL_LIST)
+ .allowedFileTypes(FileTypeSet.ANY_FILE)
+ .build();
}
@Test
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 9274b75..a2e9d23 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
@@ -425,6 +425,80 @@
a.getProvider(ExtraActionArtifactsProvider.class)
.getTransitiveExtraActionArtifacts();
assertThat(getOnlyElement(extraActionArtifacts).getLabel()).isEqualTo(Label.create("@//a", "b"));
-
}
+
+ @Test
+ public void aspectPropagatesToAllAttributes() throws Exception {
+ setRulesAvailableInTests(new TestAspects.BaseRule(),
+ new TestAspects.SimpleRule(),
+ new TestAspects.AllAttributesAspectRule());
+ pkg("a",
+ "simple(name='a', foo=[':b'], foo1=':c', txt='some text')",
+ "simple(name='b', foo=[], txt='some text')",
+ "simple(name='c', foo=[], txt='more text')",
+ "all_attributes_aspect(name='x', foo=[':a'])");
+
+ ConfiguredTarget a = getConfiguredTarget("//a:x");
+ assertThat(a.getProvider(RuleInfo.class).getData())
+ .containsExactly("aspect //a:a", "aspect //a:b", "aspect //a:c", "rule //a:x");
+ }
+
+ @Test
+ public void aspectPropagatesToAllAttributesImplicit() throws Exception {
+ setRulesAvailableInTests(new TestAspects.BaseRule(),
+ new TestAspects.SimpleRule(),
+ new TestAspects.ImplicitDepRule(),
+ new TestAspects.AllAttributesAspectRule());
+
+ scratch.file(
+ "extra/BUILD",
+ "simple(name ='extra')"
+ );
+ pkg("a",
+ "simple(name='a', foo=[':b'], foo1=':c', txt='some text')",
+ "simple(name='b', foo=[], txt='some text')",
+ "implicit_dep(name='c')",
+ "all_attributes_aspect(name='x', foo=[':a'])");
+ update();
+
+ ConfiguredTarget a = getConfiguredTarget("//a:x");
+ assertThat(a.getProvider(RuleInfo.class).getData())
+ .containsExactly(
+ "aspect //a:a",
+ "aspect //a:b",
+ "aspect //a:c",
+ "aspect //extra:extra",
+ "rule //a:x");
+ }
+
+
+ @Test
+ public void aspectPropagatesToAllAttributesLateBound() throws Exception {
+ setRulesAvailableInTests(new TestAspects.BaseRule(),
+ new TestAspects.SimpleRule(),
+ new TestAspects.LateBoundDepRule(),
+ new TestAspects.AllAttributesAspectRule());
+
+ scratch.file(
+ "extra/BUILD",
+ "simple(name ='extra')"
+ );
+ pkg("a",
+ "simple(name='a', foo=[':b'], foo1=':c', txt='some text')",
+ "simple(name='b', foo=[], txt='some text')",
+ "late_bound_dep(name='c')",
+ "all_attributes_aspect(name='x', foo=[':a'])");
+ useConfiguration("--plugin=//extra:extra");
+ update();
+
+ ConfiguredTarget a = getConfiguredTarget("//a:x");
+ assertThat(a.getProvider(RuleInfo.class).getData())
+ .containsExactly(
+ "aspect //a:a",
+ "aspect //a:b",
+ "aspect //a:c",
+ "aspect //extra:extra",
+ "rule //a:x");
+ }
+
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/DependencyResolverTest.java b/src/test/java/com/google/devtools/build/lib/analysis/DependencyResolverTest.java
index be4988e..d498709 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/DependencyResolverTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/DependencyResolverTest.java
@@ -33,7 +33,6 @@
import com.google.devtools.build.lib.packages.NoSuchThingException;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.util.OrderedSetMultimap;
-
import java.util.List;
import java.util.Set;
import javax.annotation.Nullable;
@@ -162,6 +161,19 @@
}
@Test
+ public void hasAllAttributesAspect() throws Exception {
+ setRulesAvailableInTests(new TestAspects.BaseRule(), new TestAspects.SimpleRule());
+ pkg("a",
+ "simple(name='a', foo=[':b'])",
+ "simple(name='b', foo=[])");
+ OrderedSetMultimap<Attribute, Dependency> map =
+ dependentNodeMap("//a:a", TestAspects.ALL_ATTRIBUTES_ASPECT);
+ assertDep(
+ map, "foo", "//a:b",
+ new AspectDescriptor(TestAspects.ALL_ATTRIBUTES_ASPECT));
+ }
+
+ @Test
public void hasAspectDependencies() throws Exception {
setRulesAvailableInTests(new TestAspects.BaseRule());
pkg("a", "base(name='a')");
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 351ae38..263a795 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
@@ -38,6 +38,7 @@
import com.google.devtools.build.lib.analysis.RunfilesProvider;
import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
import com.google.devtools.build.lib.analysis.TransitiveInfoProvider;
+import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
@@ -47,12 +48,14 @@
import com.google.devtools.build.lib.packages.AspectDefinition;
import com.google.devtools.build.lib.packages.AspectParameters;
import com.google.devtools.build.lib.packages.Attribute.LateBoundLabel;
+import com.google.devtools.build.lib.packages.Attribute.LateBoundLabelList;
import com.google.devtools.build.lib.packages.AttributeMap;
import com.google.devtools.build.lib.packages.NativeAspectClass;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.packages.RuleClass.Builder;
import com.google.devtools.build.lib.rules.RuleConfiguredTargetFactory;
+import com.google.devtools.build.lib.syntax.Type;
import com.google.devtools.build.lib.util.FileTypeSet;
import java.util.List;
@@ -116,12 +119,19 @@
private static NestedSet<String> collectAspectData(String me, RuleContext ruleContext) {
NestedSetBuilder<String> result = new NestedSetBuilder<>(Order.STABLE_ORDER);
result.add(me);
- if (ruleContext.attributes().has("foo", LABEL_LIST)) {
- for (AspectInfo dep : ruleContext.getPrerequisites("foo", Mode.TARGET, AspectInfo.class)) {
- result.addTransitive(dep.getData());
+
+ Iterable<String> attributeNames = ruleContext.attributes().getAttributeNames();
+ for (String attributeName : attributeNames) {
+ Type<?> attributeType = ruleContext.attributes().getAttributeType(attributeName);
+ if (!LABEL.equals(attributeType) && !LABEL_LIST.equals(attributeType)) {
+ continue;
+ }
+ Iterable<AspectInfo> prerequisites = ruleContext
+ .getPrerequisites(attributeName, Mode.DONT_CHECK, AspectInfo.class);
+ for (AspectInfo prerequisite : prerequisites) {
+ result.addTransitive(prerequisite.getData());
}
}
-
return result.build();
}
@@ -212,6 +222,24 @@
.build();
/**
+ * An aspect that propagates along all attributes.
+ */
+ public static class AllAttributesAspect extends BaseAspect {
+
+ @Override
+ public AspectDefinition getDefinition(AspectParameters aspectParameters) {
+ return ALL_ATTRIBUTES_ASPECT_DEFINITION;
+ }
+ }
+ public static final NativeAspectClass ALL_ATTRIBUTES_ASPECT = new AllAttributesAspect();
+ private static final AspectDefinition ALL_ATTRIBUTES_ASPECT_DEFINITION =
+ new AspectDefinition.Builder("all_attributes_aspect")
+ .allAttributesAspect(ALL_ATTRIBUTES_ASPECT)
+ .build();
+
+
+
+ /**
* An aspect that requires aspects on the attributes of rules it attaches to.
*/
public static class AttributeAspect extends BaseAspect {
@@ -525,6 +553,29 @@
}
/**
+ * A rule that defines an {@link AllAttributesAspect} on one of its attributes.
+ */
+ public static class AllAttributesAspectRule implements RuleDefinition {
+
+ @Override
+ public RuleClass build(Builder builder, RuleDefinitionEnvironment environment) {
+ return builder
+ .add(attr("foo", LABEL_LIST).allowedFileTypes(FileTypeSet.ANY_FILE)
+ .aspect(ALL_ATTRIBUTES_ASPECT))
+ .build();
+ }
+
+ @Override
+ public Metadata getMetadata() {
+ return RuleDefinition.Metadata.builder()
+ .name("all_attributes_aspect")
+ .factoryClass(DummyRuleFactory.class)
+ .ancestors(BaseRule.class)
+ .build();
+ }
+ }
+
+ /**
* A rule that defines a {@link WarningAspect} on one of its attributes.
*/
public static class WarningAspectRule implements RuleDefinition {
@@ -578,6 +629,8 @@
public RuleClass build(Builder builder, RuleDefinitionEnvironment environment) {
return builder
.add(attr("foo", LABEL_LIST).allowedFileTypes(FileTypeSet.ANY_FILE))
+ .add(attr("foo1", LABEL).allowedFileTypes(FileTypeSet.ANY_FILE))
+ .add(attr("txt", STRING))
.build();
}
@@ -634,4 +687,56 @@
.build();
}
}
+
+ /**
+ * Rule with an implcit dependency.
+ */
+ public static class ImplicitDepRule implements RuleDefinition {
+ @Override
+ public RuleClass build(Builder builder, RuleDefinitionEnvironment environment) {
+ return builder
+ .add(attr("$dep", LABEL).value(Label.parseAbsoluteUnchecked("//extra:extra")))
+ .build();
+ }
+
+ @Override
+ public Metadata getMetadata() {
+ return RuleDefinition.Metadata.builder()
+ .name("implicit_dep")
+ .factoryClass(DummyRuleFactory.class)
+ .ancestors(BaseRule.class)
+ .build();
+ }
+ }
+
+ /**
+ * Rule with a late-bound dependency.
+ */
+ public static class LateBoundDepRule implements RuleDefinition {
+ private static final LateBoundLabelList<BuildConfiguration> PLUGINS_LABEL_LIST =
+ new LateBoundLabelList<BuildConfiguration>() {
+ @Override
+ public List<Label> resolve(Rule rule, AttributeMap attributes,
+ BuildConfiguration configuration) {
+ return configuration.getPlugins();
+ }
+ };
+
+ @Override
+ public RuleClass build(Builder builder, RuleDefinitionEnvironment environment) {
+ return builder
+ .add(attr(":plugins", LABEL_LIST).value(PLUGINS_LABEL_LIST))
+ .build();
+ }
+
+ @Override
+ public Metadata getMetadata() {
+ return RuleDefinition.Metadata.builder()
+ .name("late_bound_dep")
+ .factoryClass(DummyRuleFactory.class)
+ .ancestors(BaseRule.class)
+ .build();
+ }
+ }
+
}
