Aspect propagation should not lose transitively-visible aspects. If aspect a3 sees aspect a2, and aspect a2 sees aspect a1, propagation of the aspect list [a1,a2,a3] should not lose any aspects. RELNOTES: None. PiperOrigin-RevId: 152786900
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java index a329a4c..f5344d1 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
@@ -356,11 +356,13 @@ ConfiguredAspect configuredAspect = aspectFactory .create(associatedTarget, ruleContext, aspect.getParameters()); - validateAdvertisedProviders( - configuredAspect, aspect.getDefinition().getAdvertisedProviders(), - associatedTarget.getTarget(), - env.getEventHandler() - ); + if (configuredAspect != null) { + validateAdvertisedProviders( + configuredAspect, aspect.getDefinition().getAdvertisedProviders(), + associatedTarget.getTarget(), + env.getEventHandler() + ); + } return configuredAspect; }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java index 2873ad1..d00c48e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
@@ -231,15 +231,21 @@ if (!key.getBaseKeys().isEmpty()) { // We transitively collect all required aspects to reduce the number of restarts. // Semantically it is enough to just request key.getBaseKeys(). - ImmutableMap<AspectDescriptor, SkyKey> aspectKeys = getSkyKeysForAspects(key.getBaseKeys()); + ImmutableList.Builder<SkyKey> aspectPathSkyKeysBuilder = ImmutableList.builder(); + ImmutableMap<AspectDescriptor, SkyKey> aspectKeys = + getSkyKeysForAspectsAndCollectAspectPath(key.getBaseKeys(), aspectPathSkyKeysBuilder); Map<SkyKey, SkyValue> values = env.getValues(aspectKeys.values()); if (env.valuesMissing()) { return null; } + ImmutableList<SkyKey> aspectPathSkyKeys = aspectPathSkyKeysBuilder.build(); + for (SkyKey aspectPathSkyKey : aspectPathSkyKeys) { + aspectPathBuilder.add(((AspectValue) values.get(aspectPathSkyKey)).getAspect()); + } try { - associatedTarget = getBaseTargetAndCollectPath( - associatedTarget, key.getBaseKeys(), values, aspectPathBuilder); + associatedTarget = getBaseTarget( + associatedTarget, key.getBaseKeys(), values); } catch (DuplicateException e) { env.getListener().handle( Event.error(associatedTarget.getTarget().getLocation(), e.getMessage())); @@ -331,15 +337,12 @@ * Merges aspects defined by {@code aspectKeys} into the {@code target} using * previously computed {@code values}. * - * Also populates {@code aspectPath}. - * * @return A {@link ConfiguredTarget} that is a result of a merge. * @throws DuplicateException if there is a duplicate provider provided by aspects. */ - private ConfiguredTarget getBaseTargetAndCollectPath(ConfiguredTarget target, + private ConfiguredTarget getBaseTarget(ConfiguredTarget target, ImmutableList<AspectKey> aspectKeys, - Map<SkyKey, SkyValue> values, - ImmutableList.Builder<Aspect> aspectPath) + Map<SkyKey, SkyValue> values) throws DuplicateException { ArrayList<ConfiguredAspect> aspectValues = new ArrayList<>(); for (AspectKey aspectKey : aspectKeys) { @@ -347,7 +350,6 @@ AspectValue aspectValue = (AspectValue) values.get(skyAspectKey); ConfiguredAspect configuredAspect = aspectValue.getConfiguredAspect(); aspectValues.add(configuredAspect); - aspectPath.add(aspectValue.getAspect()); } return MergedConfiguredTarget.of(target, aspectValues); } @@ -355,25 +357,33 @@ /** * Collect all SkyKeys that are needed for a given list of AspectKeys, * including transitive dependencies. + * + * Also collects all propagating aspects in correct order. */ - private ImmutableMap<AspectDescriptor, SkyKey> getSkyKeysForAspects( - ImmutableList<AspectKey> keys) { + private ImmutableMap<AspectDescriptor, SkyKey> getSkyKeysForAspectsAndCollectAspectPath( + ImmutableList<AspectKey> keys, + ImmutableList.Builder<SkyKey> aspectPathBuilder) { HashMap<AspectDescriptor, SkyKey> result = new HashMap<>(); for (AspectKey key : keys) { - buildSkyKeys(key, result); + buildSkyKeys(key, result, aspectPathBuilder); } return ImmutableMap.copyOf(result); } - private void buildSkyKeys(AspectKey key, HashMap<AspectDescriptor, SkyKey> result) { + private void buildSkyKeys(AspectKey key, HashMap<AspectDescriptor, SkyKey> result, + ImmutableList.Builder<SkyKey> aspectPathBuilder) { if (result.containsKey(key.getAspectDescriptor())) { return; } ImmutableList<AspectKey> baseKeys = key.getBaseKeys(); - result.put(key.getAspectDescriptor(), key.getSkyKey()); + SkyKey skyKey = key.getSkyKey(); + result.put(key.getAspectDescriptor(), skyKey); for (AspectKey baseKey : baseKeys) { - buildSkyKeys(baseKey, result); + buildSkyKeys(baseKey, result, aspectPathBuilder); } + // Post-order list of aspect SkyKeys gives the order of propagating aspects: + // the aspect comes after all aspects it transitively sees. + aspectPathBuilder.add(skyKey); } private SkyValue createAliasAspect( Environment env,
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 528fe99..004443b 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
@@ -1982,6 +1982,67 @@ + "(when propagating from //test:r2 to //test:r1 via attribute dep)"); } + /** + * Aspect a3 sees aspect a2, aspect a2 sees aspect a1, but a3 does not see a1. + * All three aspects should still propagate together. + */ + @Test + public void aspectOnAspectOnAspect() throws Exception { + scratch.file( + "test/aspect.bzl", + "p1 = provider()", + "def _a1_impl(target, ctx):", + " return [p1()]", + "a1 = aspect(_a1_impl, attr_aspects = ['dep'], provides = [p1])", + "p2 = provider()", + "def _a2_impl(target, ctx):", + " value = True if p1 in target else False", + " return [p2(has_p1 = value)]", + "a2 = aspect(_a2_impl, attr_aspects = ['dep'],", + " required_aspect_providers = [p1], provides = [p2])", + "p3 = provider()", + "def _a3_impl(target, ctx):", + " list = []", + " if ctx.rule.attr.dep:", + " list = ctx.rule.attr.dep[p3].value", + " my_value = str(target.label) +'=' + str(target[p2].has_p1 if p2 in target else False)", + " return [p3(value = list + [my_value])]", + "a3 = aspect(_a3_impl, attr_aspects = ['dep'],", + " required_aspect_providers = [p2])", + "def _r0_impl(ctx):", + " pass", + "r0 = rule(_r0_impl, attrs = { 'dep' : attr.label()})", + "def _r1_impl(ctx):", + " pass", + "def _r2_impl(ctx):", + " pass", + "r1 = rule(_r1_impl, attrs = { 'dep' : attr.label(aspects = [a1])})", + "r2 = rule(_r2_impl, attrs = { 'dep' : attr.label(aspects = [a2])})" + ); + scratch.file( + "test/BUILD", + "load(':aspect.bzl', 'r0', 'r1', 'r2')", + "r0(name = 'r0_1')", + "r0(name = 'r0_2', dep = ':r0_1')", + "r0(name = 'r0_3', dep = ':r0_2')", + "r1(name = 'r1_1', dep = ':r0_3')", + "r2(name = 'r2_1', dep = ':r1_1')" + ); + + AnalysisResult analysisResult = update(ImmutableList.of("//test:aspect.bzl%a3"), "//test:r2_1"); + SkylarkProviders skylarkProviders = Iterables.getOnlyElement(analysisResult.getAspects()) + .getConfiguredAspect().getProvider(SkylarkProviders.class); + SkylarkKey p3 = new SkylarkKey(Label.parseAbsolute("//test:aspect.bzl"), "p3"); + assertThat((SkylarkList<?>) skylarkProviders.getDeclaredProvider(p3).getValue("value")) + .containsExactly( + "//test:r0_1=True", + "//test:r0_2=True", + "//test:r0_3=True", + "//test:r1_1=False", + "//test:r2_1=False"); + } + + /** SkylarkAspectTest with "keep going" flag */ @RunWith(JUnit4.class) public static final class WithKeepGoing extends SkylarkAspectsTest {