From 849017381db007108b3a9e25b1dbf07b7587d31c Mon Sep 17 00:00:00 2001
From: Ulf Adams <ulfjack@google.com>
Date: Thu, 28 Jan 2016 15:05:16 +0000
Subject: Refactor DependencyResolver to collect and return loading errors.

This should never be triggered in production, where we always run a loading
phase first and only analyze targets that load successfully. I.e., this is
just plumbing which will be hooked up in a subsequent change.

--
MOS_MIGRATED_REVID=113258593
---
 .../devtools/build/lib/analysis/BuildView.java     |  16 ++-
 .../build/lib/analysis/DependencyResolver.java     | 154 +++++++++++++--------
 .../build/lib/skyframe/AspectFunction.java         |  47 ++++++-
 .../lib/skyframe/ConfiguredTargetFunction.java     | 125 +++++++++++------
 .../lib/skyframe/PostConfiguredTargetFunction.java |   3 +
 .../build/lib/skyframe/SkyframeBuildView.java      |   3 +
 .../lib/skyframe/SkyframeDependencyResolver.java   |  36 ++++-
 7 files changed, 278 insertions(+), 106 deletions(-)

(limited to 'src/main')

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 d409560a67..403528e020 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
@@ -803,6 +803,11 @@ public class BuildView {
         // The error must have been reported already during analysis.
       }
 
