From 4c6e324dfab9444fc51b9fb3f5074a01889725e1 Mon Sep 17 00:00:00 2001
From: jhorvitz <jhorvitz@google.com>
Date: Tue, 14 Dec 2021 20:16:21 -0800
Subject: [PATCH] Prohibit constructing `ErrorInfo` that is directly transient
 but not transitively transient.

PiperOrigin-RevId: 416455536
---
 .../devtools/build/skyframe/ErrorInfo.java    | 25 +++++++------
 .../build/skyframe/ErrorInfoTest.java         | 35 +++++++++++++------
 2 files changed, 40 insertions(+), 20 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/skyframe/ErrorInfo.java b/src/main/java/com/google/devtools/build/skyframe/ErrorInfo.java
index 141c47e922ce9d..c2a754be1b9bc0 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ErrorInfo.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ErrorInfo.java
@@ -16,7 +16,6 @@
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.Iterables;
 import com.google.devtools.build.skyframe.SkyFunctionException.ReifiedSkyFunctionException;
 import java.util.Collection;
 import java.util.Objects;
@@ -48,7 +47,7 @@ public static ErrorInfo fromCycle(CycleInfo cycleInfo) {
         ImmutableList.of(cycleInfo),
         /*isDirectlyTransient=*/ false,
         /*isTransitivelyTransient=*/ false,
-        /* isCatastrophic= */ false);
+        /*isCatastrophic=*/ false);
   }
 
   /** Create an ErrorInfo from a collection of existing errors. */
@@ -64,11 +63,11 @@ public static ErrorInfo fromChildErrors(SkyKey currentValue, Collection<ErrorInf
     for (ErrorInfo child : childErrors) {
       if (firstException == null) {
         // Arbitrarily pick the first error.
-        firstException = child.getException();
+        firstException = child.exception;
       }
       cycleBuilder.addAll(CycleInfo.prepareCycles(currentValue, child.cycles));
-      isTransitivelyTransient |= child.isTransitivelyTransient();
-      isCatastrophic |= child.isCatastrophic();
+      isTransitivelyTransient |= child.isTransitivelyTransient;
+      isCatastrophic |= child.isCatastrophic;
     }
 
     return new ErrorInfo(
@@ -80,9 +79,7 @@ public static ErrorInfo fromChildErrors(SkyKey currentValue, Collection<ErrorInf
   }
 
   @Nullable private final Exception exception;
-
   private final ImmutableList<CycleInfo> cycles;
-
   private final boolean isDirectlyTransient;
   private final boolean isTransitivelyTransient;
   private final boolean isCatastrophic;
@@ -93,14 +90,19 @@ public ErrorInfo(
       boolean isDirectlyTransient,
       boolean isTransitivelyTransient,
       boolean isCatastrophic) {
-    Preconditions.checkState(exception != null || !Iterables.isEmpty(cycles),
-        "At least one of exception and cycles must be non-null/empty, respectively");
-
     this.exception = exception;
     this.cycles = cycles;
     this.isDirectlyTransient = isDirectlyTransient;
     this.isTransitivelyTransient = isTransitivelyTransient;
     this.isCatastrophic = isCatastrophic;
+    Preconditions.checkArgument(
+        exception != null || !cycles.isEmpty(),
+        "At least one of exception and cycles must be present",
+        this);
+    Preconditions.checkArgument(
+        !isDirectlyTransient || isTransitivelyTransient,
+        "Cannot be directly transient but not transitively transient",
+        this);
   }
 
   @Override
@@ -188,6 +190,9 @@ public ImmutableList<CycleInfo> getCycleInfo() {
   /**
    * Returns true iff the error is directly transient, i.e. if there was a transient error
    * encountered during the computation itself.
+   *
+   * <p>A return of {@code true} implies that {@link #isTransitivelyTransient()} is also {@code
+   * true}.
    */
   public boolean isDirectlyTransient() {
     return isDirectlyTransient;
diff --git a/src/test/java/com/google/devtools/build/skyframe/ErrorInfoTest.java b/src/test/java/com/google/devtools/build/skyframe/ErrorInfoTest.java
index b2e58f527574ed..1537d8b63625da 100644
--- a/src/test/java/com/google/devtools/build/skyframe/ErrorInfoTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/ErrorInfoTest.java
@@ -26,14 +26,13 @@
 
 /** Tests for the non-trivial creation logic of {@link ErrorInfo}. */
 @RunWith(JUnit4.class)
-public class ErrorInfoTest {
+public final class ErrorInfoTest {
 
   /** Dummy SkyFunctionException implementation for the sake of testing. */
-  private static class DummySkyFunctionException extends SkyFunctionException {
+  private static final class DummySkyFunctionException extends SkyFunctionException {
     private final boolean isCatastrophic;
 
-    public DummySkyFunctionException(Exception cause, boolean isTransient,
-        boolean isCatastrophic) {
+    DummySkyFunctionException(Exception cause, boolean isTransient, boolean isCatastrophic) {
       super(cause, isTransient ? Transience.TRANSIENT : Transience.PERSISTENT);
       this.isCatastrophic = isCatastrophic;
     }
@@ -138,14 +137,30 @@ public void testFromChildErrors() {
   }
 
   @Test
-  public void testCannotCreateErrorInfoWithoutExceptionOrCycle() {
-    // Brittle, but confirms we failed for the right reason.
-    IllegalStateException e =
+  public void cannotCreateErrorInfoWithoutExceptionOrCycle() {
+    Exception e =
         assertThrows(
-            IllegalStateException.class,
-            () -> new ErrorInfo(/*exception=*/ null, ImmutableList.of(), false, false, false));
+            RuntimeException.class,
+            () ->
+                new ErrorInfo(
+                    /*exception=*/ null, /*cycles=*/ ImmutableList.of(), false, false, false));
+    assertThat(e).hasMessageThat().contains("At least one of exception and cycles must be present");
+  }
+
+  @Test
+  public void cannotCreateErrorInfoWithDirectTransienceButNotTransitiveTransience() {
+    Exception e =
+        assertThrows(
+            RuntimeException.class,
+            () ->
+                new ErrorInfo(
+                    new Exception(),
+                    /*cycles=*/ ImmutableList.of(),
+                    /*isDirectlyTransient=*/ true,
+                    /*isTransitivelyTransient=*/ false,
+                    /*isCatastrophic=*/ false));
     assertThat(e)
         .hasMessageThat()
-        .isEqualTo("At least one of exception and cycles must be non-null/empty, respectively");
+        .contains("Cannot be directly transient but not transitively transient");
   }
 }