Remove TransitiveTargetValue.transitive{S,Uns}uccessfulPkgs

It was only used by GenQuery, for which we can use getTransitiveRootCauses()
and getTransitiveTargets() instead.

PiperOrigin-RevId: 162471592

3 files changed, 24 insertions, 80 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 1901c7862e..54593c1744 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
@@ -87,6 +87,7 @@ import java.io.IOException;
 import java.nio.channels.ClosedByInterruptException;
 import java.util.Collection;
 import java.util.HashSet;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -211,26 +212,28 @@ public class GenQuery implements RuleConfiguredTargetFactory {
     // 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> successfulPackageNames = NestedSetBuilder.stableOrder();
+    Set<PackageIdentifier> successfulPackageNames = new LinkedHashSet<>();
     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()) {
+      if (transNode.getTransitiveRootCauses() != null) {
         // 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());
-      successfulPackageNames.addTransitive(transNode.getTransitiveSuccessfulPackages());
+      for (Label transitiveLabel : transNode.getTransitiveTargets()) {
+        successfulPackageNames.add(transitiveLabel.getPackageIdentifier());
+      }
     }
 
     // Construct the package id to package map for all successful packages.
     ImmutableMap.Builder<PackageIdentifier, Package> packageMapBuilder = ImmutableMap.builder();
-    for (PackageIdentifier pkgId : successfulPackageNames.build()) {
+    for (PackageIdentifier pkgId : successfulPackageNames) {
       PackageValue pkg = (PackageValue) env.getValue(PackageValue.key(pkgId));
       Preconditions.checkNotNull(pkg, "package %s not preloaded", pkgId);
       Preconditions.checkState(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java
index 23d22b41e7..3f8a001961 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java
@@ -21,7 +21,6 @@ import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment
 import com.google.devtools.build.lib.analysis.config.ConfigurationFragmentFactory;
 import com.google.devtools.build.lib.analysis.config.FragmentOptions;
 import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.events.Event;
@@ -144,10 +143,6 @@ public class TransitiveTargetFunction
         maybeReportErrorAboutMissingEdge(target, depLabel, e, eventHandler);
         continue;
       }
-      builder.getTransitiveSuccessfulPkgs().addTransitive(
-          transitiveTargetValue.getTransitiveSuccessfulPackages());
-      builder.getTransitiveUnsuccessfulPkgs().addTransitive(
-          transitiveTargetValue.getTransitiveUnsuccessfulPackages());
       builder.getTransitiveTargets().addTransitive(transitiveTargetValue.getTransitiveTargets());
       NestedSet<Label> rootCauses = transitiveTargetValue.getTransitiveRootCauses();
       if (rootCauses != null) {
@@ -245,14 +240,15 @@ public class TransitiveTargetFunction
     }
   }
 
-  private void addFragmentsIfNew(TransitiveTargetValueBuilder builder, Iterable<?> fragments) {
+  private void addFragmentsIfNew(
+      TransitiveTargetValueBuilder builder, Iterable<? extends Class<?>> fragments) {
     // We take Iterable<?> instead of Iterable<Class<?>> or Iterable<Class<? extends Fragment>>
     // because both of the latter are passed as actual parameters and there's no way to consistently
     // cast to one of them. In actuality, all values are Class<? extends Fragment>, but the values
     // coming from Attribute.java don't have access to the Fragment symbol since Attribute is built
     // in a different library.
-    for (Object fragment : fragments) {
-      addFragmentIfNew(builder, (Class<? extends Fragment>) fragment);
+    for (Class<?> fragment : fragments) {
+      addFragmentIfNew(builder, fragment.asSubclass(Fragment.class));
     }
   }
 
@@ -327,8 +323,6 @@ public class TransitiveTargetFunction
    */
   static class TransitiveTargetValueBuilder {
     private boolean successfulTransitiveLoading;
-    private final NestedSetBuilder<PackageIdentifier> transitiveSuccessfulPkgs;
-    private final NestedSetBuilder<PackageIdentifier> transitiveUnsuccessfulPkgs;
     private final NestedSetBuilder<Label> transitiveTargets;
     private final NestedSetBuilder<Class<? extends Fragment>> transitiveConfigFragments;
     private final Set<Class<? extends Fragment>> configFragmentsFromDeps;
@@ -336,8 +330,6 @@ public class TransitiveTargetFunction
 
     public TransitiveTargetValueBuilder(Label label, Target target,
         boolean packageLoadedSuccessfully) {
-      this.transitiveSuccessfulPkgs = NestedSetBuilder.stableOrder();
-      this.transitiveUnsuccessfulPkgs = NestedSetBuilder.stableOrder();
       this.transitiveTargets = NestedSetBuilder.stableOrder();
       this.transitiveConfigFragments = NestedSetBuilder.stableOrder();
       // No need to store directly required fragments that are also required by deps.
@@ -345,24 +337,12 @@ public class TransitiveTargetFunction
       this.transitiveRootCauses = NestedSetBuilder.stableOrder();
 
       this.successfulTransitiveLoading = packageLoadedSuccessfully;
-      PackageIdentifier packageId = target.getPackage().getPackageIdentifier();
-      if (packageLoadedSuccessfully) {
-        transitiveSuccessfulPkgs.add(packageId);
-      } else {
+      if (!packageLoadedSuccessfully) {
         transitiveRootCauses.add(label);
-        transitiveUnsuccessfulPkgs.add(packageId);
       }
       transitiveTargets.add(target.getLabel());
     }
 
-    public NestedSetBuilder<PackageIdentifier> getTransitiveSuccessfulPkgs() {
-      return transitiveSuccessfulPkgs;
-    }
-
-    public NestedSetBuilder<PackageIdentifier> getTransitiveUnsuccessfulPkgs() {
-      return transitiveUnsuccessfulPkgs;
-    }
-
     public NestedSetBuilder<Label> getTransitiveTargets() {
       return transitiveTargets;
     }
@@ -388,16 +368,15 @@ public class TransitiveTargetFunction
     }
 
     public SkyValue build(@Nullable NoSuchTargetException errorLoadingTarget) {
-      NestedSet<PackageIdentifier> successfullyLoadedPkgs = transitiveSuccessfulPkgs.build();
-      NestedSet<PackageIdentifier> unsuccessfullyLoadedPkgs = transitiveUnsuccessfulPkgs.build();
       NestedSet<Label> loadedTargets = transitiveTargets.build();
       NestedSet<Class<? extends Fragment>> configFragments = transitiveConfigFragments.build();
       return successfulTransitiveLoading
-          ? TransitiveTargetValue.successfulTransitiveLoading(successfullyLoadedPkgs,
-          unsuccessfullyLoadedPkgs, loadedTargets, configFragments)
-          : TransitiveTargetValue.unsuccessfulTransitiveLoading(successfullyLoadedPkgs,
-              unsuccessfullyLoadedPkgs, loadedTargets, transitiveRootCauses.build(),
-              errorLoadingTarget, configFragments);
+          ? TransitiveTargetValue.successfulTransitiveLoading(loadedTargets, configFragments)
+          : TransitiveTargetValue.unsuccessfulTransitiveLoading(
+              loadedTargets,
+              transitiveRootCauses.build(),
+              errorLoadingTarget,
+              configFragments);
     }
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetValue.java
index e603ca371c..843c531b3d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetValue.java
@@ -15,7 +15,6 @@ package com.google.devtools.build.lib.skyframe;
 
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
@@ -29,36 +28,28 @@ import com.google.devtools.build.skyframe.SkyValue;
 import java.io.IOException;
 import java.io.ObjectInputStream;
 import java.io.ObjectOutputStream;
-import java.util.Set;
 import javax.annotation.Nullable;
 
 /**
  * A <i>transitive</i> target reference that, when built in skyframe, loads the entire
  * transitive closure of a target.
  *
- * This will probably be unnecessary once other refactorings occur throughout the codebase
- * which make loading/analysis interleaving more feasible, or we will migrate "blaze query" to
- * use this to evaluate its Target graph.
+ * <p>Note that this class drops transitive targets during serialization!
  */
 @Immutable
 @ThreadSafe
 public class TransitiveTargetValue implements SkyValue {
-
   // Non-final for serialization purposes.
-  private NestedSet<PackageIdentifier> transitiveSuccessfulPkgs;
-  private NestedSet<PackageIdentifier> transitiveUnsuccessfulPkgs;
   private NestedSet<Label> transitiveTargets;
   @Nullable private NestedSet<Label> transitiveRootCauses;
   @Nullable private NoSuchTargetException errorLoadingTarget;
   private NestedSet<Class<? extends BuildConfiguration.Fragment>> transitiveConfigFragments;
 
-  private TransitiveTargetValue(NestedSet<PackageIdentifier> transitiveSuccessfulPkgs,
-      NestedSet<PackageIdentifier> transitiveUnsuccessfulPkgs, NestedSet<Label> transitiveTargets,
+  private TransitiveTargetValue(
+      NestedSet<Label> transitiveTargets,
       @Nullable NestedSet<Label> transitiveRootCauses,
       @Nullable NoSuchTargetException errorLoadingTarget,
       NestedSet<Class<? extends BuildConfiguration.Fragment>> transitiveConfigFragments) {
-    this.transitiveSuccessfulPkgs = transitiveSuccessfulPkgs;
-    this.transitiveUnsuccessfulPkgs = transitiveUnsuccessfulPkgs;
     this.transitiveTargets = transitiveTargets;
     this.transitiveRootCauses = transitiveRootCauses;
     this.errorLoadingTarget = errorLoadingTarget;
@@ -66,14 +57,6 @@ public class TransitiveTargetValue implements SkyValue {
   }
 
   private void writeObject(ObjectOutputStream out) throws IOException {
-    // It helps to flatten the transitiveSuccessfulPkgs nested set as it has lots of duplicates.
-    Set<PackageIdentifier> successfulPkgs = transitiveSuccessfulPkgs.toSet();
-    out.writeInt(successfulPkgs.size());
-    for (PackageIdentifier pkg : successfulPkgs) {
-      out.writeObject(pkg);
-    }
-
-    out.writeObject(transitiveUnsuccessfulPkgs);
     // Deliberately do not write out transitiveTargets. There is a lot of those and they drive
     // serialization costs through the roof, both in terms of space and of time.
     // TODO(bazel-team): Deal with this properly once we have efficient serialization of NestedSets.
@@ -84,13 +67,6 @@ public class TransitiveTargetValue implements SkyValue {
 
   @SuppressWarnings("unchecked")
   private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException {
-    int successfulPkgCount = in.readInt();
-    NestedSetBuilder<PackageIdentifier> pkgs = NestedSetBuilder.stableOrder();
-    for (int i = 0; i < successfulPkgCount; i++) {
-      pkgs.add((PackageIdentifier) in.readObject());
-    }
-    transitiveSuccessfulPkgs = pkgs.build();
-    transitiveUnsuccessfulPkgs = (NestedSet<PackageIdentifier>) in.readObject();
     // TODO(bazel-team): Deal with transitiveTargets properly.
     transitiveTargets = NestedSetBuilder.emptySet(Order.STABLE_ORDER);
     transitiveRootCauses = (NestedSet<Label>) in.readObject();
@@ -100,21 +76,17 @@ public class TransitiveTargetValue implements SkyValue {
   }
 
   static TransitiveTargetValue unsuccessfulTransitiveLoading(
-      NestedSet<PackageIdentifier> transitiveSuccessfulPkgs,
-      NestedSet<PackageIdentifier> transitiveUnsuccessfulPkgs, NestedSet<Label> transitiveTargets,
+      NestedSet<Label> transitiveTargets,
       NestedSet<Label> rootCauses, @Nullable NoSuchTargetException errorLoadingTarget,
       NestedSet<Class<? extends BuildConfiguration.Fragment>> transitiveConfigFragments) {
-    return new TransitiveTargetValue(transitiveSuccessfulPkgs, transitiveUnsuccessfulPkgs,
+    return new TransitiveTargetValue(
         transitiveTargets, rootCauses, errorLoadingTarget, transitiveConfigFragments);
   }
 
   static TransitiveTargetValue successfulTransitiveLoading(
-      NestedSet<PackageIdentifier> transitiveSuccessfulPkgs,
-      NestedSet<PackageIdentifier> transitiveUnsuccessfulPkgs,
       NestedSet<Label> transitiveTargets,
       NestedSet<Class<? extends BuildConfiguration.Fragment>> transitiveConfigFragments) {
-    return new TransitiveTargetValue(transitiveSuccessfulPkgs, transitiveUnsuccessfulPkgs,
-        transitiveTargets, null, null, transitiveConfigFragments);
+    return new TransitiveTargetValue(transitiveTargets, null, null, transitiveConfigFragments);
   }
 
   /** Returns the error, if any, from loading the target. */
@@ -123,16 +95,6 @@ public class TransitiveTargetValue implements SkyValue {
     return errorLoadingTarget;
   }
 
-  /** Returns the packages that were transitively successfully loaded. */
-  public NestedSet<PackageIdentifier> getTransitiveSuccessfulPackages() {
-    return transitiveSuccessfulPkgs;
-  }
-
-  /** Returns the packages that were transitively successfully loaded. */
-  public NestedSet<PackageIdentifier> getTransitiveUnsuccessfulPackages() {
-    return transitiveUnsuccessfulPkgs;
-  }
-
   /** Returns the targets that were transitively loaded. */
   public NestedSet<Label> getTransitiveTargets() {
     return transitiveTargets;