Fix: return an error from the analysis phase with keep_going in error cases.

- if there are failed top-level aspects
- if there are conflicting actions

I ended up rewriting how errors are signaled from the SkyframeBuildView. I
think this is safe, but please review carefully.

--
MOS_MIGRATED_REVID=113150100

4 files changed, 14 insertions, 5 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 eed892c1d4..d409560a67 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
@@ -507,7 +507,6 @@ public class BuildView {
 
     int numTargetsToAnalyze = nodes.size();
     int numSuccessful = skyframeAnalysisResult.getConfiguredTargets().size();
-    boolean analysisSuccessful = (numSuccessful == numTargetsToAnalyze);
     if (0 < numSuccessful && numSuccessful < numTargetsToAnalyze) {
       String msg = String.format("Analysis succeeded for only %d of %d top-level targets",
                                     numSuccessful, numTargetsToAnalyze);
@@ -515,6 +514,7 @@ public class BuildView {
       LOG.info(msg);
     }
 
+    boolean analysisSuccessful = !skyframeAnalysisResult.hasError();
     AnalysisResult result =
         createResult(
             eventHandler,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAnalysisResult.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAnalysisResult.java
index 7db9ca2efe..371ba8ff88 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAnalysisResult.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAnalysisResult.java
@@ -26,22 +26,29 @@ import java.util.Collection;
  *  Encapsulates the raw analysis result of top level targets and aspects coming from Skyframe.
  */
 public class SkyframeAnalysisResult {
+  private final boolean hasError;
   private final ImmutableList<ConfiguredTarget> configuredTargets;
   private final WalkableGraph walkableGraph;
   private final ImmutableList<AspectValue> aspects;
   private final ImmutableMap<PackageIdentifier, Path> packageRoots;
 
   public SkyframeAnalysisResult(
+      boolean hasError,
       ImmutableList<ConfiguredTarget> configuredTargets,
       WalkableGraph walkableGraph,
       ImmutableList<AspectValue> aspects,
       ImmutableMap<PackageIdentifier, Path> packageRoots) {
+    this.hasError = hasError;
     this.configuredTargets = configuredTargets;
     this.walkableGraph = walkableGraph;
     this.aspects = aspects;
     this.packageRoots = packageRoots;
   }
 
+  public boolean hasError() {
+    return hasError;
+  }
+
   public Collection<ConfiguredTarget> getConfiguredTargets() {
     return configuredTargets;
   }
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 6d5b0bc61a..60182d8b5d 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
@@ -280,6 +280,7 @@ public final class SkyframeBuildView {
     if (!result.hasError() && badActions.isEmpty()) {
       setDeserializedArtifactOwners();
       return new SkyframeAnalysisResult(
+          false,
           ImmutableList.copyOf(goodCts),
           result.getWalkableGraph(),
           ImmutableList.copyOf(goodAspects),
@@ -402,6 +403,7 @@ public final class SkyframeBuildView {
     }
     setDeserializedArtifactOwners();
     return new SkyframeAnalysisResult(
+        result.hasError() || !badActions.isEmpty(),
         ImmutableList.copyOf(goodCts),
         result.getWalkableGraph(),
         ImmutableList.copyOf(goodAspects),
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java
index c365cab4c8..571911115d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java
@@ -56,7 +56,7 @@ public class ToplevelSkylarkAspectFunction implements SkyFunction {
     } catch (SkylarkImportFailedException e) {
       env.getListener().handle(Event.error(e.getMessage()));
       throw new LoadSkylarkAspectFunctionException(
-          new AspectCreationException(e.getMessage()), skyKey);
+          new AspectCreationException(e.getMessage()));
     }
     if (labelLookupMap == null) {
       return null;
@@ -67,7 +67,7 @@ public class ToplevelSkylarkAspectFunction implements SkyFunction {
       skylarkAspect = AspectFunction.loadSkylarkAspect(
           env, labelLookupMap.get(extensionFile), skylarkValueName);
     } catch (AspectCreationException e) {
-      throw new LoadSkylarkAspectFunctionException(e, skyKey);
+      throw new LoadSkylarkAspectFunctionException(e);
     }
     if (skylarkAspect == null) {
       return null;
@@ -92,8 +92,8 @@ public class ToplevelSkylarkAspectFunction implements SkyFunction {
    * Exceptions thrown from ToplevelSkylarkAspectFunction.
    */
   public class LoadSkylarkAspectFunctionException extends SkyFunctionException {
-    public LoadSkylarkAspectFunctionException(AspectCreationException cause, SkyKey childKey) {
-      super(cause, childKey);
+    public LoadSkylarkAspectFunctionException(AspectCreationException cause) {
+      super(cause, Transience.PERSISTENT);
     }
   }
 }