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

src/main/java/com/google/devtools/build/lib/pkgcache/TargetParsingCompleteEvent.java

@@ -72,6 +72,7 @@ public boolean isNotATestOrTestSuite() {
   }
 
   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 TargetParsingCompleteEvent(
       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 TargetParsingCompleteEvent(Collection<Target> targets) {
         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 Collection<BuildEventId> postedAfter() {
   @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.

src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java

@@ -62,6 +62,7 @@
 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 boolean shouldIgnoreBuildEvent(BuildEvent event) {
       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;
   }
 

src/main/java/com/google/devtools/build/lib/skyframe/LegacyLoadingPhaseRunner.java

@@ -15,6 +15,7 @@
 
 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 LoadingResult execute(
             filteredTargets,
             testFilteredTargets,
             targetPatterns,
-            expandedTargetsToLoad));
+            expandedTargetsToLoad,
+            ImmutableList.of()));
     eventHandler.post(new TargetParsingPhaseTimeEvent(targetPatternEvalTime));
     if (callback != null) {
       callback.notifyTargets(expandedTargetsToLoad);

src/main/java/com/google/devtools/build/lib/skyframe/PatternExpandingError.java

@@ -47,6 +47,12 @@ public static PatternExpandingError failed(List<String> pattern, String message)
     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);
   }

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java

