Restrict the type of exception that can be thrown during target patte…

…rn parsing (and general target-oriented graph visitation) to QueryException.

In practice, this is the only exception that can be thrown, but that fact is extremely non-obvious. We make it obvious by restricting the type of Exception that can be thrown to QueryExceptionMarkerInterface, an interface that is only non-trivially implemented by QueryException.

PiperOrigin-RevId: 410355114

src/main/java/com/google/devtools/build/lib/cmdline/BUILD

@@ -23,7 +23,9 @@ java_library(
     exports = [":cmdline-primitives"],
     deps = [
         ":LabelValidator",
+        ":batch_callback",
         ":cmdline-primitives",
+        ":query_exception_marker_interface",
         "//src/main/java/com/google/devtools/build/docgen/annot",
         "//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
         "//src/main/java/com/google/devtools/build/lib/concurrent",
@@ -72,6 +74,31 @@ java_library(
     ],
 )
 
+java_library(
+    name = "parallel_visitor",
+    srcs = ["ParallelVisitor.java"],
+    deps = [
+        ":batch_callback",
+        ":query_exception_marker_interface",
+        "//src/main/java/com/google/devtools/build/lib/concurrent",
+        "//third_party:guava",
+    ],
+)
+
+java_library(
+    name = "batch_callback",
+    srcs = ["BatchCallback.java"],
+    deps = [
+        ":query_exception_marker_interface",
+        "//src/main/java/com/google/devtools/build/lib/concurrent:thread_safety",
+    ],
+)
+
+java_library(
+    name = "query_exception_marker_interface",
+    srcs = ["QueryExceptionMarkerInterface.java"],
+)
+
 # LabelValidator provides validation of Bazel build labels.
 # This is a public API.
 # TODO(adonovan): unsplit the lib.cmdline Java package by moving this logic to a subpackage.

src/main/java/com/google/devtools/build/lib/concurrent/BatchCallback.java → src/main/java/com/google/devtools/build/lib/cmdline/BatchCallback.java

@@ -11,17 +11,17 @@
 // 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.concurrent;
+package com.google.devtools.build.lib.cmdline;
 
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 
 /**
- * Callback to be invoked when part of a result has been computed. Allows a client interested in
- * the result to process it as it is computed, for instance by streaming it, if it is too big to
- * fit in memory.
+ * Callback to be invoked when part of a result has been computed. Allows a client interested in the
+ * result to process it as it is computed, for instance by streaming it, if it is too big to fit in
+ * memory.
  */
 @ThreadSafe
-public interface BatchCallback<T, E extends Exception> {
+public interface BatchCallback<T, E extends Exception & QueryExceptionMarkerInterface> {
   /**
    * Called when part of a result has been computed.
    *
@@ -33,8 +33,12 @@ public interface BatchCallback<T, E extends Exception> {
    */
   void process(Iterable<T> partialResult) throws E, InterruptedException;
 
-  /** {@link BatchCallback} that does precisely nothing. */
-  class NullCallback<T> implements BatchCallback<T, RuntimeException> {
+  /** {@link BatchCallback} that doesn't throw. */
+  interface SafeBatchCallback<T>
+      extends BatchCallback<T, QueryExceptionMarkerInterface.MarkerRuntimeException> {}
+
+  /** {@link SafeBatchCallback} that does precisely nothing. */
+  class NullCallback<T> implements SafeBatchCallback<T> {
     private static final NullCallback<Object> INSTANCE = new NullCallback<>();
 
     @Override

src/main/java/com/google/devtools/build/lib/concurrent/ParallelVisitor.java → src/main/java/com/google/devtools/build/lib/cmdline/ParallelVisitor.java

@@ -11,12 +11,15 @@
 // 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.concurrent;
+package com.google.devtools.build.lib.cmdline;
 
 import com.google.common.base.Preconditions;
 import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
+import com.google.devtools.build.lib.concurrent.AbstractQueueVisitor;
+import com.google.devtools.build.lib.concurrent.ErrorClassifier;
+import com.google.devtools.build.lib.concurrent.QuiescingExecutor;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -47,7 +50,7 @@ public abstract class ParallelVisitor<
     VisitKeyT,
     OutputKeyT,
     OutputResultT,
-    ExceptionT extends Exception,
+    ExceptionT extends Exception & QueryExceptionMarkerInterface,
     CallbackT extends BatchCallback<OutputResultT, ExceptionT>> {
   protected final CallbackT callback;
   protected final Class<ExceptionT> exceptionClass;
@@ -142,7 +145,7 @@ public interface Factory<
       VisitKeyT,
       OutputKeyT,
       OutputResultT,
-      ExceptionT extends Exception,
+      ExceptionT extends Exception & QueryExceptionMarkerInterface,
       CallbackT extends BatchCallback<OutputResultT, ExceptionT>> {
     ParallelVisitor<InputT, VisitKeyT, OutputKeyT, OutputResultT, ExceptionT, CallbackT> create();
   }
@@ -166,12 +169,6 @@ public void onVisitTaskCompleted() {}
   protected abstract Iterable<OutputResultT> outputKeysToOutputValues(
       Iterable<OutputKeyT> targetKeys) throws ExceptionT, InterruptedException;
 
-  /**
-   * Suitable exception type to use with {@link ParallelVisitor} when no checked exception is
-   * appropriate.
-   */
-  public static final class UnusedException extends RuntimeException {}
-
   /** An object to hold keys to visit and keys ready for processing. */
   protected final class Visit {
     private final Iterable<OutputKeyT> keysToUseForResult;

src/main/java/com/google/devtools/build/lib/cmdline/QueryExceptionMarkerInterface.java

@@ -0,0 +1,39 @@
+// Copyright 2021 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.cmdline;
+
+/**
+ * Marker interface indicating either a {@link
+ * com.google.devtools.build.lib.query2.engine.QueryException} or a {@link MarkerRuntimeException}.
+ * Used with a generic type to indicate that a method can optionally throw {@code QueryException} if
+ * the caller passes {@code QueryException.class} as a parameter to the method.
+ *
+ * <p>The only outside implementation of this interface is {@link
+ * com.google.devtools.build.lib.query2.engine.QueryException}. Do not implement or extend!
+ *
+ * <p>Used to narrow a generic type like {@code E extends Exception} to {@code E extends Exception &
+ * QueryExceptionMarkerInterface}, guaranteeing that the method will only throw {@link
+ * com.google.devtools.build.lib.query2.engine.QueryException} if any exception of type E is thrown.
+ * Because {@code E} will appear in the "throws" clause of a method, it must extend {@link
+ * Exception}.
+ */
+@SuppressWarnings("InterfaceWithOnlyStatics")
+public interface QueryExceptionMarkerInterface {
+  /**
+   * Marker class indicating that a given method does not throw QueryException. Pass {@code
+   * MarkerRuntimeException.class} as a parameter.
+   */
+  class MarkerRuntimeException extends RuntimeException implements QueryExceptionMarkerInterface {}
+}

src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java

@@ -29,7 +29,6 @@
 import com.google.common.util.concurrent.ListeningExecutorService;
 import com.google.devtools.build.lib.cmdline.LabelValidator.BadLabelException;
 import com.google.devtools.build.lib.cmdline.LabelValidator.PackageAndTarget;
-import com.google.devtools.build.lib.concurrent.BatchCallback;
 import com.google.devtools.build.lib.server.FailureDetails.TargetPatterns;
 import com.google.devtools.build.lib.server.FailureDetails.TargetPatterns.Code;
 import com.google.devtools.build.lib.supplier.InterruptibleSupplier;
@@ -146,7 +145,7 @@ public String getOriginalPattern() {
    *     excludedSubdirectories} is nonempty and this pattern does not have type {@code
    *     Type.TARGETS_BELOW_DIRECTORY}.
    */
-  public abstract <T, E extends Exception> void eval(
+  public abstract <T, E extends Exception & QueryExceptionMarkerInterface> void eval(
       TargetPatternResolver<T> resolver,
       InterruptibleSupplier<ImmutableSet<PathFragment>> ignoredSubdirectories,
       ImmutableSet<PathFragment> excludedSubdirectories,
@@ -162,12 +161,13 @@ public abstract <T, E extends Exception> void eval(
    * ExecutionException}, the cause will be an instance of either {@link TargetParsingException} or
    * the given {@code exceptionClass}.
    */
-  public final <T, E extends Exception> ListenableFuture<Void> evalAdaptedForAsync(
-      TargetPatternResolver<T> resolver,
-      InterruptibleSupplier<ImmutableSet<PathFragment>> ignoredSubdirectories,
-      ImmutableSet<PathFragment> excludedSubdirectories,
-      BatchCallback<T, E> callback,
-      Class<E> exceptionClass) {
+  public final <T, E extends Exception & QueryExceptionMarkerInterface>
+      ListenableFuture<Void> evalAdaptedForAsync(
+          TargetPatternResolver<T> resolver,
+          InterruptibleSupplier<ImmutableSet<PathFragment>> ignoredSubdirectories,
+          ImmutableSet<PathFragment> excludedSubdirectories,
+          BatchCallback<T, E> callback,
+          Class<E> exceptionClass) {
     try {
       eval(resolver, ignoredSubdirectories, excludedSubdirectories, callback, exceptionClass);
       return Futures.immediateFuture(null);
@@ -191,7 +191,7 @@ public final <T, E extends Exception> ListenableFuture<Void> evalAdaptedForAsync
    * ExecutionException}, the cause will be an instance of either {@link TargetParsingException} or
    * the given {@code exceptionClass}.
    */
-  public <T, E extends Exception> ListenableFuture<Void> evalAsync(
+  public <T, E extends Exception & QueryExceptionMarkerInterface> ListenableFuture<Void> evalAsync(
       TargetPatternResolver<T> resolver,
       InterruptibleSupplier<ImmutableSet<PathFragment>> ignoredSubdirectories,
       ImmutableSet<PathFragment> excludedSubdirectories,
@@ -258,7 +258,7 @@ private SingleTarget(String targetName, PackageIdentifier directory, String orig
     }
 
     @Override
-    public <T, E extends Exception> void eval(
+    public <T, E extends Exception & QueryExceptionMarkerInterface> void eval(
         TargetPatternResolver<T> resolver,
         InterruptibleSupplier<ImmutableSet<PathFragment>> ignoredSubdirectories,
         ImmutableSet<PathFragment> excludedSubdirectories,
@@ -320,7 +320,7 @@ private InterpretPathAsTarget(String path, String originalPattern) {
     }
 
     @Override
-    public <T, E extends Exception> void eval(
+    public <T, E extends Exception & QueryExceptionMarkerInterface> void eval(
         TargetPatternResolver<T> resolver,
         InterruptibleSupplier<ImmutableSet<PathFragment>> ignoredSubdirectories,
         ImmutableSet<PathFragment> excludedSubdirectories,
@@ -418,7 +418,7 @@ private TargetsInPackage(
     }
 
     @Override
-    public <T, E extends Exception> void eval(
+    public <T, E extends Exception & QueryExceptionMarkerInterface> void eval(
         TargetPatternResolver<T> resolver,
         InterruptibleSupplier<ImmutableSet<PathFragment>> ignoredSubdirectories,
         ImmutableSet<PathFragment> excludedSubdirectories,
@@ -544,7 +544,7 @@ private TargetsBelowDirectory(
     }
 
     @Override
-    public <T, E extends Exception> void eval(
+    public <T, E extends Exception & QueryExceptionMarkerInterface> void eval(
         TargetPatternResolver<T> resolver,
         InterruptibleSupplier<ImmutableSet<PathFragment>> ignoredSubdirectories,
         ImmutableSet<PathFragment> excludedSubdirectories,
@@ -573,13 +573,14 @@ public <T, E extends Exception> void eval(
     }
 
     @Override
-    public <T, E extends Exception> ListenableFuture<Void> evalAsync(
-        TargetPatternResolver<T> resolver,
-        InterruptibleSupplier<ImmutableSet<PathFragment>> ignoredSubdirectories,
-        ImmutableSet<PathFragment> excludedSubdirectories,
-        BatchCallback<T, E> callback,
-        Class<E> exceptionClass,
-        ListeningExecutorService executor) {
+    public <T, E extends Exception & QueryExceptionMarkerInterface>
+        ListenableFuture<Void> evalAsync(
+            TargetPatternResolver<T> resolver,
+            InterruptibleSupplier<ImmutableSet<PathFragment>> ignoredSubdirectories,
+            ImmutableSet<PathFragment> excludedSubdirectories,
+            BatchCallback<T, E> callback,
+            Class<E> exceptionClass,
+            ListeningExecutorService executor) {
       Preconditions.checkState(
           !excludedSubdirectories.contains(directory.getPackageFragment()),
           "Fully excluded target pattern %s should have already been filtered out (%s)",

src/main/java/com/google/devtools/build/lib/cmdline/TargetPatternResolver.java

@@ -14,11 +14,13 @@
 
 package com.google.devtools.build.lib.cmdline;
 
+import static com.google.common.util.concurrent.Futures.immediateCancelledFuture;
+import static com.google.common.util.concurrent.Futures.immediateFailedFuture;
+import static com.google.common.util.concurrent.Futures.immediateVoidFuture;
+
 import com.google.common.collect.ImmutableSet;
-import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.ListeningExecutorService;
-import com.google.devtools.build.lib.concurrent.BatchCallback;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.Collection;
 
@@ -88,32 +90,34 @@ public abstract Collection<T> getTargetsInPackage(
    * @param exceptionClass The class type of the parameterized exception.
    * @throws TargetParsingException under implementation-specific failure conditions
    */
-  public abstract <E extends Exception> void findTargetsBeneathDirectory(
-      RepositoryName repository,
-      String originalPattern,
-      String directory,
-      boolean rulesOnly,
-      ImmutableSet<PathFragment> forbiddenSubdirectories,
-      ImmutableSet<PathFragment> excludedSubdirectories,
-      BatchCallback<T, E> callback,
-      Class<E> exceptionClass)
-      throws TargetParsingException, E, InterruptedException;
+  public abstract <E extends Exception & QueryExceptionMarkerInterface>
+      void findTargetsBeneathDirectory(
+          RepositoryName repository,
+          String originalPattern,
+          String directory,
+          boolean rulesOnly,
+          ImmutableSet<PathFragment> forbiddenSubdirectories,
+          ImmutableSet<PathFragment> excludedSubdirectories,
+          BatchCallback<T, E> callback,
+          Class<E> exceptionClass)
+          throws TargetParsingException, E, InterruptedException;
 
   /**
    * Async version of {@link #findTargetsBeneathDirectory}
    *
    * <p>Default implementation is synchronous.
    */
-  public <E extends Exception> ListenableFuture<Void> findTargetsBeneathDirectoryAsync(
-      RepositoryName repository,
-      String originalPattern,
-      String directory,
-      boolean rulesOnly,
-      ImmutableSet<PathFragment> forbiddenSubdirectories,
-      ImmutableSet<PathFragment> excludedSubdirectories,
-      BatchCallback<T, E> callback,
-      Class<E> exceptionClass,
-      ListeningExecutorService executor) {
+  public <E extends Exception & QueryExceptionMarkerInterface>
+      ListenableFuture<Void> findTargetsBeneathDirectoryAsync(
+          RepositoryName repository,
+          String originalPattern,
+          String directory,
+          boolean rulesOnly,
+          ImmutableSet<PathFragment> forbiddenSubdirectories,
+          ImmutableSet<PathFragment> excludedSubdirectories,
+          BatchCallback<T, E> callback,
+          Class<E> exceptionClass,
+          ListeningExecutorService executor) {
       try {
       findTargetsBeneathDirectory(
           repository,
@@ -124,14 +128,14 @@ public <E extends Exception> ListenableFuture<Void> findTargetsBeneathDirectoryA
           excludedSubdirectories,
           callback,
           exceptionClass);
-        return Futures.immediateFuture(null);
+      return immediateVoidFuture();
       } catch (TargetParsingException e) {
-        return Futures.immediateFailedFuture(e);
+      return immediateFailedFuture(e);
       } catch (InterruptedException e) {
-        return Futures.immediateCancelledFuture();
+      return immediateCancelledFuture();
       } catch (Exception e) {
         if (exceptionClass.isInstance(e)) {
-          return Futures.immediateFailedFuture(e);
+        return immediateFailedFuture(e);
         }
         throw new IllegalStateException(e);
       }

src/main/java/com/google/devtools/build/lib/concurrent/BUILD

@@ -18,3 +18,8 @@ java_library(
         "//third_party:jsr305",
     ],
 )
+
+java_library(
+    name = "thread_safety",
+    srcs = ["ThreadSafety.java"],
+)

src/main/java/com/google/devtools/build/lib/pkgcache/BUILD

@@ -27,6 +27,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/cmdline:LabelValidator",
+        "//src/main/java/com/google/devtools/build/lib/cmdline:batch_callback",
         "//src/main/java/com/google/devtools/build/lib/collect/compacthashset",
         "//src/main/java/com/google/devtools/build/lib/concurrent",
         "//src/main/java/com/google/devtools/build/lib/events",