Restrict aspects visible to other aspects according to their advertised providers.

--
PiperOrigin-RevId: 147526961
MOS_MIGRATED_REVID=147526961

4 files changed, 126 insertions, 92 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
index 4f8835befb..3d5aa88e92 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
@@ -34,6 +34,7 @@ import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.Aspect;
+import com.google.devtools.build.lib.packages.AspectDescriptor;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
 import com.google.devtools.build.lib.packages.NativeAspectClass;
@@ -60,6 +61,7 @@ import com.google.devtools.build.skyframe.SkyFunctionException;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.Map;
 import javax.annotation.Nullable;
 
@@ -218,16 +220,18 @@ public final class AspectFunction implements SkyFunction {
 
     ImmutableList.Builder<Aspect> aspectPathBuilder = ImmutableList.builder();
 
-    if (key.getBaseKey() != null) {
-      ImmutableList<SkyKey> aspectKeys = getSkyKeysForAspects(key.getBaseKey());
+    if (!key.getBaseKeys().isEmpty()) {
+      // We transitively collect all required aspects to reduce the number of restarts.
+      // Semantically it is enough to just request key.getBaseKeys().
+      ImmutableMap<AspectDescriptor, SkyKey> aspectKeys = getSkyKeysForAspects(key.getBaseKeys());
 
-      Map<SkyKey, SkyValue> values = env.getValues(aspectKeys);
+      Map<SkyKey, SkyValue> values = env.getValues(aspectKeys.values());
       if (env.valuesMissing()) {
         return null;
       }
       try {
         associatedTarget = getBaseTargetAndCollectPath(
-            associatedTarget, aspectKeys, values, aspectPathBuilder);
+            associatedTarget, key.getBaseKeys(), values, aspectPathBuilder);
       } catch (DuplicateException e) {
         env.getListener().handle(
             Event.error(associatedTarget.getTarget().getLocation(), e.getMessage()));
@@ -321,11 +325,13 @@ public final class AspectFunction implements SkyFunction {
    * @throws DuplicateException if there is a duplicate provider provided by aspects.
    */
   private ConfiguredTarget getBaseTargetAndCollectPath(ConfiguredTarget target,
-      ImmutableList<SkyKey> aspectKeys, Map<SkyKey, SkyValue> values,
+      ImmutableList<AspectKey> aspectKeys,
+      Map<SkyKey, SkyValue> values,
       ImmutableList.Builder<Aspect> aspectPath)
       throws DuplicateException {
     ArrayList<ConfiguredAspect> aspectValues = new ArrayList<>();
-    for (SkyKey skyAspectKey : aspectKeys) {
+    for (AspectKey aspectKey : aspectKeys) {
+      SkyKey skyAspectKey = aspectKey.getSkyKey();
       AspectValue aspectValue = (AspectValue) values.get(skyAspectKey);
       ConfiguredAspect configuredAspect = aspectValue.getConfiguredAspect();
       aspectValues.add(configuredAspect);
@@ -335,19 +341,28 @@ public final class AspectFunction implements SkyFunction {
   }
 
   /**
-   *  Returns a list of SkyKeys for all aspects the given aspect key depends on.
-   *  The order corresponds to the order the aspects should be merged into a configured target.
+   *  Collect all SkyKeys that are needed for a given list of AspectKeys,
+   *  including transitive dependencies.
    */
-  private ImmutableList<SkyKey> getSkyKeysForAspects(AspectKey key) {
-    ImmutableList.Builder<SkyKey> aspectKeysBuilder = ImmutableList.builder();
-    AspectKey baseKey = key;
-    while (baseKey != null) {
-      aspectKeysBuilder.add(baseKey.getSkyKey());
-      baseKey = baseKey.getBaseKey();
+  private ImmutableMap<AspectDescriptor, SkyKey> getSkyKeysForAspects(
+      ImmutableList<AspectKey> keys) {
+    HashMap<AspectDescriptor, SkyKey> result = new HashMap<>();
+    for (AspectKey key : keys) {
+      buildSkyKeys(key, result);
     }
-    return aspectKeysBuilder.build().reverse();
+    return ImmutableMap.copyOf(result);
   }
 
+  private void buildSkyKeys(AspectKey key, HashMap<AspectDescriptor, SkyKey> result) {
+    if (result.containsKey(key.getAspectDescriptor())) {
+      return;
+    }
+    ImmutableList<AspectKey> baseKeys = key.getBaseKeys();
+    result.put(key.getAspectDescriptor(), key.getSkyKey());
+    for (AspectKey baseKey : baseKeys) {
+      buildSkyKeys(baseKey, result);
+    }
+  }
   private static SkyValue createAliasAspect(
       Environment env,
       Target originalTarget,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
index 7accbc1cb0..df6048df17 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.skyframe;
 
 import com.google.common.base.Objects;
+import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
 import com.google.devtools.build.lib.analysis.ConfiguredAspect;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
@@ -47,36 +48,22 @@ public final class AspectValue extends ActionLookupValue {
    */
   public static final class AspectKey extends AspectValueKey {
     private final Label label;
-    private final AspectKey baseKey;
+    private final ImmutableList<AspectKey> baseKeys;
     private final BuildConfiguration aspectConfiguration;
     private final BuildConfiguration baseConfiguration;
-    private final AspectClass aspectClass;
-    private final AspectParameters parameters;
+    private final AspectDescriptor aspectDescriptor;
 
     private AspectKey(
         Label label,
-        BuildConfiguration aspectConfiguration,
         BuildConfiguration baseConfiguration,
-        AspectClass aspectClass,
-        AspectParameters parameters) {
-      this.baseKey = null;
+        ImmutableList<AspectKey> baseKeys,
+        AspectDescriptor aspectDescriptor,
+        BuildConfiguration aspectConfiguration) {
+      this.baseKeys = baseKeys;
       this.label = label;
-      this.aspectConfiguration = aspectConfiguration;
       this.baseConfiguration = baseConfiguration;
-      this.aspectClass = aspectClass;
-      this.parameters = parameters;
-    }
-
-    private AspectKey(
-        AspectKey baseKey,
-        AspectClass aspectClass, AspectParameters aspectParameters,
-        BuildConfiguration aspectConfiguration) {
-      this.baseKey = baseKey;
-      this.label = baseKey.label;
-      this.baseConfiguration = baseKey.getBaseConfiguration();
       this.aspectConfiguration = aspectConfiguration;
-      this.aspectClass = aspectClass;
-      this.parameters = aspectParameters;
+      this.aspectDescriptor = aspectDescriptor;
     }
 
     @Override
@@ -91,25 +78,31 @@ public final class AspectValue extends ActionLookupValue {
     }
 
     public AspectClass getAspectClass() {
-      return aspectClass;
+      return aspectDescriptor.getAspectClass();
     }
 
     @Nullable
     public AspectParameters getParameters() {
-      return parameters;
+      return aspectDescriptor.getParameters();
+    }
+
+    public AspectDescriptor getAspectDescriptor() {
+      return aspectDescriptor;
     }
 
     @Nullable
-    public AspectKey getBaseKey() {
-      return baseKey;
+    public ImmutableList<AspectKey> getBaseKeys() {
+      return baseKeys;
     }
 
     @Override
     public String getDescription() {
-      if (baseKey == null) {
-        return String.format("%s of %s", aspectClass.getName(), getLabel());
+      if (baseKeys.isEmpty()) {
+        return String.format("%s of %s",
+            aspectDescriptor.getAspectClass().getName(), getLabel());
       } else {
-        return String.format("%s on top of %s", aspectClass.getName(), baseKey.toString());
+        return String.format("%s on top of %s",
+            aspectDescriptor.getAspectClass().getName(), baseKeys.toString());
       }
     }
 
@@ -153,11 +146,10 @@ public final class AspectValue extends ActionLookupValue {
     public int hashCode() {
       return Objects.hashCode(
           label,
-          baseKey,
+          baseKeys,
           aspectConfiguration,
           baseConfiguration,
-          aspectClass,
-          parameters);
+          aspectDescriptor);
     }
 
     @Override
@@ -172,45 +164,52 @@ public final class AspectValue extends ActionLookupValue {
 
       AspectKey that = (AspectKey) other;
       return Objects.equal(label, that.label)
-          && Objects.equal(baseKey, that.baseKey)
+          && Objects.equal(baseKeys, that.baseKeys)
           && Objects.equal(aspectConfiguration, that.aspectConfiguration)
           && Objects.equal(baseConfiguration, that.baseConfiguration)
-          && Objects.equal(aspectClass, that.aspectClass)
-          && Objects.equal(parameters, that.parameters);
+          && Objects.equal(aspectDescriptor, that.aspectDescriptor);
     }
 
     public String prettyPrint() {
       if (label == null) {
         return "null";
       }
-      return String.format("%s with aspect %s%s",
-          baseKey == null ? label.toString() : baseKey.prettyPrint(),
-          aspectClass.getName(),
+
+      String baseKeysString =
+          baseKeys.isEmpty()
+          ? ""
+          : String.format(" (over %s)", baseKeys.toString());
+      return String.format("%s with aspect %s%s%s",
+          label.toString(),
+          aspectDescriptor.getAspectClass().getName(),
           (aspectConfiguration != null && aspectConfiguration.isHostConfiguration())
-              ? "(host) " : "");
+              ? "(host) " : "",
+          baseKeysString
+      );
     }
 
     @Override
     public String toString() {
-      return (baseKey == null ? label : baseKey.toString())
+      return (baseKeys == null ? label : baseKeys.toString())
           + "#"
-          + aspectClass.getName()
+          + aspectDescriptor.getAspectClass().getName()
           + " "
           + (aspectConfiguration == null ? "null" : aspectConfiguration.checksum())
           + " "
           + (baseConfiguration == null ? "null" : baseConfiguration.checksum())
           + " "
-          + parameters;
+          + aspectDescriptor.getParameters();
     }
 
     public AspectKey withLabel(Label label) {
-      if (baseKey == null) {
-        return new AspectKey(
-            label, aspectConfiguration, baseConfiguration, aspectClass, parameters);
-      } else {
-        return new AspectKey(
-            baseKey.withLabel(label), aspectClass, parameters, aspectConfiguration);
+      ImmutableList.Builder<AspectKey> newBaseKeys = ImmutableList.builder();
+      for (AspectKey baseKey : baseKeys) {
+        newBaseKeys.add(baseKey.withLabel(label));
       }
+
+      return new AspectKey(
+          label, baseConfiguration,
+          newBaseKeys.build(), aspectDescriptor, aspectConfiguration);
     }
   }
 
@@ -354,10 +353,14 @@ public final class AspectValue extends ActionLookupValue {
     return transitivePackages;
   }
 
-  public static AspectKey createAspectKey(AspectKey baseKey, AspectDescriptor aspectDescriptor,
+  public static AspectKey createAspectKey(
+      Label label, BuildConfiguration baseConfiguration,
+      ImmutableList<AspectKey> baseKeys, AspectDescriptor aspectDescriptor,
       BuildConfiguration aspectConfiguration) {
     return new AspectKey(
-        baseKey, aspectDescriptor.getAspectClass(), aspectDescriptor.getParameters(),
+        label, baseConfiguration,
+        baseKeys,
+        aspectDescriptor,
         aspectConfiguration
     );
   }
@@ -368,8 +371,8 @@ public final class AspectValue extends ActionLookupValue {
       BuildConfiguration baseConfiguration, AspectDescriptor aspectDescriptor,
       BuildConfiguration aspectConfiguration) {
     return new AspectKey(
-        label, aspectConfiguration, baseConfiguration,
-        aspectDescriptor.getAspectClass(), aspectDescriptor.getParameters());
+        label, baseConfiguration, ImmutableList.<AspectKey>of(),
+        aspectDescriptor, aspectConfiguration);
   }
 
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
index e481dbd1d9..673a18fc40 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
@@ -31,6 +31,8 @@ import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
 import com.google.devtools.build.lib.actions.Actions;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
+import com.google.devtools.build.lib.analysis.AspectCollection;
+import com.google.devtools.build.lib.analysis.AspectCollection.AspectDeps;
 import com.google.devtools.build.lib.analysis.CachingAnalysisEnvironment;
 import com.google.devtools.build.lib.analysis.ConfiguredAspect;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
@@ -82,6 +84,7 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
@@ -849,18 +852,14 @@ final class ConfiguredTargetFunction implements SkyFunction {
       NestedSetBuilder<Package> transitivePackages)
       throws AspectCreationException, InterruptedException {
     OrderedSetMultimap<SkyKey, ConfiguredAspect> result = OrderedSetMultimap.create();
-    Set<SkyKey> aspectKeys = new HashSet<>();
+    Set<SkyKey> allAspectKeys = new HashSet<>();
     for (Dependency dep : deps) {
-      AspectKey key = null;
-      for (Entry<AspectDescriptor, BuildConfiguration> depAspect
-          : dep.getAspectConfigurations().entrySet()) {
-        key = getNextAspectKey(key, dep, depAspect);
-        aspectKeys.add(key.getSkyKey());
-      }
+      allAspectKeys.addAll(getAspectKeys(dep).values());
     }
 
     Map<SkyKey, ValueOrException2<AspectCreationException, NoSuchThingException>> depAspects =
-        env.getValuesOrThrow(aspectKeys, AspectCreationException.class, NoSuchThingException.class);
+        env.getValuesOrThrow(allAspectKeys,
+            AspectCreationException.class, NoSuchThingException.class);
 
     for (Dependency dep : deps) {
       SkyKey depKey = TO_KEYS.apply(dep);
@@ -869,12 +868,12 @@ final class ConfiguredTargetFunction implements SkyFunction {
       if (result.containsKey(depKey)) {
         continue;
       }
-      AspectKey key = null;
+      Map<AspectDescriptor, SkyKey> aspectToKeys = getAspectKeys(dep);
+
       ConfiguredTarget depConfiguredTarget = configuredTargetMap.get(depKey);
-      for (Entry<AspectDescriptor, BuildConfiguration> depAspect
-          : dep.getAspectConfigurations().entrySet()) {
-        key = getNextAspectKey(key, dep, depAspect);
-        SkyKey aspectKey = key.getSkyKey();
+      for (AspectDeps depAspect : dep.getAspects().getVisibleAspects()) {
+        SkyKey aspectKey = aspectToKeys.get(depAspect.getAspect());
+
         AspectValue aspectValue;
         try {
           // TODO(ulfjack): Catch all thrown AspectCreationException and NoSuchThingException
@@ -884,7 +883,7 @@ final class ConfiguredTargetFunction implements SkyFunction {
           throw new AspectCreationException(
               String.format(
                   "Evaluation of aspect %s on %s failed: %s",
-                  depAspect.getKey().getAspectClass().getName(),
+                  depAspect.getAspect().getAspectClass().getName(),
                   dep.getLabel(),
                   e.toString()));
         }
@@ -904,16 +903,32 @@ final class ConfiguredTargetFunction implements SkyFunction {
     return result;
   }
 
-  private static AspectKey getNextAspectKey(AspectKey key, Dependency dep,
-      Entry<AspectDescriptor, BuildConfiguration> depAspect) {
-    if (key == null) {
-      key = AspectValue.createAspectKey(dep.getLabel(),
-          dep.getConfiguration(), depAspect.getKey(), depAspect.getValue()
-      );
-    } else {
-      key = AspectValue.createAspectKey(key, depAspect.getKey(), depAspect.getValue());
+  private static Map<AspectDescriptor, SkyKey> getAspectKeys(Dependency dep) {
+    HashMap<AspectDescriptor, SkyKey> result = new HashMap<>();
+    AspectCollection aspects = dep.getAspects();
+    for (AspectDeps aspectDeps : aspects.getVisibleAspects()) {
+      buildAspectKey(aspectDeps, result, dep);
+    }
+    return result;
+  }
+
+  private static AspectKey buildAspectKey(AspectDeps aspectDeps,
+      HashMap<AspectDescriptor, SkyKey> result, Dependency dep) {
+    if (result.containsKey(aspectDeps.getAspect())) {
+      return (AspectKey) result.get(aspectDeps.getAspect()).argument();
+    }
+
+    ImmutableList.Builder<AspectKey> dependentAspects = ImmutableList.builder();
+    for (AspectDeps path : aspectDeps.getDependentAspects()) {
+      dependentAspects.add(buildAspectKey(path, result, dep));
     }
-    return key;
+    AspectKey aspectKey = AspectValue.createAspectKey(
+        dep.getLabel(), dep.getConfiguration(),
+        dependentAspects.build(),
+        aspectDeps.getAspect(),
+        dep.getAspectConfiguration(aspectDeps.getAspect()));
+    result.put(aspectKey.getAspectDescriptor(), aspectKey.getSkyKey());
+    return aspectKey;
   }
 
   private static boolean aspectMatchesConfiguredTarget(final ConfiguredTarget dep, Aspect aspect) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 10e565714b..46b2b9df47 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -50,6 +50,7 @@ import com.google.devtools.build.lib.actions.Executor;
 import com.google.devtools.build.lib.actions.PackageRootResolutionException;
 import com.google.devtools.build.lib.actions.ResourceManager;
 import com.google.devtools.build.lib.actions.Root;
+import com.google.devtools.build.lib.analysis.AspectCollection;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.BuildView.Options;
 import com.google.devtools.build.lib.analysis.ConfiguredAspect;
@@ -1227,7 +1228,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       }
       for (BuildConfiguration depConfig : configs.get(key)) {
         skyKeys.add(ConfiguredTargetValue.key(key.getLabel(), depConfig));
-        for (AspectDescriptor aspectDescriptor : key.getAspects()) {
+        for (AspectDescriptor aspectDescriptor : key.getAspects().getAllAspects()) {
           skyKeys.add(ActionLookupValue.key(AspectValue.createAspectKey(key.getLabel(), depConfig,
               aspectDescriptor, depConfig)));
         }
@@ -1260,7 +1261,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
             ((ConfiguredTargetValue) result.get(configuredTargetKey)).getConfiguredTarget();
         List<ConfiguredAspect> configuredAspects = new ArrayList<>();
 
-        for (AspectDescriptor aspectDescriptor : key.getAspects()) {
+        for (AspectDescriptor aspectDescriptor : key.getAspects().getAllAspects()) {
           SkyKey aspectKey = ActionLookupValue.key(AspectValue.createAspectKey(key.getLabel(),
               depConfig, aspectDescriptor, depConfig));
           if (result.get(aspectKey) == null) {
@@ -1450,7 +1451,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       dep = Dependency.withNullConfiguration(label);
     } else if (configuration.useDynamicConfigurations()) {
       dep = Dependency.withTransitionAndAspects(label, Attribute.ConfigurationTransition.NONE,
-          ImmutableSet.<AspectDescriptor>of());
+          AspectCollection.EMPTY);
     } else {
       dep = Dependency.withConfiguration(label, configuration);
     }