Fix SkyframeBuilder's error handling, and change the contract of EvaluationResult to return true for hasError() iff errorMap is non-empty or there is a catastrophe.

There was no good reason for the previous behavior of saying hasError even if there was a transitive recovered-from error, since callers shouldn't care. This was a latent bug that was only benign since none of the consumers of hasError were invoking Skyframe with recoverable SkyFunctions.

Also add an EvaluationResultSubject so that tests can more fluently assert things about EvaluationResult objects going forward.

--
MOS_MIGRATED_REVID=113192415

8 files changed, 128 insertions, 79 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java b/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java
index 33471d4aa3..97513c0020 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java
@@ -208,16 +208,6 @@ public class SkyframeBuilder implements Builder {
     }
   }
 
-  private static boolean resultHasCatastrophicError(EvaluationResult<?> result) {
-    for (ErrorInfo errorInfo : result.errorMap().values()) {
-      if (errorInfo.isCatastrophic()) {
-        return true;
-      }
-    }
-    // An unreported catastrophe manifests with hasError() being true but no errors visible.
-    return result.hasError() && result.errorMap().isEmpty();
-  }
-
   /**
    * Process the Skyframe update, taking into account the keepGoing setting.
    *
@@ -228,25 +218,29 @@ public class SkyframeBuilder implements Builder {
       boolean keepGoing, SkyframeExecutor skyframeExecutor)
           throws BuildFailedException, TestExecException {
     if (result.hasError()) {
-      boolean hasCycles = false;
       for (Map.Entry<SkyKey, ErrorInfo> entry : result.errorMap().entrySet()) {
         Iterable<CycleInfo> cycles = entry.getValue().getCycleInfo();
         skyframeExecutor.reportCycles(eventHandler, cycles, entry.getKey());
-        hasCycles |= !Iterables.isEmpty(cycles);
       }
-      boolean hasCatastrophe = resultHasCatastrophicError(result);
-      if (keepGoing && !hasCatastrophe) {
+
+      if (result.getCatastrophe() != null) {
+        rethrow(result.getCatastrophe());
+      }
+      if (keepGoing) {
+        // keepGoing doesn't throw if there were just ordinary errors.
         return false;
       }
-      if (hasCycles || result.errorMap().isEmpty()) {
-        // error map may be empty in the case of a catastrophe, but the
-        // catastrophe-causing exception may be available.
-        if (result.getCatastrophe() != null) {
-          rethrow(result.getCatastrophe());
-        }
-        throw new BuildFailedException(null, hasCatastrophe);
+      ErrorInfo errorInfo = Preconditions.checkNotNull(result.getError(), result);
+      Exception exception = errorInfo.getException();
+      if (exception == null) {
+        Preconditions.checkState(!Iterables.isEmpty(errorInfo.getCycleInfo()), errorInfo);
+        // If a keepGoing=false build found a cycle, that means there were no other errors thrown
+        // during evaluation (otherwise, it wouldn't have bothered to find a cycle). So the best
+        // we can do is throw a generic build failure exception, since we've already reported the
+        // cycles above.
+        throw new BuildFailedException(null, /*hasCatastrophe=*/ false);
       } else {
-        rethrow(Preconditions.checkNotNull(result.getError().getException()));
+        rethrow(exception);
       }
     }
     return true;
