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/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
