Rewrite TargetPattern failure reporting

Report failures in TargetPatternFunction, rather than in its callers. Since we
can't distinguish between keep_going and nokeep_going modes, we otherwise end
up double-reporting errors. In the particular case that's covered by the
build_event_stream_test.sh, we end up reporting the same target pattern as both
skipped and failed.

Unfortunately, this means we cannot report whether the target pattern was
skipped or failed, so the pattern_skipped event is now unused (if we agree that
this is acceptable, I'll remove the corresponding infrastructure).

PiperOrigin-RevId: 199593700

7 files changed, 71 insertions, 13 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/TargetParsingCompleteEvent.java b/src/main/java/com/google/devtools/build/lib/pkgcache/TargetParsingCompleteEvent.java
index 63abf1be0b..58d67f8309 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/TargetParsingCompleteEvent.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/TargetParsingCompleteEvent.java
@@ -72,6 +72,7 @@ public class TargetParsingCompleteEvent implements BuildEventWithOrderConstraint
   }
 
   private final ImmutableList<String> originalTargetPattern;
+  private final ImmutableList<String> failedTargetPatterns;
   private final ImmutableSet<ThinTarget> targets;
   private final ImmutableSet<ThinTarget> filteredTargets;
   private final ImmutableSet<ThinTarget> testFilteredTargets;
@@ -87,12 +88,14 @@ public class TargetParsingCompleteEvent implements BuildEventWithOrderConstraint
       Collection<Target> filteredTargets,
       Collection<Target> testFilteredTargets,
       List<String> originalTargetPattern,
-      Collection<Target> expandedTargets) {
+      Collection<Target> expandedTargets,
+      List<String> failedTargetPatterns) {
     this.targets = asThinTargets(targets);
     this.filteredTargets = asThinTargets(filteredTargets);
     this.testFilteredTargets = asThinTargets(testFilteredTargets);
     this.originalTargetPattern = ImmutableList.copyOf(originalTargetPattern);
     this.expandedTargets = asThinTargets(expandedTargets);
+    this.failedTargetPatterns = ImmutableList.copyOf(failedTargetPatterns);
   }
 
   @VisibleForTesting
@@ -102,7 +105,16 @@ public class TargetParsingCompleteEvent implements BuildEventWithOrderConstraint
         ImmutableSet.<Target>of(),
         ImmutableSet.<Target>of(),
         ImmutableList.<String>of(),
-        targets);
+        targets,
+        ImmutableList.<String>of());
+  }
+
+  public ImmutableList<String> getOriginalTargetPattern() {
+    return originalTargetPattern;
+  }
+
+  public ImmutableList<String> getFailedTargetPatterns() {
+    return failedTargetPatterns;
   }
 
   /** @return the parsed targets, which will subsequently be loaded */
