Post PatternExpandingError from the skyframe target pattern evaluator as needed.

This fixes https://github.com/bazelbuild/bazel/issues/4731.

Also, add test coverage for posting of ParsingFailedEvent and handling of cycles in LoadingPhaseRunnerTest.

Change-Id: I88c9d33417b9c3c7a06c92a6137d58f37b991b0c
PiperOrigin-RevId: 188138972

4 files changed, 95 insertions, 27 deletions

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 1d94871501..f24f095ef2 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
@@ -35,6 +35,14 @@ public final class PatternExpandingError implements BuildEvent {
     this.skipped = skipped;
   }
 
+  public List<String> getPattern() {
+    return pattern;
+  }
+
+  public boolean getSkipped() {
+    return skipped;
+  }
+
   public static PatternExpandingError failed(List<String> pattern, String message) {
     return new PatternExpandingError(pattern, message, false);
   }
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 1514290052..4a8bf8ed80 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
@@ -2090,23 +2090,25 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
           buildDriver.evaluate(ImmutableList.of(key), keepGoing, /*numThreads=*/ 10, eventHandler);
       if (evalResult.hasError()) {
         ErrorInfo errorInfo = evalResult.getError(key);
+        TargetParsingException exc;
         if (!Iterables.isEmpty(errorInfo.getCycleInfo())) {
-          String errorMessage = "cycles detected during target parsing";
+          exc = new TargetParsingException("cycles detected during target parsing");
           getCyclesReporter().reportCycles(errorInfo.getCycleInfo(), key, eventHandler);
-          throw new TargetParsingException(errorMessage);
-        }
-        if (errorInfo.getException() != null) {
-          Exception e = errorInfo.getException();
-          Throwables.propagateIfInstanceOf(e, TargetParsingException.class);
-          if (!keepGoing) {
-            // This is the same code as in SkyframeTargetPatternEvaluator; we allow any exception
-            // and turn it into a TargetParsingException here.
-            throw new TargetParsingException(e.getMessage());
-          }
-          throw new IllegalStateException("Unexpected Exception type from TargetPatternPhaseValue "
-              + "for '" + targetPatterns + "'' with root causes: "
-              + Iterables.toString(errorInfo.getRootCauses()), e);
+        } else {
+          // TargetPatternPhaseFunction never directly throws. Thus, the only way
+          // evalResult.hasError() && keepGoing can hold is if there are cycles, which is handled
+          // above.
+          Preconditions.checkState(!keepGoing);
+          // Following SkyframeTargetPatternEvaluator, we convert any exception into a
+          // TargetParsingException.
+          Exception e = Preconditions.checkNotNull(errorInfo.getException());
+          exc =
+              (e instanceof TargetParsingException)
+                  ? (TargetParsingException) e
+                  : new TargetParsingException(e.getMessage(), e);
         }
+        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/TargetPatternPhaseFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
index 28803ff1bb..ee6cddcb35 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
@@ -250,6 +250,7 @@ final class TargetPatternPhaseFunction implements SkyFunction {
       } catch (TargetParsingException e) {
         String rawPattern = pattern.getPattern();
         String errorMessage = e.getMessage();
+        env.getListener().post(PatternExpandingError.skipped(rawPattern, errorMessage));
         env.getListener().handle(Event.error("Skipping '" + rawPattern + "': " + errorMessage));
         builder.setError();
         continue;
diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
index cf7c61bba0..de8523eecf 100644
--- a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
@@ -24,6 +24,8 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.Iterators;
+import com.google.common.collect.MoreCollectors;
 import com.google.devtools.build.lib.actions.ActionKeyContext;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.BuildView;
@@ -43,6 +45,7 @@ import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.packages.util.MockToolsConfig;
 import com.google.devtools.build.lib.skyframe.BazelSkyframeExecutorConstants;
 import com.google.devtools.build.lib.skyframe.DiffAwareness;
+import com.google.devtools.build.lib.skyframe.PatternExpandingError;
 import com.google.devtools.build.lib.skyframe.SequencedSkyframeExecutor;
 import com.google.devtools.build.lib.skyframe.SkyValueDirtinessChecker;
 import com.google.devtools.build.lib.skyframe.SkyframeExecutor;
@@ -133,6 +136,9 @@ public class LoadingPhaseRunnerTest {
     assertThat(loadingResult.getTestsToRun()).isNull();
     tester.assertContainsError("Skipping '//base:missing': no such package 'base'");
     tester.assertContainsWarning("Target pattern parsing failed.");
+    PatternExpandingError err = tester.findPostOnce(PatternExpandingError.class);
+    assertThat(err.getPattern()).containsExactly("//base:missing");
+    assertThat(err.getSkipped()).isTrue();
   }
 
   @Test
@@ -145,6 +151,9 @@ public class LoadingPhaseRunnerTest {
     assertThat(loadingResult.getTestsToRun()).isNull();
     tester.assertContainsError("Skipping '//base:missing': no such target '//base:missing'");
     tester.assertContainsWarning("Target pattern parsing failed.");
+    PatternExpandingError err = tester.findPostOnce(PatternExpandingError.class);
+    assertThat(err.getPattern()).containsExactly("//base:missing");
+    assertThat(err.getSkipped()).isTrue();
   }
 
   @Test
@@ -158,6 +167,8 @@ public class LoadingPhaseRunnerTest {
           + "invalid package name 'foo//bar': "
           + "package names may not contain '//' path separators");
     }
+    ParsingFailedEvent err = tester.findPostOnce(ParsingFailedEvent.class);
+    assertThat(err.getPattern()).isEqualTo("foo//bar:missing");
   }
 
   @Test