diff --git a/src/main/java/com/google/devtools/build/skyframe/EvaluationResult.java b/src/main/java/com/google/devtools/build/skyframe/EvaluationResult.java
index cab7e6a819..8a885fe879 100644
--- a/src/main/java/com/google/devtools/build/skyframe/EvaluationResult.java
+++ b/src/main/java/com/google/devtools/build/skyframe/EvaluationResult.java
@@ -39,7 +39,6 @@ import javax.annotation.Nullable;
  */
 public class EvaluationResult<T extends SkyValue> {
 
-  private final boolean hasError;
   @Nullable private final Exception catastrophe;
 
   private final Map<SkyKey, T> resultMap;
@@ -52,14 +51,10 @@ public class EvaluationResult<T extends SkyValue> {
   private EvaluationResult(
       Map<SkyKey, T> result,
       Map<SkyKey, ErrorInfo> errorMap,
-      boolean hasError,
       @Nullable Exception catastrophe,
       @Nullable WalkableGraph walkableGraph) {
-    Preconditions.checkState(errorMap.isEmpty() || hasError,
-        "result=%s, errorMap=%s", result, errorMap);
     this.resultMap = Preconditions.checkNotNull(result);
     this.errorMap = Preconditions.checkNotNull(errorMap);
-    this.hasError = hasError;
     this.catastrophe = catastrophe;
     this.walkableGraph = walkableGraph;
   }
@@ -73,12 +68,11 @@ public class EvaluationResult<T extends SkyValue> {
   }
 
   /**
-   * @return Whether or not the eval successfully evaluated all requested values. Note that this
-   * may return true even if all values returned are available in get(). This happens if a top-level
-   * value depends transitively on some value that recovered from a {@link SkyFunctionException}.
+   * @return Whether or not the eval successfully evaluated all requested values. True iff
+   * {@link #getCatastrophe} or {@link #getError} returns non-null.
    */
   public boolean hasError() {
-    return hasError;
+    return catastrophe != null || !errorMap.isEmpty();
   }
 
   /** @return catastrophic error encountered during evaluation, if any */
@@ -145,7 +139,7 @@ public class EvaluationResult<T extends SkyValue> {
   @Override
   public String toString() {
     return MoreObjects.toStringHelper(this)
-        .add("hasError", hasError)
+        .add("catastrophe", catastrophe)
         .add("errorMap", errorMap)
         .add("resultMap", resultMap)
         .toString();
@@ -163,7 +157,6 @@ public class EvaluationResult<T extends SkyValue> {
   public static class Builder<T extends SkyValue> {
     private final Map<SkyKey, T> result = new HashMap<>();
     private final Map<SkyKey, ErrorInfo> errors = new HashMap<>();
-    private boolean hasError = false;
     @Nullable private Exception catastrophe = null;
     private WalkableGraph walkableGraph = null;
 
@@ -192,17 +185,12 @@ public class EvaluationResult<T extends SkyValue> {
     public Builder<T> mergeFrom(EvaluationResult<T> otherResult) {
       result.putAll(otherResult.resultMap);
       errors.putAll(otherResult.errorMap);
-      hasError |= otherResult.hasError;
       catastrophe = otherResult.catastrophe;
       return this;
     }
 
     public EvaluationResult<T> build() {
-      return new EvaluationResult<>(result, errors, hasError, catastrophe, walkableGraph);
-    }
-
-    public void setHasError(boolean hasError) {
-      this.hasError = hasError;
+      return new EvaluationResult<>(result, errors, catastrophe, walkableGraph);
     }
 
     public void setCatastrophe(Exception catastrophe) {
diff --git a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
index d443175aba..739618a5ea 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
@@ -1534,7 +1534,6 @@ public final class ParallelEvaluator implements Evaluator {
         bubbleErrorInfo);
     EvaluationResult.Builder<T> result = EvaluationResult.builder();
     List<SkyKey> cycleRoots = new ArrayList<>();
-    boolean hasError = false;
     for (SkyKey skyKey : skyKeys) {
       ValueWithMetadata valueWithMetadata = getValueMaybeFromError(skyKey, bubbleErrorInfo);
       // Cycle checking: if there is a cycle, evaluation cannot progress, therefore,
@@ -1544,7 +1543,6 @@ public final class ParallelEvaluator implements Evaluator {
         if (bubbleErrorInfo == null) {
           cycleRoots.add(skyKey);
         }
-        hasError = true;
         continue;
       }
       SkyValue value = valueWithMetadata.getValue();
@@ -1555,7 +1553,6 @@ public final class ParallelEvaluator implements Evaluator {
       replayingNestedSetEventVisitor.visit(valueWithMetadata.getTransitiveEvents());
       ErrorInfo errorInfo = valueWithMetadata.getErrorInfo();
       Preconditions.checkState(value != null || errorInfo != null, skyKey);
-      hasError = hasError || (errorInfo != null);
       if (!keepGoing && errorInfo != null) {
         // value will be null here unless the value was already built on a prior keepGoing build.
         result.addError(skyKey, errorInfo);
@@ -1574,9 +1571,6 @@ public final class ParallelEvaluator implements Evaluator {
       Preconditions.checkState(visitor != null, skyKeys);
       checkForCycles(cycleRoots, result, visitor, keepGoing);
     }
-    Preconditions.checkState(bubbleErrorInfo == null || hasError,
-        "If an error bubbled up, some top-level node must be in error", bubbleErrorInfo, skyKeys);
-    result.setHasError(hasError);
     if (catastrophe) {
       // We may not have a top-level node completed. Inform the caller of the catastrophic exception
       // that shut down the evaluation so that it has some context.
@@ -1591,7 +1585,14 @@ public final class ParallelEvaluator implements Evaluator {
           bubbleErrorInfo);
       result.setCatastrophe(errorInfo.getException());
     }
-    return result.build();
+    EvaluationResult<T> builtResult = result.build();
+    Preconditions.checkState(
+        bubbleErrorInfo == null || builtResult.hasError(),
+        "If an error bubbled up, some top-level node must be in error: %s %s %s",
+        bubbleErrorInfo,
+        skyKeys,
+        builtResult);
+    return builtResult;
   }
 
   private <T extends SkyValue> void checkForCycles(
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 e1d64e5ada..1d9022022f 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
@@ -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.EvaluationResultSubjectFactory.assertThatEvaluationResult;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
@@ -56,8 +57,7 @@ public class PrepareDepsOfPatternsFunctionTest extends BuildViewTestCase {
     ImmutableList<String> patternSequence = ImmutableList.of("//foo");
 
     // When PrepareDepsOfPatternsFunction successfully completes evaluation,
-    WalkableGraph walkableGraph =
-        getGraphFromPatternsEvaluation(patternSequence, /*successExpected=*/ true);
+    WalkableGraph walkableGraph = getGraphFromPatternsEvaluation(patternSequence);
 
     // Then the graph contains a value for the target "//foo:foo",
     assertValidValue(walkableGraph, getKeyForLabel(Label.create("foo", "foo")));
@@ -76,8 +76,7 @@ public class PrepareDepsOfPatternsFunctionTest extends BuildViewTestCase {
     ImmutableList<String> patternSequence = ImmutableList.of("//foo");
 
     // When PrepareDepsOfPatternsFunction successfully completes evaluation,
-    WalkableGraph walkableGraph =
-        getGraphFromPatternsEvaluation(patternSequence, /*successExpected=*/ true);
+    WalkableGraph walkableGraph = getGraphFromPatternsEvaluation(patternSequence);
 
     // Then the graph contains an entry for ":foo"'s dependency, ":foo2".
     assertValidValue(walkableGraph, getKeyForLabel(Label.create("foo", "foo2")));
@@ -92,8 +91,7 @@ public class PrepareDepsOfPatternsFunctionTest extends BuildViewTestCase {
     ImmutableList<String> patternSequence = ImmutableList.of("//foo:*");
 
     // When PrepareDepsOfPatternsFunction successfully completes evaluation,
-    WalkableGraph walkableGraph =
-        getGraphFromPatternsEvaluation(patternSequence, /*successExpected=*/ true);
+    WalkableGraph walkableGraph = getGraphFromPatternsEvaluation(patternSequence);
 
     // Then the graph contains an entry for ":foo" and ":foo2".
     assertValidValue(walkableGraph, getKeyForLabel(Label.create("foo", "foo")));
@@ -107,8 +105,7 @@ public class PrepareDepsOfPatternsFunctionTest extends BuildViewTestCase {
     ImmutableList<String> patternSequence = ImmutableList.of(nonexistentTarget);
 
     // When PrepareDepsOfPatternsFunction completes evaluation,
-    WalkableGraph walkableGraph =
-        getGraphFromPatternsEvaluation(patternSequence, /*successExpected=*/ false);
+    WalkableGraph walkableGraph = getGraphFromPatternsEvaluation(patternSequence);
 
     // Then the graph does not contain an entry for ":foo",
     assertFalse(walkableGraph.exists(getKeyForLabel(Label.create("foo", "foo"))));
@@ -124,8 +121,7 @@ public class PrepareDepsOfPatternsFunctionTest extends BuildViewTestCase {
     ImmutableList<String> patternSequence = ImmutableList.of("//foo");
 
     // When PrepareDepsOfPatternsFunction completes evaluation,
-    WalkableGraph walkableGraph =
-        getGraphFromPatternsEvaluation(patternSequence, /*successExpected=*/ false);
+    WalkableGraph walkableGraph = getGraphFromPatternsEvaluation(patternSequence);
 
     // Then the graph contains an entry for ":foo",
     assertValidValue(
@@ -148,8 +144,7 @@ public class PrepareDepsOfPatternsFunctionTest extends BuildViewTestCase {
     ImmutableList<String> patternSequence = ImmutableList.of("//foo");
 
     // When PrepareDepsOfPatternsFunction completes evaluation,
-    WalkableGraph walkableGraph =
-        getGraphFromPatternsEvaluation(patternSequence, /*successExpected=*/ false);
+    WalkableGraph walkableGraph = getGraphFromPatternsEvaluation(patternSequence);
 
     // Then the graph contains an entry for ":foo" which has both a value and an exception,
     assertValidValue(
@@ -189,8 +184,7 @@ public class PrepareDepsOfPatternsFunctionTest extends BuildViewTestCase {
 
     // When PrepareDepsOfPatternsFunction runs in the selected keep-going mode,
     WalkableGraph walkableGraph =
-        getGraphFromPatternsEvaluation(
-            patternSequence, /*successExpected=*/ true, /*keepGoing=*/ keepGoing);
+        getGraphFromPatternsEvaluation(patternSequence, /*keepGoing=*/ keepGoing);
 
     // Then it skips evaluation of the malformed target pattern, but logs about it,
     assertContainsEvent("Skipping '" + bogusPattern + "': ");
@@ -201,14 +195,13 @@ public class PrepareDepsOfPatternsFunctionTest extends BuildViewTestCase {
 
   // Helpers:
 
-  private WalkableGraph getGraphFromPatternsEvaluation(
-      ImmutableList<String> patternSequence, boolean successExpected) throws InterruptedException {
-    return getGraphFromPatternsEvaluation(patternSequence, successExpected, /*keepGoing=*/ true);
+  private WalkableGraph getGraphFromPatternsEvaluation(ImmutableList<String> patternSequence)
+      throws InterruptedException {
+    return getGraphFromPatternsEvaluation(patternSequence, /*keepGoing=*/ true);
   }
 
   private WalkableGraph getGraphFromPatternsEvaluation(
-      ImmutableList<String> patternSequence, boolean successExpected, boolean keepGoing)
-      throws InterruptedException {
+      ImmutableList<String> patternSequence, boolean keepGoing) throws InterruptedException {
     SkyKey independentTarget = PrepareDepsOfPatternsValue.key(patternSequence, "");
     ImmutableList<SkyKey> singletonTargetPattern = ImmutableList.of(independentTarget);
 
@@ -217,14 +210,10 @@ public class PrepareDepsOfPatternsFunctionTest extends BuildViewTestCase {
         getSkyframeExecutor()
             .getDriverForTesting()
             .evaluate(singletonTargetPattern, keepGoing, LOADING_PHASE_THREADS, eventCollector);
-
-    if (successExpected) {
-      // Then the evaluation completed successfully.
-      assertFalse(evaluationResult.hasError());
-    } else {
-      // Then the evaluation resulted in some errors.
-      assertTrue(evaluationResult.hasError());
-    }
+    // Currently all callers either expect success or pass keepGoing=true, which implies success,
+    // since PrepareDepsOfPatternsFunction swallows all errors. Will need to be changed if a test
+    // that evaluates with keepGoing=false and expects errors is added.
+    assertThatEvaluationResult(evaluationResult).hasNoError();
 
     return Preconditions.checkNotNull(evaluationResult.getWalkableGraph());
   }
diff --git a/src/test/java/com/google/devtools/build/skyframe/BUILD b/src/test/java/com/google/devtools/build/skyframe/BUILD
index 1b24b4a979..cbe8e9d905 100644
--- a/src/test/java/com/google/devtools/build/skyframe/BUILD
+++ b/src/test/java/com/google/devtools/build/skyframe/BUILD
@@ -1,5 +1,7 @@
 TESTUTIL_FILES = [
     "DeterministicInMemoryGraph.java",
+    "EvaluationResultSubject.java",
+    "EvaluationResultSubjectFactory.java",
     "NotifyingInMemoryGraph.java",
     "TrackingAwaiter.java",
     "GraphTester.java",
diff --git a/src/test/java/com/google/devtools/build/skyframe/EvaluationResultSubject.java b/src/test/java/com/google/devtools/build/skyframe/EvaluationResultSubject.java
new file mode 100644
index 0000000000..63a8d396dd
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/skyframe/EvaluationResultSubject.java
@@ -0,0 +1,40 @@
+// Copyright 2016 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.skyframe;
+
+import com.google.common.truth.FailureStrategy;
+import com.google.common.truth.Subject;
+
+/**
+ * {@link Subject} for {@link EvaluationResult}. Please add to this class if you need more
+ * functionality!
+ */
+public class EvaluationResultSubject extends Subject<EvaluationResultSubject, EvaluationResult<?>> {
+  public EvaluationResultSubject(
+      FailureStrategy failureStrategy, EvaluationResult<?> evaluationResult) {
+    super(failureStrategy, evaluationResult);
+  }
+
+  public void hasError() {
+    if (!getSubject().hasError()) {
+      fail("has error");
+    }
+  }
+
+  public void hasNoError() {
+    if (getSubject().hasError()) {
+      fail("has no error");
+    }
+  }
+}
diff --git a/src/test/java/com/google/devtools/build/skyframe/EvaluationResultSubjectFactory.java b/src/test/java/com/google/devtools/build/skyframe/EvaluationResultSubjectFactory.java
new file mode 100644
index 0000000000..2bb7e4b7e9
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/skyframe/EvaluationResultSubjectFactory.java
@@ -0,0 +1,36 @@
+// Copyright 2016 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.skyframe;
+
+import com.google.common.truth.FailureStrategy;
+import com.google.common.truth.SubjectFactory;
+import com.google.common.truth.Truth;
+
+/**
+ * {@link SubjectFactory} for {@link EvaluationResult} objects, providing
+ * {@link EvaluationResultSubject}s.
+ */
+public class EvaluationResultSubjectFactory
+    extends SubjectFactory<EvaluationResultSubject, EvaluationResult<?>> {
+  public static EvaluationResultSubject assertThatEvaluationResult(
+      EvaluationResult<?> evaluationResult) {
+    return Truth.assertAbout(new EvaluationResultSubjectFactory()).that(evaluationResult);
+  }
+
+  @Override
+  public EvaluationResultSubject getSubject(
+      FailureStrategy failureStrategy, EvaluationResult<?> evaluationResult) {
+    return new EvaluationResultSubject(failureStrategy, evaluationResult);
+  }
+}
diff --git a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
index 95f9e4057f..c0389e02c4 100644
--- a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
@@ -19,6 +19,7 @@ import static com.google.common.truth.Truth.assertWithMessage;
 import static com.google.devtools.build.lib.testutil.MoreAsserts.assertContainsEvent;
 import static com.google.devtools.build.lib.testutil.MoreAsserts.assertEventCount;
 import static com.google.devtools.build.lib.testutil.MoreAsserts.assertNoEvents;
+import static com.google.devtools.build.skyframe.EvaluationResultSubjectFactory.assertThatEvaluationResult;
 import static com.google.devtools.build.skyframe.GraphTester.CONCATENATE;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -665,11 +666,11 @@ public class ParallelEvaluatorTest {
 
     EvaluationResult<SkyValue> result = eval(/*keepGoing=*/true, ImmutableList.of(recoveryKey));
     assertThat(result.errorMap()).isEmpty();
-    assertTrue(result.hasError());
+    assertThatEvaluationResult(result).hasNoError();
     assertEquals(new StringValue("i recovered"), result.get(recoveryKey));
 
     result = eval(/*keepGoing=*/false, ImmutableList.of(topKey));
-    assertTrue(result.hasError());
+    assertThatEvaluationResult(result).hasError();
     assertThat(result.keyNames()).isEmpty();
     assertEquals(1, result.errorMap().size());
     assertNotNull(result.getError(topKey).getException());
@@ -1552,11 +1553,9 @@ public class ParallelEvaluatorTest {
       assertThat(result.keyNames()).isEmpty();
       assertEquals(topException, result.getError(topKey).getException());
       assertThat(result.getError(topKey).getRootCauses()).containsExactly(topKey);
-      assertTrue(result.hasError());
+      assertThatEvaluationResult(result).hasError();
     } else {
-      // result.hasError() is set to true even if the top-level value returned has recovered from
-      // an error.
-      assertTrue(result.hasError());
+      assertThatEvaluationResult(result).hasNoError();
       assertSame(topValue, result.get(topKey));
     }
   }