@@ -137,6 +149,10 @@ public class TargetParsingCompleteEvent implements BuildEventWithOrderConstraint
   @Override
   public Collection<BuildEventId> getChildrenEvents() {
     ImmutableList.Builder<BuildEventId> childrenBuilder = ImmutableList.builder();
+    for (String failedTargetPattern : failedTargetPatterns) {
+      childrenBuilder.add(
+          BuildEventId.targetPatternExpanded(ImmutableList.of(failedTargetPattern)));
+    }
     for (ThinTarget target : expandedTargets) {
       // Test suits won't produce target configuration and  target-complete events, so do not
       // announce here completion as children.
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java b/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java
index 773682af18..6af82547ca 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java
@@ -62,6 +62,7 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetView;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.events.Reporter;
+import com.google.devtools.build.lib.pkgcache.TargetParsingCompleteEvent;
 import com.google.devtools.build.lib.util.Pair;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -591,6 +592,15 @@ public class BuildEventStreamer implements EventHandler {
       return true;
     }
 
+    if (event instanceof TargetParsingCompleteEvent) {
+      // If there is only one pattern and we have one failed pattern, then we already posted a
+      // pattern expanded error, so we don't post the completion event.
+      // TODO(ulfjack): This is brittle. It would be better to always post one PatternExpanded event
+      // for each pattern given on the command line instead of one event for all of them combined.
+      return ((TargetParsingCompleteEvent) event).getOriginalTargetPattern().size() == 1
+          && !((TargetParsingCompleteEvent) event).getFailedTargetPatterns().isEmpty();
+    }
+
     return false;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/LegacyLoadingPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/skyframe/LegacyLoadingPhaseRunner.java
index 00aa64e5e3..5587284333 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/LegacyLoadingPhaseRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/LegacyLoadingPhaseRunner.java
@@ -15,6 +15,7 @@ package com.google.devtools.build.lib.skyframe;
 
 import com.google.common.base.Preconditions;
 import com.google.common.base.Stopwatch;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -220,7 +221,8 @@ public final class LegacyLoadingPhaseRunner extends LoadingPhaseRunner {
             filteredTargets,
             testFilteredTargets,
             targetPatterns,
-            expandedTargetsToLoad));
+            expandedTargetsToLoad,
+            ImmutableList.of()));
     eventHandler.post(new TargetParsingPhaseTimeEvent(targetPatternEvalTime));
     if (callback != null) {
       callback.notifyTargets(expandedTargetsToLoad);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PatternExpandingError.java b/src/main/java/com/google/devtools/build/lib/skyframe/PatternExpandingError.java
index 99a76c840c..b6145a56b7 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PatternExpandingError.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PatternExpandingError.java
@@ -47,6 +47,12 @@ public final class PatternExpandingError implements BuildEvent {
     return new PatternExpandingError(pattern, message, false);
   }
 
+  public static PatternExpandingError failed(String term, String message) {
+    return new PatternExpandingError(ImmutableList.of(term), message, false);
+  }
+
+  // This is unused right now - when we generate the error, we don't know if we're in keep_going
+  // mode or not.
   public static PatternExpandingError skipped(String term, String message) {
     return new PatternExpandingError(ImmutableList.of(term), message, true);
   }
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 2b4da1b4a0..d52b9ed16e 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
@@ -2258,6 +2258,9 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
         if (!Iterables.isEmpty(errorInfo.getCycleInfo())) {
           exc = new TargetParsingException("cycles detected during target parsing");
           getCyclesReporter().reportCycles(errorInfo.getCycleInfo(), key, eventHandler);
+          // Fallback: we don't know which patterns failed, specifically, so we report the entire
+          // set as being in error.
+          eventHandler.post(PatternExpandingError.failed(targetPatterns, exc.getMessage()));
         } else {
           // TargetPatternPhaseFunction never directly throws. Thus, the only way
           // evalResult.hasError() && keepGoing can hold is if there are cycles, which is handled
@@ -2270,8 +2273,12 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
               (e instanceof TargetParsingException)
                   ? (TargetParsingException) e
                   : new TargetParsingException(e.getMessage(), e);
+          if (!(e instanceof TargetParsingException)) {
+            // If it's a TargetParsingException, then the TargetPatternPhaseFunction has already
+            // reported the error, so we don't need to report it again.
+            eventHandler.post(PatternExpandingError.failed(targetPatterns, exc.getMessage()));
+          }
         }
-        eventHandler.post(PatternExpandingError.failed(targetPatterns, exc.getMessage()));
         throw exc;
       }
       long timeMillis = timer.stop().elapsed(TimeUnit.MILLISECONDS);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java
index 322061fe69..754917d201 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java
@@ -74,6 +74,8 @@ public class TargetPatternFunction implements SkyFunction {
           /*blacklistedSubdirectories=*/ ImmutableSet.of(),
           excludedSubdirectories,
           callback,
+          // The exception type here has to match the one on the BatchCallback. Since the callback
+          // defined above never throws, the exact type here is not really relevant.
           RuntimeException.class);
       resolvedTargets = ResolvedTargets.<Target>builder().addAll(results).build();
     } catch (TargetParsingException e) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
index c087cc369c..2598dd8e9c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
@@ -77,7 +77,8 @@ final class TargetPatternPhaseFunction implements SkyFunction {
     }
 
     // Determine targets to build:
-    ResolvedTargets<Target> targets = getTargetsToBuild(env, options);
+    List<String> failedPatterns = new ArrayList<String>();
+    ResolvedTargets<Target> targets = getTargetsToBuild(env, options, failedPatterns);
 
     // If the --build_tests_only option was specified or we want to run tests, we need to determine
     // the list of targets to test. For that, we remove manual tests and apply the command-line