+      @Override
+      protected void missingEdgeHook(Target from, Label to, NoSuchThingException e) {
+        // The error must have been reported already during analysis.
+      }
+
       @Override
       protected Target getTarget(Label label) {
         if (targetCache == null) {
@@ -858,12 +863,19 @@ public class BuildView {
     class SilentDependencyResolver extends DependencyResolver {
       @Override
       protected void invalidVisibilityReferenceHook(TargetAndConfiguration node, Label label) {
-        // The error must have been reported already during analysis.
+        throw new RuntimeException("bad visibility on " + label + " during testing unexpected");
       }
 
       @Override
       protected void invalidPackageGroupReferenceHook(TargetAndConfiguration node, Label label) {
-        // The error must have been reported already during analysis.
+        throw new RuntimeException("bad package group on " + label + " during testing unexpected");
+      }
+
+      @Override
+      protected void missingEdgeHook(Target from, Label to, NoSuchThingException e) {
+        throw new RuntimeException(
+            "missing dependency from " + from.getLabel() + " to " + to + ": " + e.getMessage(),
+            e);
       }
 
       @Override
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 17d9d23b19..8d30b12f47 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
@@ -22,6 +22,7 @@ import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.ImmutableSortedKeyListMultimap;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.packages.Aspect;
 import com.google.devtools.build.lib.packages.AspectClass;
 import com.google.devtools.build.lib.packages.AspectDefinition;
@@ -89,21 +90,56 @@ public abstract class DependencyResolver {
       Aspect aspect,
       Set<ConfigMatchingProvider> configConditions)
       throws EvalException, InterruptedException {
+    NestedSetBuilder<Label> rootCauses = NestedSetBuilder.<Label>stableOrder();
+    ListMultimap<Attribute, Dependency> outgoingEdges =
+        dependentNodeMap(node, hostConfig, aspect, configConditions, rootCauses);
+    if (!rootCauses.isEmpty()) {
+      throw new IllegalStateException(rootCauses.build().iterator().next().toString());
+    }
+    return outgoingEdges;
+  }
+
+  /**
+   * Returns ids for dependent nodes of a given node, sorted by attribute. Note that some
+   * dependencies do not have a corresponding attribute here, and we use the null attribute to
+   * represent those edges. Visibility attributes are only visited if {@code visitVisibility} is
+   * {@code true}.
+   *
+   * <p>If {@code aspect} is null, returns the dependent nodes of the configured
+   * target node representing the given target and configuration, otherwise that of the aspect
+   * node accompanying the aforementioned configured target node for the specified aspect.
+   *
+   * <p>The values are not simply labels because this also implements the first step of applying
+   * configuration transitions, namely, split transitions. This needs to be done before the labels
+   * are resolved because late bound attributes depend on the configuration. A good example for this
+   * is @{code :cc_toolchain}.
+   *
+   * <p>The long-term goal is that most configuration transitions be applied here. However, in order
+   * to do that, we first have to eliminate transitions that depend on the rule class of the
+   * dependency.
+   */
+  public final ListMultimap<Attribute, Dependency> dependentNodeMap(
+      TargetAndConfiguration node,
+      BuildConfiguration hostConfig,
+      Aspect aspect,
+      Set<ConfigMatchingProvider> configConditions,
+      NestedSetBuilder<Label> rootCauses)
+      throws EvalException, InterruptedException {
     Target target = node.getTarget();
     BuildConfiguration config = node.getConfiguration();
     ListMultimap<Attribute, Dependency> outgoingEdges = ArrayListMultimap.create();
     if (target instanceof OutputFile) {
       Preconditions.checkNotNull(config);
-      visitTargetVisibility(node, outgoingEdges.get(null));
+      visitTargetVisibility(node, rootCauses, outgoingEdges.get(null));
       Rule rule = ((OutputFile) target).getGeneratingRule();
       outgoingEdges.put(null, Dependency.withConfiguration(rule.getLabel(), config));
     } else if (target instanceof InputFile) {
-      visitTargetVisibility(node, outgoingEdges.get(null));
+      visitTargetVisibility(node, rootCauses, outgoingEdges.get(null));
     } else if (target instanceof EnvironmentGroup) {
-      visitTargetVisibility(node, outgoingEdges.get(null));
+      visitTargetVisibility(node, rootCauses, outgoingEdges.get(null));
     } else if (target instanceof Rule) {
       Preconditions.checkNotNull(config);
-      visitTargetVisibility(node, outgoingEdges.get(null));
+      visitTargetVisibility(node, rootCauses, outgoingEdges.get(null));
       Rule rule = (Rule) target;
       ListMultimap<Attribute, LabelAndConfiguration> labelMap =
           resolveAttributes(
@@ -112,15 +148,26 @@ public abstract class DependencyResolver {
               config,
               hostConfig,
               configConditions);
-      visitRule(rule, aspect, labelMap, outgoingEdges);
+      visitRule(rule, aspect, labelMap, rootCauses, outgoingEdges);
     } else if (target instanceof PackageGroup) {
-      visitPackageGroup(node, (PackageGroup) target, outgoingEdges.get(null));
+      visitPackageGroup(node, (PackageGroup) target, rootCauses, outgoingEdges.get(null));
     } else {
       throw new IllegalStateException(target.getLabel().toString());
     }
     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)
@@ -398,34 +445,30 @@ public abstract class DependencyResolver {
    */
   public final Collection<Dependency> resolveRuleLabels(
       TargetAndConfiguration node, ListMultimap<Attribute,
-      LabelAndConfiguration> labelMap) {
+      LabelAndConfiguration> labelMap, NestedSetBuilder<Label> rootCauses) {
     Preconditions.checkArgument(node.getTarget() instanceof Rule);
     Rule rule = (Rule) node.getTarget();
     ListMultimap<Attribute, Dependency> outgoingEdges = ArrayListMultimap.create();
-    visitRule(rule, labelMap, outgoingEdges);
+    visitRule(rule, labelMap, rootCauses, outgoingEdges);
     return outgoingEdges.values();
   }
 
   private void visitPackageGroup(TargetAndConfiguration node, PackageGroup packageGroup,
-      Collection<Dependency> outgoingEdges) {
+      NestedSetBuilder<Label> rootCauses, Collection<Dependency> outgoingEdges) {
     for (Label label : packageGroup.getIncludes()) {
-      try {
-        Target target = getTarget(label);
-        if (target == null) {
-          return;
-        }
-        if (!(target instanceof PackageGroup)) {
-          // Note that this error could also be caught in PackageGroupConfiguredTarget, but since
-          // these have the null configuration, visiting the corresponding target would trigger an
-          // analysis of a rule with a null configuration, which doesn't work.
-          invalidPackageGroupReferenceHook(node, label);
-          continue;
-        }
-
-        outgoingEdges.add(Dependency.withNullConfiguration(label));
-      } catch (NoSuchThingException e) {
-        // Don't visit targets that don't exist (--keep_going)
+      Target target = getTarget(packageGroup, label, rootCauses);
+      if (target == null) {
+        continue;
+      }
+      if (!(target instanceof PackageGroup)) {
+        // Note that this error could also be caught in PackageGroupConfiguredTarget, but since
+        // these have the null configuration, visiting the corresponding target would trigger an
+        // analysis of a rule with a null configuration, which doesn't work.
+        invalidPackageGroupReferenceHook(node, label);
+        continue;
       }
+
+      outgoingEdges.add(Dependency.withNullConfiguration(label));
     }
   }
 
@@ -467,14 +510,15 @@ public abstract class DependencyResolver {
   }
 
   private void visitRule(Rule rule, ListMultimap<Attribute, LabelAndConfiguration> labelMap,
-      ListMultimap<Attribute, Dependency> outgoingEdges) {
-    visitRule(rule, /*aspect=*/ null, labelMap, outgoingEdges);
+      NestedSetBuilder<Label> rootCauses, ListMultimap<Attribute, Dependency> outgoingEdges) {
+    visitRule(rule, /*aspect=*/ null, labelMap, rootCauses, outgoingEdges);
   }
 
   private void visitRule(
       Rule rule,
       Aspect aspect,
       ListMultimap<Attribute, LabelAndConfiguration> labelMap,
+      NestedSetBuilder<Label> rootCauses,
       ListMultimap<Attribute, Dependency> outgoingEdges) {
     Preconditions.checkNotNull(labelMap);
     for (Map.Entry<Attribute, Collection<LabelAndConfiguration>> entry :
@@ -484,13 +528,7 @@ public abstract class DependencyResolver {
         Label label = dep.getLabel();
         BuildConfiguration config = dep.getConfiguration();
 
-        Target toTarget;
-        try {
-          toTarget = getTarget(label);
-        } catch (NoSuchThingException e) {
-          throw new IllegalStateException("not found: " + label + " from " + rule + " in "
-              + attribute.getName());
-        }
+        Target toTarget = getTarget(rule, label, rootCauses);
         if (toTarget == null) {
           continue;
         }
@@ -526,28 +564,25 @@ public abstract class DependencyResolver {
   }
 
   private void visitTargetVisibility(TargetAndConfiguration node,
-      Collection<Dependency> outgoingEdges) {
-    for (Label label : node.getTarget().getVisibility().getDependencyLabels()) {
-      try {
-        Target visibilityTarget = getTarget(label);
-        if (visibilityTarget == null) {
-          return;
-        }
-        if (!(visibilityTarget instanceof PackageGroup)) {
-          // Note that this error could also be caught in
-          // AbstractConfiguredTarget.convertVisibility(), but we have an
-          // opportunity here to avoid dependency cycles that result from
-          // the visibility attribute of a rule referring to a rule that
-          // depends on it (instead of its package)
-          invalidVisibilityReferenceHook(node, label);
-          continue;
-        }
-
-        // Visibility always has null configuration
-        outgoingEdges.add(Dependency.withNullConfiguration(label));
-      } catch (NoSuchThingException e) {
-        // Don't visit targets that don't exist (--keep_going)
+      NestedSetBuilder<Label> rootCauses, Collection<Dependency> outgoingEdges) {
+    Target target = node.getTarget();
+    for (Label label : target.getVisibility().getDependencyLabels()) {
+      Target visibilityTarget = getTarget(target, label, rootCauses);
+      if (visibilityTarget == null) {
+        continue;
       }
+      if (!(visibilityTarget instanceof PackageGroup)) {
+        // Note that this error could also be caught in
+        // AbstractConfiguredTarget.convertVisibility(), but we have an
+        // opportunity here to avoid dependency cycles that result from
+        // the visibility attribute of a rule referring to a rule that
+        // depends on it (instead of its package)
+        invalidVisibilityReferenceHook(node, label);
+        continue;
+      }
+
+      // Visibility always has null configuration
+      outgoingEdges.add(Dependency.withNullConfiguration(label));
     }
   }
 
@@ -568,6 +603,15 @@ public abstract class DependencyResolver {
   protected abstract void invalidPackageGroupReferenceHook(TargetAndConfiguration node,
       Label label);
 
+  /**
+   * Hook for the error case where a dependency is missing.
+   *
+   * @param from the target referencing the missing target
+   * @param to the missing target
+   * @param e the exception that was thrown, e.g., by {@link #getTarget}
+   */
+  protected abstract void missingEdgeHook(Target from, Label to, NoSuchThingException e);
+
   /**
    * Returns the target by the given label.
    *
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
index 2d0549b163..4917d28397 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
@@ -26,7 +26,9 @@ import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.cmdline.Label;
+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;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.Attribute;
@@ -105,6 +107,7 @@ public final class AspectFunction implements SkyFunction {
       throws AspectFunctionException, InterruptedException {
     SkyframeBuildView view = buildViewProvider.getSkyframeBuildView();
     NestedSetBuilder<Package> transitivePackages = NestedSetBuilder.stableOrder();
+    NestedSetBuilder<Label> transitiveRootCauses = NestedSetBuilder.stableOrder();
     AspectKey key = (AspectKey) skyKey.argument();
     ConfiguredAspectFactory aspectFactory;
     if (key.getAspectClass() instanceof NativeAspectClass<?>) {
@@ -152,9 +155,15 @@ public final class AspectFunction implements SkyFunction {
           "aspects must be attached to rules"));
     }
 
-    final ConfiguredTargetValue configuredTargetValue =
-        (ConfiguredTargetValue)
-            env.getValue(ConfiguredTargetValue.key(key.getLabel(), key.getConfiguration()));
+    final ConfiguredTargetValue configuredTargetValue;
+    try {
+      configuredTargetValue =
+          (ConfiguredTargetValue) env.getValueOrThrow(
+              ConfiguredTargetValue.key(key.getLabel(), key.getConfiguration()),
+              ConfiguredValueCreationException.class);
+    } catch (ConfiguredValueCreationException e) {
+      throw new AspectFunctionException(new AspectCreationException(e.getRootCauses()));
+    }
     if (configuredTargetValue == null) {
       // TODO(bazel-team): remove this check when top-level targets also use dynamic configurations.
       // Right now the key configuration may be dynamic while the original target's configuration
@@ -175,7 +184,7 @@ public final class AspectFunction implements SkyFunction {
     try {
       // Get the configuration targets that trigger this rule's configurable attributes.
       Set<ConfigMatchingProvider> configConditions = ConfiguredTargetFunction.getConfigConditions(
-          target, env, resolver, ctgValue, transitivePackages);
+          target, env, resolver, ctgValue, transitivePackages, transitiveRootCauses);
       if (configConditions == null) {
         // Those targets haven't yet been resolved.
         return null;
@@ -190,7 +199,15 @@ public final class AspectFunction implements SkyFunction {
               configConditions,
               ruleClassProvider,
               view.getHostConfiguration(ctgValue.getConfiguration()),
-              transitivePackages);
+              transitivePackages,
+              transitiveRootCauses);
+      if (depValueMap == null) {
+        return null;
+      }
+      if (!transitiveRootCauses.isEmpty()) {
+        throw new AspectFunctionException(
+            new AspectCreationException("Loading failed", transitiveRootCauses.build()));
+      }
 
       return createAspect(
           env,
@@ -282,6 +299,9 @@ public final class AspectFunction implements SkyFunction {
    * An exception indicating that there was a problem creating an aspect.
    */
   public static final class AspectCreationException extends Exception {
+    /** Targets in the transitive closure that failed to load. May be empty. */
+    private final NestedSet<Label> loadingRootCauses;
+
     /**
      * The target for which analysis failed, if any. We can't represent aspects with labels, so if
      * the aspect analysis fails, this will be {@code null}.
@@ -290,11 +310,26 @@ public final class AspectFunction implements SkyFunction {
 
     public AspectCreationException(String message, Label analysisRootCause) {
       super(message);
+      this.loadingRootCauses = NestedSetBuilder.<Label>emptySet(Order.STABLE_ORDER);
       this.analysisRootCause = analysisRootCause;
     }
 
+    public AspectCreationException(String message, NestedSet<Label> loadingRootCauses) {
+      super(message);
+      this.loadingRootCauses = loadingRootCauses;
+      this.analysisRootCause = null;
+    }
+
+    public AspectCreationException(NestedSet<Label> loadingRootCauses) {
+      this("Loading failed", loadingRootCauses);
+    }
+
     public AspectCreationException(String message) {
-      this(message, null);
+      this(message, (Label) null);
+    }
+
+    public NestedSet<Label> getRootCauses() {
+      return loadingRootCauses;
     }
 
     @Nullable public Label getAnalysisRootCause() {
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 f4a74460bd..dba5155f25 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
@@ -42,7 +42,9 @@ import com.google.devtools.build.lib.analysis.config.HostTransition;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.analysis.config.PatchTransition;
 import com.google.devtools.build.lib.cmdline.Label;
+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;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.StoredEventHandler;
@@ -68,7 +70,7 @@ import com.google.devtools.build.skyframe.SkyFunctionException;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 import com.google.devtools.build.skyframe.ValueOrException;
-import com.google.devtools.build.skyframe.ValueOrException3;
+import com.google.devtools.build.skyframe.ValueOrException2;
 
 import java.util.Collection;
 import java.util.HashMap;
@@ -124,6 +126,7 @@ final class ConfiguredTargetFunction implements SkyFunction {
       InterruptedException {
     SkyframeBuildView view = buildViewProvider.getSkyframeBuildView();
     NestedSetBuilder<Package> transitivePackages = NestedSetBuilder.stableOrder();
+    NestedSetBuilder<Label> transitiveLoadingRootCauses = NestedSetBuilder.stableOrder();
     ConfiguredTargetKey configuredTargetKey = (ConfiguredTargetKey) key.argument();
     LabelAndConfiguration lc = LabelAndConfiguration.of(
         configuredTargetKey.getLabel(), configuredTargetKey.getConfiguration());
@@ -162,8 +165,9 @@ final class ConfiguredTargetFunction implements SkyFunction {
         new TargetAndConfiguration(target, configuration);
     try {
       // Get the configuration targets that trigger this rule's configurable attributes.
-      Set<ConfigMatchingProvider> configConditions =
-          getConfigConditions(ctgValue.getTarget(), env, resolver, ctgValue, transitivePackages);
+      Set<ConfigMatchingProvider> configConditions = getConfigConditions(
+          ctgValue.getTarget(), env, resolver, ctgValue, transitivePackages,
+          transitiveLoadingRootCauses);
       if (env.valuesMissing()) {
         return null;
       }
@@ -177,7 +181,16 @@ final class ConfiguredTargetFunction implements SkyFunction {
               configConditions,
               ruleClassProvider,
               view.getHostConfiguration(configuration),
-              transitivePackages);
+              transitivePackages,
+              transitiveLoadingRootCauses);
+      if (env.valuesMissing()) {
+        return null;
+      }
+      if (!transitiveLoadingRootCauses.isEmpty()) {
+        throw new ConfiguredTargetFunctionException(
+            new ConfiguredValueCreationException(transitiveLoadingRootCauses.build()));
+      }
+      Preconditions.checkNotNull(depValueMap);
       ConfiguredTargetValue ans = createConfiguredTarget(
           view, env, target, configuration, depValueMap, configConditions, transitivePackages);
       return ans;
@@ -194,12 +207,12 @@ final class ConfiguredTargetFunction implements SkyFunction {
       }
     } catch (AspectCreationException e) {
       // getAnalysisRootCause may be null if the analysis of the aspect itself failed.
-      Label rootCause = target.getLabel();
+      Label analysisRootCause = target.getLabel();
       if (e.getAnalysisRootCause() != null) {
-        rootCause = e.getAnalysisRootCause();
+        analysisRootCause = e.getAnalysisRootCause();
       }
       throw new ConfiguredTargetFunctionException(
-          new ConfiguredValueCreationException(e.getMessage(), rootCause));
+          new ConfiguredValueCreationException(e.getMessage(), analysisRootCause));
     }
   }
 
@@ -229,13 +242,14 @@ final class ConfiguredTargetFunction implements SkyFunction {
       Set<ConfigMatchingProvider> configConditions,
       RuleClassProvider ruleClassProvider,
       BuildConfiguration hostConfiguration,
-      NestedSetBuilder<Package> transitivePackages)
+      NestedSetBuilder<Package> transitivePackages,
+      NestedSetBuilder<Label> transitiveLoadingRootCauses)
       throws DependencyEvaluationException, AspectCreationException, InterruptedException {
     // Create the map from attributes to list of (target, configuration) pairs.
     ListMultimap<Attribute, Dependency> depValueNames;
     try {
-      depValueNames =
-          resolver.dependentNodeMap(ctgValue, hostConfiguration, aspect, configConditions);
+      depValueNames = resolver.dependentNodeMap(
+          ctgValue, hostConfiguration, aspect, configConditions, transitiveLoadingRootCauses);
     } catch (EvalException e) {
       // EvalException can only be thrown by computed Skylark attributes in the current rule.
       env.getListener().handle(Event.error(e.getLocation(), e.getMessage()));
@@ -256,14 +270,14 @@ final class ConfiguredTargetFunction implements SkyFunction {
 
     // Resolve configured target dependencies and handle errors.
     Map<SkyKey, ConfiguredTarget> depValues = resolveConfiguredTargetDependencies(env,
-        depValueNames.values(), transitivePackages);
+        depValueNames.values(), transitivePackages, transitiveLoadingRootCauses);
     if (depValues == null) {
       return null;
     }
 
     // Resolve required aspects.
-    ListMultimap<SkyKey, ConfiguredAspect> depAspects =
-        resolveAspectDependencies(env, depValues, depValueNames.values(), transitivePackages);
+    ListMultimap<SkyKey, ConfiguredAspect> depAspects = resolveAspectDependencies(
+        env, depValues, depValueNames.values(), transitivePackages);
     if (depAspects == null) {
       return null;
     }
@@ -566,10 +580,8 @@ final class ConfiguredTargetFunction implements SkyFunction {
       }
     }
 
-    Map<SkyKey, ValueOrException3<
-        AspectCreationException, NoSuchThingException, ConfiguredValueCreationException>>
-        depAspects = env.getValuesOrThrow(aspectKeys, AspectCreationException.class,
-            NoSuchThingException.class, ConfiguredValueCreationException.class);
+    Map<SkyKey, ValueOrException2<AspectCreationException, NoSuchThingException>> depAspects =
+        env.getValuesOrThrow(aspectKeys, AspectCreationException.class, NoSuchThingException.class);
 
     for (Dependency dep : deps) {
       SkyKey depKey = TO_KEYS.apply(dep);
@@ -587,10 +599,9 @@ final class ConfiguredTargetFunction implements SkyFunction {
         SkyKey aspectKey = createAspectKey(dep.getLabel(), dep.getConfiguration(), depAspect);
         AspectValue aspectValue = null;
         try {
+          // TODO(ulfjack): Catch all thrown AspectCreationException and NoSuchThingException
+          // instances and merge them into a single Exception to get full root cause data.
           aspectValue = (AspectValue) depAspects.get(aspectKey).get();
-        } catch (ConfiguredValueCreationException e) {
-          // The configured target should have been created in resolveConfiguredTargetDependencies()
-          throw new IllegalStateException(e);
         } catch (NoSuchThingException e) {
           throw new AspectCreationException(
               String.format(
@@ -639,7 +650,8 @@ final class ConfiguredTargetFunction implements SkyFunction {
   @Nullable
   static Set<ConfigMatchingProvider> getConfigConditions(Target target, Environment env,
       SkyframeDependencyResolver resolver, TargetAndConfiguration ctgValue,
-      NestedSetBuilder<Package> transitivePackages)
+      NestedSetBuilder<Package> transitivePackages,
+      NestedSetBuilder<Label> transitiveLoadingRootCauses)
       throws DependencyEvaluationException {
     if (!(target instanceof Rule)) {
       return ImmutableSet.of();
@@ -665,7 +677,8 @@ final class ConfiguredTargetFunction implements SkyFunction {
 
     // Collect the corresponding Skyframe configured target values. Abort early if they haven't
     // been computed yet.
-    Collection<Dependency> configValueNames = resolver.resolveRuleLabels(ctgValue, configLabelMap);
+    Collection<Dependency> configValueNames = resolver.resolveRuleLabels(
+        ctgValue, configLabelMap, transitiveLoadingRootCauses);
 
     // No need to get new configs from Skyframe - config_setting rules always use the current
     // target's config.
@@ -682,7 +695,7 @@ final class ConfiguredTargetFunction implements SkyFunction {
     }
 
     Map<SkyKey, ConfiguredTarget> configValues = resolveConfiguredTargetDependencies(
-        env, configValueNames, transitivePackages);
+        env, configValueNames, transitivePackages, transitiveLoadingRootCauses);
     if (configValues == null) {
       return null;
     }
@@ -714,30 +727,39 @@ final class ConfiguredTargetFunction implements SkyFunction {
    */
   @Nullable
   private static Map<SkyKey, ConfiguredTarget> resolveConfiguredTargetDependencies(
-      Environment env, Collection<Dependency> deps, NestedSetBuilder<Package> transitivePackages)
-          throws DependencyEvaluationException {
-    boolean ok = !env.valuesMissing();
+      Environment env, Collection<Dependency> deps, NestedSetBuilder<Package> transitivePackages,
+      NestedSetBuilder<Label> transitiveLoadingRootCauses) throws DependencyEvaluationException {
+    boolean missedValues = env.valuesMissing();
+    boolean failed = false;
     Iterable<SkyKey> depKeys = Iterables.transform(deps, TO_KEYS);
-    Map<SkyKey, ValueOrException<ConfiguredValueCreationException>> depValues =
+    Map<SkyKey, ValueOrException<ConfiguredValueCreationException>> depValuesOrExceptions =
         env.getValuesOrThrow(depKeys, ConfiguredValueCreationException.class);
-    Map<SkyKey, ConfiguredTarget> result = Maps.newHashMapWithExpectedSize(depValues.size());
-    for (Map.Entry<SkyKey, ValueOrException<ConfiguredValueCreationException>> entry :
-        depValues.entrySet()) {
-      ConfiguredTargetValue depValue;
+    Map<SkyKey, ConfiguredTarget> result =
+        Maps.newHashMapWithExpectedSize(depValuesOrExceptions.size());
+    for (Map.Entry<SkyKey, ValueOrException<ConfiguredValueCreationException>> entry
+        : depValuesOrExceptions.entrySet()) {
       try {
-        depValue = (ConfiguredTargetValue) entry.getValue().get();
+        ConfiguredTargetValue depValue = (ConfiguredTargetValue) entry.getValue().get();
+        if (depValue == null) {
+          missedValues = true;
+        } else {
+          result.put(entry.getKey(), depValue.getConfiguredTarget());
+          transitivePackages.addTransitive(depValue.getTransitivePackages());
+        }
       } catch (ConfiguredValueCreationException e) {
-        throw new DependencyEvaluationException(e);
-      }
-      if (depValue == null) {
-        ok = false;
-      } else {
-        result.put(entry.getKey(), depValue.getConfiguredTarget());
-        transitivePackages.addTransitive(depValue.getTransitivePackages());
+        // TODO(ulfjack): If there is an analysis root cause, we drop all loading root causes.
+        if (e.getAnalysisRootCause() != null) {
+          throw new DependencyEvaluationException(e);
+        }
+        transitiveLoadingRootCauses.addTransitive(e.loadingRootCauses);
+        failed = true;
       }
     }
-    if (!ok) {
+    if (missedValues) {
       return null;
+    } else if (failed) {
+      throw new DependencyEvaluationException(
+          new ConfiguredValueCreationException(transitiveLoadingRootCauses.build()));
     } else {
       return result;
     }
@@ -766,6 +788,7 @@ final class ConfiguredTargetFunction implements SkyFunction {
       return null;
     }
 
+    Preconditions.checkNotNull(depValueMap);
     ConfiguredTarget configuredTarget = view.createConfiguredTarget(target, configuration,
         analysisEnvironment, depValueMap, configConditions);
 
@@ -817,14 +840,34 @@ final class ConfiguredTargetFunction implements SkyFunction {
    * a ConfiguredTargetValue.
    */
   public static final class ConfiguredValueCreationException extends Exception {
+    private final NestedSet<Label> loadingRootCauses;
     // TODO(ulfjack): Collect all analysis root causes, not just the first one.
-    private final Label analysisRootCause;
+    @Nullable private final Label analysisRootCause;
 
     public ConfiguredValueCreationException(String message, Label currentTarget) {
       super(message);
+      this.loadingRootCauses = NestedSetBuilder.<Label>emptySet(Order.STABLE_ORDER);
       this.analysisRootCause = Preconditions.checkNotNull(currentTarget);
     }
 
+    public ConfiguredValueCreationException(String message, NestedSet<Label> rootCauses) {
+      super(message);
+      this.loadingRootCauses = rootCauses;
+      this.analysisRootCause = null;
+    }
+
+    public ConfiguredValueCreationException(NestedSet<Label> rootCauses) {
+      this("Loading failed", rootCauses);
+    }
+
+    public ConfiguredValueCreationException(String message) {
+      this(message, NestedSetBuilder.<Label>emptySet(Order.STABLE_ORDER));
+    }
+
+    public NestedSet<Label> getRootCauses() {
+      return loadingRootCauses;
+    }
+
     public Label getAnalysisRootCause() {
       return analysisRootCause;
     }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PostConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PostConfiguredTargetFunction.java
index 284491d996..878caeca08 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PostConfiguredTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PostConfiguredTargetFunction.java
@@ -101,6 +101,9 @@ public class PostConfiguredTargetFunction implements SkyFunction {
           buildViewProvider.getSkyframeBuildView().getHostConfiguration(ct.getConfiguration());
       SkyframeDependencyResolver resolver =
           buildViewProvider.getSkyframeBuildView().createDependencyResolver(env);
+      // We don't track root causes here - this function is only invoked for successfully analyzed
+      // targets - as long as we redo the exact same steps here as in ConfiguredTargetFunction, this
+      // can never fail.
       deps =
           resolver.dependentNodeMap(
               ctgValue, hostConfiguration, /*aspect=*/ null, configConditions);
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 60182d8b5d..2802fadc40 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
@@ -499,6 +499,9 @@ public final class SkyframeBuildView {
       Set<ConfigMatchingProvider> configConditions) throws InterruptedException {
     Preconditions.checkState(enableAnalysis,
         "Already in execution phase %s %s", target, configuration);
+    Preconditions.checkNotNull(analysisEnvironment);
+    Preconditions.checkNotNull(target);
+    Preconditions.checkNotNull(prerequisiteMap);
     return factory.createConfiguredTarget(analysisEnvironment, artifactFactory, target,
         configuration, getHostConfiguration(configuration), prerequisiteMap,
         configConditions);
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 acff770241..c609e99148 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
@@ -17,7 +17,11 @@ 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.events.Event;
+import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
+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.Package;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.skyframe.SkyFunction.Environment;
@@ -50,17 +54,45 @@ public final class SkyframeDependencyResolver extends DependencyResolver {
             "label '%s' does not refer to a package group", label)));
   }
 
+  @Override
+  protected void missingEdgeHook(Target from, Label to, NoSuchThingException e) {
+    if (e instanceof NoSuchTargetException) {
+      NoSuchTargetException nste = (NoSuchTargetException) e;
+      if (to.equals(nste.getLabel())) {
+        env.getListener().handle(
+            Event.error(
+                TargetUtils.getLocationMaybe(from),
+                TargetUtils.formatMissingEdge(from, to, e)));
+      }
+    } else if (e instanceof NoSuchPackageException) {
+      NoSuchPackageException nspe = (NoSuchPackageException) e;
+      if (nspe.getPackageId().equals(to.getPackageIdentifier())) {
+        env.getListener().handle(
+            Event.error(
+                TargetUtils.getLocationMaybe(from),
+                TargetUtils.formatMissingEdge(from, to, e)));
+      }
+    }
+  }
+
   @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) {
       return null;
     }
     SkyKey key = PackageValue.key(label.getPackageIdentifier());
-    PackageValue packageValue = (PackageValue) env.getValue(key);
-    if (packageValue == null || packageValue.getPackage().containsErrors()) {
+    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());
+    }
     return packageValue.getPackage().getTarget(label.getName());
   }
 }
-- 
cgit v1.2.3