Remove SkyframeQueryHelper#setBlockUniverseEvaluationErrors.

Instead flip the testing strategy around and have a test case's expectations be conditional on the new `SkyframeQueryHelper#reportsUniverseEvaluationErrors`.

PiperOrigin-RevId: 638799855
Change-Id: I628c40e63cdcef19c64f6a69d3424fa12ba3d90e

src/main/java/com/google/devtools/build/lib/query2/QueryEnvironmentFactory.java

@@ -58,7 +58,6 @@ public AbstractBlazeQueryEnvironment<Target> create(
       Set<Setting> settings,
       Iterable<QueryFunction> extraFunctions,
       @Nullable PathPackageLocator packagePath,
-      boolean blockUniverseEvaluationErrors,
       boolean useGraphlessQuery,
       LabelPrinter labelPrinter) {
     Preconditions.checkNotNull(universeScope);
@@ -74,7 +73,6 @@ public AbstractBlazeQueryEnvironment<Target> create(
           graphFactory,
           universeScope,
           packagePath,
-          blockUniverseEvaluationErrors,
           labelPrinter);
     } else if (useGraphlessQuery) {
       return new GraphlessBlazeQueryEnvironment(

src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java

@@ -50,9 +50,7 @@
 import com.google.devtools.build.lib.concurrent.BlockingStack;
 import com.google.devtools.build.lib.concurrent.MultisetSemaphore;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
-import com.google.devtools.build.lib.events.DelegatingEventHandler;
 import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.events.EventKind;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
 import com.google.devtools.build.lib.packages.DependencyFilter;
@@ -154,8 +152,6 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment<Target>
   protected final int loadingPhaseThreads;
   protected final WalkableGraphFactory graphFactory;
   protected final UniverseScope universeScope;
-  protected boolean blockUniverseEvaluationErrors;
-  protected ExtendedEventHandler universeEvalEventHandler;
   protected final TargetPattern.Parser mainRepoTargetParser;
   protected final PathFragment parserPrefix;
   protected final PathPackageLocator pkgPath;
@@ -181,7 +177,6 @@ public SkyQueryEnvironment(
       WalkableGraphFactory graphFactory,
       UniverseScope universeScope,
       PathPackageLocator pkgPath,
-      boolean blockUniverseEvaluationErrors,
       LabelPrinter labelPrinter) {
     this(
         keepGoing,
@@ -197,7 +192,6 @@ public SkyQueryEnvironment(
         graphFactory,
         universeScope,
         pkgPath,
-        blockUniverseEvaluationErrors,
         labelPrinter);
   }
 
@@ -213,7 +207,6 @@ protected SkyQueryEnvironment(
       WalkableGraphFactory graphFactory,
       UniverseScope universeScope,
       PathPackageLocator pkgPath,
-      boolean blockUniverseEvaluationErrors,
       LabelPrinter labelPrinter) {
     super(
         keepGoing,
@@ -230,11 +223,6 @@ protected SkyQueryEnvironment(
     this.mainRepoTargetParser = mainRepoTargetParser;
     this.parserPrefix = parserPrefix;
     this.queryEvaluationParallelismLevel = queryEvaluationParallelismLevel;
-    this.blockUniverseEvaluationErrors = blockUniverseEvaluationErrors;
-    this.universeEvalEventHandler =
-        this.blockUniverseEvaluationErrors
-            ? new ErrorBlockingForwardingEventHandler(this.eventHandler)
-            : this.eventHandler;
     // In #getAllowedDeps we have special treatment of deps entailed by the `visibility` attribute.
     // Since this attribute is of the NODEP type, that means we need a special implementation of
     // NO_NODEP_DEPS.
@@ -261,7 +249,7 @@ protected Set<SkyKey> getGraphRootsFromUniverseKeyAndExpression(
   protected EvaluationContext newEvaluationContext() {
     return EvaluationContext.newBuilder()
         .setParallelism(loadingPhaseThreads)
-        .setEventHandler(universeEvalEventHandler)
+        .setEventHandler(eventHandler)
         .build();
   }
 
@@ -1480,32 +1468,4 @@ public QueryTaskFuture<Void> getRdepsBoundedParallel(
             ParallelSkyQueryUtils.getRdepsInUniverseBoundedParallel(
                 this, expression, depth, universePredicate, context, callback));
   }
-
-  /**
-   * Query evaluation behavior is specified with respect to errors it emits. (Or at least it should
-   * be. Tools rely on it.) Notably, errors that occur during evaluation of a query's universe must
-   * not be emitted during query command evaluation. Consider the case of a simple single target
-   * query when {@code //...} is the universe: errors in far flung parts of the workspace should not
-   * be emitted when that query command is evaluated.
-   *
-   * <p>Non-error message events are not specified. For instance, it's useful (and expected by some
-   * unit tests that should know better) for query commands to emit {@link EventKind#PROGRESS}
-   * events during package loading.
-   *
-   * <p>Therefore, this class is used to forward only non-{@link EventKind#ERROR} events during
-   * universe loading to the {@link SkyQueryEnvironment}'s {@link ExtendedEventHandler}.
-   */
-  protected static class ErrorBlockingForwardingEventHandler extends DelegatingEventHandler {
-
-    public ErrorBlockingForwardingEventHandler(ExtendedEventHandler delegate) {
-      super(delegate);
-    }
-
-    @Override
-    public void handle(Event e) {
-      if (!e.getKind().equals(EventKind.ERROR)) {
-        super.handle(e);
-      }
-    }
-  }
 }

src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java

@@ -327,7 +327,6 @@ private static GenQueryResult doQuery(
               settings,
               /* extraFunctions= */ ImmutableList.of(),
               /* packagePath= */ null,
-              /* blockUniverseEvaluationErrors= */ false,
               /* useGraphlessQuery= */ graphlessQuery,
               LabelPrinter.legacy());
       QueryExpression expr = QueryExpression.parse(query, queryEnvironment);

src/main/java/com/google/devtools/build/lib/runtime/commands/QueryEnvironmentBasedCommand.java

@@ -280,7 +280,6 @@ public static AbstractBlazeQueryEnvironment<Target> newQueryEnvironment(
             settings,
             env.getRuntime().getQueryFunctions(),
             env.getPackageManager().getPackagePath(),
-            /* blockUniverseEvaluationErrors= */ false,
             useGraphlessQuery,
             labelPrinter);
   }

src/test/java/com/google/devtools/build/lib/query2/engine/GraphlessQueryTest.java

@@ -119,7 +119,6 @@ public AbstractBlazeQueryEnvironment<Target> create(
               Set<Setting> settings,
               Iterable<QueryFunction> extraFunctions,
               @Nullable PathPackageLocator packagePath,
-              boolean blockUniverseEvaluationErrors,
               boolean useGraphlessQuery,
               LabelPrinter labelPrinter) {
             return new GraphlessBlazeQueryEnvironment(

src/test/java/com/google/devtools/build/lib/query2/testutil/AbstractQueryKeepGoingTest.java

@@ -24,6 +24,7 @@
 import com.google.devtools.build.lib.query2.engine.QueryException;
 import com.google.devtools.build.lib.query2.testutil.AbstractQueryTest.QueryHelper.ResultAndTargets;
 import com.google.devtools.build.lib.server.FailureDetails.PackageLoading.Code;
+import com.google.devtools.build.lib.server.FailureDetails.Query;
 import com.google.devtools.build.lib.testutil.TestConstants;
 import com.google.devtools.build.lib.util.ExitCode;
 import java.util.Set;
@@ -111,8 +112,15 @@ public void testErrorWhenResultContainsLabelsCrossingSubpackage() throws Excepti
         """);
     writeFile("pear/plum/BUILD");
 
-    assertPackageLoadingCode(evalFail("//pear:apple"), Code.LABEL_CROSSES_PACKAGE_BOUNDARY);
+    ResultAndTargets<Target> resultAndTargets = evalFail("//pear:apple");
     assertContainsEvent("is invalid because 'pear/plum' is a subpackage");
+    if (helper.reportsUniverseEvaluationErrors()) {
+      assertPackageLoadingCode(resultAndTargets, Code.LABEL_CROSSES_PACKAGE_BOUNDARY);
+    } else {
+      assertQueryCode(
+          resultAndTargets.getQueryEvalResult().getDetailedExitCode().getFailureDetail(),
+          Query.Code.BUILD_FILE_ERROR);
+    }
   }
 
   @Test
@@ -132,8 +140,15 @@ public void testErrorWhenWildcardResultContainsLabelsCrossingSubpackage() throws
         """);
     writeFile("pear/plum/BUILD");
 
-    assertPackageLoadingCode(evalFail("//pear:all"), Code.LABEL_CROSSES_PACKAGE_BOUNDARY);
+    ResultAndTargets<Target> resultAndTargets = evalFail("//pear:all");
     assertContainsEvent("is invalid because 'pear/plum' is a subpackage");
+    if (helper.reportsUniverseEvaluationErrors()) {
+      assertPackageLoadingCode(resultAndTargets, Code.LABEL_CROSSES_PACKAGE_BOUNDARY);
+    } else {
+      assertQueryCode(
+          resultAndTargets.getQueryEvalResult().getDetailedExitCode().getFailureDetail(),
+          Query.Code.BUILD_FILE_ERROR);
+    }
   }
 
   @Override
@@ -217,7 +232,13 @@ public void testNoFailFastOnLabelsExpression() throws Exception {
   public void testBadBuildFileKeepGoing() throws Exception {
     writeFile("bad/BUILD", "blah blah blah");
     ResultAndTargets<Target> result = evalFail("bad:*");
-    assertPackageLoadingCode(result, Code.SYNTAX_ERROR);
+    if (helper.reportsUniverseEvaluationErrors()) {
+      assertPackageLoadingCode(result, Code.SYNTAX_ERROR);
+    } else {
+      assertQueryCode(
+          result.getQueryEvalResult().getDetailedExitCode().getFailureDetail(),
+          Query.Code.BUILD_FILE_ERROR);
+    }
     assertContainsEvent("syntax error at 'blah'");
     assertContainsEvent("--keep_going specified, ignoring errors. Results may be inaccurate");
 
@@ -301,14 +322,17 @@ public void testErrorReportedWhenStarlarkLoadRefersToMissingPkgExistingFile_IPAT
 
   private void runTestErrorReportedWhenStarlarkLoadRefersToMissingPkgExistingFile(
       String queryExpression, int numExpectedTargets) throws Exception {
+    if (helper.reportsUniverseEvaluationErrors()) {
+      // This family of test cases are interesting only for query environments that don't report
+      // universe evaluation errors. This way we can assert that any error message came from query
+      // evaluation.
+      return;
+    }
+
     // Starlark imports must refer to files in packages. When the file being imported exists, but
     // it has no containing package, an error should be reported for queries that involve the
     // package containing that import.
 
-    // This ensures that any error message must come from query evaluation, not universe evaluation
-    // (in the case of SkyQueryEnvironment).
-    helper.setBlockUniverseEvaluationErrors(true);
-
     // The package "//foo" can be loaded and has no errors.
     writeFile("foo/BUILD", "sh_library(name='apple', srcs=['apple.sh'])");
 
@@ -419,13 +443,16 @@ public void testErrorReportedWhenStarlarkLoadRefersToExistingPkgMissingFile_IPAT
 
   private void runTestErrorReportedWhenStarlarkLoadRefersToExistingPkgMissingFile(
       String queryExpression, int numExpectedTargets) throws Exception {
+    if (helper.reportsUniverseEvaluationErrors()) {
+      // This family of test cases are interesting only for query environments that don't report
+      // universe evaluation errors. This way we can assert that any error message came from query
+      // evaluation.
+      return;
+    }
+
     // Starlark imports must refer to files that exist, otherwise they will fail and an error should
     // be reported. How shocking!
 
-    // This ensures that any error message must come from query evaluation, not universe evaluation
-    // (in the case of SkyQueryEnvironment).
-    helper.setBlockUniverseEvaluationErrors(true);
-
     // The package "//foo" can be loaded and has no errors.
     writeFile("foo/BUILD", "sh_library(name='apple', srcs=['apple.sh'])");
 
@@ -475,13 +502,16 @@ public void testErrorReportedWhenStarlarkLoadRefersToFileInSymlinkCycle_IPAT() t
 
   private void runTestErrorReportedWhenStarlarkLoadRefersToFileInSymlinkCycle(
       String queryExpression, int numExpectedTargets) throws Exception {
+    if (helper.reportsUniverseEvaluationErrors()) {
+      // This family of test cases are interesting only for query environments that don't report
+      // universe evaluation errors. This way we can assert that any error message came from query
+      // evaluation.
+      return;
+    }
+
     // Starlark imports must refer to files that don't point into a symlink cycle, otherwise they
     // will fail and an error should be reported. Quite astonishing!
 
-    // This ensures that any error message must come from query evaluation, not universe evaluation
-    // (in the case of SkyQueryEnvironment).
-    helper.setBlockUniverseEvaluationErrors(true);
-
     // The package "//foo" can be loaded and has no errors.
     writeFile("foo/BUILD", "sh_library(name='apple', srcs=['apple.sh'])");
 
@@ -514,19 +544,12 @@ private void runTestErrorReportedWhenStarlarkLoadRefersToFileInSymlinkCycle(
 
   @Test
   public void testNoErrorReportedWhenUniverseIncludesBrokenPkgButQueryDoesNot() throws Exception {
-    // The SkyQueryEnvironment implementation can emit errors from two sources: graph evaluation
-    // to prepare the query's universe scope, and query evaluation (which includes things like
-    // reading packages out of the graph). Whether the SkyQueryEnvironment emits errors during graph
-    // evaluation of the universe is controlled by the blockUniverseEvaluationErrors parameter (on
-    // QueryEnvironmentFactory#create and so on).
-    //
-    // The BlazeQueryEnvironment implementation never emits errors during universe evaluation,
-    // because it doesn't *do* universe evaluation. Its graph evaluation is limited to evaluating
-    // the target patterns that appear in the query expression.
-    //
-    // This test asserts that, when told to block errors that only occur during universe evaluation,
-    // neither QueryEnvironment implementation reports them.
-    helper.setBlockUniverseEvaluationErrors(true);
+    if (helper.reportsUniverseEvaluationErrors()) {
+      // This test case is interesting only for query environments that don't report universe
+      // evaluation errors. This way we can assert that any error message came from query
+      // evaluation.
+      return;
+    }
 
     // The package "//foo" is healthy.
     writeFile("foo/BUILD", "sh_library(name='apple', srcs=['apple.sh'])");

src/test/java/com/google/devtools/build/lib/query2/testutil/AbstractQueryTest.java

@@ -1510,9 +1510,9 @@ public void testRegression1309697() throws Exception {
     writeFile("x/BUILD", "cc_library(name='x', srcs=['a.cc', 'a.cc'])");
     String expectedError = "Label '//x:a.cc' is duplicated in the 'srcs' attribute of rule 'x'";
     if (helper.isKeepGoing()) {
-      assertThat(evalThrows("//x", false).getMessage()).isEqualTo(expectedError);
+      assertThat(evalThrows("//x:all", false).getMessage()).contains(expectedError);
     } else {
-      evalThrows("//x", false);
+      evalThrows("//x:all", false);
       assertContainsEvent(expectedError);
     }
   }
@@ -2690,7 +2690,9 @@ public interface QueryHelper<T> {
 
     void setUniverseScope(String universeScope);
 
-    void setBlockUniverseEvaluationErrors(boolean blockUniverseEvaluationErrors);
+    default boolean reportsUniverseEvaluationErrors() {
+      return true;
+    }
 
     /** Re-initializes the query environment with the given settings. */
     void setQuerySettings(Setting... settings);

src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryHelper.java

@@ -132,9 +132,6 @@ public void setWholeTestUniverseScope(String universeScope) {
     wholeTestUniverse = true;
   }
 
-  @Override
-  public void setBlockUniverseEvaluationErrors(boolean blockUniverseEvaluationErrors) {}
-
   @Override
   public Path getRootDirectory() {
     return analysisHelper.getRootDirectory();

src/test/java/com/google/devtools/build/lib/query2/testutil/SkyframeQueryHelper.java

@@ -112,7 +112,6 @@ public abstract class SkyframeQueryHelper extends AbstractQueryHelper<Target> {
 
   private PackageManager pkgManager;
   private TargetPatternPreloader targetParser;
-  private boolean blockUniverseEvaluationErrors;
   protected final ActionKeyContext actionKeyContext = new ActionKeyContext();
 
   private final PathFragment ignoredPackagePrefixesFile = PathFragment.create("ignored");
@@ -178,14 +177,6 @@ public PathFragment getIgnoredPackagePrefixesFile() {
     return ignoredPackagePrefixesFile;
   }
 
-  @Override
-  public void setBlockUniverseEvaluationErrors(boolean blockUniverseEvaluationErrors) {
-    if (this.blockUniverseEvaluationErrors == blockUniverseEvaluationErrors) {
-      return;
-    }
-    this.blockUniverseEvaluationErrors = blockUniverseEvaluationErrors;
-  }
-
   @ForOverride
   protected QueryEnvironmentFactory makeQueryEnvironmentFactory() {
     return new QueryEnvironmentFactory();
@@ -246,7 +237,6 @@ public AbstractBlazeQueryEnvironment<Target> getQueryEnvironment() {
         this.settings,
         getExtraQueryFunctions(),
         pkgManager.getPackagePath(),
-        blockUniverseEvaluationErrors,
         /* useGraphlessQuery= */ false,
         LabelPrinter.legacy());
   }