@@ -2258,6 +2258,9 @@ public LoadingResult execute(
         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 LoadingResult execute(
               (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);

src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java

@@ -74,6 +74,8 @@ public void process(Iterable<Target> partialResult) {
           /*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) {

src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java

@@ -77,7 +77,8 @@ public TargetPatternPhaseValue compute(SkyKey key, Environment env) throws Inter
     }
 
     // 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 @@ public TargetPatternPhaseValue compute(SkyKey key, Environment env) throws Inter
 
     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 @@ public TargetPatternPhaseValue compute(SkyKey key, Environment env) throws Inter
                 filteredTargets,
                 testFilteredTargets,
                 options.getTargetPatterns(),
-                expandedTargets.getTargets()));
+                expandedTargets.getTargets(),
+                failedPatterns));
     return result;
   }
 
@@ -217,7 +218,7 @@ public TargetPatternPhaseValue compute(SkyKey key, Environment env) throws Inter
    * @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 @@ private static ResolvedTargets<Target> getTargetsToBuild(
       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,33 +255,42 @@ private static ResolvedTargets<Target> getTargetsToBuild(
         builder.setError();
       }
     }
+
     Map<SkyKey, ValueOrException<TargetParsingException>> resolvedPatterns =
         env.getValuesOrThrow(patternSkyKeys, TargetParsingException.class);
-    if (env.valuesMissing()) {
-      return null;
-    }
-
     for (TargetPatternKey pattern : patternSkyKeys) {
       TargetPatternValue value;
       try {
         value = (TargetPatternValue) resolvedPatterns.get(pattern).get();
       } 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()))

src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java

@@ -105,6 +105,23 @@ protected boolean useSkyframeTargetPatternEval() {
     return false;
   }
 
+  private void assertCircularSymlinksDuringTargetParsing(String targetPattern) throws Exception {
+    try {
+      tester.load(targetPattern);
+      fail();
+    } catch (TargetParsingException e) {
+      // Expected.
+      tester.assertContainsError("circular symlinks detected");
+    }
+  }
+
+  private LoadingResult assertNoErrors(LoadingResult loadingResult) {
+    assertThat(loadingResult.hasTargetPatternError()).isFalse();
+    assertThat(loadingResult.hasLoadingError()).isFalse();
+    tester.assertNoEvents();
+    return loadingResult;
+  }
+
   @Test
   public void testSmoke() throws Exception {
     tester.addFile("base/BUILD",
@@ -140,7 +157,32 @@ public void testNonExistentPackage() throws Exception {
     tester.assertContainsWarning("Target pattern parsing failed.");
     PatternExpandingError err = tester.findPostOnce(PatternExpandingError.class);
     assertThat(err.getPattern()).containsExactly("//base:missing");
-    assertThat(err.getSkipped()).isTrue();
+  }
+
+  @Test
+  public void testNonExistentPackageWithKeepGoing() throws Exception {
+    tester.addFile("base/BUILD",
+        "filegroup(name = 'hello', srcs = ['foo.txt'])");
+    tester.loadKeepGoing("//base:hello", "//base:missing");
+    PatternExpandingError err = tester.findPostOnce(PatternExpandingError.class);
+    assertThat(err.getPattern()).containsExactly("//base:missing");
+    TargetParsingCompleteEvent event = tester.findPostOnce(TargetParsingCompleteEvent.class);
+    assertThat(event.getOriginalTargetPattern()).containsExactly("//base:hello", "//base:missing");
+    if (useSkyframeTargetPatternEval()) {
+      // The legacy code path wasn't updated to provide this information, and will be deleted soon.
+      assertThat(event.getFailedTargetPatterns()).containsExactly("//base:missing");
+    }
+  }
+
+  @Test
+  public void testNonExistentPackageWithoutKeepGoing() throws Exception {
+    try {
+      tester.load("//does/not/exist");
+      fail();
+    } catch (TargetParsingException expected) {
+    }
+    PatternExpandingError err = tester.findPostOnce(PatternExpandingError.class);
+    assertThat(err.getPattern()).containsExactly("//does/not/exist");
   }
 
   @Test
@@ -155,7 +197,6 @@ public void testNonExistentTarget() throws Exception {
     tester.assertContainsWarning("Target pattern parsing failed.");
     PatternExpandingError err = tester.findPostOnce(PatternExpandingError.class);
     assertThat(err.getPattern()).containsExactly("//base:missing");
-    assertThat(err.getSkipped()).isTrue();
   }
 
   @Test
@@ -567,7 +608,6 @@ public void testTopLevelTargetErrorsPrintedExactlyOnce_NoKeepGoing() throws Exce
     tester.assertContainsEventWithFrequency("name 'undefined_symbol' is not defined", 1);
     PatternExpandingError err = tester.findPostOnce(PatternExpandingError.class);
     assertThat(err.getPattern()).containsExactly("//bad");
-    assertThat(err.getSkipped()).isFalse();
   }
 
   @Test
@@ -678,24 +718,6 @@ public void testCyclesNoKeepGoing() throws Exception {
     tester.assertContainsEventWithFrequency("cycle detected in extension", 1);
     PatternExpandingError err = tester.findPostOnce(PatternExpandingError.class);
     assertThat(err.getPattern()).containsExactly("//test:cycle1");
-    assertThat(err.getSkipped()).isFalse();
-  }
-
-  private void assertCircularSymlinksDuringTargetParsing(String targetPattern) throws Exception {
-    try {
-      tester.load(targetPattern);
-      fail();
-    } catch (TargetParsingException e) {
-      // Expected.
-      tester.assertContainsError("circular symlinks detected");
-    }
-  }
-
-  private LoadingResult assertNoErrors(LoadingResult loadingResult) {
-    assertThat(loadingResult.hasTargetPatternError()).isFalse();
-    assertThat(loadingResult.hasLoadingError()).isFalse();
-    tester.assertNoEvents();
-    return loadingResult;
   }
 
   private static class LoadingPhaseTester {

src/test/shell/integration/build_event_stream_test.sh

@@ -673,7 +673,6 @@ function test_loading_failure() {
   # reason for the target expansion event not resulting in targets
   # being expanded.
   (bazel build --build_event_text_file=$TEST_log \
-         --noexperimental_skyframe_target_pattern_evaluator \
          //does/not/exist && fail "build failure expected") || true
   expect_log_once 'aborted'
   expect_log_once 'reason: LOADING_FAILURE'