Report inconsistent aspect order error to the user.

--
PiperOrigin-RevId: 148342788
MOS_MIGRATED_REVID=148342788

7 files changed, 198 insertions, 44 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index 015fea9071..77cefb5258 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -35,6 +35,7 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactFactory;
 import com.google.devtools.build.lib.actions.ArtifactOwner;
 import com.google.devtools.build.lib.actions.Root;
+import com.google.devtools.build.lib.analysis.DependencyResolver.InconsistentAspectOrderException;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
@@ -962,7 +963,8 @@ public class BuildView {
   @VisibleForTesting
   public Iterable<ConfiguredTarget> getDirectPrerequisitesForTesting(
       EventHandler eventHandler, ConfiguredTarget ct, BuildConfigurationCollection configurations)
-          throws EvalException, InvalidConfigurationException, InterruptedException {
+      throws EvalException, InvalidConfigurationException,
+      InterruptedException, InconsistentAspectOrderException {
     return skyframeExecutor.getConfiguredTargets(
         eventHandler, ct.getConfiguration(),
         ImmutableSet.copyOf(
@@ -974,7 +976,8 @@ public class BuildView {
   public OrderedSetMultimap<Attribute, Dependency> getDirectPrerequisiteDependenciesForTesting(
       final EventHandler eventHandler, final ConfiguredTarget ct,
       BuildConfigurationCollection configurations)
-      throws EvalException, InvalidConfigurationException, InterruptedException {
+      throws EvalException, InvalidConfigurationException, InterruptedException,
+             InconsistentAspectOrderException {
     if (!(ct.getTarget() instanceof Rule)) {
       return OrderedSetMultimap.create();
     }
@@ -1062,7 +1065,8 @@ public class BuildView {
   private OrderedSetMultimap<Attribute, ConfiguredTarget> getPrerequisiteMapForTesting(
       final EventHandler eventHandler, ConfiguredTarget target,
       BuildConfigurationCollection configurations)
-      throws EvalException, InvalidConfigurationException, InterruptedException {
+      throws EvalException, InvalidConfigurationException,
+             InterruptedException, InconsistentAspectOrderException {
     OrderedSetMultimap<Attribute, Dependency> depNodeNames =
         getDirectPrerequisiteDependenciesForTesting(eventHandler, target, configurations);
 
@@ -1094,7 +1098,8 @@ public class BuildView {
   public RuleContext getRuleContextForTesting(
       ConfiguredTarget target, StoredEventHandler eventHandler,
       BuildConfigurationCollection configurations)
-          throws EvalException, InvalidConfigurationException, InterruptedException {
+      throws EvalException, InvalidConfigurationException, InterruptedException,
+             InconsistentAspectOrderException {
     BuildConfiguration targetConfig = target.getConfiguration();
     CachingAnalysisEnvironment env =
         new CachingAnalysisEnvironment(getArtifactFactory(),
@@ -1111,7 +1116,8 @@ public class BuildView {
   @VisibleForTesting
   public RuleContext getRuleContextForTesting(EventHandler eventHandler, ConfiguredTarget target,
       AnalysisEnvironment env, BuildConfigurationCollection configurations)
-          throws EvalException, InvalidConfigurationException, InterruptedException {
+      throws EvalException, InvalidConfigurationException, InterruptedException,
+             InconsistentAspectOrderException {
     BuildConfiguration targetConfig = target.getConfiguration();
     return new RuleContext.Builder(
             env,
@@ -1139,7 +1145,8 @@ public class BuildView {
   public ConfiguredTarget getPrerequisiteConfiguredTargetForTesting(
       EventHandler eventHandler, ConfiguredTarget dependentTarget, Label desiredTarget,
       BuildConfigurationCollection configurations)
-      throws EvalException, InvalidConfigurationException, InterruptedException {
+      throws EvalException, InvalidConfigurationException, InterruptedException,
+             InconsistentAspectOrderException {
     Collection<ConfiguredTarget> configuredTargets =
         getPrerequisiteMapForTesting(eventHandler, dependentTarget, configurations).values();
     for (ConfiguredTarget ct : configuredTargets) {
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 06d561bf48..440e7b56cd 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
@@ -26,6 +26,7 @@ import com.google.devtools.build.lib.analysis.config.InvalidConfigurationExcepti
 import com.google.devtools.build.lib.analysis.config.PatchTransition;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.packages.Aspect;
 import com.google.devtools.build.lib.packages.AspectClass;
 import com.google.devtools.build.lib.packages.AspectDescriptor;
@@ -91,7 +92,8 @@ public abstract class DependencyResolver {
       BuildConfiguration hostConfig,
       @Nullable Aspect aspect,
       ImmutableMap<Label, ConfigMatchingProvider> configConditions)
-      throws EvalException, InvalidConfigurationException, InterruptedException {
+      throws EvalException, InvalidConfigurationException, InterruptedException,
+             InconsistentAspectOrderException {
     NestedSetBuilder<Label> rootCauses = NestedSetBuilder.<Label>stableOrder();
     OrderedSetMultimap<Attribute, Dependency> outgoingEdges = dependentNodeMap(
         node, hostConfig,
@@ -138,7 +140,8 @@ public abstract class DependencyResolver {
       Iterable<Aspect> aspects,
       ImmutableMap<Label, ConfigMatchingProvider> configConditions,
       NestedSetBuilder<Label> rootCauses)
-      throws EvalException, InvalidConfigurationException, InterruptedException {
+      throws EvalException, InvalidConfigurationException, InterruptedException,
+      InconsistentAspectOrderException {
     Target target = node.getTarget();
     BuildConfiguration config = node.getConfiguration();
     OrderedSetMultimap<Attribute, Dependency> outgoingEdges = OrderedSetMultimap.create();
@@ -158,6 +161,7 @@ public abstract class DependencyResolver {
     } else {
       throw new IllegalStateException(target.getLabel().toString());
     }
+
     return outgoingEdges;
   }
 
@@ -168,7 +172,8 @@ public abstract class DependencyResolver {
       ImmutableMap<Label, ConfigMatchingProvider> configConditions,
       NestedSetBuilder<Label> rootCauses,
       OrderedSetMultimap<Attribute, Dependency> outgoingEdges)
-      throws EvalException, InvalidConfigurationException, InterruptedException {
+      throws EvalException, InvalidConfigurationException, InconsistentAspectOrderException,
+             InterruptedException{
     Preconditions.checkArgument(node.getTarget() instanceof Rule);
     BuildConfiguration ruleConfig = Preconditions.checkNotNull(node.getConfiguration());
     Rule rule = (Rule) node.getTarget();
@@ -188,7 +193,7 @@ public abstract class DependencyResolver {
    * (which require special processing: see {@link #resolveLateBoundAttributes}).
    */
   private void resolveEarlyBoundAttributes(RuleResolver depResolver)
-      throws EvalException, InterruptedException {
+      throws EvalException, InterruptedException, InconsistentAspectOrderException {
     Rule rule = depResolver.rule;
 
     resolveExplicitAttributes(depResolver);
@@ -231,7 +236,11 @@ public abstract class DependencyResolver {
   }
 
   private void resolveExplicitAttributes(final RuleResolver depResolver)
-      throws InterruptedException {
+      throws InterruptedException, InconsistentAspectOrderException {
+
+    // Record error that might happen during label visitation.
+    final InconsistentAspectOrderException[] exception = new InconsistentAspectOrderException[1];
+
     depResolver.attributeMap.visitLabels(
         new AttributeMap.AcceptsLabelAttribute() {
           @Override
@@ -242,13 +251,24 @@ public abstract class DependencyResolver {
                 || attribute.isLateBound()) {
               return;
             }
-            depResolver.resolveDep(new AttributeAndOwner(attribute), label);
+            try {
+              depResolver.resolveDep(new AttributeAndOwner(attribute), label);
+            } catch (InconsistentAspectOrderException e) {
+              if (exception[0] == null) {
+                exception[0] = e;
+              }
+            }
           }
         });
+
+    if (exception[0] != null) {
+      throw exception[0];
+    }
   }
 
   /** Resolves the dependencies for all implicit attributes in this rule. */
-  private void resolveImplicitAttributes(RuleResolver depResolver) throws InterruptedException {
+  private void resolveImplicitAttributes(RuleResolver depResolver)
+      throws InterruptedException, InconsistentAspectOrderException {
     // Since the attributes that come from aspects do not appear in attributeMap, we have to get
     // their values from somewhere else. This incidentally means that aspects attributes are not
     // configurable. It would be nice if that wasn't the case, but we'd have to revamp how
@@ -318,7 +338,8 @@ public abstract class DependencyResolver {
       RuleResolver depResolver,
       BuildConfiguration ruleConfig,
       BuildConfiguration hostConfig)
-      throws EvalException, InvalidConfigurationException, InterruptedException {
+      throws EvalException, InvalidConfigurationException, InconsistentAspectOrderException,
+             InterruptedException{
     ConfiguredAttributeMapper attributeMap = depResolver.attributeMap;
     for (AttributeAndOwner attributeAndOwner : depResolver.attributes) {
       Attribute attribute = attributeAndOwner.attribute;
@@ -459,7 +480,7 @@ public abstract class DependencyResolver {
    * @param labels the dependencies to add
    */
   private void addExplicitDeps(RuleResolver depResolver, String attrName, Iterable<Label> labels)
-      throws InterruptedException {
+      throws InterruptedException, InconsistentAspectOrderException {
     Rule rule = depResolver.rule;
     if (!rule.isAttrDefined(attrName, BuildType.LABEL_LIST)
         && !rule.isAttrDefined(attrName, BuildType.NODEP_LABEL_LIST)) {
@@ -481,7 +502,7 @@ public abstract class DependencyResolver {
       TargetAndConfiguration node,
       OrderedSetMultimap<Attribute, Label> depLabels,
       NestedSetBuilder<Label> rootCauses)
-      throws InterruptedException {
+      throws InterruptedException, InconsistentAspectOrderException {
     Preconditions.checkArgument(node.getTarget() instanceof Rule);
     Rule rule = (Rule) node.getTarget();
     OrderedSetMultimap<Attribute, Dependency> outgoingEdges = OrderedSetMultimap.create();
@@ -521,11 +542,11 @@ public abstract class DependencyResolver {
   }
 
 
-  private static AspectCollection requiredAspects(
+  private AspectCollection requiredAspects(
       Iterable<Aspect> aspectPath,
       AttributeAndOwner attributeAndOwner,
       final Target target,
-      Rule originalRule) {
+      Rule originalRule) throws InconsistentAspectOrderException {
     if (!(target instanceof Rule)) {
       return AspectCollection.EMPTY;
     }
@@ -538,17 +559,17 @@ public abstract class DependencyResolver {
     ImmutableList.Builder<Aspect> filteredAspectPath = ImmutableList.builder();
     ImmutableSet.Builder<AspectDescriptor> visibleAspects = ImmutableSet.builder();
 
-    collectOriginatingAspects(originalRule, attributeAndOwner.attribute, (Rule) target,
+    Attribute attribute = attributeAndOwner.attribute;
+    collectOriginatingAspects(originalRule, attribute, (Rule) target,
         filteredAspectPath, visibleAspects);
 
     collectPropagatingAspects(aspectPath,
-        attributeAndOwner.attribute,
+        attribute,
         (Rule) target, filteredAspectPath, visibleAspects);
     try {
       return AspectCollection.create(filteredAspectPath.build(), visibleAspects.build());
     } catch (AspectCycleOnPathException e) {
-      // TODO(dslomov): propagate the error and report to user.
-      throw new AssertionError(e);
+      throw  new InconsistentAspectOrderException(originalRule, attribute, target, e);
     }
   }
 
@@ -603,19 +624,6 @@ public abstract class DependencyResolver {
     }
   }
 
-  private static Iterable<Aspect> extractAspectCandidates(
-      Iterable<Aspect> aspects,
-      Attribute attribute, Rule originalRule) {
-    ImmutableList.Builder<Aspect> aspectCandidates = ImmutableList.builder();
-    aspectCandidates.addAll(attribute.getAspects(originalRule));
-    for (Aspect aspect : aspects) {
-      if (aspect.getDefinition().propagateAlong(attribute)) {
-        aspectCandidates.add(aspect);
-      }
-    }
-    return aspectCandidates.build();
-  }
-
   /**
    * Pair of (attribute, owner aspect if attribute is from an aspect).
    *
@@ -700,7 +708,7 @@ public abstract class DependencyResolver {
      * apply to it.
      */
     void resolveDep(AttributeAndOwner attributeAndOwner, Label depLabel)
-        throws InterruptedException {
+        throws InterruptedException, InconsistentAspectOrderException {
       Target toTarget = getTarget(rule, depLabel, rootCauses);
       if (toTarget == null) {
         return; // Skip this round: we still need to Skyframe-evaluate the dep's target.
@@ -725,7 +733,7 @@ public abstract class DependencyResolver {
      * #resolveDep(AttributeAndOwner, Label)}.
      */
     void resolveDep(AttributeAndOwner attributeAndOwner, Label depLabel, BuildConfiguration config)
-        throws InterruptedException {
+        throws InterruptedException, InconsistentAspectOrderException {
       Target toTarget = getTarget(rule, depLabel, rootCauses);
       if (toTarget == null) {
         return; // Skip this round: this is either a loading error or unevaluated Skyframe dep.
@@ -852,4 +860,27 @@ public abstract class DependencyResolver {
       Set<Class<? extends BuildConfiguration.Fragment>> fragments,
       Iterable<BuildOptions> buildOptions)
       throws InvalidConfigurationException, InterruptedException;
+
+  /**
+   * Signals an inconsistency on aspect path: an aspect occurs twice on the path and
+   * the second occurrence sees a different set of aspects.
+   *
+   * {@see AspectCycleOnPathException}
+   */
+  public class InconsistentAspectOrderException extends Exception {
+    private final Location location;
+    public InconsistentAspectOrderException(Rule originalRule, Attribute attribute, Target target,
+        AspectCycleOnPathException e) {
+      super(String.format("%s (when propagating from %s to %s via attribute %s)",
+          e.getMessage(),
+          originalRule.getLabel(),
+          target.getLabel(),
+          attribute.getName()));
+      this.location = originalRule.getLocation();
+    }
+
+    public Location getLocation() {
+      return location;
+    }
+  }
 }
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 3d5aa88e92..71f691255b 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
@@ -21,6 +21,7 @@ import com.google.devtools.build.lib.analysis.CachingAnalysisEnvironment;
 import com.google.devtools.build.lib.analysis.ConfiguredAspect;
 import com.google.devtools.build.lib.analysis.ConfiguredAspectFactory;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.DependencyResolver.InconsistentAspectOrderException;
 import com.google.devtools.build.lib.analysis.MergedConfiguredTarget;
 import com.google.devtools.build.lib.analysis.MergedConfiguredTarget.DuplicateException;
 import com.google.devtools.build.lib.analysis.TargetAndConfiguration;
@@ -304,6 +305,10 @@ public final class AspectFunction implements SkyFunction {
         ConfiguredValueCreationException cause = (ConfiguredValueCreationException) e.getCause();
         throw new AspectFunctionException(new AspectCreationException(
             cause.getMessage(), cause.getAnalysisRootCause()));
+      } else if (e.getCause() instanceof InconsistentAspectOrderException) {
+        InconsistentAspectOrderException cause = (InconsistentAspectOrderException) e.getCause();
+        throw new AspectFunctionException(new AspectCreationException(
+            cause.getMessage()));
       } else {
         // Cast to InvalidConfigurationException as a consistency check. If you add any
         // DependencyEvaluationException constructors, you may need to change this code, too.
@@ -519,5 +524,9 @@ public final class AspectFunction implements SkyFunction {
     public AspectFunctionException(AspectCreationException e) {
       super(e, Transience.PERSISTENT);
     }
+
+    public AspectFunctionException(InconsistentAspectOrderException cause) {
+      super(cause, Transience.PERSISTENT);
+    }
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
index 673a18fc40..c19e3c0c31 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
@@ -37,6 +37,7 @@ import com.google.devtools.build.lib.analysis.CachingAnalysisEnvironment;
 import com.google.devtools.build.lib.analysis.ConfiguredAspect;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.Dependency;
+import com.google.devtools.build.lib.analysis.DependencyResolver.InconsistentAspectOrderException;
 import com.google.devtools.build.lib.analysis.LabelAndConfiguration;
 import com.google.devtools.build.lib.analysis.MergedConfiguredTarget;
 import com.google.devtools.build.lib.analysis.MergedConfiguredTarget.DuplicateException;
@@ -122,6 +123,10 @@ final class ConfiguredTargetFunction implements SkyFunction {
       super(cause);
     }
 
+    public DependencyEvaluationException(InconsistentAspectOrderException cause) {
+      super(cause);
+    }
+
     @Override
     public synchronized Exception getCause() {
       return (Exception) super.getCause();
@@ -258,6 +263,10 @@ final class ConfiguredTargetFunction implements SkyFunction {
       if (e.getCause() instanceof ConfiguredValueCreationException) {
         throw new ConfiguredTargetFunctionException(
             (ConfiguredValueCreationException) e.getCause());
+      } else if (e.getCause() instanceof InconsistentAspectOrderException) {
+        InconsistentAspectOrderException cause = (InconsistentAspectOrderException) e.getCause();
+        throw new ConfiguredTargetFunctionException(
+            new ConfiguredValueCreationException(cause.getMessage(), target.getLabel()));
       } else {
         // Cast to InvalidConfigurationException as a consistency check. If you add any
         // DependencyEvaluationException constructors, you may need to change this code, too.
@@ -320,6 +329,9 @@ final class ConfiguredTargetFunction implements SkyFunction {
           new ConfiguredValueCreationException(e.print(), ctgValue.getLabel()));
     } catch (InvalidConfigurationException e) {
       throw new DependencyEvaluationException(e);
+    } catch (InconsistentAspectOrderException e) {
+      env.getListener().handle(Event.error(e.getLocation(), e.getMessage()));
+      throw new DependencyEvaluationException(e);
     }
 
     // Trim each dep's configuration so it only includes the fragments needed by its transitive
@@ -986,8 +998,13 @@ final class ConfiguredTargetFunction implements SkyFunction {
 
     // Collect the corresponding Skyframe configured target values. Abort early if they haven't
     // been computed yet.
-    Collection<Dependency> configValueNames = resolver.resolveRuleLabels(
-        ctgValue, configLabelMap, transitiveLoadingRootCauses);
+    Collection<Dependency> configValueNames = null;
+    try {
+      configValueNames = resolver.resolveRuleLabels(
+          ctgValue, configLabelMap, transitiveLoadingRootCauses);
+    } catch (InconsistentAspectOrderException e) {
+      throw new DependencyEvaluationException(e);
+    }
     if (env.valuesMissing()) {
       return null;
     }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PostConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PostConfiguredTargetFunction.java
index bd5eef5c0d..77891647ec 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PostConfiguredTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PostConfiguredTargetFunction.java
@@ -19,6 +19,7 @@ import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.Dependency;
+import com.google.devtools.build.lib.analysis.DependencyResolver.InconsistentAspectOrderException;
 import com.google.devtools.build.lib.analysis.LabelAndConfiguration;
 import com.google.devtools.build.lib.analysis.TargetAndConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
@@ -111,11 +112,8 @@ public class PostConfiguredTargetFunction implements SkyFunction {
         deps = ConfiguredTargetFunction.getDynamicConfigurations(env, ctgValue, deps,
             hostConfiguration, ruleClassProvider);
       }
-    } catch (EvalException e) {
-      throw new PostConfiguredTargetFunctionException(e);
-    } catch (ConfiguredTargetFunction.DependencyEvaluationException e) {
-      throw new PostConfiguredTargetFunctionException(e);
-    } catch (InvalidConfigurationException e) {
+    } catch (EvalException | ConfiguredTargetFunction.DependencyEvaluationException
+        | InvalidConfigurationException | InconsistentAspectOrderException e) {
       throw new PostConfiguredTargetFunctionException(e);
     }
 
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 09a2c17760..a4d7cb4edf 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
@@ -61,6 +61,7 @@ public class DependencyResolverTest extends AnalysisTestCase {
   @Before
   public final void createResolver() throws Exception {
     dependencyResolver = new DependencyResolver() {
+
       @Override
       protected void invalidVisibilityReferenceHook(TargetAndConfiguration node, Label label) {
         throw new IllegalStateException();
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 1c0a3a6175..18942dcb09 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
@@ -1499,6 +1499,7 @@ public class SkylarkAspectsTest extends AnalysisTestCase {
         "//test:r0[\"//test:aspect.bzl%a1\", \"//test:aspect.bzl%a2\"]=yes",
         "//test:r1[\"//test:aspect.bzl%a2\"]=no");
   }
+
   /**
    * Diamond case.
    * rule r1 depends or r0 with aspect a1.
@@ -1769,6 +1770,96 @@ public class SkylarkAspectsTest extends AnalysisTestCase {
         + "does not provide advertised provider 'foo'");
   }
 
+  @Test
+  public void aspectOnAspectInconsistentVisibility() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "a1p = provider()",
+        "def _a1_impl(target,ctx):",
+        "  return struct(a1p = a1p(text = 'random'))",
+        "a1 = aspect(_a1_impl, attr_aspects = ['dep'], provides = ['a1p'])",
+        "a2p = provider()",
+        "def _a2_impl(target,ctx):",
+        "  return struct(a2p = a2p(value = 'random'))",
+        "a2 = aspect(_a2_impl, attr_aspects = ['dep'], required_aspect_providers = ['a1p'])",
+        "def _r1_impl(ctx):",
+        "  pass",
+        "def _r2_impl(ctx):",
+        "  return struct(result = ctx.attr.dep.a2p.value)",
+        "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', 'r1', 'r2')",
+        "r1(name = 'r0')",
+        "r1(name = 'r1', dep = ':r0')",
+        "r2(name = 'r2', dep = ':r1')",
+        "r1(name = 'r1_1', dep = ':r2')",
+        "r2(name = 'r2_1', dep = ':r1_1')"
+
+    );
+    reporter.removeHandler(failFastHandler);
+
+    try {
+      AnalysisResult analysisResult = update("//test:r2_1");
+      assertThat(analysisResult.hasError()).isTrue();
+      assertThat(keepGoing()).isTrue();
+    } catch (ViewCreationFailedException e) {
+      // expected
+    }
+    assertContainsEvent("ERROR /workspace/test/BUILD:4:1: Aspect //test:aspect.bzl%a2 is"
+        + " applied twice, both before and after aspect //test:aspect.bzl%a1 "
+        + "(when propagating from //test:r2 to //test:r1 via attribute dep)");
+  }
+
+  @Test
+  public void aspectOnAspectInconsistentVisibilityIndirect() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "a1p = provider()",
+        "def _a1_impl(target,ctx):",
+        "  return struct(a1p = a1p(text = 'random'))",
+        "a1 = aspect(_a1_impl, attr_aspects = ['dep'], provides = ['a1p'])",
+        "a2p = provider()",
+        "def _a2_impl(target,ctx):",
+        "  return struct(a2p = a2p(value = 'random'))",
+        "a2 = aspect(_a2_impl, attr_aspects = ['dep'], required_aspect_providers = ['a1p'])",
+        "def _r1_impl(ctx):",
+        "  pass",
+        "def _r2_impl(ctx):",
+        "  return struct(result = ctx.attr.dep.a2p.value)",
+        "r1 = rule(_r1_impl, attrs = { 'dep' : attr.label(aspects = [a1])})",
+        "r2 = rule(_r2_impl, attrs = { 'dep' : attr.label(aspects = [a2])})",
+        "def _r0_impl(ctx):",
+        "  pass",
+        "r0 = rule(_r0_impl, attrs = { 'dep' : attr.label()})"
+    );
+    scratch.file(
+        "test/BUILD",
+        "load(':aspect.bzl', 'r0', 'r1', 'r2')",
+        "r0(name = 'r0')",
+        "r1(name = 'r1', dep = ':r0')",
+        "r2(name = 'r2', dep = ':r1')",
+        "r1(name = 'r1_1', dep = ':r2')",
+        "r2(name = 'r2_1', dep = ':r1_1')",
+        "r0(name = 'r0_2', dep = ':r2_1')"
+    );
+    reporter.removeHandler(failFastHandler);
+
+    try {
+      AnalysisResult analysisResult = update("//test:r0_2");
+      assertThat(analysisResult.hasError()).isTrue();
+      assertThat(keepGoing()).isTrue();
+    } catch (ViewCreationFailedException e) {
+      // expected
+    }
+    assertContainsEvent("ERROR /workspace/test/BUILD:4:1: Aspect //test:aspect.bzl%a2 is"
+        + " applied twice, both before and after aspect //test:aspect.bzl%a1 "
+        + "(when propagating from //test:r2 to //test:r1 via attribute dep)");
+  }
+
+
   @RunWith(JUnit4.class)
   public static final class WithKeepGoing extends SkylarkAspectsTest {
     @Override