Implement proper error handling for interleaved loading and analysis.

Add test coverage by re-running BuildViewTest with the new Skyframe loading
phase runner.

--
MOS_MIGRATED_REVID=113517509

6 files changed, 116 insertions, 64 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index 403528e020..35d3c123b1 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -809,7 +809,7 @@ public class BuildView {
       }
 
       @Override
-      protected Target getTarget(Label label) {
+      protected Target getTarget(Target from, Label label, NestedSetBuilder<Label> rootCauses) {
         if (targetCache == null) {
           try {
             return LoadedPackageProvider.Bridge.getLoadedTarget(
@@ -879,9 +879,13 @@ public class BuildView {
       }
 
       @Override
-      protected Target getTarget(Label label) throws NoSuchThingException {
-        return LoadedPackageProvider.Bridge.getLoadedTarget(
-            skyframeExecutor.getPackageManager(), eventHandler, label);
+      protected Target getTarget(Target from, Label label, NestedSetBuilder<Label> rootCauses) {
+        try {
+          return LoadedPackageProvider.Bridge.getLoadedTarget(
+              skyframeExecutor.getPackageManager(), eventHandler, label);
+        } catch (NoSuchThingException e) {
+          throw new IllegalStateException(e);
+        }
       }
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
index 8d30b12f47..2d7edd2419 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
@@ -157,17 +157,6 @@ public abstract class DependencyResolver {
     return outgoingEdges;
   }
 
-  @Nullable
-  private Target getTarget(Target from, Label label, NestedSetBuilder<Label> rootCauses) {
-    try {
-      return getTarget(label);
-    } catch (NoSuchThingException e) {
-      rootCauses.add(label);
-      missingEdgeHook(from, label, e);
-    }
-    return null;
-  }
-
   private ListMultimap<Attribute, LabelAndConfiguration> resolveAttributes(
       Rule rule, AspectDefinition aspect, BuildConfiguration configuration,
       BuildConfiguration hostConfiguration, Set<ConfigMatchingProvider> configConditions)
@@ -615,13 +604,11 @@ public abstract class DependencyResolver {
   /**
    * Returns the target by the given label.
    *
-   * <p>Throws {@link NoSuchThingException} if the target is known not to exist.
-   *
    * <p>Returns null if the target is not ready to be returned at this moment. If getTarget returns
    * null once or more during a {@link #dependentNodeMap} call, the results of that call will be
    * incomplete. For use within Skyframe, where several iterations may be needed to discover
    * all dependencies.
    */
   @Nullable
-  protected abstract Target getTarget(Label label) throws NoSuchThingException;
+  protected abstract Target getTarget(Target from, Label label, NestedSetBuilder<Label> rootCauses);
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
index dba5155f25..1224c3ec42 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
@@ -73,6 +73,7 @@ import com.google.devtools.build.skyframe.ValueOrException;
 import com.google.devtools.build.skyframe.ValueOrException2;
 
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
@@ -85,6 +86,10 @@ import javax.annotation.Nullable;
  * SkyFunction for {@link ConfiguredTargetValue}s.
  */
 final class ConfiguredTargetFunction implements SkyFunction {
+  // This construction is a bit funky, but guarantees that the Object reference here is globally
+  // unique.
+  static final Set<ConfigMatchingProvider> NO_CONFIG_CONDITIONS =
+      Collections.unmodifiableSet(ImmutableSet.<ConfigMatchingProvider>of());
 
   /**
    * Exception class that signals an error during the evaluation of a dependency.
@@ -138,19 +143,27 @@ final class ConfiguredTargetFunction implements SkyFunction {
     if (packageValue == null) {
       return null;
     }
+
+    // TODO(ulfjack): This tries to match the logic in TransitiveTargetFunction /
+    // TargetMarkerFunction. Maybe we can merge the two?
     Package pkg = packageValue.getPackage();
-    if (pkg.containsErrors()) {
-      throw new ConfiguredTargetFunctionException(
-          new BuildFileContainsErrorsException(lc.getLabel().getPackageIdentifier()));
-    }
     Target target;
     try {
-      target = packageValue.getPackage().getTarget(lc.getLabel().getName());
+      target = pkg.getTarget(lc.getLabel().getName());
     } catch (NoSuchTargetException e) {
-      throw new ConfiguredTargetFunctionException(e);
+      throw new ConfiguredTargetFunctionException(
+          new ConfiguredValueCreationException(e.getMessage()));
     }
-
-    transitivePackages.add(packageValue.getPackage());
+    if (pkg.containsErrors()) {
+      if (target == null) {
+        throw new ConfiguredTargetFunctionException(new ConfiguredValueCreationException(
+            new BuildFileContainsErrorsException(
+                lc.getLabel().getPackageIdentifier()).getMessage()));
+      } else {
+        transitiveLoadingRootCauses.add(lc.getLabel());
+      }
+    }
+    transitivePackages.add(pkg);
     // TODO(bazel-team): This is problematic - we create the right key, but then end up with a value
     // that doesn't match; we can even have the same value multiple times. However, I think it's
     // only triggered in tests (i.e., in normal operation, the configuration passed in is already
@@ -158,11 +171,9 @@ final class ConfiguredTargetFunction implements SkyFunction {
     if (!target.isConfigurable()) {
       configuration = null;
     }
+    TargetAndConfiguration ctgValue = new TargetAndConfiguration(target, configuration);
 
     SkyframeDependencyResolver resolver = view.createDependencyResolver(env);
-
-    TargetAndConfiguration ctgValue =
-        new TargetAndConfiguration(target, configuration);
     try {
       // Get the configuration targets that trigger this rule's configurable attributes.
       Set<ConfigMatchingProvider> configConditions = getConfigConditions(
@@ -171,6 +182,17 @@ final class ConfiguredTargetFunction implements SkyFunction {
       if (env.valuesMissing()) {
         return null;
       }
+      // TODO(ulfjack): ConfiguredAttributeMapper (indirectly used from computeDependencies) isn't
+      // safe to use if there are missing config conditions, so we stop here, but only if there are
+      // config conditions - though note that we can't check if configConditions is non-empty - it
+      // may be empty for other reasons. It would be better to continue here so that we can collect
+      // more root causes during computeDependencies.
+      // Note that this doesn't apply to AspectFunction, because aspects can't have configurable
+      // attributes.
+      if (!transitiveLoadingRootCauses.isEmpty() && configConditions != NO_CONFIG_CONDITIONS) {
+        throw new ConfiguredTargetFunctionException(
+            new ConfiguredValueCreationException(transitiveLoadingRootCauses.build()));
+      }
 
       ListMultimap<Attribute, ConfiguredTarget> depValueMap =
           computeDependencies(
@@ -654,7 +676,7 @@ final class ConfiguredTargetFunction implements SkyFunction {
       NestedSetBuilder<Label> transitiveLoadingRootCauses)
       throws DependencyEvaluationException {
     if (!(target instanceof Rule)) {
-      return ImmutableSet.of();
+      return NO_CONFIG_CONDITIONS;
     }
 
     ImmutableSet.Builder<ConfigMatchingProvider> configConditions = ImmutableSet.builder();
@@ -672,7 +694,7 @@ final class ConfiguredTargetFunction implements SkyFunction {
       }
     }
     if (configLabelMap.isEmpty()) {
-      return ImmutableSet.of();
+      return NO_CONFIG_CONDITIONS;
     }
 
     // Collect the corresponding Skyframe configured target values. Abort early if they haven't
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
index c2d17150cd..3f0aa18735 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -51,9 +51,12 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.NoSuchPackageException;
+import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.RuleClassProvider;
 import com.google.devtools.build.lib.packages.Target;
+import com.google.devtools.build.lib.pkgcache.LoadingFailureEvent;
 import com.google.devtools.build.lib.pkgcache.LoadingPhaseRunner;
 import com.google.devtools.build.lib.skyframe.ActionLookupValue.ActionLookupKey;
 import com.google.devtools.build.lib.skyframe.AspectFunction.AspectCreationException;
@@ -336,39 +339,39 @@ public final class SkyframeBuildView {
     // --keep_going : We notify the error and return a ConfiguredTargetValue
     for (Map.Entry<SkyKey, ErrorInfo> errorEntry : result.errorMap().entrySet()) {
       // Only handle errors of configured targets, not errors of top-level aspects.
+      // TODO(ulfjack): this is quadratic - if there are a lot of CTs, this could be rather slow.
       if (!values.contains(errorEntry.getKey().argument())) {
         continue;
       }
       SkyKey errorKey = errorEntry.getKey();
       ConfiguredTargetKey label = (ConfiguredTargetKey) errorKey.argument();
+      Label topLevelLabel = label.getLabel();
       ErrorInfo errorInfo = errorEntry.getValue();
       assertSaneAnalysisError(errorInfo, errorKey);
 
       skyframeExecutor.getCyclesReporter().reportCycles(errorInfo.getCycleInfo(), errorKey,
           eventHandler);
       Exception cause = errorInfo.getException();
-      // We try to get the root cause key first from ErrorInfo rootCauses. If we don't have one
-      // we try to use the cycle culprit if the error is a cycle. Otherwise we use the top-level
-      // error key.
-      Label analysisRootCause;
+      Label analysisRootCause = null;
       if (cause instanceof ConfiguredValueCreationException) {
-        analysisRootCause = ((ConfiguredValueCreationException) cause).getAnalysisRootCause();
-      } else if (!Iterables.isEmpty(errorEntry.getValue().getRootCauses())) {
-        SkyKey culprit = Preconditions.checkNotNull(Iterables.getFirst(
-            errorEntry.getValue().getRootCauses(), null));
-        analysisRootCause = ((ConfiguredTargetKey) culprit.argument()).getLabel();
-      } else {
-        analysisRootCause = maybeGetConfiguredTargetCycleCulprit(errorInfo.getCycleInfo());
-      }
-      if (cause instanceof ActionConflictException) {
+        ConfiguredValueCreationException ctCause = (ConfiguredValueCreationException) cause;
+        for (Label rootCause : ctCause.getRootCauses()) {
+          eventBus.post(new LoadingFailureEvent(topLevelLabel, rootCause));
+        }
+        analysisRootCause = ctCause.getAnalysisRootCause();
+      } else if (!Iterables.isEmpty(errorInfo.getCycleInfo())) {
+        analysisRootCause = maybeGetConfiguredTargetCycleCulprit(
+            topLevelLabel, errorInfo.getCycleInfo());
+      } else if (cause instanceof ActionConflictException) {
         ((ActionConflictException) cause).reportTo(eventHandler);
       }
       eventHandler.handle(
           Event.warn("errors encountered while analyzing target '"
-              + label.getLabel() + "': it will not be built"));
-      eventBus.post(new AnalysisFailureEvent(
-          LabelAndConfiguration.of(label.getLabel(), label.getConfiguration()),
-          analysisRootCause));
+              + topLevelLabel + "': it will not be built"));
+      if (analysisRootCause != null) {
+        eventBus.post(new AnalysisFailureEvent(
+            LabelAndConfiguration.of(topLevelLabel, label.getConfiguration()), analysisRootCause));
+      }
     }
 
     Collection<Exception> reportedExceptions = Sets.newHashSet();
@@ -413,14 +416,27 @@ public final class SkyframeBuildView {
   }
 
   @Nullable
-  private Label maybeGetConfiguredTargetCycleCulprit(Iterable<CycleInfo> cycleInfos) {
+  private static Label maybeGetConfiguredTargetCycleCulprit(
+      Label labelToLoad, Iterable<CycleInfo> cycleInfos) {
     for (CycleInfo cycleInfo : cycleInfos) {
       SkyKey culprit = Iterables.getFirst(cycleInfo.getCycle(), null);
       if (culprit == null) {
         continue;
       }
       if (culprit.functionName().equals(SkyFunctions.CONFIGURED_TARGET)) {
-        return ((LabelAndConfiguration) culprit.argument()).getLabel();
+        return ((ConfiguredTargetKey) culprit.argument()).getLabel();
+      } else {
+        // For other types of cycles (e.g. file symlink cycles), the root cause is the furthest
+        // target dependency that itself depended on the cycle.
+        Label furthestTarget = labelToLoad;
+        for (SkyKey skyKey : cycleInfo.getPathToCycle()) {
+          if (skyKey.functionName().equals(SkyFunctions.CONFIGURED_TARGET)) {
+            furthestTarget = (Label) skyKey.argument();
+          } else {
+            break;
+          }
+        }
+        return furthestTarget;
       }
     }
     return null;
@@ -436,7 +452,10 @@ public final class SkyframeBuildView {
               || cause instanceof ActionConflictException
               // For top-level aspects
               || cause instanceof AspectCreationException
-              || cause instanceof SkylarkImportFailedException,
+              || cause instanceof SkylarkImportFailedException
+              // Only if we run the reduced loading phase and then analyze with --nokeep_going.
+              || cause instanceof NoSuchTargetException
+              || cause instanceof NoSuchPackageException,
           "%s -> %s",
           key,
           errorInfo);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeDependencyResolver.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeDependencyResolver.java
index c609e99148..0655d47910 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeDependencyResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeDependencyResolver.java
@@ -16,6 +16,7 @@ package com.google.devtools.build.lib.skyframe;
 import com.google.devtools.build.lib.analysis.DependencyResolver;
 import com.google.devtools.build.lib.analysis.TargetAndConfiguration;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
@@ -77,22 +78,38 @@ public final class SkyframeDependencyResolver extends DependencyResolver {
 
   @Nullable
   @Override
-  protected Target getTarget(Label label) throws NoSuchThingException {
-    // TODO(ulfjack): This swallows all loading errors without reporting. That's ok for now, as we
-    // generally run a loading phase first, and only analyze targets that load successfully.
-    if (env.getValue(TargetMarkerValue.key(label)) == null) {
+  protected Target getTarget(Target from, Label label, NestedSetBuilder<Label> rootCauses) {
+    SkyKey key = PackageValue.key(label.getPackageIdentifier());
+    PackageValue packageValue;
+    try {
+      packageValue = (PackageValue) env.getValueOrThrow(key, NoSuchPackageException.class);
+    } catch (NoSuchPackageException e) {
+      rootCauses.add(label);
+      missingEdgeHook(from, label, e);
       return null;
     }
-    SkyKey key = PackageValue.key(label.getPackageIdentifier());
-    PackageValue packageValue =
-        (PackageValue) env.getValueOrThrow(key, NoSuchPackageException.class);
     if (packageValue == null) {
       return null;
     }
     Package pkg = packageValue.getPackage();
-    if (pkg.containsErrors()) {
-      throw new BuildFileContainsErrorsException(label.getPackageIdentifier());
+    try {
+      Target target = pkg.getTarget(label.getName());
+      if (pkg.containsErrors()) {
+        NoSuchPackageException e =
+            new BuildFileContainsErrorsException(label.getPackageIdentifier());
+        missingEdgeHook(from, label, e);
+        if (target != null) {
+          rootCauses.add(label);
+          return target;
+        } else {
+          return null;
+        }
+      }
+      return target;
+    } catch (NoSuchTargetException e) {
+      rootCauses.add(label);
+      missingEdgeHook(from, label, e);
+      return null;
     }
-    return packageValue.getPackage().getTarget(label.getName());
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index d7eda03665..bfc7fb3453 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -316,7 +316,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       Root buildDataDirectory,
       PackageFactory pkgFactory,
       Predicate<PathFragment> allowedMissingInputs) {
-    RuleClassProvider ruleClassProvider = pkgFactory.getRuleClassProvider();
+    ConfiguredRuleClassProvider ruleClassProvider =
+        (ConfiguredRuleClassProvider) pkgFactory.getRuleClassProvider();
     // We use an immutable map builder for the nice side effect that it throws if a duplicate key
     // is inserted.
     ImmutableMap.Builder<SkyFunctionName, SkyFunction> map = ImmutableMap.builder();
@@ -1632,8 +1633,10 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     return new CyclesReporter(
         new TransitiveTargetCycleReporter(packageManager),
         new ActionArtifactCycleReporter(packageManager),
-        new SkylarkModuleCycleReporter(),
-        new ConfiguredTargetCycleReporter(packageManager));
+        // TODO(ulfjack): The SkylarkModuleCycleReporter swallows previously reported cycles
+        // unconditionally! Is that intentional?
+        new ConfiguredTargetCycleReporter(packageManager),
+        new SkylarkModuleCycleReporter());
   }
 
   CyclesReporter getCyclesReporter() {