Restrict aspects visible to other aspects according to their advertis…

…ed providers.

--
PiperOrigin-RevId: 147526961
MOS_MIGRATED_REVID=147526961

src/main/java/com/google/devtools/build/lib/analysis/AspectCollection.java

@@ -213,6 +213,10 @@ public ImmutableList<AspectDeps> getDependentAspects() {
     }
   }
 
+  public static AspectCollection createForTests(AspectDescriptor... descriptors) {
+    return createForTests(ImmutableSet.copyOf(descriptors));
+  }
+
   public static AspectCollection createForTests(ImmutableSet<AspectDescriptor> descriptors) {
     ImmutableSet.Builder<AspectDeps> depsBuilder = ImmutableSet.builder();
     for (AspectDescriptor descriptor : descriptors) {

src/main/java/com/google/devtools/build/lib/analysis/BuildView.java

@@ -857,7 +857,7 @@ private List<TargetAndConfiguration> getDynamicConfigurations(
                 targetAndConfig.getLabel(),
                 Attribute.ConfigurationTransition.NONE,
                 // TODO(bazel-team): support top-level aspects
-                ImmutableSet.<AspectDescriptor>of()));
+                AspectCollection.EMPTY));
       }
     }
 
@@ -1003,7 +1003,7 @@ protected List<BuildConfiguration> getConfigurations(
           Iterable<BuildOptions> buildOptions) {
         Preconditions.checkArgument(ct.getConfiguration().fragmentClasses().equals(fragments));
         Dependency asDep = Dependency.withTransitionAndAspects(ct.getLabel(),
-            Attribute.ConfigurationTransition.NONE, ImmutableSet.<AspectDescriptor>of());
+            Attribute.ConfigurationTransition.NONE, AspectCollection.EMPTY);
         ImmutableList.Builder<BuildConfiguration> builder = ImmutableList.builder();
         for (BuildOptions options : buildOptions) {
           builder.add(Iterables.getOnlyElement(

src/main/java/com/google/devtools/build/lib/analysis/Dependency.java

@@ -14,7 +14,6 @@
 package com.google.devtools.build.lib.analysis;
 
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.AspectDescriptor;
@@ -62,7 +61,9 @@ public static Dependency withNullConfiguration(Label label) {
    */
   public static Dependency withConfiguration(Label label, BuildConfiguration configuration) {
     return new StaticConfigurationDependency(
-        label, configuration, ImmutableMap.<AspectDescriptor, BuildConfiguration>of());
+        label, configuration,
+        AspectCollection.EMPTY,
+        ImmutableMap.<AspectDescriptor, BuildConfiguration>of());
   }
 
   /**
@@ -72,13 +73,13 @@ public static Dependency withConfiguration(Label label, BuildConfiguration confi
    * <p>The configuration and aspects must not be {@code null}.
    */
   public static Dependency withConfigurationAndAspects(
-      Label label, BuildConfiguration configuration, Iterable<AspectDescriptor> aspects) {
+      Label label, BuildConfiguration configuration, AspectCollection aspects) {
     ImmutableMap.Builder<AspectDescriptor, BuildConfiguration> aspectBuilder =
         new ImmutableMap.Builder<>();
-    for (AspectDescriptor aspect : aspects) {
+    for (AspectDescriptor aspect : aspects.getAllAspects()) {
       aspectBuilder.put(aspect, configuration);
     }
-    return new StaticConfigurationDependency(label, configuration, aspectBuilder.build());
+    return new StaticConfigurationDependency(label, configuration, aspects, aspectBuilder.build());
   }
 
   /**
@@ -90,18 +91,19 @@ public static Dependency withConfigurationAndAspects(
    */
   public static Dependency withConfiguredAspects(
       Label label, BuildConfiguration configuration,
+      AspectCollection aspects,
       Map<AspectDescriptor, BuildConfiguration> aspectConfigurations) {
     return new StaticConfigurationDependency(
-        label, configuration, ImmutableMap.copyOf(aspectConfigurations));
+        label, configuration, aspects, ImmutableMap.copyOf(aspectConfigurations));
   }
 
   /**
    * Creates a new {@link Dependency} with the given transition and aspects, suitable for dynamic
    * configuration builds.
    */
   public static Dependency withTransitionAndAspects(
-      Label label, Attribute.Transition transition, Iterable<AspectDescriptor> aspects) {
-    return new DynamicConfigurationDependency(label, transition, ImmutableSet.copyOf(aspects));
+      Label label, Attribute.Transition transition, AspectCollection aspects) {
+    return new DynamicConfigurationDependency(label, transition, aspects);
   }
 
   protected final Label label;
@@ -144,18 +146,16 @@ public Label getLabel() {
    * Returns the set of aspects which should be evaluated and combined with the configured target
    * pointed to by this dependency.
    *
-   * @see #getAspectConfigurations()
+   * @see #getAspectConfiguration(AspectDescriptor)
    */
-  public abstract ImmutableSet<AspectDescriptor> getAspects();
+  public abstract AspectCollection getAspects();
 
   /**
-   * Returns the mapping from aspects to the static configurations they should be evaluated with.
-   *
-   * <p>The {@link Map#keySet()} of this map is equal to that returned by {@link #getAspects()}.
-   *
+   * Returns the the static configuration an aspect should be evaluated with
+   **
    * @throws IllegalStateException if {@link #hasStaticConfiguration()} returns false.
    */
-  public abstract ImmutableMap<AspectDescriptor, BuildConfiguration> getAspectConfigurations();
+  public abstract BuildConfiguration getAspectConfiguration(AspectDescriptor aspect);
 
   /**
    * Implementation of a dependency with no configuration, suitable for static configuration
@@ -184,13 +184,13 @@ public Attribute.Transition getTransition() {
     }
 
     @Override
-    public ImmutableSet<AspectDescriptor> getAspects() {
-      return ImmutableSet.of();
+    public AspectCollection getAspects() {
+      return AspectCollection.EMPTY;
     }
 
     @Override
-    public ImmutableMap<AspectDescriptor, BuildConfiguration> getAspectConfigurations() {
-      return ImmutableMap.of();
+    public BuildConfiguration getAspectConfiguration(AspectDescriptor aspect) {
+      return null;
     }
 
     @Override
@@ -219,14 +219,17 @@ public String toString() {
    */
   private static final class StaticConfigurationDependency extends Dependency {
     private final BuildConfiguration configuration;
+    private final AspectCollection aspects;
     private final ImmutableMap<AspectDescriptor, BuildConfiguration> aspectConfigurations;
 
     public StaticConfigurationDependency(
         Label label, BuildConfiguration configuration,
-        ImmutableMap<AspectDescriptor, BuildConfiguration> aspects) {
+        AspectCollection aspects,
+        ImmutableMap<AspectDescriptor, BuildConfiguration> aspectConfigurations) {
       super(label);
       this.configuration = Preconditions.checkNotNull(configuration);
-      this.aspectConfigurations = Preconditions.checkNotNull(aspects);
+      this.aspects = Preconditions.checkNotNull(aspects);
+      this.aspectConfigurations = Preconditions.checkNotNull(aspectConfigurations);
     }
 
     @Override
@@ -246,13 +249,13 @@ public Attribute.Transition getTransition() {
     }
 
     @Override
-    public ImmutableSet<AspectDescriptor> getAspects() {
-      return aspectConfigurations.keySet();
+    public AspectCollection getAspects() {
+      return aspects;
     }
 
     @Override
-    public ImmutableMap<AspectDescriptor, BuildConfiguration> getAspectConfigurations() {
-      return aspectConfigurations;
+    public BuildConfiguration getAspectConfiguration(AspectDescriptor aspect) {
+      return aspectConfigurations.get(aspect);
     }
 
     @Override
@@ -268,6 +271,7 @@ public boolean equals(Object other) {
       StaticConfigurationDependency otherDep = (StaticConfigurationDependency) other;
       return label.equals(otherDep.label)
           && configuration.equals(otherDep.configuration)
+          && aspects.equals(otherDep.aspects)
           && aspectConfigurations.equals(otherDep.aspectConfigurations);
     }
 
@@ -285,10 +289,10 @@ public String toString() {
    */
   private static final class DynamicConfigurationDependency extends Dependency {
     private final Attribute.Transition transition;
-    private final ImmutableSet<AspectDescriptor> aspects;
+    private final AspectCollection aspects;
 
     public DynamicConfigurationDependency(
-        Label label, Attribute.Transition transition, ImmutableSet<AspectDescriptor> aspects) {
+        Label label, Attribute.Transition transition, AspectCollection aspects) {
       super(label);
       this.transition = Preconditions.checkNotNull(transition);
       this.aspects = Preconditions.checkNotNull(aspects);
@@ -311,15 +315,15 @@ public Attribute.Transition getTransition() {
     }
 
     @Override
-    public ImmutableSet<AspectDescriptor> getAspects() {
+    public AspectCollection getAspects() {
       return aspects;
     }
 
     @Override
-    public ImmutableMap<AspectDescriptor, BuildConfiguration> getAspectConfigurations() {
+    public BuildConfiguration getAspectConfiguration(AspectDescriptor aspect) {
       throw new IllegalStateException(
           "A dependency with a dynamic configuration transition does not have aspect "
-          + "configurations.");
+              + "configurations.");
     }
 
     @Override

src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java

@@ -18,6 +18,7 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
+import com.google.devtools.build.lib.analysis.AspectCollection.AspectCycleOnPathException;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
@@ -520,36 +521,86 @@ private void visitPackageGroup(
   }
 
 
-  private static ImmutableSet<AspectDescriptor> requiredAspects(
-      Iterable<Aspect> aspects,
+  private static AspectCollection requiredAspects(
+      Iterable<Aspect> aspectPath,
       AttributeAndOwner attributeAndOwner,
       final Target target,
       Rule originalRule) {
     if (!(target instanceof Rule)) {
-      return ImmutableSet.of();
+      return AspectCollection.EMPTY;
     }
 
     if (attributeAndOwner.ownerAspect != null) {
       // Do not propagate aspects along aspect attributes.
-      return ImmutableSet.of();
+      return AspectCollection.EMPTY;
     }
 
-    Iterable<Aspect> aspectCandidates =
-        extractAspectCandidates(aspects, attributeAndOwner.attribute, originalRule);
-    RuleClass ruleClass = ((Rule) target).getRuleClassObject();
-    ImmutableSet.Builder<AspectDescriptor> result = ImmutableSet.builder();
+    ImmutableList.Builder<Aspect> filteredAspectPath = ImmutableList.builder();
+    ImmutableSet.Builder<AspectDescriptor> visibleAspects = ImmutableSet.builder();
+
+    collectOriginatingAspects(originalRule, attributeAndOwner.attribute, (Rule) target,
+        filteredAspectPath, visibleAspects);
+
+    collectPropagatingAspects(aspectPath,
+        attributeAndOwner.attribute,
+        (Rule) target, filteredAspectPath, visibleAspects);
+    try {
+      return AspectCollection.create(filteredAspectPath.build(), visibleAspects.build());
+    } catch (AspectCycleOnPathException e) {
+      // TODO(dslomov): propagate the error and report to user.
+      throw new AssertionError(e);
+    }
+  }
 
-    for (Aspect aspectCandidate : aspectCandidates) {
-      if (aspectCandidate.getDefinition()
-          .getRequiredProviders()
+  /**
+   * Collects into {@code filteredAspectPath}
+   * aspects from {@code aspectPath} that propagate along {@code attribute}
+   * and apply to a given {@code target}.
+   *
+   * The last aspect in {@code aspectPath} is (potentially) visible and recorded
+   * in {@code visibleAspects}.
+   */
+  private static void collectPropagatingAspects(Iterable<Aspect> aspectPath,
+      Attribute attribute, Rule target,
+      ImmutableList.Builder<Aspect> filteredAspectPath,
+      ImmutableSet.Builder<AspectDescriptor> visibleAspects) {
+
+    Aspect lastAspect = null;
+    for (Aspect aspect : aspectPath) {
+      lastAspect = aspect;
+      if (aspect.getDefinition().propagateAlong(attribute)
+          && aspect.getDefinition().getRequiredProviders()
+                  .isSatisfiedBy(target.getRuleClassObject().getAdvertisedProviders())) {
+        filteredAspectPath.add(aspect);
+      } else {
+        lastAspect = null;
+      }
+    }
+
+    if (lastAspect != null) {
+      visibleAspects.add(lastAspect.getDescriptor());
+    }
+  }
+
+  /**
+   * Collect all aspects that originate on {@code attribute} of {@code originalRule}
+   * and are applicable to a {@code target}
+   *
+   * They are appended to {@code filteredAspectPath} and registered in {@code visibleAspects} set.
+   */
+  private static void collectOriginatingAspects(
+      Rule originalRule, Attribute attribute, Rule target,
+      ImmutableList.Builder<Aspect> filteredAspectPath,
+      ImmutableSet.Builder<AspectDescriptor> visibleAspects) {
+    ImmutableList<Aspect> baseAspects = attribute.getAspects(originalRule);
+    RuleClass ruleClass = target.getRuleClassObject();
+    for (Aspect baseAspect : baseAspects) {
+      if (baseAspect.getDefinition().getRequiredProviders()
           .isSatisfiedBy(ruleClass.getAdvertisedProviders())) {
-        result.add(
-            new AspectDescriptor(
-                aspectCandidate.getAspectClass(),
-                aspectCandidate.getParameters()));
+        filteredAspectPath.add(baseAspect);
+        visibleAspects.add(baseAspect.getDescriptor());
       }
     }
-    return result.build();
   }
 
   private static Iterable<Aspect> extractAspectCandidates(
@@ -686,7 +737,7 @@ void resolveDep(AttributeAndOwner attributeAndOwner, Label depLabel, BuildConfig
         applyNullTransition = true;
       }
 
-      ImmutableSet<AspectDescriptor> aspects =
+      AspectCollection aspects =
           requiredAspects(this.aspects, attributeAndOwner, toTarget, rule);
       Dependency dep;
       if (config.useDynamicConfigurations() && !applyNullTransition) {

src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java

@@ -34,6 +34,7 @@
 import com.google.common.collect.Multimap;
 import com.google.common.collect.MutableClassToInstanceMap;
 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.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.Dependency;
@@ -45,7 +46,6 @@
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
-import com.google.devtools.build.lib.packages.AspectDescriptor;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition;
 import com.google.devtools.build.lib.packages.Attribute.Configurator;
@@ -1612,7 +1612,7 @@ public interface TransitionApplier {
      * for each configuration represented by this instance.
      * TODO(bazel-team): this is a really ugly reverse dependency: factor this away.
      */
-    Iterable<Dependency> getDependencies(Label label, ImmutableSet<AspectDescriptor> aspects);
+    Iterable<Dependency> getDependencies(Label label, AspectCollection aspects);
   }
 
   /**
@@ -1706,7 +1706,7 @@ public void applyConfigurationHook(Rule fromRule, Attribute attribute, Target to
 
     @Override
     public Iterable<Dependency> getDependencies(
-        Label label, ImmutableSet<AspectDescriptor> aspects) {
+        Label label, AspectCollection aspects) {
       ImmutableList.Builder<Dependency> deps = ImmutableList.builder();
       for (BuildConfiguration config : toConfigurations) {
         deps.add(config != null
@@ -1829,7 +1829,7 @@ private Transitions getCurrentTransitions() {
 
     @Override
     public Iterable<Dependency> getDependencies(
-        Label label, ImmutableSet<AspectDescriptor> aspects) {
+        Label label, AspectCollection aspects) {
       return ImmutableList.of(
           isNull()
               // We can trivially set the final value for null-configured targets now. This saves

src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java

@@ -280,10 +280,8 @@ public Builder requireAspectsWithProviders(
     }
 
     public Builder requireAspectsWithNativeProviders(
-        Iterable<ImmutableSet<SkylarkProviderIdentifier>> providerSets) {
-      for (ImmutableSet<SkylarkProviderIdentifier> providerSet : providerSets) {
-        requiredAspectProviders.addSkylarkSet(providerSet);
-      }
+        Class<?>... providers) {
+      requiredAspectProviders.addNativeSet(ImmutableSet.copyOf(providers));
       return this;
     }
 

src/main/java/com/google/devtools/build/lib/rules/android/DexArchiveAspect.java

@@ -136,7 +136,8 @@ public AspectDefinition getDefinition(AspectParameters params) {
             .allowedRuleClasses("android_sdk", "filegroup")
             .value(new AndroidRuleClasses.AndroidSdkLabel(
                 Label.parseAbsoluteUnchecked(toolsRepository + AndroidRuleClasses.DEFAULT_SDK))))
-        .requiresConfigurationFragments(AndroidConfiguration.class);
+        .requiresConfigurationFragments(AndroidConfiguration.class)
+        .requireAspectsWithNativeProviders(JavaCompilationArgsAspectProvider.class);
     if (TriState.valueOf(params.getOnlyValueOfAttribute("incremental_dexing")) != TriState.NO) {
       // Marginally improves "query2" precision for targets that disable incremental dexing
       result.add(attr(ASPECT_DEXBUILDER_PREREQ, LABEL).cfg(HOST).exec()