@@ -164,7 +165,6 @@ final class TargetPatternPhaseFunction implements SkyFunction {
 
     LoadingPhaseRunner.maybeReportDeprecation(env.getListener(), targets.getTargets());
 
-    boolean preExpansionError = targets.hasError();
     ResolvedTargets.Builder<Label> expandedLabelsBuilder = ResolvedTargets.builder();
     for (Target target : targets.getTargets()) {
       if (TargetUtils.isTestSuiteRule(target) && options.isExpandTestSuites()) {
@@ -207,7 +207,8 @@ final class TargetPatternPhaseFunction implements SkyFunction {
                 filteredTargets,
                 testFilteredTargets,
                 options.getTargetPatterns(),
-                expandedTargets.getTargets()));
+                expandedTargets.getTargets(),
+                failedPatterns));
     return result;
   }
 
@@ -217,7 +218,7 @@ final class TargetPatternPhaseFunction implements SkyFunction {
    * @param options the command-line arguments in structured form
    */
   private static ResolvedTargets<Target> getTargetsToBuild(
-      Environment env, TargetPatternPhaseKey options)
+      Environment env, TargetPatternPhaseKey options, List<String> failedPatterns)
           throws InterruptedException {
     List<TargetPatternKey> patternSkyKeys = new ArrayList<>();
     ResolvedTargets.Builder<Target> builder = ResolvedTargets.builder();
@@ -231,6 +232,11 @@ final class TargetPatternPhaseFunction implements SkyFunction {
       try {
         patternSkyKeys.add(keyOrException.getSkyKey());
       } catch (TargetParsingException e) {
+        failedPatterns.add(keyOrException.getOriginalPattern());
+        // We post a PatternExpandingError here - the pattern could not be parsed, so we don't even
+        // get to run TargetPatternFunction.
+        env.getListener().post(
+            PatternExpandingError.failed(keyOrException.getOriginalPattern(), e.getMessage()));
         // We generally skip patterns that don't parse. We report a parsing failed exception to the
         // event bus here, but not in determineTests below, which goes through the same list. Note
         // that the TargetPatternFunction otherwise reports these events (but only if the target
@@ -249,12 +255,9 @@ final class TargetPatternPhaseFunction implements SkyFunction {
         builder.setError();
       }
     }
+
     Map<SkyKey, ValueOrException<TargetParsingException>> resolvedPatterns =
         env.getValuesOrThrow(patternSkyKeys, TargetParsingException.class);
-    if (env.valuesMissing()) {
-      return null;
-    }
-
     for (TargetPatternKey pattern : patternSkyKeys) {
       TargetPatternValue value;
       try {
@@ -262,20 +265,32 @@ final class TargetPatternPhaseFunction implements SkyFunction {
       } catch (TargetParsingException e) {
         String rawPattern = pattern.getPattern();
         String errorMessage = e.getMessage();
-        env.getListener().post(PatternExpandingError.skipped(rawPattern, errorMessage));
+        failedPatterns.add(rawPattern);
+        env.getListener().post(PatternExpandingError.failed(rawPattern, errorMessage));
         env.getListener().handle(Event.error("Skipping '" + rawPattern + "': " + errorMessage));
         builder.setError();
         continue;
       }
+      if (value == null) {
+        continue;
+      }
       // TODO(ulfjack): This is terribly inefficient.
       ResolvedTargets<Target> asTargets = TestSuiteExpansionFunction.labelsToTargets(
           env, value.getTargets().getTargets(), value.getTargets().hasError());
+      if (asTargets == null) {
+        continue;
+      }
       if (pattern.isNegative()) {
         builder.filter(Predicates.not(Predicates.in(asTargets.getTargets())));
       } else {
         builder.merge(asTargets);
       }
     }
+    // Only check for missing values after reporting errors. Otherwise we will miss errors in the
+    // nokeep_going case.
+    if (env.valuesMissing()) {
+      return null;
+    }
 
     ResolvedTargets<Target> result = builder
         .filter(TargetUtils.tagFilter(options.getBuildTargetFilter()))