Report cycles involving aspects correctly.

This involved refactoring BuildViewTestCase a bit so that its behavior is closer to that of Bazel with --experimental_interleave_loading_and_analysis.

RELNOTES:

--
MOS_MIGRATED_REVID=124816624

6 files changed, 66 insertions, 9 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
index 33cf880a6a..2db8a4d907 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
@@ -156,6 +156,17 @@ public final class AspectValue extends ActionLookupValue {
           && Objects.equal(parameters, that.parameters);
     }
 
+    public String prettyPrint() {
+      if (label == null) {
+        return "null";
+      }
+      return String.format("%s with aspect %s%s",
+          label.toString(),
+          aspectClass.getName(),
+          (aspectConfiguration != null && aspectConfiguration.isHostConfiguration())
+              ? "(host) " : "");
+    }
+
     @Override
     public String toString() {
       return label
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetCycleReporter.java
index 70b7ed8fac..ca05c1c175 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetCycleReporter.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetCycleReporter.java
@@ -18,11 +18,11 @@ import static com.google.devtools.build.lib.skyframe.SkyFunctions.TRANSITIVE_TAR
 import com.google.common.base.Function;
 import com.google.common.base.Predicate;
 import com.google.common.base.Predicates;
-import com.google.common.base.Verify;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.pkgcache.PackageProvider;
+import com.google.devtools.build.lib.skyframe.AspectValue.AspectKey;
 import com.google.devtools.build.skyframe.CycleInfo;
 import com.google.devtools.build.skyframe.SkyKey;
 
@@ -37,7 +37,9 @@ import com.google.devtools.build.skyframe.SkyKey;
 class ConfiguredTargetCycleReporter extends AbstractLabelCycleReporter {
 
   private static final Predicate<SkyKey> IS_CONFIGURED_TARGET_SKY_KEY =
-      SkyFunctions.isSkyFunction(SkyFunctions.CONFIGURED_TARGET);
+      Predicates.or(
+        SkyFunctions.isSkyFunction(SkyFunctions.CONFIGURED_TARGET),
+        SkyFunctions.isSkyFunction(SkyFunctions.ASPECT));
 
   private static final Predicate<SkyKey> IS_TRANSITIVE_TARGET_SKY_KEY =
       SkyFunctions.isSkyFunction(TRANSITIVE_TARGET);
@@ -97,8 +99,10 @@ class ConfiguredTargetCycleReporter extends AbstractLabelCycleReporter {
 
   @Override
   public String prettyPrint(SkyKey key) {
-    if (IS_CONFIGURED_TARGET_SKY_KEY.apply(key)) {
+    if (SkyFunctions.isSkyFunction(SkyFunctions.CONFIGURED_TARGET).apply(key)) {
       return ((ConfiguredTargetKey) key.argument()).prettyPrint();
+    } else if (SkyFunctions.isSkyFunction(SkyFunctions.ASPECT).apply(key)) {
+      return ((AspectKey) key.argument()).prettyPrint();
     } else {
       return getLabel(key).toString();
     }
@@ -106,11 +110,14 @@ class ConfiguredTargetCycleReporter extends AbstractLabelCycleReporter {
 
   @Override
   public Label getLabel(SkyKey key) {
-    if (IS_CONFIGURED_TARGET_SKY_KEY.apply(key)) {
+    if (SkyFunctions.isSkyFunction(SkyFunctions.CONFIGURED_TARGET).apply(key)) {
       return ((ConfiguredTargetKey) key.argument()).getLabel();
-    } else {
-      Verify.verify(IS_TRANSITIVE_TARGET_SKY_KEY.apply(key));
+    } else if (SkyFunctions.isSkyFunction(SkyFunctions.ASPECT).apply(key)) {
+      return ((AspectKey) key.argument()).getLabel();
+    } else if (SkyFunctions.isSkyFunction(TRANSITIVE_TARGET).apply(key)) {
       return (Label) key.argument();
+    } else {
+      throw new UnsupportedOperationException();
     }
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index e33c7e8c8d..63486e66ad 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -1200,6 +1200,13 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     }
 
     EvaluationResult<SkyValue> result = evaluateSkyKeys(eventHandler, skyKeys);
+    for (Map.Entry<SkyKey, ErrorInfo> entry : result.errorMap().entrySet()) {
+      getCyclesReporter().reportCycles(
+          entry.getValue().getCycleInfo(),
+          entry.getKey(),
+          eventHandler);
+    }
+
     ImmutableMap.Builder<Dependency, ConfiguredTarget> cts = ImmutableMap.builder();
 
   DependentNodeLoop:
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
index 2c2adf7e8c..51ba85f8e8 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
@@ -887,7 +887,6 @@ public class BuildViewTest extends BuildViewTestBase {
           .matches("Analysis of target '//foo:(java|cpp)' failed; build aborted.*");
     }
     assertContainsEvent("cycle in dependency graph");
-    assertContainsEvent("This cycle occurred because of a configuration option");
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/CircularDependencyTest.java b/src/test/java/com/google/devtools/build/lib/analysis/CircularDependencyTest.java
index 26c7666b21..4e0bfc2290 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/CircularDependencyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/CircularDependencyTest.java
@@ -186,6 +186,40 @@ public class CircularDependencyTest extends BuildViewTestCase {
         "genrule(name='b', srcs=['c'], tools=['c'], outs=['b.out'], cmd=':')",
         "genrule(name='c', srcs=['b.out'], outs=[], cmd=':')");
     getConfiguredTarget("//x:b"); // doesn't crash!
-    assertContainsEvent("in genrule rule //x:b: cycle in dependency graph");
+    assertContainsEvent("cycle in dependency graph");
+  }
+
+  @Test
+  public void testAspectCycle() throws Exception {
+    reporter.removeHandler(failFastHandler);
+    scratch.file("x/BUILD",
+        "load('//x:x.bzl', 'aspected', 'plain')",
+        // Using data= makes the dependency graph clearer because then the aspect does not propagate
+        // from aspectdep through a to b (and c)
+        "plain(name = 'a', noaspect_deps = [':b'])",
+        "aspected(name = 'b', aspect_deps = ['c'])",
+        "plain(name = 'c')",
+        "plain(name = 'aspectdep', aspect_deps = ['a'])");
+
+    scratch.file("x/x.bzl",
+        "def _impl(ctx):",
+        "    return struct()",
+        "",
+        "rule_aspect = aspect(",
+        "    implementation = _impl,",
+        "    attr_aspects = ['aspect_deps'],",
+        "    attrs = { '_implicit': attr.label(default = Label('//x:aspectdep')) })",
+        "",
+        "plain = rule(",
+        "    implementation = _impl,",
+        "    attrs = { 'aspect_deps': attr.label_list(), 'noaspect_deps': attr.label_list() })",
+        "",
+        "aspected = rule(",
+        "    implementation = _impl,",
+        "    attrs = { 'aspect_deps': attr.label_list(aspects = [rule_aspect]) })");
+
+    getConfiguredTarget("//x:a");
+    assertContainsEvent("cycle in dependency graph");
+    assertContainsEvent("//x:c with aspect //x:x.bzl%rule_aspect");
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
index 84ece3d89f..9460d95ab3 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -652,7 +652,6 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
    */
   protected ConfiguredTarget getConfiguredTarget(Label label, BuildConfiguration config)
       throws NoSuchPackageException, NoSuchTargetException, InterruptedException {
-    ensureTargetsVisited(label);
     return view.getConfiguredTargetForTesting(
         reporter, BlazeTestUtils.convertLabel(label), config);
   }