Clean up Aspect checks in query tests.

--
MOS_MIGRATED_REVID=103977080

5 files changed, 183 insertions, 103 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java b/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java
index 41cdfb48dc..9bacb4ac87 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java
@@ -53,6 +53,7 @@ public final class AspectDefinition {
 
   private final String name;
   private final ImmutableSet<Class<?>> requiredProviders;
+  private final ImmutableSet<String> requiredProviderNames;
   private final ImmutableMap<String, Attribute> attributes;
   private final ImmutableMultimap<String, Class<? extends AspectFactory<?, ?, ?>>> attributeAspects;
 
@@ -63,6 +64,7 @@ public final class AspectDefinition {
       ImmutableMultimap<String, Class<? extends AspectFactory<?, ?, ?>>> attributeAspects) {
     this.name = name;
     this.requiredProviders = requiredProviders;
+    this.requiredProviderNames = toStringSet(requiredProviders);
     this.attributes = attributes;
     this.attributeAspects = attributeAspects;
   }
@@ -95,6 +97,20 @@ public final class AspectDefinition {
   }
 
   /**
+   * Returns the set of {@link com.google.devtools.build.lib.analysis.TransitiveInfoProvider}
+   * instances that must be present on a configured target so that this aspect can be applied to it.
+   *
+   * <p>We cannot refer to that class here due to our dependency structure, so this returns a set
+   * of unconstrained class objects.
+   *
+   * <p>If a configured target does not have a required provider, the aspect is silently not created
+   * for it.
+   */
+  public ImmutableSet<String> getRequiredProviderNames() {
+    return requiredProviderNames;
+  }
+
+  /**
    * Returns the attribute -&gt; set of required aspects map.
    *
    * <p>Note that the map actually contains {@link AspectFactory}
@@ -113,14 +129,27 @@ public final class AspectDefinition {
     if (!(from instanceof Rule) || !(to instanceof Rule)) {
       return ImmutableMultimap.of();
     }
-    LinkedHashMultimap<Attribute, Label> result = LinkedHashMultimap.create();
     RuleClass ruleClass = ((Rule) to).getRuleClassObject();
+    ImmutableSet<Class<?>> providers = ruleClass.getAdvertisedProviders();
+    return visitAspectsIfRequired(from, attribute, toStringSet(providers));
+  }
+
+  /**
+   * Returns the attribute -&gt; set of labels that are provided by aspects of attribute.
+   */
+  public static ImmutableMultimap<Attribute, Label> visitAspectsIfRequired(
+      Target from, Attribute attribute, Set<String> advertisedProviders) {
+    if (advertisedProviders.isEmpty()) {
+      return ImmutableMultimap.of();
+    }
+
+    LinkedHashMultimap<Attribute, Label> result = LinkedHashMultimap.create();
     for (Class<? extends AspectFactory<?, ?, ?>> candidateClass : attribute.getAspects()) {
       AspectFactory<?, ?, ?> candidate = AspectFactory.Util.create(candidateClass);
       // Check if target satisfies condition for this aspect (has to provide all required
       // TransitiveInfoProviders)
-      if (!ruleClass.getAdvertisedProviders().containsAll(
-          candidate.getDefinition().getRequiredProviders())) {
+      if (!advertisedProviders.containsAll(
+            candidate.getDefinition().getRequiredProviderNames())) {
         continue;
       }
       addAllAttributesOfAspect((Rule) from, result, candidate.getDefinition(), Rule.ALL_DEPS);
@@ -128,6 +157,14 @@ public final class AspectDefinition {
     return ImmutableMultimap.copyOf(result);
   }
 
+  private static ImmutableSet<String> toStringSet(ImmutableSet<Class<?>> classes) {
+    ImmutableSet.Builder<String> classStrings = new ImmutableSet.Builder<>();
+    for (Class<?> clazz : classes) {
+      classStrings.add(clazz.getName());
+    }
+    return classStrings.build();
+  }
+
   /**
    * Collects all attribute labels from the specified aspectDefinition.
    */
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 e13f6594e5..5bdcd9c01d 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
@@ -14,11 +14,12 @@
 package com.google.devtools.build.lib.skyframe;
 
 import com.google.common.base.Preconditions;
-import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
+import com.google.common.collect.Multimap;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
+import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
 import com.google.devtools.build.lib.packages.InputFile;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
@@ -36,8 +37,11 @@ import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 import com.google.devtools.build.skyframe.ValueOrException2;
 
+import java.util.Collection;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Set;
 
@@ -113,23 +117,20 @@ abstract class TransitiveBaseTraversalFunction<TProcessedTargets>
     TargetAndErrorIfAny targetAndErrorIfAny = (TargetAndErrorIfAny) loadTargetResults;
     TProcessedTargets processedTargets = processTarget(label, targetAndErrorIfAny);
 
-    // Process deps from attributes and conservative aspects of current target.
+    // Process deps from attributes.
     Iterable<SkyKey> labelDepKeys = getLabelDepKeys(targetAndErrorIfAny.getTarget());
-    Iterable<SkyKey> labelAspectKeys =
-        getConservativeLabelAspectKeys(targetAndErrorIfAny.getTarget());
-    Iterable<SkyKey> depAndAspectKeys = Iterables.concat(labelDepKeys, labelAspectKeys);
 
-    Set<Entry<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>>>
-        depsAndAspectEntries = env.getValuesOrThrow(depAndAspectKeys,
-        NoSuchPackageException.class, NoSuchTargetException.class).entrySet();
-    processDeps(processedTargets, env.getListener(), targetAndErrorIfAny, depsAndAspectEntries);
+    Map<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> depMap =
+        env.getValuesOrThrow(labelDepKeys, NoSuchPackageException.class,
+            NoSuchTargetException.class);
+    processDeps(processedTargets, env.getListener(), targetAndErrorIfAny, depMap.entrySet());
     if (env.valuesMissing()) {
       return null;
     }
 
-
-    // Process deps from strict aspects.
-    labelAspectKeys = getStrictLabelAspectKeys(targetAndErrorIfAny.getTarget(), env);
+    // Process deps from aspects.
+    Iterable<SkyKey> labelAspectKeys =
+        getStrictLabelAspectKeys(targetAndErrorIfAny.getTarget(), depMap, env);
     Set<Entry<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>>>
         labelAspectEntries = env.getValuesOrThrow(labelAspectKeys, NoSuchPackageException.class,
         NoSuchTargetException.class).entrySet();
@@ -151,18 +152,41 @@ abstract class TransitiveBaseTraversalFunction<TProcessedTargets>
    *
    *  <p>This method may return a precise set of aspect keys, but may need to request additional
    *  dependencies from the env to do so.
-   *
-   *  <p>Subclasses should implement only one of #getStrictLabelAspectKeys and
-   *  @getConservativeLabelAspectKeys.
    */
-  protected abstract Iterable<SkyKey> getStrictLabelAspectKeys(Target target, Environment env);
+  private Iterable<SkyKey> getStrictLabelAspectKeys(Target target,
+          Map<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> depMap,
+          Environment env) {
+    List<SkyKey> depKeys = Lists.newArrayList();
+    if (target instanceof Rule) {
+      Map<Label, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> labelDepMap =
+          new HashMap<>(depMap.size());
+      for (Entry<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> entry :
+          depMap.entrySet()) {
+        labelDepMap.put((Label) entry.getKey().argument(), entry.getValue());
+      }
+
+      Multimap<Attribute, Label> transitions =
+              ((Rule) target).getTransitions(Rule.NO_NODEP_ATTRIBUTES);
+      for (Entry<Attribute, Label> entry : transitions.entries()) {
+        ValueOrException2<NoSuchPackageException, NoSuchTargetException> value =
+            labelDepMap.get(entry.getValue());
+        for (Label label :
+                getAspectLabels(target, entry.getKey(), entry.getValue(), value, env)) {
+          depKeys.add(getKey(label));
+        }
+      }
+    }
+    return depKeys;
+  }
 
   /**
-   * Return an Iterable of SkyKeys corresponding to the Aspect-related dependencies of target.
-   *
-   *  <p>This method may return a conservative over-approximation of the exact set.
+   * Get the Aspect-related Label deps for the given edge.
    */
-  protected abstract Iterable<SkyKey> getConservativeLabelAspectKeys(Target target);
+  protected abstract Collection<Label> getAspectLabels(Target fromTarget, Attribute attr,
+          Label toLabel,
+          ValueOrException2<NoSuchPackageException, NoSuchTargetException> toVal,
+          Environment env);
+
 
   private Iterable<SkyKey> getLabelDepKeys(Target target) {
     List<SkyKey> depKeys = Lists.newArrayList();
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 a01bec072c..06e068dd2e 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
@@ -13,9 +13,8 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Lists;
-import com.google.common.collect.Multimap;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment;
@@ -41,7 +40,6 @@ import com.google.devtools.build.skyframe.ValueOrException2;
 
 import java.util.Collection;
 import java.util.LinkedHashSet;
-import java.util.List;
 import java.util.Map.Entry;
 import java.util.Set;
 
@@ -154,43 +152,29 @@ public class TransitiveTargetFunction
     return builder.build(errorLoadingTarget);
   }
 
-  @Override
-  protected Iterable<SkyKey> getStrictLabelAspectKeys(Target target, Environment env) {
-    List<SkyKey> depKeys = Lists.newArrayList();
-    if (target instanceof Rule) {
-      Multimap<Attribute, Label> transitions =
-          ((Rule) target).getTransitions(Rule.NO_NODEP_ATTRIBUTES);
-      for (Entry<Attribute, Label> entry : transitions.entries()) {
-        SkyKey packageKey = PackageValue.key(entry.getValue().getPackageIdentifier());
-        try {
-          PackageValue pkgValue =
-              (PackageValue) env.getValueOrThrow(packageKey, NoSuchPackageException.class);
-          if (pkgValue == null) {
-            continue;
-          }
-          Package pkg = pkgValue.getPackage();
-          if (pkg.containsErrors()) {
-            // Do nothing. This error was handled when we computed the corresponding
-            // TransitiveTargetValue.
-            continue;
-          }
-          Collection<Label> labels = AspectDefinition.visitAspectsIfRequired(target, entry.getKey(),
-              pkgValue.getPackage().getTarget(entry.getValue().getName())).values();
-          for (Label label : labels) {
-            depKeys.add(getKey(label));
-          }
-        } catch (NoSuchThingException e) {
-          // Do nothing. This error was handled when we computed the corresponding
-          // TransitiveTargetValue.
-        }
+  protected Collection<Label> getAspectLabels(Target fromTarget, Attribute attr, Label toLabel,
+      ValueOrException2<NoSuchPackageException, NoSuchTargetException> toVal,
+      Environment env) {
+    SkyKey packageKey = PackageValue.key(toLabel.getPackageIdentifier());
+    try {
+      PackageValue pkgValue =
+          (PackageValue) env.getValueOrThrow(packageKey, NoSuchPackageException.class);
+      if (pkgValue == null) {
+        return ImmutableList.of();
+      }
+      Package pkg = pkgValue.getPackage();
+      if (pkg.containsErrors()) {
+        // Do nothing. This error was handled when we computed the corresponding
+        // TransitiveTargetValue.
+        return ImmutableList.of();
       }
+      Target dependedTarget = pkgValue.getPackage().getTarget(toLabel.getName());
+      return AspectDefinition.visitAspectsIfRequired(fromTarget, attr, dependedTarget).values();
+    } catch (NoSuchThingException e) {
+      // Do nothing. This error was handled when we computed the corresponding
+      // TransitiveTargetValue.
+      return ImmutableList.of();
     }
-    return depKeys;
-  }
-
-  @Override
-  protected Iterable<SkyKey> getConservativeLabelAspectKeys(Target target) {
-    return ImmutableSet.of();
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java
index 955d8ee238..bd28a39b4c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java
@@ -13,25 +13,24 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.LinkedHashMultimap;
-import com.google.common.collect.Multimap;
+import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.AspectDefinition;
-import com.google.devtools.build.lib.packages.AspectFactory;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
-import com.google.devtools.build.lib.packages.Rule;
+import com.google.devtools.build.lib.packages.NoSuchThingException;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.skyframe.TransitiveTraversalFunction.DummyAccumulator;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 import com.google.devtools.build.skyframe.ValueOrException2;
 
+import java.util.Collection;
 import java.util.Map.Entry;
+import java.util.Set;
 
 /**
  * This class is like {@link TransitiveTargetFunction}, but the values it returns do not contain
@@ -77,41 +76,36 @@ public class TransitiveTraversalFunction extends TransitiveBaseTraversalFunction
     }
   }
 
+  protected Collection<Label> getAspectLabels(Target fromTarget, Attribute attr, Label toLabel,
+      ValueOrException2<NoSuchPackageException, NoSuchTargetException> toVal, Environment env) {
+    try {
+      if (toVal == null) {
+        return ImmutableList.of();
+      }
+      TransitiveTraversalValue traversalVal = (TransitiveTraversalValue) toVal.get();
+      if (traversalVal == null || traversalVal.getProviders() == null) {
+        return ImmutableList.of();
+      }
+      // Retrieve the providers of the dep from the TransitiveTraversalValue, so we can avoid
+      // issuing a dep on its defining Package.
+      Set<String> providers = traversalVal.getProviders();
+      return AspectDefinition.visitAspectsIfRequired(fromTarget, attr, providers).values();
+    } catch (NoSuchThingException e) {
+      // Do nothing. This error was handled when we computed the corresponding
+      // TransitiveTargetValue.
+      return ImmutableList.of();
+    }
+  }
+
   @Override
   SkyValue computeSkyValue(TargetAndErrorIfAny targetAndErrorIfAny,
       DummyAccumulator processedTargets) {
     NoSuchTargetException errorLoadingTarget = targetAndErrorIfAny.getErrorLoadingTarget();
     return errorLoadingTarget == null
-        ? TransitiveTraversalValue.SUCCESSFUL_TRANSITIVE_TRAVERSAL_VALUE
+        ? TransitiveTraversalValue.forTarget(targetAndErrorIfAny.getTarget())
         : TransitiveTraversalValue.unsuccessfulTransitiveTraversal(errorLoadingTarget);
   }
 
-  @Override
-  protected Iterable<SkyKey> getStrictLabelAspectKeys(Target target, Environment env) {
-    return ImmutableSet.of();
-  }
-
-  @Override
-  protected Iterable<SkyKey> getConservativeLabelAspectKeys(Target target) {
-    if (!(target instanceof Rule)) {
-      return ImmutableSet.of();
-    }
-    Rule rule = (Rule) target;
-    Multimap<Attribute, Label> attibuteMap = LinkedHashMultimap.create();
-    for (Attribute attribute : rule.getTransitions(Rule.NO_NODEP_ATTRIBUTES).keys()) {
-      for (Class<? extends AspectFactory<?, ?, ?>> aspectFactory : attribute.getAspects()) {
-        AspectDefinition.addAllAttributesOfAspect(rule, attibuteMap,
-            AspectFactory.Util.create(aspectFactory).getDefinition(), Rule.ALL_DEPS);
-      }
-    }
-
-    ImmutableSet.Builder<SkyKey> depKeys = new ImmutableSet.Builder<>();
-    for (Label label : attibuteMap.values()) {
-      depKeys.add(getKey(label));
-    }
-    return depKeys.build();
-  }
-
  /**
    * Because {@link TransitiveTraversalFunction} is invoked only when its side-effects are desired,
    * this value accumulator has nothing to keep track of.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalValue.java
index e01588b7a2..483167811a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalValue.java
@@ -14,14 +14,21 @@
 package com.google.devtools.build.lib.skyframe;
 
 import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
+import com.google.devtools.build.lib.packages.Rule;
+import com.google.devtools.build.lib.packages.Target;
+import com.google.devtools.build.lib.util.StringCanonicalizer;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 
+import java.util.Collection;
 import java.util.Objects;
+import java.util.Set;
 
 import javax.annotation.Nullable;
 
@@ -35,20 +42,53 @@ public class TransitiveTraversalValue implements SkyValue {
 
   @Nullable
   private final NoSuchTargetException errorLoadingTarget;
+  @Nullable
+  private final ImmutableSet<String> providers;
 
-  // Note that this value does not guarantee singleton-like reference equality for successful
-  // {@link TransitiveTraversalValue}s because we use Java deserialization. Java deserialization can
-  // create other instances.
-  public static final TransitiveTraversalValue SUCCESSFUL_TRANSITIVE_TRAVERSAL_VALUE =
-      new TransitiveTraversalValue(null);
+  private TransitiveTraversalValue(@Nullable Iterable<String> providers,
+      @Nullable NoSuchTargetException errorLoadingTarget) {
+    this.errorLoadingTarget = errorLoadingTarget;
+    this.providers = (providers == null) ? null : canonicalSet(providers);
+  }
 
   public static TransitiveTraversalValue unsuccessfulTransitiveTraversal(
       NoSuchTargetException errorLoadingTarget) {
-    return new TransitiveTraversalValue(Preconditions.checkNotNull(errorLoadingTarget));
+    return new TransitiveTraversalValue(null, Preconditions.checkNotNull(errorLoadingTarget));
   }
 
-  private TransitiveTraversalValue(@Nullable NoSuchTargetException errorLoadingTarget) {
-    this.errorLoadingTarget = errorLoadingTarget;
+  public static TransitiveTraversalValue forTarget(Target target) {
+    if (target instanceof Rule) {
+      Rule rule = (Rule) target;
+      return new TransitiveTraversalValue(
+          toStringSet(rule.getRuleClassObject().getAdvertisedProviders()), null);
+    }
+    return new TransitiveTraversalValue(ImmutableList.<String>of(), null);
+  }
+
+  public static TransitiveTraversalValue withProviders(Collection<String> vals) {
+    return new TransitiveTraversalValue(ImmutableSet.copyOf(vals), null);
+  }
+
+  private static ImmutableSet<String> canonicalSet(Iterable<String> strIterable) {
+    ImmutableSet.Builder<String> builder = new ImmutableSet.Builder<>();
+    for (String str : strIterable) {
+      builder.add(StringCanonicalizer.intern(str));
+    }
+    return builder.build();
+  }
+
+  private static ImmutableSet<String> toStringSet(Iterable<Class<?>> providers) {
+    ImmutableSet.Builder<String> pBuilder = new ImmutableSet.Builder<>();
+    if (providers != null) {
+      for (Class<?> clazz : providers) {
+        pBuilder.add(StringCanonicalizer.intern(clazz.getName()));
+      }
+    }
+    return pBuilder.build();
+  }
+
+  public Set<String> getProviders() {
+    return providers;
   }
 
   /** Returns the error, if any, from loading the target. */
@@ -66,12 +106,13 @@ public class TransitiveTraversalValue implements SkyValue {
       return false;
     }
     TransitiveTraversalValue that = (TransitiveTraversalValue) o;
-    return Objects.equals(this.errorLoadingTarget, that.errorLoadingTarget);
+    return Objects.equals(this.errorLoadingTarget, that.errorLoadingTarget)
+        && Objects.equals(this.providers, that.providers);
   }
 
   @Override
   public int hashCode() {
-    return Objects.hashCode(errorLoadingTarget);
+    return 31 * Objects.hashCode(errorLoadingTarget) + Objects.hashCode(providers);
   }
 
   @ThreadSafe