Remove WalkableGraph#exists and allow WalkableGraph#getValue and WalkableGraph#getException to be given non-existent keys without crashing. Add WalkableGraph#isCycle to fill the gap in testing for the difference between non-existence and depending on a cycle.

--
PiperOrigin-RevId: 143205289
MOS_MIGRATED_REVID=143205289

5 files changed, 38 insertions, 33 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternsFunctionSmartNegationTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternsFunctionSmartNegationTest.java
index 278567a09d..dbbcf495ab 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternsFunctionSmartNegationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternsFunctionSmartNegationTest.java
@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.skyframe;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.skyframe.WalkableGraphUtils.exists;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
@@ -27,13 +28,11 @@ import com.google.devtools.build.skyframe.EvaluationResult;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 import com.google.devtools.build.skyframe.WalkableGraph;
-
+import java.io.IOException;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
-import java.io.IOException;
-
 /** Tests for {@link PrepareDepsOfPatternsFunction}. */
 @RunWith(JUnit4.class)
 public class PrepareDepsOfPatternsFunctionSmartNegationTest extends BuildViewTestCase {
@@ -60,11 +59,11 @@ public class PrepareDepsOfPatternsFunctionSmartNegationTest extends BuildViewTes
             patternSequence, /*successExpected=*/ true, /*keepGoing=*/ true);
 
     // Then the graph contains package values for "@//foo" and "@//foo/foo",
-    assertTrue(walkableGraph.exists(PackageValue.key(PackageIdentifier.parse("@//foo"))));
-    assertTrue(walkableGraph.exists(PackageValue.key(PackageIdentifier.parse("@//foo/foo"))));
+    assertTrue(exists(PackageValue.key(PackageIdentifier.parse("@//foo")), walkableGraph));
+    assertTrue(exists(PackageValue.key(PackageIdentifier.parse("@//foo/foo")), walkableGraph));
 
     // But the graph does not contain a value for the target "@//foo/foo:foofoo".
-    assertFalse(walkableGraph.exists(getKeyForLabel(Label.create("@//foo/foo", "foofoo"))));
+    assertFalse(exists(getKeyForLabel(Label.create("@//foo/foo", "foofoo")), walkableGraph));
   }
 
   @Test
@@ -104,14 +103,14 @@ public class PrepareDepsOfPatternsFunctionSmartNegationTest extends BuildViewTes
             patternSequence, /*successExpected=*/ true, /*keepGoing=*/ true);
 
     // Then the graph contains a package value for "@//foo",
-    assertTrue(walkableGraph.exists(PackageValue.key(PackageIdentifier.parse("@//foo"))));
+    assertTrue(exists(PackageValue.key(PackageIdentifier.parse("@//foo")), walkableGraph));
 
     // But no package value for "@//foo/foo",
-    assertFalse(walkableGraph.exists(PackageValue.key(PackageIdentifier.parse("@//foo/foo"))));
+    assertFalse(exists(PackageValue.key(PackageIdentifier.parse("@//foo/foo")), walkableGraph));
 
     // And the graph does not contain a value for the target "@//foo/foo:foofoo".
     Label label = Label.create("@//foo/foo", "foofoo");
-    assertFalse(walkableGraph.exists(getKeyForLabel(label)));
+    assertFalse(exists(getKeyForLabel(label), walkableGraph));
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternsFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternsFunctionTest.java
index b29ec04b0d..cda7fbc775 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternsFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternsFunctionTest.java
@@ -15,6 +15,7 @@ package com.google.devtools.build.lib.skyframe;
 
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.devtools.build.skyframe.EvaluationResultSubjectFactory.assertThatEvaluationResult;
+import static com.google.devtools.build.skyframe.WalkableGraphUtils.exists;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
@@ -61,7 +62,7 @@ public class PrepareDepsOfPatternsFunctionTest extends BuildViewTestCase {
     assertValidValue(walkableGraph, getKeyForLabel(Label.create("@//foo", "foo")));
 
     // And the graph does not contain a value for the target "@//foo:foo2".
-    assertFalse(walkableGraph.exists(getKeyForLabel(Label.create("@//foo", "foo2"))));
+    assertFalse(exists(getKeyForLabel(Label.create("@//foo", "foo2")), walkableGraph));
   }
 
   @Test
