Refactor cycle detection logic to handle dynamic configurations.

Currently, analysis-time cycle detection expects all cycles to come from ConfiguredTargetFunction. 

With dynamic configurations, ConfiguredTargetFunction calls out to TransitiveTargetFunction to figure out which configuration fragments its deps need.

If there's a cycle between the current target and a dep, the dep's TransitiveTargetFunction fails, which the current cycle detection code can't handle.

But even if it could handle it, since the failure occurs in the dep we'd get error messages like:

    "in cc_library rule //the:dep: cycle in dependency graph"

instead of the expected:

    "in cc_library rule //the:top_level_rule: cycle in dependency graph"

This used to not be a problem because loading-phase cycle detection caught the cycle before all this triggered. But interleaved loading and analysis removes that gate.

Tested: BuildViewTest cycle detection tests with dynamic configurations turned on

--
MOS_MIGRATED_REVID=124391277

3 files changed, 79 insertions, 10 deletions

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 c7a542f1ef..70b7ed8fac 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
@@ -13,8 +13,12 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
+import static com.google.devtools.build.lib.skyframe.SkyFunctions.TRANSITIVE_TARGET;
+
+import com.google.common.base.Function;
 import com.google.common.base.Predicate;
-import com.google.common.collect.ImmutableList;
+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;
@@ -35,29 +39,78 @@ class ConfiguredTargetCycleReporter extends AbstractLabelCycleReporter {
   private static final Predicate<SkyKey> IS_CONFIGURED_TARGET_SKY_KEY =
       SkyFunctions.isSkyFunction(SkyFunctions.CONFIGURED_TARGET);
 
+  private static final Predicate<SkyKey> IS_TRANSITIVE_TARGET_SKY_KEY =
+      SkyFunctions.isSkyFunction(TRANSITIVE_TARGET);
+
+  private final TransitiveTargetCycleReporter targetReporter;
+
   ConfiguredTargetCycleReporter(PackageProvider packageProvider) {
     super(packageProvider);
+    targetReporter = new TransitiveTargetCycleReporter(packageProvider);
   }
 
   @Override
   protected boolean canReportCycle(SkyKey topLevelKey, CycleInfo cycleInfo) {
-    return Iterables.all(Iterables.concat(ImmutableList.of(topLevelKey),
-        cycleInfo.getPathToCycle(), cycleInfo.getCycle()), IS_CONFIGURED_TARGET_SKY_KEY);
+    if (!IS_CONFIGURED_TARGET_SKY_KEY.apply(topLevelKey)) {
+      return false;
+    }
+    Iterable<SkyKey> cycleKeys = Iterables.concat(cycleInfo.getPathToCycle(), cycleInfo.getCycle());
+    // Static configurations expect all keys to be ConfiguredTargetValue keys. Dynamic
+    // configurations expect the top-level key to be a ConfiguredTargetValue key, but cycles and
+    // paths to them can travel through TransitiveTargetValue keys because ConfiguredTargetFunction
+    // visits TransitiveTargetFunction as a part of dynamic configuration computation.
+    //
+    // Unfortunately this class can't easily figure out if we're in static or dynamic configuration
+    // mode, so we loosely permit both cases.
+    //
+    // TODO: remove the static-style checking once dynamic configurations fully replace them
+    return Iterables.all(cycleKeys,
+        Predicates.<SkyKey>or(IS_CONFIGURED_TARGET_SKY_KEY, IS_TRANSITIVE_TARGET_SKY_KEY));
   }
 
   @Override
   protected String getAdditionalMessageAboutCycle(
       EventHandler eventHandler, SkyKey topLevelKey, CycleInfo cycleInfo) {
-    return "\nThis cycle occurred because of a configuration option";
+    if (Iterables.all(cycleInfo.getCycle(), IS_TRANSITIVE_TARGET_SKY_KEY)) {
+      // The problem happened strictly in loading, so delegate the explanation to
+      // TransitiveTargetCycleReporter.
+      Iterable<SkyKey> pathAsTargetKeys = Iterables.transform(cycleInfo.getPathToCycle(),
+          new Function<SkyKey, SkyKey>() {
+            @Override
+            public SkyKey apply(SkyKey key) {
+              return asTransitiveTargetKey(key);
+            }
+          });
+      return targetReporter.getAdditionalMessageAboutCycle(eventHandler,
+          asTransitiveTargetKey(topLevelKey),
+          new CycleInfo(pathAsTargetKeys, cycleInfo.getCycle()));
+    } else {
+      return "\nThis cycle occurred because of a configuration option";
+    }
+  }
+
+  private SkyKey asTransitiveTargetKey(SkyKey key) {
+    return IS_TRANSITIVE_TARGET_SKY_KEY.apply(key)
+        ? key
+        : SkyKey.create(TRANSITIVE_TARGET, ((ConfiguredTargetKey) key.argument()).getLabel());
   }
 
   @Override
   public String prettyPrint(SkyKey key) {
-    return ((ConfiguredTargetKey) key.argument()).prettyPrint();
+    if (IS_CONFIGURED_TARGET_SKY_KEY.apply(key)) {
+      return ((ConfiguredTargetKey) key.argument()).prettyPrint();
+    } else {
+      return getLabel(key).toString();
+    }
   }
 
   @Override
   public Label getLabel(SkyKey key) {
-    return ((ConfiguredTargetKey) key.argument()).getLabel();
+    if (IS_CONFIGURED_TARGET_SKY_KEY.apply(key)) {
+      return ((ConfiguredTargetKey) key.argument()).getLabel();
+    } else {
+      Verify.verify(IS_TRANSITIVE_TARGET_SKY_KEY.apply(key));
+      return (Label) key.argument();
+    }
   }
-}
\ No newline at end of file
+}
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 e8706f2537..b0a8137c57 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
@@ -134,6 +134,10 @@ final class ConfiguredTargetFunction implements SkyFunction {
     this.ruleClassProvider = ruleClassProvider;
   }
 