@@ -182,6 +193,8 @@ public class LoadingPhaseRunnerTest {
           "invalid target format 'foo//bar:missing': "
           + "invalid package name 'foo//bar': "
           + "package names may not contain '//' path separators");
+    ParsingFailedEvent err = tester.findPostOnce(ParsingFailedEvent.class);
+    assertThat(err.getPattern()).isEqualTo("foo//bar:missing");
   }
 
   @Test
@@ -540,6 +553,9 @@ public class LoadingPhaseRunnerTest {
     } catch (TargetParsingException expected) {
     }
     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
@@ -607,11 +623,50 @@ public class LoadingPhaseRunnerTest {
   public void testParsingFailureReported() throws Exception {
     LoadingResult loadingResult = tester.loadKeepGoing("//does_not_exist");
     assertThat(loadingResult.hasTargetPatternError()).isTrue();
-    ParsingFailedEvent event = tester.findPost(ParsingFailedEvent.class);
-    assertThat(event).isNotNull();
+    ParsingFailedEvent event = tester.findPostOnce(ParsingFailedEvent.class);
     assertThat(event.getPattern()).isEqualTo("//does_not_exist");
     assertThat(event.getMessage()).contains("BUILD file not found on package path");
-    assertThat(Iterables.filter(tester.getPosts(), ParsingFailedEvent.class)).hasSize(1);
+  }
+
+  @Test
+  public void testCyclesKeepGoing() throws Exception {
+    tester.addFile("test/BUILD", "load(':cycle1.bzl', 'make_cycle')");
+    tester.addFile("test/cycle1.bzl", "load(':cycle2.bzl', 'make_cycle')");
+    tester.addFile("test/cycle2.bzl", "load(':cycle1.bzl', 'make_cycle')");
+    if (useSkyframeTargetPatternEval()) {
+      // The skyframe target pattern evaluator isn't able to provide partial results in the presence
+      // of cycles, so it simply raises an exception rather than returning an empty LoadingResult.
+      try {
+        tester.load("//test:cycle1");
+        fail();
+      } catch (TargetParsingException e) {
+        assertThat(e).hasMessageThat().contains("cycles detected");
+      }
+    } else {
+      LoadingResult loadingResult = tester.loadKeepGoing("//test:cycle1");
+      assertThat(loadingResult.hasTargetPatternError()).isTrue();
+    }
+    tester.assertContainsEventWithFrequency("cycle detected in extension", 1);
+    PatternExpandingError err = tester.findPostOnce(PatternExpandingError.class);
+    assertThat(err.getPattern()).containsExactly("//test:cycle1");
+    assertThat(err.getSkipped()).isEqualTo(!useSkyframeTargetPatternEval());
+  }
+
+  @Test
+  public void testCyclesNoKeepGoing() throws Exception {
+    tester.addFile("test/BUILD", "load(':cycle1.bzl', 'make_cycle')");
+    tester.addFile("test/cycle1.bzl", "load(':cycle2.bzl', 'make_cycle')");
+    tester.addFile("test/cycle2.bzl", "load(':cycle1.bzl', 'make_cycle')");
+    try {
+      tester.load("//test:cycle1");
+      fail();
+    } catch (TargetParsingException e) {
+      assertThat(e).hasMessageThat().contains("cycles detected");
+    }
+    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 {
@@ -870,17 +925,19 @@ public class LoadingPhaseRunnerTest {
           filteredEvents(), expectedMessage, expectedFrequency);
     }
 
-    public Iterable<Postable> getPosts() {
-      return storedErrors.getPosts();
-    }
-
     public <T extends Postable> T findPost(Class<T> clazz) {
-      for (Postable p : storedErrors.getPosts()) {
-        if (clazz.isInstance(p)) {
-          return clazz.cast(p);
-        }
-      }
-      return null;
+      return Iterators.getNext(
+          storedErrors.getPosts().stream().filter(clazz::isInstance).map(clazz::cast).iterator(),
+          null);
+    }
+
+    public <T extends Postable> T findPostOnce(Class<T> clazz) {
+      return storedErrors
+          .getPosts()
+          .stream()
+          .filter(clazz::isInstance)
+          .map(clazz::cast)
+          .collect(MoreCollectors.onlyElement());
     }
   }
 }