Automated rollback of commit 7fe59b98eefc96a6310f0b0221d4e0f18e2a9000.

*** Reason for rollback ***

Fixed bug due to TransitiveTargetFunction requesting multiple Package dependencies when computing its aspect deps by only applying the optimization to TransitiveTraversalFunction.

*** Original change description ***

Automated rollback of commit cce164aed44aba1de244f0d764cd33a5cc6980b2.

PiperOrigin-RevId: 186766812

1 files changed, 29 insertions, 10 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java
index b51bab41d5..e438d917d4 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.Collections2;
 import com.google.common.collect.ImmutableSet;
@@ -61,7 +62,6 @@ import javax.annotation.Nullable;
  * return.
  */
 abstract class TransitiveBaseTraversalFunction<TProcessedTargets> implements SkyFunction {
-
   /**
    * Returns a {@link SkyKey} corresponding to the traversal of a target specified by {@code label}
    * and its transitive dependencies.
@@ -126,9 +126,9 @@ abstract class TransitiveBaseTraversalFunction<TProcessedTargets> implements Sky
         loadTargetResultsType);
     TargetAndErrorIfAny targetAndErrorIfAny = (TargetAndErrorIfAny) loadTargetResults;
 
-    // Process deps from attributes.
-    Collection<SkyKey> labelDepKeys =
-        Collections2.transform(getLabelDeps(targetAndErrorIfAny.getTarget()), this::getKey);
+    // Process deps from attributes. It is essential that the last getValue(s) call we made to
+    // skyframe for building this node was for the corresponding PackageValue.
+    Collection<SkyKey> labelDepKeys = getLabelDepKeys(env, targetAndErrorIfAny);
 
     Map<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> depMap =
         env.getValuesOrThrow(labelDepKeys, NoSuchPackageException.class,
@@ -136,9 +136,10 @@ abstract class TransitiveBaseTraversalFunction<TProcessedTargets> implements Sky
     if (env.valuesMissing()) {
       return null;
     }
-    // Process deps from aspects.
+    // Process deps from attributes. It is essential that the second-to-last getValue(s) call we
+    // made to skyframe for building this node was for the corresponding PackageValue.
     Iterable<SkyKey> labelAspectKeys =
-        getStrictLabelAspectKeys(targetAndErrorIfAny.getTarget(), depMap, env);
+        getStrictLabelAspectDepKeys(env, depMap, targetAndErrorIfAny);
     Set<Entry<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>>>
         labelAspectEntries = env.getValuesOrThrow(labelAspectKeys, NoSuchPackageException.class,
         NoSuchTargetException.class).entrySet();
@@ -153,6 +154,20 @@ abstract class TransitiveBaseTraversalFunction<TProcessedTargets> implements Sky
     return computeSkyValue(targetAndErrorIfAny, processedTargets);
   }
 
+  Collection<SkyKey> getLabelDepKeys(
+      SkyFunction.Environment env, TargetAndErrorIfAny targetAndErrorIfAny)
+      throws InterruptedException {
+    return Collections2.transform(getLabelDeps(targetAndErrorIfAny.getTarget()), this::getKey);
+  }
+
+  Iterable<SkyKey> getStrictLabelAspectDepKeys(
+      SkyFunction.Environment env,
+      Map<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> depMap,
+      TargetAndErrorIfAny targetAndErrorIfAny)
+      throws InterruptedException {
+    return getStrictLabelAspectKeys(targetAndErrorIfAny.getTarget(), depMap, env);
+  }
+
   @Override
   public String extractTag(SkyKey skyKey) {
     return Label.print(argumentFromKey(skyKey));
@@ -269,14 +284,18 @@ abstract class TransitiveBaseTraversalFunction<TProcessedTargets> implements Sky
     Target getTarget();
   }
 
-  private static class TargetAndErrorIfAnyImpl implements TargetAndErrorIfAny, LoadTargetResults {
+  @VisibleForTesting
+  static class TargetAndErrorIfAnyImpl implements TargetAndErrorIfAny, LoadTargetResults {
 
     private final boolean packageLoadedSuccessfully;
     @Nullable private final NoSuchTargetException errorLoadingTarget;
     private final Target target;
 
-    private TargetAndErrorIfAnyImpl(boolean packageLoadedSuccessfully,
-        @Nullable NoSuchTargetException errorLoadingTarget, Target target) {
+    @VisibleForTesting
+    TargetAndErrorIfAnyImpl(
+        boolean packageLoadedSuccessfully,
+        @Nullable NoSuchTargetException errorLoadingTarget,
+        Target target) {
       this.packageLoadedSuccessfully = packageLoadedSuccessfully;
       this.errorLoadingTarget = errorLoadingTarget;
       this.target = target;
@@ -304,7 +323,7 @@ abstract class TransitiveBaseTraversalFunction<TProcessedTargets> implements Sky
     }
   }
 
-  private LoadTargetResults loadTarget(Environment env, Label label)
+  LoadTargetResults loadTarget(Environment env, Label label)
       throws NoSuchTargetException, NoSuchPackageException, InterruptedException {
     SkyKey packageKey = PackageValue.key(label.getPackageIdentifier());
     SkyKey targetKey = TargetMarkerValue.key(label);