Fix error handling in skyframe target pattern parsing

Bazel completely swallowed errors in some cases, e.g., if the pattern is
invalid like bazel build foo//bar:baz.

Note that it previously silently ignored empty targets if --experimental_skyframe_target_pattern_evaluator was passed, and now fails (which is consistent with legacy behavior). This is an intentional change, but may break users who are using the experimental flag and are passing empty strings to Bazel.

PiperOrigin-RevId: 184282856

5 files changed, 94 insertions, 3 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingResult.java b/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingResult.java
index f78e0afb75..c36de6fd0c 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingResult.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingResult.java
@@ -13,9 +13,9 @@
 // limitations under the License.
 package com.google.devtools.build.lib.pkgcache;
 
+import com.google.common.base.MoreObjects;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.packages.Target;
-
 import java.util.Collection;
 
 /**
@@ -63,4 +63,15 @@ public final class LoadingResult {
   public String getWorkspaceName() {
     return workspaceName;
   }
+
+  @Override
+  public String toString() {
+    return MoreObjects.toStringHelper(LoadingResult.class)
+        .add("hasTargetPatternError", hasTargetPatternError)
+        .add("hasLoadingError", hasLoadingError)
+        .add("targetsToAnalyze", targetsToAnalyze)
+        .add("testsToRun", testsToRun)
+        .add("workspaceName", workspaceName)
+        .toString();
+  }
 }
\ No newline at end of file
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java
index b8b1e292b7..04ff12caf5 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java
@@ -47,6 +47,8 @@ public final class SkyFunctions {
   public static final SkyFunctionName PACKAGE_ERROR = SkyFunctionName.create("PACKAGE_ERROR");
   public static final SkyFunctionName TARGET_MARKER = SkyFunctionName.create("TARGET_MARKER");
   public static final SkyFunctionName TARGET_PATTERN = SkyFunctionName.create("TARGET_PATTERN");
+  public static final SkyFunctionName TARGET_PATTERN_ERROR =
+      SkyFunctionName.create("TARGET_PATTERN_ERROR");
   public static final SkyFunctionName PREPARE_DEPS_OF_PATTERNS =
       SkyFunctionName.create("PREPARE_DEPS_OF_PATTERNS");
   public static final SkyFunctionName PREPARE_DEPS_OF_PATTERN =
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 7ae5ac59e7..93775243af 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
@@ -410,6 +410,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
             packageProgress));
     map.put(SkyFunctions.PACKAGE_ERROR, new PackageErrorFunction());
     map.put(SkyFunctions.TARGET_MARKER, new TargetMarkerFunction());
+    map.put(SkyFunctions.TARGET_PATTERN_ERROR, new TargetPatternErrorFunction());
     map.put(SkyFunctions.TRANSITIVE_TARGET, new TransitiveTargetFunction(ruleClassProvider));
     map.put(Label.TRANSITIVE_TRAVERSAL, new TransitiveTraversalFunction());
     map.put(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternErrorFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternErrorFunction.java
new file mode 100644
index 0000000000..cf21520c85
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternErrorFunction.java
@@ -0,0 +1,61 @@
+// Copyright 2015 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.skyframe;
+
+import com.google.devtools.build.lib.cmdline.TargetParsingException;
+import com.google.devtools.build.skyframe.LegacySkyKey;
+import com.google.devtools.build.skyframe.SkyFunction;
+import com.google.devtools.build.skyframe.SkyFunctionException;
+import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
+import com.google.devtools.build.skyframe.SkyKey;
+import com.google.devtools.build.skyframe.SkyValue;
+import javax.annotation.Nullable;
+
+/**
+ * SkyFunction that throws a {@link TargetParsingException} for target pattern that could not be
+ * parsed. Must only be requested when a SkyFunction wishes to ignore the errors
+ * in a target pattern in keep_going mode, but to shut down the build in nokeep_going mode.
+ *
+ * <p>This SkyFunction never returns a value, only throws a {@link TargetParsingException}, and
+ * should never return null, since all of its dependencies should already be present.
+ */
+public class TargetPatternErrorFunction implements SkyFunction {
+  // We pass in the error message, which isn't ideal. We could consider reparsing the original
+  // pattern instead, but that requires more information.
+  public static SkyKey key(String errorMessage) {
+    return LegacySkyKey.create(SkyFunctions.TARGET_PATTERN_ERROR, errorMessage);
+  }
+
+  @Nullable
+  @Override
+  public SkyValue compute(SkyKey skyKey, Environment env)
+      throws TargetErrorFunctionException, InterruptedException {
+    String errorMessage = (String) skyKey.argument();
+    throw new TargetErrorFunctionException(
+        new TargetParsingException(errorMessage), Transience.PERSISTENT);
+  }
+
+  @Nullable
+  @Override
+  public String extractTag(SkyKey skyKey) {
+    return null;
+  }
+
+  private static class TargetErrorFunctionException extends SkyFunctionException {
+    public TargetErrorFunctionException(
+        TargetParsingException cause, Transience transience) {
+      super(cause, transience);
+    }
+  }
+}
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 75d38b81d3..28803ff1bb 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
@@ -206,8 +206,9 @@ final class TargetPatternPhaseFunction implements SkyFunction {
    */
   private static ResolvedTargets<Target> getTargetsToBuild(
       Environment env, TargetPatternPhaseKey options, PathPackageLocator pkgPath)
-      throws InterruptedException {
+          throws InterruptedException {
     List<TargetPatternKey> patternSkyKeys = new ArrayList<>();
+    ResolvedTargets.Builder<Target> builder = ResolvedTargets.builder();
     for (TargetPatternSkyKeyOrException keyOrException :
         TargetPatternValue.keys(
             options.getTargetPatterns(),
@@ -224,6 +225,16 @@ final class TargetPatternPhaseFunction implements SkyFunction {
         // pattern could be parsed successfully).
         env.getListener().post(
             new ParsingFailedEvent(keyOrException.getOriginalPattern(),  e.getMessage()));
+        try {
+          env.getValueOrThrow(
+              TargetPatternErrorFunction.key(e.getMessage()), TargetParsingException.class);
+        } catch (TargetParsingException ignore) {
+          // We ignore this. Keep going is active.
+        }
+        env.getListener().handle(
+            Event.error(
+                "Skipping '" + keyOrException.getOriginalPattern() + "': " + e.getMessage()));
+        builder.setError();
       }
     }
     Map<SkyKey, ValueOrException<TargetParsingException>> resolvedPatterns =
@@ -232,7 +243,6 @@ final class TargetPatternPhaseFunction implements SkyFunction {
       return null;
     }
 
-    ResolvedTargets.Builder<Target> builder = ResolvedTargets.builder();
     for (TargetPatternKey pattern : patternSkyKeys) {
       TargetPatternValue value;
       try {
@@ -265,6 +275,12 @@ final class TargetPatternPhaseFunction implements SkyFunction {
       } catch (MissingDepException e) {
         return null;
       } catch (TargetParsingException e) {
+        try {
+          env.getValueOrThrow(
+              TargetPatternErrorFunction.key(e.getMessage()), TargetParsingException.class);
+        } catch (TargetParsingException ignore) {
+          // We ignore this. Keep going is active.
+        }
         env.getListener().handle(Event.error(e.getMessage()));
         return ResolvedTargets.failed();
       }