@@ -106,7 +107,7 @@ public class PrepareDepsOfPatternsFunctionTest extends BuildViewTestCase {
     WalkableGraph walkableGraph = getGraphFromPatternsEvaluation(patternSequence);
 
     // Then the graph does not contain an entry for ":foo",
-    assertFalse(walkableGraph.exists(getKeyForLabel(Label.create("@//foo", "foo"))));
+    assertFalse(exists(getKeyForLabel(Label.create("@//foo", "foo")), walkableGraph));
   }
 
   @Test
@@ -188,7 +189,7 @@ public class PrepareDepsOfPatternsFunctionTest extends BuildViewTestCase {
     assertContainsEvent("Skipping '" + bogusPattern + "': ");
 
     // And then the graph contains a value for the legit target pattern's target "@//foo:foo".
-    assertTrue(walkableGraph.exists(getKeyForLabel(Label.create("@//foo", "foo"))));
+    assertTrue(exists(getKeyForLabel(Label.create("@//foo", "foo")), walkableGraph));
   }
 
   // Helpers:
@@ -260,7 +261,6 @@ public class PrepareDepsOfPatternsFunctionTest extends BuildViewTestCase {
   private static void assertValidValue(
       WalkableGraph graph, SkyKey key, boolean expectTransitiveException)
       throws InterruptedException {
-    assertTrue(graph.exists(key));
     assertNotNull(graph.getValue(key));
     if (expectTransitiveException) {
       assertNotNull(graph.getException(key));
@@ -271,7 +271,6 @@ public class PrepareDepsOfPatternsFunctionTest extends BuildViewTestCase {
 
   private static Exception assertException(WalkableGraph graph, SkyKey key)
       throws InterruptedException {
-    assertTrue(graph.exists(key));
     assertNull(graph.getValue(key));
     Exception exception = graph.getException(key);
     assertNotNull(exception);
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfTargetsUnderDirectoryFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfTargetsUnderDirectoryFunctionTest.java
index 0a8aed0a29..169a3fcccd 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfTargetsUnderDirectoryFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfTargetsUnderDirectoryFunctionTest.java
@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.skyframe;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.skyframe.WalkableGraphUtils.exists;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
@@ -34,14 +35,12 @@ import com.google.devtools.build.skyframe.BuildDriver;
 import com.google.devtools.build.skyframe.EvaluationResult;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.WalkableGraph;
-
+import java.io.IOException;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
-import java.io.IOException;
-
 /**
  * Tests for {@link PrepareDepsOfTargetsUnderDirectoryFunction}. Insert excuses here.
  */
@@ -104,7 +103,7 @@ public class PrepareDepsOfTargetsUnderDirectoryFunctionTest extends BuildViewTes
 
     // Then the TransitiveTraversalValue for "@//a:a" is evaluated,
     SkyKey aaKey = TransitiveTraversalValue.key(Label.create("@//a", "a"));
-    assertThat(graph.exists(aaKey)).isTrue();
+    assertThat(exists(aaKey, graph)).isTrue();
 
     // And that TransitiveTraversalValue depends on "@//b:b.txt".
     Iterable<SkyKey> depsOfAa =
@@ -113,7 +112,7 @@ public class PrepareDepsOfTargetsUnderDirectoryFunctionTest extends BuildViewTes
     assertThat(depsOfAa).contains(bTxtKey);
 
     // And the TransitiveTraversalValue for "b:b.txt" is evaluated.
-    assertThat(graph.exists(bTxtKey)).isTrue();
+    assertThat(exists(bTxtKey, graph)).isTrue();
   }
 
   @Test
@@ -130,11 +129,11 @@ public class PrepareDepsOfTargetsUnderDirectoryFunctionTest extends BuildViewTes
 
     // Then the TransitiveTraversalValue for "@//a:a" is not evaluated,
     SkyKey aaKey = TransitiveTraversalValue.key(Label.create("@//a", "a"));
-    assertThat(graph.exists(aaKey)).isFalse();
+    assertThat(exists(aaKey, graph)).isFalse();
 
     // But the TransitiveTraversalValue for "@//a:aTest" is.
     SkyKey aaTestKey = TransitiveTraversalValue.key(Label.create("@//a", "aTest"));
-    assertThat(graph.exists(aaTestKey)).isTrue();
+    assertThat(exists(aaTestKey, graph)).isTrue();
   }
 
   /**
@@ -188,13 +187,18 @@ public class PrepareDepsOfTargetsUnderDirectoryFunctionTest extends BuildViewTes
 
     // Also, the computation graph does not contain a cached value for "a/b".
     WalkableGraph graph = Preconditions.checkNotNull(evaluationResult.getWalkableGraph());
-    assertFalse(graph.exists(createPrepDepsKey(rootDirectory, excludedPathFragment,
-        ImmutableSet.<PathFragment>of())));
+    assertFalse(
+        exists(
+            createPrepDepsKey(rootDirectory, excludedPathFragment, ImmutableSet.<PathFragment>of()),
+            graph));
 
     // And the computation graph does contain a cached value for "a/c" with the empty set excluded,
     // because that key was evaluated.
-    assertTrue(graph.exists(createPrepDepsKey(rootDirectory, new PathFragment("a/c"),
-        ImmutableSet.<PathFragment>of())));
+    assertTrue(
+        exists(
+            createPrepDepsKey(
+                rootDirectory, new PathFragment("a/c"), ImmutableSet.<PathFragment>of()),
+            graph));
   }
 
   @Test
@@ -233,7 +237,7 @@ public class PrepareDepsOfTargetsUnderDirectoryFunctionTest extends BuildViewTes
     // "a/b/c" does live underneath "a/b".
     WalkableGraph graph = Preconditions.checkNotNull(evaluationResult.getWalkableGraph());
     SkyKey abKey = createCollectPackagesKey(rootDirectory, new PathFragment("a/b"), excludedPaths);
-    assertThat(graph.exists(abKey)).isTrue();
+    assertThat(exists(abKey, graph)).isTrue();
     CollectPackagesUnderDirectoryValue abValue =
         (CollectPackagesUnderDirectoryValue) Preconditions.checkNotNull(graph.getValue(abKey));
 
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunctionTest.java
index d6c6459c8e..89f961a0a5 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunctionTest.java
@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.skyframe;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.skyframe.WalkableGraphUtils.exists;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
@@ -31,7 +32,6 @@ import com.google.devtools.build.skyframe.BuildDriver;
 import com.google.devtools.build.skyframe.EvaluationResult;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.WalkableGraph;
-
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -140,16 +140,18 @@ public class RecursivePkgFunctionTest extends BuildViewTestCase {
     // Also, the computation graph does not contain a cached value for "a/b".
     WalkableGraph graph = Preconditions.checkNotNull(evaluationResult.getWalkableGraph());
     assertFalse(
-        graph.exists(
+        exists(
             buildRecursivePkgKey(
-                rootDirectory, excludedPathFragment, ImmutableSet.<PathFragment>of())));
+                rootDirectory, excludedPathFragment, ImmutableSet.<PathFragment>of()),
+            graph));
 
     // And the computation graph does contain a cached value for "a/c" with the empty set excluded,
     // because that key was evaluated.
     assertTrue(
-        graph.exists(
+        exists(
             buildRecursivePkgKey(
-                rootDirectory, new PathFragment("a/c"), ImmutableSet.<PathFragment>of())));
+                rootDirectory, new PathFragment("a/c"), ImmutableSet.<PathFragment>of()),
+            graph));
   }
 
   @Test
@@ -176,6 +178,6 @@ public class RecursivePkgFunctionTest extends BuildViewTestCase {
     // "a/b/c" does live underneath "a/b".
     WalkableGraph graph = Preconditions.checkNotNull(evaluationResult.getWalkableGraph());
     assertTrue(
-        graph.exists(buildRecursivePkgKey(rootDirectory, new PathFragment("a/b"), excludedPaths)));
+        exists(buildRecursivePkgKey(rootDirectory, new PathFragment("a/b"), excludedPaths), graph));
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeLabelVisitorTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeLabelVisitorTestCase.java
index 80650fdf84..1fd0b74cc6 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeLabelVisitorTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeLabelVisitorTestCase.java
@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.skyframe;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.skyframe.WalkableGraphUtils.exists;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotSame;
 
@@ -191,7 +192,7 @@ abstract public class SkyframeLabelVisitorTestCase extends PackageLoadingTestCas
     while (!Iterables.isEmpty(nodesToVisit)) {
       List<SkyKey> existingNodes = new ArrayList<>();
       for (SkyKey key : nodesToVisit) {
-        if (graph.exists(key) && graph.getValue(key) != null && visitedNodes.add(key)) {
+        if (exists(key, graph) && graph.getValue(key) != null && visitedNodes.add(key)) {
           existingNodes.add(key);
         }
       }