+  private static boolean useDynamicConfigurations(BuildConfiguration config) {
+    return config != null && config.useDynamicConfigurations();
+  }
+
   @Override
   public SkyValue compute(SkyKey key, Environment env) throws ConfiguredTargetFunctionException,
       InterruptedException {
@@ -173,6 +177,17 @@ final class ConfiguredTargetFunction implements SkyFunction {
     if (!target.isConfigurable()) {
       configuration = null;
     }
+
+    // This line is only needed for accurate error messaging. Say this target has a circular
+    // dependency with one of its deps. With this line, loading this target fails so Bazel
+    // associates the corresponding error with this target, as expected. Without this line,
+    // the first TransitiveTargetValue call happens on its dep (in trimConfigurations), so Bazel
+    // associates the error with the dep, which is misleading.
+    if (useDynamicConfigurations(configuration)
+        && env.getValue(TransitiveTargetValue.key(lc.getLabel())) == null) {
+      return null;
+    }
+
     TargetAndConfiguration ctgValue = new TargetAndConfiguration(target, configuration);
 
     SkyframeDependencyResolver resolver = view.createDependencyResolver(env);
@@ -283,8 +298,7 @@ final class ConfiguredTargetFunction implements SkyFunction {
 
     // Trim each dep's configuration so it only includes the fragments needed by its transitive
     // closure (only dynamic configurations support this).
-    if (ctgValue.getConfiguration() != null
-        && ctgValue.getConfiguration().useDynamicConfigurations()) {
+    if (useDynamicConfigurations(ctgValue.getConfiguration())) {
       depValueNames = trimConfigurations(env, ctgValue, depValueNames, hostConfiguration,
           ruleClassProvider);
       if (depValueNames == null) {
@@ -725,7 +739,7 @@ final class ConfiguredTargetFunction implements SkyFunction {
     // TODO(bazel-team): remove the need for this special transformation. We can probably do this by
     // simply passing this through trimConfigurations.
     BuildConfiguration targetConfig = ctgValue.getConfiguration();
-    if (targetConfig != null && targetConfig.useDynamicConfigurations()) {
+    if (useDynamicConfigurations(targetConfig)) {
       ImmutableList.Builder<Dependency> staticConfigs = ImmutableList.builder();
       for (Dependency dep : configValueNames) {
         staticConfigs.add(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
index 4231583217..556d781aa0 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -391,6 +391,8 @@ public final class SkyframeBuildView {
       }
       if (culprit.functionName().equals(SkyFunctions.CONFIGURED_TARGET)) {
         return ((ConfiguredTargetKey) culprit.argument()).getLabel();
+      } else if (culprit.functionName().equals(SkyFunctions.TRANSITIVE_TARGET)) {
+        return (Label) culprit.argument();
       } else {
         return labelToLoad;
       }