Track skylark import cycles in terms of PackageIdentifiers

We don't have any strong guarantees around the stability of SkyKey#toString(),
which we rely on to canonicalize cycles. We can reasonably expect
PackageIdentifier#toString() to be stable enough though. We also hide away the
cycle detection details from external code in this change.

--
MOS_MIGRATED_REVID=105093095

4 files changed, 31 insertions, 26 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index ddc14accb8..78090e0a83 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -67,7 +67,6 @@ import com.google.devtools.build.skyframe.ValueOrExceptionUtils;
 import java.io.IOException;
 import java.util.Collection;
 import java.util.HashMap;
-import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -641,11 +640,9 @@ public class PackageFunction implements SkyFunction {
                 SkylarkImportFailedException, InconsistentFilesystemException,
                 ASTLookupInputException, BuildFileNotFoundException>
             lookupResult;
-        Set<SkyKey> visitedKeysForCycle = new LinkedHashSet<>();
         try {
           SkyValue value =
-              skylarkImportLookupFunctionForInlining.computeWithInlineCalls(importKey, env,
-                  visitedKeysForCycle);
+              skylarkImportLookupFunctionForInlining.computeWithInlineCalls(importKey, env);
           if (value == null) {
             Preconditions.checkState(env.valuesMissing(), importKey);
             // Don't give up on computing. This is against the Skyframe contract, but we don't want
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
index abc44906b9..b510f15532 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
@@ -39,6 +39,7 @@ import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 
+import java.util.LinkedHashSet;
 import java.util.Map;
 import java.util.Set;
 
@@ -61,22 +62,28 @@ public class SkylarkImportLookupFunction implements SkyFunction {
   @Override
   public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException,
       InterruptedException {
-    return computeInternal(skyKey, env, null);
+    return computeInternal((PackageIdentifier) skyKey.argument(), env, null);
   }
 
-  SkyValue computeWithInlineCalls(SkyKey skyKey, Environment env, Set<SkyKey> visitedKeysForCycle)
+  SkyValue computeWithInlineCalls(SkyKey skyKey, Environment env)
       throws SkyFunctionException, InterruptedException {
-    return computeInternal(skyKey, env, Preconditions.checkNotNull(visitedKeysForCycle, skyKey));
+    return computeWithInlineCallsInternal(
+        (PackageIdentifier) skyKey.argument(), env, new LinkedHashSet<PackageIdentifier>());
   }
 
-  SkyValue computeInternal(SkyKey skyKey, Environment env,
-      @Nullable Set<SkyKey> visitedKeysForCycle)
-      throws SkyFunctionException, InterruptedException {
-    PackageIdentifier arg = (PackageIdentifier) skyKey.argument();
-    PathFragment file = arg.getPackageFragment();
+  private SkyValue computeWithInlineCallsInternal(
+      PackageIdentifier pkgId, Environment env, Set<PackageIdentifier> visited)
+          throws SkyFunctionException, InterruptedException {
+    return computeInternal(pkgId, env, Preconditions.checkNotNull(visited, pkgId));
+  }
+
+  SkyValue computeInternal(
+      PackageIdentifier pkgId, Environment env, @Nullable Set<PackageIdentifier> visited)
+          throws SkyFunctionException, InterruptedException {
+    PathFragment file = pkgId.getPackageFragment();
     ASTFileLookupValue astLookupValue = null;
     try {
-      SkyKey astLookupKey = ASTFileLookupValue.key(arg);
+      SkyKey astLookupKey = ASTFileLookupValue.key(pkgId);
       astLookupValue = (ASTFileLookupValue) env.getValueOrThrow(astLookupKey,
           ErrorReadingSkylarkExtensionException.class, InconsistentFilesystemException.class);
     } catch (ErrorReadingSkylarkExtensionException e) {
@@ -99,7 +106,7 @@ public class SkylarkImportLookupFunction implements SkyFunction {
           SkylarkImportFailedException.skylarkErrors(file));
     }
 
-    Label label = pathFragmentToLabel(arg.getRepository(), file, env);
+    Label label = pathFragmentToLabel(pkgId.getRepository(), file, env);
     if (label == null) {
       Preconditions.checkState(env.valuesMissing(), "null label with no missing %s", file);
       return null;
@@ -112,7 +119,7 @@ public class SkylarkImportLookupFunction implements SkyFunction {
     for (Map.Entry<Location, PathFragment> entry : ast.getImports().entrySet()) {
       try {
         skylarkImports.put(
-            PackageFunction.getImportKey(entry, ruleClassProvider.getPreludePath(), file, arg),
+            PackageFunction.getImportKey(entry, ruleClassProvider.getPreludePath(), file, pkgId),
             entry.getValue());
       } catch (ASTLookupInputException e) {
         throw new SkylarkImportLookupFunctionException(e, Transience.PERSISTENT);
@@ -121,15 +128,15 @@ public class SkylarkImportLookupFunction implements SkyFunction {
 
     Map<SkyKey, SkyValue> skylarkImportMap;
     boolean valuesMissing = false;
-    if (visitedKeysForCycle == null) {
+    if (visited == null) {
       // Not inlining.
       skylarkImportMap = env.getValues(skylarkImports.keySet());
       valuesMissing = env.valuesMissing();
     } else {
       // inlining calls to SkylarkImportLookupFunction.
-      if (!visitedKeysForCycle.add(skyKey)) {
-        ImmutableList<SkyKey> cycle =
-            CycleUtils.splitIntoPathAndChain(Predicates.equalTo(skyKey), visitedKeysForCycle)
+      if (!visited.add(pkgId)) {
+        ImmutableList<PackageIdentifier> cycle =
+            CycleUtils.splitIntoPathAndChain(Predicates.equalTo(pkgId), visited)
                 .second;
         if (env.getValue(SkylarkImportUniqueCycleValue.key(cycle)) == null) {
           return null;
@@ -139,7 +146,9 @@ public class SkylarkImportLookupFunction implements SkyFunction {
       }
       skylarkImportMap = Maps.newHashMapWithExpectedSize(astImports.size());
       for (SkyKey skylarkImport : skylarkImports.keySet()) {
-        SkyValue skyValue = this.computeWithInlineCalls(skylarkImport, env, visitedKeysForCycle);
+        SkyValue skyValue =
+            this.computeWithInlineCallsInternal(
+                (PackageIdentifier) skylarkImport.argument(), env, visited);
         if (skyValue == null) {
           Preconditions.checkState(
               env.valuesMissing(), "no skylark import value for %s", skylarkImport);
@@ -151,7 +160,7 @@ public class SkylarkImportLookupFunction implements SkyFunction {
         }
       }
       // All imports traversed, this key can no longer be part of a cycle.
-      visitedKeysForCycle.remove(skyKey);
+      visited.remove(pkgId);
     }
 
     if (valuesMissing) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleFunction.java
index e7c4decae7..a16ad9ab0a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleFunction.java
@@ -14,14 +14,13 @@
 package com.google.devtools.build.lib.skyframe;
 
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
-import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 
 /**
  * Emits an error message exactly once when a Skylark import cycle is found when running inlined
  * {@link SkylarkImportLookupFunction}s.
  */
-class SkylarkImportUniqueCycleFunction extends AbstractChainUniquenessFunction<SkyKey> {
+class SkylarkImportUniqueCycleFunction extends AbstractChainUniquenessFunction<PackageIdentifier> {
   private static final SkyValue INSTANCE = new SkyValue() {};
 
   @Override
@@ -45,8 +44,7 @@ class SkylarkImportUniqueCycleFunction extends AbstractChainUniquenessFunction<S
   }
 
   @Override
-  protected String elementToString(SkyKey elt) {
-    PackageIdentifier pkgId = (PackageIdentifier) elt.argument();
+  protected String elementToString(PackageIdentifier pkgId) {
     return pkgId.getPathFragment().getPathString();
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleValue.java
index 242fc25467..0b9df89721 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleValue.java
@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.skyframe;
 
 import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 
@@ -22,7 +23,7 @@ import com.google.devtools.build.skyframe.SkyValue;
  * once.
  */
 public class SkylarkImportUniqueCycleValue implements SkyValue {
-  static SkyKey key(ImmutableList<SkyKey> cycle) {
+  static SkyKey key(ImmutableList<PackageIdentifier> cycle) {
     return AbstractChainUniquenessValue.key(SkyFunctions.SKYLARK_IMPORT_CYCLE, cycle);
   }
 }