Retain first error message during transitive traversal

Previously, a TransitiveTraversalFunction computed a value that
contained an exception only if the exception resulted from a failure
to load the function's immediate target. If the target had transitive
dependencies on other targets, those other targets would be loaded, but
any errors resulting from loading those targets would not be retained.

SkyQueryEnvironment impromperly used a SkyFrame mechanism (which was
solely intended to ensure equivalent semantics between keep-going and
no-keep-going evaluations) to discover errors in the set of transitive
children of TransitiveTraversal nodes.

In order to transition SkyQueryEnvironment away from that mechanism,
this CL changes TransitiveTraversalFunction to remember the first error
message encountered while loading its target and its transitive
dependencies.

By remembering just the error message as a string, and not the full
exception object, this also helps TransitiveTraversalValue have more
consistent equality semantics for change-pruning.

--
MOS_MIGRATED_REVID=105977182

2 files changed, 94 insertions, 43 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java
index 416ef33e9b..7084ecfd01 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
+import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
@@ -23,7 +24,7 @@ import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.NoSuchThingException;
 import com.google.devtools.build.lib.packages.Target;
-import com.google.devtools.build.lib.skyframe.TransitiveTraversalFunction.DummyAccumulator;
+import com.google.devtools.build.lib.skyframe.TransitiveTraversalFunction.FirstErrorMessageAccumulator;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 import com.google.devtools.build.skyframe.ValueOrException2;
@@ -32,13 +33,17 @@ import java.util.Collection;
 import java.util.Map.Entry;
 import java.util.Set;
 
+import javax.annotation.Nullable;
+
 /**
  * This class is like {@link TransitiveTargetFunction}, but the values it returns do not contain
- * {@link NestedSet}s. It should be used only when the side-effects of {@link
- * TransitiveTargetFunction} on the skyframe graph are desired (i.e., ensuring that transitive
- * targets and their packages have been loaded).
+ * {@link NestedSet}s. It performs the side-effects of {@link TransitiveTargetFunction} (i.e.,
+ * ensuring that transitive targets and their packages have been loaded). It evaluates to a
+ * {@link TransitiveTraversalValue} that contains the first error message it encountered, and a
+ * set of names of providers if the target is a rule.
  */
