Stop GenQuery evaluation when an error is found in the transitive closure of the scope.

This seems to only have been a problem when the error was in an unselect path, as the query would have failed earlier otherwise.

--
PiperOrigin-RevId: 145282112
MOS_MIGRATED_REVID=145282112

1 files changed, 35 insertions, 8 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java b/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java
index 1fa0af0fcc..87925af0ff 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java
@@ -196,30 +196,45 @@ public class GenQuery implements RuleConfiguredTargetFactory {
     return ruleContext.getAnalysisEnvironment().getEventHandler();
   }
 
+  /**
+   * Precomputes the transitive closure of the scope. Returns two maps: one identifying the
+   * successful packages, and the other identifying the valid targets. Breaks in the transitive
+   * closure of the scope will cause the query to error out early.
+   */
   @Nullable
   private static Pair<ImmutableMap<PackageIdentifier, Package>, ImmutableMap<Label, Target>>
       constructPackageMap(SkyFunction.Environment env, Collection<Target> scope)
-          throws InterruptedException {
+          throws InterruptedException, BrokenQueryScopeException {
     // It is not necessary for correctness to construct intermediate NestedSets; we could iterate
     // over individual targets in scope immediately. However, creating a composite NestedSet first
     // saves us from iterating over the same sub-NestedSets multiple times.
     NestedSetBuilder<Label> validTargets = NestedSetBuilder.stableOrder();
-    NestedSetBuilder<PackageIdentifier> packageNames = NestedSetBuilder.stableOrder();
+    NestedSetBuilder<PackageIdentifier> successfulPackageNames = NestedSetBuilder.stableOrder();
     for (Target target : scope) {
       SkyKey key = TransitiveTargetValue.key(target.getLabel());
       TransitiveTargetValue transNode = (TransitiveTargetValue) env.getValue(key);
       if (transNode == null) {
         return null;
       }
+      if (!transNode.getTransitiveUnsuccessfulPackages().isEmpty()) {
+        // This should only happen if the unsuccessful package was loaded in a non-selected
+        // path, as otherwise this configured target would have failed earlier. See b/34132681.
+        throw new BrokenQueryScopeException(
+            "errors were encountered while computing transitive closure of the scope.");
+      }
       validTargets.addTransitive(transNode.getTransitiveTargets());
-      packageNames.addTransitive(transNode.getTransitiveSuccessfulPackages());
+      successfulPackageNames.addTransitive(transNode.getTransitiveSuccessfulPackages());
     }
 
+    // Construct the package id to package map for all successful packages.
     ImmutableMap.Builder<PackageIdentifier, Package> packageMapBuilder = ImmutableMap.builder();
-    for (PackageIdentifier pkgId : packageNames.build()) {
+    for (PackageIdentifier pkgId : successfulPackageNames.build()) {
       PackageValue pkg = (PackageValue) env.getValue(PackageValue.key(pkgId));
       Preconditions.checkNotNull(pkg, "package %s not preloaded", pkgId);
-      Preconditions.checkState(!pkg.getPackage().containsErrors(), pkgId);
+      Preconditions.checkState(
+          !pkg.getPackage().containsErrors(),
+          "package %s was found to both have and not have errors.",
+          pkgId);
       packageMapBuilder.put(pkg.getPackage().getPackageIdentifier(), pkg.getPackage());
     }
     ImmutableMap<PackageIdentifier, Package> packageMap = packageMapBuilder.build();
@@ -239,11 +254,17 @@ public class GenQuery implements RuleConfiguredTargetFactory {
   private byte[] executeQuery(RuleContext ruleContext, QueryOptions queryOptions,
       Set<Target> scope, String query) throws InterruptedException {
     SkyFunction.Environment env = ruleContext.getAnalysisEnvironment().getSkyframeEnv();
-    Pair<ImmutableMap<PackageIdentifier, Package>, ImmutableMap<Label, Target>> closureInfo =
-        constructPackageMap(env, scope);
-    if (closureInfo == null) {
+    Pair<ImmutableMap<PackageIdentifier, Package>, ImmutableMap<Label, Target>> closureInfo;
+    try {
+      closureInfo = constructPackageMap(env, scope);
+      if (closureInfo == null) {
+        return null;
+      }
+    } catch (BrokenQueryScopeException e) {
+      ruleContext.ruleError(e.getMessage());
       return null;
     }
+
     ImmutableMap<PackageIdentifier, Package> packageMap = closureInfo.first;
     ImmutableMap<Label, Target> validTargetsMap = closureInfo.second;
     PackageProvider packageProvider = new PreloadedMapPackageProvider(packageMap, validTargetsMap);
@@ -528,4 +549,10 @@ public class GenQuery implements RuleConfiguredTargetFactory {
       throw new UnsupportedOperationException();
     }
   }
+
+  private static class BrokenQueryScopeException extends Exception {
+    public BrokenQueryScopeException(String message) {
+      super(message);
+    }
+  }
 }