Do not propagate aspect to own attributes when using '*'.
RELNOTES: Do not propagate aspect to its own attributes when using '*'.
--
MOS_MIGRATED_REVID=138194456
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 32b7354..00e7693 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
@@ -238,7 +238,7 @@
|| attribute.isLateBound()) {
return;
}
- depResolver.resolveDep(attribute, label);
+ depResolver.resolveDep(new AttributeAndOwner(attribute), label);
}
});
}
@@ -253,7 +253,8 @@
Label ruleLabel = rule.getLabel();
ConfiguredAttributeMapper attributeMap = depResolver.attributeMap;
ImmutableSet<String> mappedAttributes = ImmutableSet.copyOf(attributeMap.getAttributeNames());
- for (Attribute attribute : depResolver.attributes) {
+ for (AttributeAndOwner attributeAndOwner : depResolver.attributes) {
+ Attribute attribute = attributeAndOwner.attribute;
if (!attribute.isImplicit() || !attribute.getCondition().apply(attributeMap)) {
continue;
}
@@ -265,7 +266,7 @@
if (label != null) {
label = ruleLabel.resolveRepositoryRelative(label);
- depResolver.resolveDep(attribute, label);
+ depResolver.resolveDep(attributeAndOwner, label);
}
} else if (attribute.getType() == BuildType.LABEL_LIST) {
List<Label> labelList;
@@ -278,7 +279,7 @@
labelList = BuildType.LABEL_LIST.cast(attribute.getDefaultValue(rule));
}
for (Label label : labelList) {
- depResolver.resolveDep(attribute, ruleLabel.resolveRepositoryRelative(label));
+ depResolver.resolveDep(attributeAndOwner, ruleLabel.resolveRepositoryRelative(label));
}
}
}
@@ -315,7 +316,8 @@
BuildConfiguration hostConfig)
throws EvalException, InvalidConfigurationException, InterruptedException {
ConfiguredAttributeMapper attributeMap = depResolver.attributeMap;
- for (Attribute attribute : depResolver.attributes) {
+ for (AttributeAndOwner attributeAndOwner : depResolver.attributes) {
+ Attribute attribute = attributeAndOwner.attribute;
if (!attribute.isLateBound() || !attribute.getCondition().apply(attributeMap)) {
continue;
}
@@ -353,7 +355,7 @@
// is because the split already had to be applied to determine the attribute's value.
// This makes the split logic in the normal pipeline redundant and potentially
// incorrect.
- depResolver.resolveDep(attribute, dep, splitConfig);
+ depResolver.resolveDep(attributeAndOwner, dep, splitConfig);
}
}
} else {
@@ -361,7 +363,7 @@
for (Label dep : resolveLateBoundAttribute(depResolver.rule, attribute,
lateBoundDefault.useHostConfiguration() ? hostConfig : ruleConfig, attributeMap)) {
// Process this dep like a normal attribute.
- depResolver.resolveDep(attribute, dep);
+ depResolver.resolveDep(attributeAndOwner, dep);
}
}
}
@@ -459,7 +461,7 @@
}
Attribute attribute = rule.getRuleClassObject().getAttributeByName(attrName);
for (Label label : labels) {
- depResolver.resolveDep(attribute, label);
+ depResolver.resolveDep(new AttributeAndOwner(attribute), label);
}
}
@@ -482,7 +484,7 @@
Map<Attribute, Collection<Label>> m = depLabels.asMap();
for (Map.Entry<Attribute, Collection<Label>> entry : depLabels.asMap().entrySet()) {
for (Label depLabel : entry.getValue()) {
- depResolver.resolveDep(entry.getKey(), depLabel);
+ depResolver.resolveDep(new AttributeAndOwner(entry.getKey()), depLabel);
}
}
return outgoingEdges.values();
@@ -512,12 +514,16 @@
}
private static ImmutableSet<AspectDescriptor> requiredAspects(
- @Nullable Aspect aspect, Attribute attribute, final Target target, Rule originalRule) {
+ @Nullable Aspect aspect,
+ AttributeAndOwner attributeAndOwner,
+ final Target target,
+ Rule originalRule) {
if (!(target instanceof Rule)) {
return ImmutableSet.of();
}
- Iterable<Aspect> aspectCandidates = extractAspectCandidates(aspect, attribute, originalRule);
+ Iterable<Aspect> aspectCandidates =
+ extractAspectCandidates(aspect, attributeAndOwner, originalRule);
RuleClass ruleClass = ((Rule) target).getRuleClassObject();
ImmutableSet.Builder<AspectDescriptor> result = ImmutableSet.builder();
for (Aspect aspectCandidate : aspectCandidates) {
@@ -535,14 +541,15 @@
}
private static Iterable<Aspect> extractAspectCandidates(
- @Nullable Aspect aspect, Attribute attribute, Rule originalRule) {
+ @Nullable Aspect aspect, AttributeAndOwner attributeAndOwner, Rule originalRule) {
// The order of this set will be deterministic. This is necessary because this order eventually
// influences the order in which aspects are merged into the main configured target, which in
// turn influences which aspect takes precedence if two emit the same provider (maybe this
// should be an error)
+ Attribute attribute = attributeAndOwner.attribute;
Set<Aspect> aspectCandidates = new LinkedHashSet<>();
aspectCandidates.addAll(attribute.getAspects(originalRule));
- if (aspect != null) {
+ if (aspect != null && !aspect.getAspectClass().equals(attributeAndOwner.ownerAspect)) {
for (AspectClass aspectClass :
aspect.getDefinition().getAttributeAspects(attribute)) {
if (aspectClass.equals(aspect.getAspectClass())) {
@@ -562,6 +569,25 @@
}
/**
+ * Pair of (attribute, owner aspect if attribute is from an aspect).
+ *
+ * <p>For "plain" rule attributes, this wrapper class will have value (attribute, null).
+ */
+ final class AttributeAndOwner {
+ final Attribute attribute;
+ final @Nullable AspectClass ownerAspect;
+
+ AttributeAndOwner(Attribute attribute) {
+ this(attribute, null);
+ }
+
+ AttributeAndOwner(Attribute attribute, @Nullable AspectClass ownerAspect) {
+ this.attribute = attribute;
+ this.ownerAspect = ownerAspect;
+ }
+ }
+
+ /**
* Supplies the logic for translating <Attribute, Label> pairs for a rule into the
* <Attribute, Dependency> pairs DependencyResolver ultimately returns.
*
@@ -576,7 +602,7 @@
private final ConfiguredAttributeMapper attributeMap;
private final NestedSetBuilder<Label> rootCauses;
private final OrderedSetMultimap<Attribute, Dependency> outgoingEdges;
- private final List<Attribute> attributes;
+ private final List<AttributeAndOwner> attributes;
/**
* Constructs a new dependency resolver for the specified rule context.
@@ -600,36 +626,38 @@
this.attributes = getAttributes(rule, aspect);
}
- /**
- * Returns the attributes that should be visited for this rule/aspect combination.
- */
- private List<Attribute> getAttributes(Rule rule, @Nullable Aspect aspect) {
+ /** Returns the attributes that should be visited for this rule/aspect combination. */
+ private List<AttributeAndOwner> getAttributes(Rule rule, @Nullable Aspect aspect) {
+ ImmutableList.Builder<AttributeAndOwner> result = ImmutableList.builder();
List<Attribute> ruleDefs = rule.getRuleClassObject().getAttributes();
- if (aspect == null) {
- return ruleDefs;
- } else {
- return ImmutableList.<Attribute>builder()
- .addAll(ruleDefs)
- .addAll(aspect.getDefinition().getAttributes().values())
- .build();
+ for (Attribute attribute : ruleDefs) {
+ result.add(new AttributeAndOwner(attribute));
}
+ if (aspect != null) {
+ for (Attribute attribute : aspect.getDefinition().getAttributes().values()) {
+ result.add(new AttributeAndOwner(attribute, aspect.getAspectClass()));
+ }
+ }
+ return result.build();
}
/**
* Resolves the given dep for the given attribute, including determining which configurations to
* apply to it.
*/
- void resolveDep(Attribute attribute, Label depLabel) throws InterruptedException {
+ void resolveDep(AttributeAndOwner attributeAndOwner, Label depLabel)
+ throws InterruptedException {
Target toTarget = getTarget(rule, depLabel, rootCauses);
if (toTarget == null) {
return; // Skip this round: we still need to Skyframe-evaluate the dep's target.
}
BuildConfiguration.TransitionApplier resolver = ruleConfig.getTransitionApplier();
- ruleConfig.evaluateTransition(rule, attribute, toTarget, resolver);
+ ruleConfig.evaluateTransition(rule, attributeAndOwner.attribute, toTarget, resolver);
// An <Attribute, Label> pair can resolve to multiple deps because of split transitions.
for (Dependency dependency :
- resolver.getDependencies(depLabel, requiredAspects(aspect, attribute, toTarget, rule))) {
- outgoingEdges.put(attribute, dependency);
+ resolver.getDependencies(
+ depLabel, requiredAspects(aspect, attributeAndOwner, toTarget, rule))) {
+ outgoingEdges.put(attributeAndOwner.attribute, dependency);
}
}
@@ -640,9 +668,9 @@
* BuildConfiguration#evaluateTransition}). That means attributes passed through here won't obey
* standard rules on which configurations apply to their deps. This should only be done for
* special circumstances that really justify the difference. When in doubt, use {@link
- * #resolveDep(Attribute, Label)}.
+ * #resolveDep(AttributeAndOwner, Label)}.
*/
- void resolveDep(Attribute attribute, Label depLabel, BuildConfiguration config)
+ void resolveDep(AttributeAndOwner attributeAndOwner, Label depLabel, BuildConfiguration config)
throws InterruptedException {
Target toTarget = getTarget(rule, depLabel, rootCauses);
if (toTarget == null) {
@@ -655,7 +683,8 @@
applyNullTransition = true;
}
- ImmutableSet<AspectDescriptor> aspects = requiredAspects(aspect, attribute, toTarget, rule);
+ ImmutableSet<AspectDescriptor> aspects =
+ requiredAspects(aspect, attributeAndOwner, toTarget, rule);
Dependency dep;
if (config.useDynamicConfigurations() && !applyNullTransition) {
// Pass a transition rather than directly feeding the configuration so deps get trimmed.
@@ -665,7 +694,7 @@
dep = Iterables.getOnlyElement(transitionApplier.getDependencies(depLabel, aspects));
}
- outgoingEdges.put(attribute, dep);
+ outgoingEdges.put(attributeAndOwner.attribute, dep);
}
}
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 a2e9d23..acae678 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
@@ -501,4 +501,48 @@
"rule //a:x");
}
+ /**
+ * Ensures an aspect with attr = '*' doesn't try to propagate to its own implicit attributes.
+ * Doing so leads to a dependency cycle.
+ */
+ @Test
+ public void aspectWithAllAttributesDoesNotPropagateToOwnImplicitAttributes() throws Exception {
+ setRulesAvailableInTests(
+ new TestAspects.BaseRule(),
+ new TestAspects.SimpleRule(),
+ new TestAspects.AllAttributesWithToolAspectRule());
+ pkg(
+ "a",
+ "simple(name='tool')",
+ "simple(name='a')",
+ "all_attributes_with_tool_aspect(name='x', foo=[':a'])");
+
+ ConfiguredTarget a = getConfiguredTarget("//a:x");
+ assertThat(a.getProvider(RuleInfo.class).getData())
+ .containsExactly("aspect //a:a", "rule //a:x");
+ }
+
+ /**
+ * Makes sure the aspect *will* propagate to its implicit attributes if there is a "regular"
+ * dependency path to it (i.e. not through its own implicit attributes).
+ */
+ @Test
+ public void aspectWithAllAttributesPropagatesToItsToolIfThereIsPath() throws Exception {
+ setRulesAvailableInTests(
+ new TestAspects.BaseRule(),
+ new TestAspects.SimpleRule(),
+ new TestAspects.AllAttributesWithToolAspectRule());
+ pkg(
+ "a",
+ "simple(name='tool')",
+ "simple(name='a', foo=[':b'], foo1=':c', txt='some text')",
+ "simple(name='b', foo=[], txt='some text')",
+ "simple(name='c', foo=[':tool'], txt='more text')",
+ "all_attributes_with_tool_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", "aspect //a:tool", "rule //a:x");
+ }
}
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 263a795..f6f693e 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
@@ -50,6 +50,7 @@
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.BuildType;
import com.google.devtools.build.lib.packages.NativeAspectClass;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.RuleClass;
@@ -237,7 +238,25 @@
.allAttributesAspect(ALL_ATTRIBUTES_ASPECT)
.build();
+ /** An aspect that propagates along all attributes and has a tool dependency. */
+ public static class AllAttributesWithToolAspect extends BaseAspect {
+ @Override
+ public AspectDefinition getDefinition(AspectParameters aspectParameters) {
+ return ALL_ATTRIBUTES_WITH_TOOL_ASPECT_DEFINITION;
+ }
+ }
+
+ public static final NativeAspectClass ALL_ATTRIBUTES_WITH_TOOL_ASPECT =
+ new AllAttributesWithToolAspect();
+ private static final AspectDefinition ALL_ATTRIBUTES_WITH_TOOL_ASPECT_DEFINITION =
+ new AspectDefinition.Builder("all_attributes_with_tool_aspect")
+ .allAttributesAspect(ALL_ATTRIBUTES_WITH_TOOL_ASPECT)
+ .add(
+ attr("$tool", BuildType.LABEL)
+ .allowedFileTypes(FileTypeSet.ANY_FILE)
+ .value(Label.parseAbsoluteUnchecked("//a:tool")))
+ .build();
/**
* An aspect that requires aspects on the attributes of rules it attaches to.
@@ -575,6 +594,29 @@
}
}
+ /** A rule that defines an {@link AllAttributesWithToolAspect} on one of its attributes. */
+ public static class AllAttributesWithToolAspectRule implements RuleDefinition {
+
+ @Override
+ public RuleClass build(Builder builder, RuleDefinitionEnvironment environment) {
+ return builder
+ .add(
+ attr("foo", LABEL_LIST)
+ .allowedFileTypes(FileTypeSet.ANY_FILE)
+ .aspect(ALL_ATTRIBUTES_WITH_TOOL_ASPECT))
+ .build();
+ }
+
+ @Override
+ public Metadata getMetadata() {
+ return RuleDefinition.Metadata.builder()
+ .name("all_attributes_with_tool_aspect")
+ .factoryClass(DummyRuleFactory.class)
+ .ancestors(BaseRule.class)
+ .build();
+ }
+ }
+
/**
* A rule that defines a {@link WarningAspect} on one of its attributes.
*/
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
index e0eb465..cad9704 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
@@ -1174,6 +1174,57 @@
})).contains("file.xa");
}
+ @Test
+ public void aspectsPropagatingToAllAttributes() throws Exception {
+ scratch.file(
+ "test/aspect.bzl",
+ "def _impl(target, ctx):",
+ " s = set([target.label])",
+ " if hasattr(ctx.rule.attr, 'runtime_deps'):",
+ " for i in ctx.rule.attr.runtime_deps:",
+ " s += i.target_labels",
+ " return struct(target_labels = s)",
+ "",
+ "MyAspect = aspect(",
+ " implementation=_impl,",
+ " attrs = { '_tool' : attr.label(default = Label('//test:tool')) },",
+ " attr_aspects=['*'],",
+ ")");
+ scratch.file(
+ "test/BUILD",
+ "java_library(",
+ " name = 'tool',",
+ ")",
+ "java_library(",
+ " name = 'bar',",
+ " runtime_deps = [':tool'],",
+ ")",
+ "java_library(",
+ " name = 'foo',",
+ " runtime_deps = [':bar'],",
+ ")");
+ AnalysisResult analysisResult =
+ update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:foo");
+ AspectValue aspectValue = analysisResult.getAspects().iterator().next();
+ SkylarkProviders skylarkProviders =
+ aspectValue.getConfiguredAspect().getProvider(SkylarkProviders.class);
+ assertThat(skylarkProviders).isNotNull();
+ Object names = skylarkProviders.getValue("target_labels");
+ assertThat(names).isInstanceOf(SkylarkNestedSet.class);
+ assertThat(
+ transform(
+ (SkylarkNestedSet) names,
+ new Function<Object, String>() {
+ @Nullable
+ @Override
+ public String apply(Object o) {
+ assertThat(o).isInstanceOf(Label.class);
+ return ((Label) o).getName();
+ }
+ }))
+ .containsExactly("foo", "bar", "tool");
+ }
+
@RunWith(JUnit4.class)
public static final class WithKeepGoing extends SkylarkAspectsTest {
@Override