-public class TransitiveTraversalFunction extends TransitiveBaseTraversalFunction<DummyAccumulator> {
+public class TransitiveTraversalFunction
+    extends TransitiveBaseTraversalFunction<FirstErrorMessageAccumulator> {
 
   @Override
   SkyKey getKey(Label label) {
@@ -46,15 +51,36 @@ public class TransitiveTraversalFunction extends TransitiveBaseTraversalFunction
   }
 
   @Override
-  DummyAccumulator processTarget(Label label, TargetAndErrorIfAny targetAndErrorIfAny) {
-    return DummyAccumulator.INSTANCE;
+  FirstErrorMessageAccumulator processTarget(Label label, TargetAndErrorIfAny targetAndErrorIfAny) {
+    NoSuchTargetException errorIfAny = targetAndErrorIfAny.getErrorLoadingTarget();
+    String errorMessageIfAny = errorIfAny == null ? null : errorIfAny.getMessage();
+    return new FirstErrorMessageAccumulator(errorMessageIfAny);
   }
 
   @Override
-  void processDeps(DummyAccumulator processedTargets, EventHandler eventHandler,
+  void processDeps(
+      FirstErrorMessageAccumulator accumulator,
+      EventHandler eventHandler,
       TargetAndErrorIfAny targetAndErrorIfAny,
       Iterable<Entry<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>>>
           depEntries) {
+    for (Entry<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> entry :
+        depEntries) {
+      TransitiveTraversalValue transitiveTraversalValue;
+      try {
+        transitiveTraversalValue = (TransitiveTraversalValue) entry.getValue().get();
+        if (transitiveTraversalValue == null) {
+          continue;
+        }
+      } catch (NoSuchPackageException | NoSuchTargetException e) {
+        accumulator.maybeSet(e.getMessage());
+        continue;
+      }
+      String firstErrorMessage = transitiveTraversalValue.getFirstErrorMessage();
+      if (firstErrorMessage != null) {
+        accumulator.maybeSet(firstErrorMessage);
+      }
+    }
   }
 
   protected Collection<Label> getAspectLabels(Target fromTarget, Attribute attr, Label toLabel,
@@ -79,21 +105,38 @@ public class TransitiveTraversalFunction extends TransitiveBaseTraversalFunction
   }
 
   @Override
-  SkyValue computeSkyValue(TargetAndErrorIfAny targetAndErrorIfAny,
-      DummyAccumulator processedTargets) {
-    NoSuchTargetException errorLoadingTarget = targetAndErrorIfAny.getErrorLoadingTarget();
-    return errorLoadingTarget == null
-        ? TransitiveTraversalValue.forTarget(targetAndErrorIfAny.getTarget())
-        : TransitiveTraversalValue.unsuccessfulTransitiveTraversal(errorLoadingTarget);
+  SkyValue computeSkyValue(
+      TargetAndErrorIfAny targetAndErrorIfAny, FirstErrorMessageAccumulator accumulator) {
+    boolean targetLoadedSuccessfully = targetAndErrorIfAny.getErrorLoadingTarget() == null;
+    String firstErrorMessage = accumulator.getFirstErrorMessage();
+    return targetLoadedSuccessfully
+        ? TransitiveTraversalValue.forTarget(targetAndErrorIfAny.getTarget(), firstErrorMessage)
+        : TransitiveTraversalValue.unsuccessfulTransitiveTraversal(firstErrorMessage);
   }
 
- /**
-   * Because {@link TransitiveTraversalFunction} is invoked only when its side-effects are desired,
-   * this value accumulator has nothing to keep track of.
+  /**
+   * Keeps track of the first error message encountered while traversing itself and its
+   * dependencies.
    */
-  static class DummyAccumulator {
-    static final DummyAccumulator INSTANCE = new DummyAccumulator();
+  static class FirstErrorMessageAccumulator {
+
+    @Nullable private String firstErrorMessage;
 
-    private DummyAccumulator() {}
+    public FirstErrorMessageAccumulator(@Nullable String firstErrorMessage) {
+      this.firstErrorMessage = firstErrorMessage;
+    }
+
+    /** Remembers {@param errorMessage} if it is the first error message. */
+    void maybeSet(String errorMessage) {
+      Preconditions.checkNotNull(errorMessage);
+      if (firstErrorMessage == null) {
+        firstErrorMessage = errorMessage;
+      }
+    }
+
+    @Nullable
+    String getFirstErrorMessage() {
+      return firstErrorMessage;
+    }
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalValue.java
index 483167811a..05b3c6a32c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalValue.java
@@ -19,7 +19,6 @@ import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
-import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.util.StringCanonicalizer;
@@ -33,40 +32,41 @@ import java.util.Set;
 import javax.annotation.Nullable;
 
 /**
- * A <i>transitive</i> target reference that, when built in skyframe, loads the entire
- * transitive closure of a target. Contains no information about the targets traversed.
+ * A <i>transitive</i> target reference that, when built in skyframe, loads the entire transitive
+ * closure of a target. Retains the first error message found during the transitive traversal,
+ * and a set of names of providers if the target is a {@link Rule}.
  */
 @Immutable
 @ThreadSafe
 public class TransitiveTraversalValue implements SkyValue {
 
-  @Nullable
-  private final NoSuchTargetException errorLoadingTarget;
-  @Nullable
-  private final ImmutableSet<String> providers;
+  @Nullable private final ImmutableSet<String> providers;
+  @Nullable private final String firstErrorMessage;
 
-  private TransitiveTraversalValue(@Nullable Iterable<String> providers,
-      @Nullable NoSuchTargetException errorLoadingTarget) {
-    this.errorLoadingTarget = errorLoadingTarget;
+  private TransitiveTraversalValue(
+      @Nullable Iterable<String> providers, @Nullable String firstErrorMessage) {
     this.providers = (providers == null) ? null : canonicalSet(providers);
+    this.firstErrorMessage =
+        (firstErrorMessage == null) ? null : StringCanonicalizer.intern(firstErrorMessage);
   }
 
-  public static TransitiveTraversalValue unsuccessfulTransitiveTraversal(
-      NoSuchTargetException errorLoadingTarget) {
-    return new TransitiveTraversalValue(null, Preconditions.checkNotNull(errorLoadingTarget));
+  public static TransitiveTraversalValue unsuccessfulTransitiveTraversal(String firstErrorMessage) {
+    return new TransitiveTraversalValue(null, Preconditions.checkNotNull(firstErrorMessage));
   }
 
-  public static TransitiveTraversalValue forTarget(Target target) {
+  public static TransitiveTraversalValue forTarget(
+      Target target, @Nullable String firstErrorMessage) {
     if (target instanceof Rule) {
       Rule rule = (Rule) target;
       return new TransitiveTraversalValue(
-          toStringSet(rule.getRuleClassObject().getAdvertisedProviders()), null);
+          toStringSet(rule.getRuleClassObject().getAdvertisedProviders()), firstErrorMessage);
     }
-    return new TransitiveTraversalValue(ImmutableList.<String>of(), null);
+    return new TransitiveTraversalValue(ImmutableList.<String>of(), firstErrorMessage);
   }
 
-  public static TransitiveTraversalValue withProviders(Collection<String> vals) {
-    return new TransitiveTraversalValue(ImmutableSet.copyOf(vals), null);
+  public static TransitiveTraversalValue withProviders(
+      Collection<String> providers, @Nullable String firstErrorMessage) {
+    return new TransitiveTraversalValue(ImmutableSet.copyOf(providers), firstErrorMessage);
   }
 
   private static ImmutableSet<String> canonicalSet(Iterable<String> strIterable) {
@@ -87,14 +87,22 @@ public class TransitiveTraversalValue implements SkyValue {
     return pBuilder.build();
   }
 
+  /**
+   * Returns the set of provider names from the target, if the target is a {@link Rule}. If there
+   * were errors loading the target, returns {@code null}.
+   */
+  @Nullable
   public Set<String> getProviders() {
     return providers;
   }
 
-  /** Returns the error, if any, from loading the target. */
+  /**
+   * Returns the first error message, if any, from loading the target and its transitive
+   * dependencies.
+   */
   @Nullable
-  public NoSuchTargetException getErrorLoadingTarget() {
-    return errorLoadingTarget;
+  public String getFirstErrorMessage() {
+    return firstErrorMessage;
   }
 
   @Override
@@ -106,13 +114,13 @@ public class TransitiveTraversalValue implements SkyValue {
       return false;
     }
     TransitiveTraversalValue that = (TransitiveTraversalValue) o;
-    return Objects.equals(this.errorLoadingTarget, that.errorLoadingTarget)
+    return Objects.equals(this.firstErrorMessage, that.firstErrorMessage)
         && Objects.equals(this.providers, that.providers);
   }
 
   @Override
   public int hashCode() {
-    return 31 * Objects.hashCode(errorLoadingTarget) + Objects.hashCode(providers);
+    return 31 * Objects.hashCode(firstErrorMessage) + Objects.hashCode(providers);
   }
 
   @ThreadSafe