Refactor ErrorInfo creation to share single constructor

Single constructor allows us to enforce/document high-level constraints
in a single place. Move previous constructors to static methods which do
their custom transformations but ultimately funnel their final calculated
fields through the common constructor.

Also added some tests to demonstrate expected behavior of static methods.

--
MOS_MIGRATED_REVID=104142909

3 files changed, 81 insertions, 56 deletions

diff --git a/src/main/java/com/google/devtools/build/skyframe/CycleInfo.java b/src/main/java/com/google/devtools/build/skyframe/CycleInfo.java
index 9759122def..a410ec51ca 100644
--- a/src/main/java/com/google/devtools/build/skyframe/CycleInfo.java
+++ b/src/main/java/com/google/devtools/build/skyframe/CycleInfo.java
@@ -39,7 +39,7 @@ public class CycleInfo implements Serializable {
     this(ImmutableList.<SkyKey>of(), cycle);
   }
 
-  CycleInfo(Iterable<SkyKey> pathToCycle, Iterable<SkyKey> cycle) {
+  public CycleInfo(Iterable<SkyKey> pathToCycle, Iterable<SkyKey> cycle) {
     this.pathToCycle = ImmutableList.copyOf(pathToCycle);
     this.cycle = ImmutableList.copyOf(cycle);
   }
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 210b8ed3f5..67c8da96b7 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ErrorInfo.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ErrorInfo.java
@@ -15,6 +15,7 @@ package com.google.devtools.build.skyframe;
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
@@ -32,47 +33,37 @@ import javax.annotation.Nullable;
  * <p>This is intended only for use in alternative {@code MemoizingEvaluator} implementations.
  */
 public class ErrorInfo implements Serializable {
-  /** The set of descendants of this value that failed to build. */
-  // Non-final only to allow for serialization.
-  private transient NestedSet<SkyKey> rootCauses;
-
-  /**
-   * An exception thrown upon a value's failure to build. The exception is used for reporting, and
-   * thus may ultimately be rethrown by the caller. As well, during a --nokeep_going evaluation, if
-   * an error value is encountered from an earlier --keep_going build, the exception to be thrown is
-   * taken from here.
-   */
-  @Nullable private final Exception exception;
-  private final SkyKey rootCauseOfException;
 
-  private final Iterable<CycleInfo> cycles;
-
-  private final boolean isTransient;
-  private final boolean isCatastrophic;
-
-  public ErrorInfo(ReifiedSkyFunctionException builderException) {
-    this.rootCauseOfException = builderException.getRootCauseSkyKey();
-    this.rootCauses = NestedSetBuilder.create(Order.STABLE_ORDER, rootCauseOfException);
-    this.exception = Preconditions.checkNotNull(builderException.getCause(), builderException);
-    this.cycles = ImmutableList.of();
-    this.isTransient = builderException.isTransient();
-    this.isCatastrophic = builderException.isCatastrophic();
+  /** Create an ErrorInfo from a {@link ReifiedSkyFunctionException}. */
+  public static ErrorInfo fromException(ReifiedSkyFunctionException skyFunctionException) {
+    SkyKey rootCauseSkyKey = skyFunctionException.getRootCauseSkyKey();
+    Exception rootCauseException = skyFunctionException.getCause();
+    return new ErrorInfo(
+        NestedSetBuilder.create(Order.STABLE_ORDER, rootCauseSkyKey),
+        Preconditions.checkNotNull(rootCauseException, "Cause null %s", rootCauseException),
+        rootCauseSkyKey,
+        /*cycles=*/ ImmutableList.<CycleInfo>of(),
+        skyFunctionException.isTransient(),
+        skyFunctionException.isCatastrophic());
   }
 
-  ErrorInfo(CycleInfo cycleInfo) {
-    this.rootCauses = NestedSetBuilder.emptySet(Order.STABLE_ORDER);
-    this.exception = null;
-    this.rootCauseOfException = null;
-    this.cycles = ImmutableList.of(cycleInfo);
-    this.isTransient = false;
-    this.isCatastrophic = false;
+  /** Create an ErrorInfo from a {@link CycleInfo}. */
+  static ErrorInfo fromCycle(CycleInfo cycleInfo) {
+    return new ErrorInfo(
+        /*rootCauses=*/ NestedSetBuilder.<SkyKey>emptySet(Order.STABLE_ORDER),
+        /*exception=*/ null,
+        /*rootCauseOfException=*/ null,
+        ImmutableList.of(cycleInfo),
+        /*isTransient=*/ false,
+        /*isCatostrophic=*/ false);
   }
 
-  public ErrorInfo(SkyKey currentValue, Collection<ErrorInfo> childErrors) {
-    Preconditions.checkNotNull(currentValue);
-    Preconditions.checkState(!childErrors.isEmpty(),
-        "Error value %s with no exception must depend on another error value", currentValue);
-    NestedSetBuilder<SkyKey> builder = NestedSetBuilder.stableOrder();
+  /** Create an ErrorInfo from a collection of existing errors. */
+  public static ErrorInfo fromChildErrors(SkyKey currentValue, Collection<ErrorInfo> childErrors) {
+    Preconditions.checkNotNull(currentValue, "currentValue must not be null");
+    Preconditions.checkState(!childErrors.isEmpty(), "childErrors may not be empty");
+
+    NestedSetBuilder<SkyKey> rootCausesBuilder = NestedSetBuilder.stableOrder();
     ImmutableList.Builder<CycleInfo> cycleBuilder = ImmutableList.builder();
     Exception firstException = null;
     SkyKey firstChildKey = null;
@@ -83,18 +74,48 @@ public class ErrorInfo implements Serializable {
         firstException = child.getException();
         firstChildKey = child.getRootCauseOfException();
       }
-      builder.addTransitive(child.rootCauses);
+      rootCausesBuilder.addTransitive(child.rootCauses);
       cycleBuilder.addAll(CycleInfo.prepareCycles(currentValue, child.cycles));
       isCatastrophic |= child.isCatastrophic();
     }
-    this.rootCauses = builder.build();
-    this.exception = firstException;
-    this.rootCauseOfException = firstChildKey;
-    this.cycles = cycleBuilder.build();
-    // Parent errors should not be transient -- we depend on the child's transience, if any, to
-    // force re-evaluation if necessary.
-    this.isTransient = false;
-    this.isCatastrophic = isCatastrophic;
+
+    return new ErrorInfo(
+        rootCausesBuilder.build(),
+        firstException,
+        firstChildKey,
+        cycleBuilder.build(),
+        // Parent errors should not be transient -- we depend on the child's transience, if any, to
+        // force re-evaluation if necessary.
+        /*isTransient=*/ false,
+        isCatastrophic);
+  }
+
+  // Non-final only to allow for serialization.
+  private transient NestedSet<SkyKey> rootCauses;
+
+  @Nullable private final Exception exception;
+  private final SkyKey rootCauseOfException;
+
+  private final ImmutableList<CycleInfo> cycles;
+
+  private final boolean isTransient;
+  private final boolean isCatastrophic;
+
+  public ErrorInfo(NestedSet<SkyKey> rootCauses, @Nullable Exception exception,
+      SkyKey rootCauseOfException, ImmutableList<CycleInfo> cycles, boolean isTransient,
+      boolean isCatostrophic) {
+    Preconditions.checkState(exception != null || !Iterables.isEmpty(cycles),
+        "At least one of exception and cycles must be non-null/empty, respectively");
+    Preconditions.checkState((exception == null) == (rootCauseOfException == null),
+        "exception and rootCauseOfException must both be null or non-null, got %s  %s",
+        exception, rootCauseOfException);
+
+    this.rootCauses = rootCauses;
+    this.exception = exception;
+    this.rootCauseOfException = rootCauseOfException;
+    this.cycles = cycles;
+    this.isTransient = isTransient;
+    this.isCatastrophic = isCatostrophic;
   }
 
   @Override
@@ -116,6 +137,10 @@ public class ErrorInfo implements Serializable {
   /**
    * The exception thrown when building a value. May be null if value's only error is depending
    * on a cycle.
+   *
+   * <p>The exception is used for reporting and thus may ultimately be rethrown by the caller.
+   * As well, during a --nokeep_going evaluation, if an error value is encountered from an earlier
+   * --keep_going build, the exception to be thrown is taken from here.
    */
   @Nullable public Exception getException() {
     return exception;
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 e8de292b26..6c080f4b38 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
@@ -117,7 +117,7 @@ public final class ParallelEvaluator implements Evaluator {
       return state.getValue();
     }
   }
-  
+
   /** An general interface for {@link ParallelEvaluator} to receive objects of type {@code T}. */
   public interface Receiver<T> {
     // TODO(dmarting): should we just make it a common object for all Bazel codebase?
@@ -269,7 +269,7 @@ public final class ParallelEvaluator implements Evaluator {
      */
     private void finalizeErrorInfo() {
       if (errorInfo == null && !childErrorInfos.isEmpty()) {
-        errorInfo = new ErrorInfo(skyKey, childErrorInfos);
+        errorInfo = ErrorInfo.fromChildErrors(skyKey, childErrorInfos);
       }
     }
 
@@ -826,7 +826,7 @@ public final class ParallelEvaluator implements Evaluator {
           }
 
           registerNewlyDiscoveredDepsForDoneEntry(skyKey, state, env);
-          ErrorInfo errorInfo = new ErrorInfo(reifiedBuilderException);
+          ErrorInfo errorInfo = ErrorInfo.fromException(reifiedBuilderException);
           env.setError(errorInfo);
           env.commit(/*enqueueParents=*/keepGoing);
           if (!shouldFailFast) {
@@ -1297,9 +1297,9 @@ public final class ParallelEvaluator implements Evaluator {
         ReifiedSkyFunctionException reifiedBuilderException =
             new ReifiedSkyFunctionException(builderException, parent);
         if (reifiedBuilderException.getRootCauseSkyKey().equals(parent)) {
-          error = new ErrorInfo(reifiedBuilderException);
+          error = ErrorInfo.fromException(reifiedBuilderException);
           bubbleErrorInfo.put(errorKey,
-              ValueWithMetadata.error(new ErrorInfo(errorKey, ImmutableSet.of(error)),
+              ValueWithMetadata.error(ErrorInfo.fromChildErrors(errorKey, ImmutableSet.of(error)),
                   env.buildEvents(/*missingChildren=*/true)));
           continue;
         }
@@ -1314,7 +1314,7 @@ public final class ParallelEvaluator implements Evaluator {
       }
       // Builder didn't throw an exception, so just propagate this one up.
       bubbleErrorInfo.put(errorKey,
-          ValueWithMetadata.error(new ErrorInfo(errorKey, ImmutableSet.of(error)),
+          ValueWithMetadata.error(ErrorInfo.fromChildErrors(errorKey, ImmutableSet.of(error)),
               env.buildEvents(/*missingChildren=*/true)));
     }
 
@@ -1483,7 +1483,7 @@ public final class ParallelEvaluator implements Evaluator {
             "Value %s was not successfully evaluated, but had no child errors. ValueEntry: %s", key,
             entry);
         SkyFunctionEnvironment env = new SkyFunctionEnvironment(key, directDeps, visitor);
-        env.setError(new ErrorInfo(key, errorDeps));
+        env.setError(ErrorInfo.fromChildErrors(key, errorDeps));
         env.commit(/*enqueueParents=*/false);
       }
 
@@ -1531,8 +1531,8 @@ public final class ParallelEvaluator implements Evaluator {
               getChildrenErrors(entry.getTemporaryDirectDeps(), /*unfinishedChild=*/cycleChild);
           CycleInfo cycleInfo = new CycleInfo(cycle);
           // Add in this cycle.
-          allErrors.add(new ErrorInfo(cycleInfo));
-          env.setError(new ErrorInfo(key, allErrors));
+          allErrors.add(ErrorInfo.fromCycle(cycleInfo));
+          env.setError(ErrorInfo.fromChildErrors(key, allErrors));
           env.commit(/*enqueueParents=*/false);
           continue;
         } else {
@@ -1540,7 +1540,7 @@ public final class ParallelEvaluator implements Evaluator {
           // path) and return.
           Preconditions.checkState(graphPath.get(0).equals(root),
               "%s not reached from %s. ValueEntry: %s", key, root, entry);
-          return new ErrorInfo(new CycleInfo(graphPath.subList(0, cycleStart), cycle));
+          return ErrorInfo.fromCycle(new CycleInfo(graphPath.subList(0, cycleStart), cycle));
         }
       }