Aspects-on-aspect see and propagate over aspect attributes.

If an aspect is applied to a rule+aspect node, all attributes are merged
into ctx.rule.attr collection, and the first one with the same name wins
(in particular, rule attribute will always win over aspect attribute).
This is backwards-compatible, and unlikely to cause problems in
practice.

RELNOTES: Aspects-on-aspect see and propagate over aspect attributes.
PiperOrigin-RevId: 179675660

7 files changed, 602 insertions, 285 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index b84126d518..8261c7c7ac 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -1121,7 +1121,7 @@ public class BuildView {
     return new RuleContext.Builder(
             env,
             (Rule) target.getTarget(),
-            ImmutableList.<AspectDescriptor>of(),
+            ImmutableList.of(),
             targetConfig,
             configurations.getHostConfiguration(),
             ruleClassProvider.getPrerequisiteValidator(),
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
index 3bb309550c..981ade1c92 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
@@ -14,8 +14,6 @@
 
 package com.google.devtools.build.lib.analysis;
 
-import static com.google.common.collect.Iterables.transform;
-
 import com.google.common.base.Function;
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
@@ -72,8 +70,11 @@ import com.google.devtools.build.lib.util.OrderedSetMultimap;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.skyframe.SkyFunction;
 import java.util.ArrayList;
+import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
 import java.util.Set;
 import javax.annotation.Nullable;
 
@@ -312,7 +313,7 @@ public final class ConfiguredTargetFactory {
         new RuleContext.Builder(
                 env,
                 rule,
-                ImmutableList.<AspectDescriptor>of(),
+                ImmutableList.of(),
                 configuration,
                 hostConfiguration,
                 ruleClassProvider.getPrerequisiteValidator(),
@@ -418,21 +419,25 @@ public final class ConfiguredTargetFactory {
       toolchainContext.resolveToolchains(prerequisiteMap);
     }
 
-    RuleContext.Builder builder = new RuleContext.Builder(
-        env,
-        associatedTarget.getTarget().getAssociatedRule(),
-        ImmutableList.copyOf(transform(aspectPath, ASPECT_TO_DESCRIPTOR)),
-        aspectConfiguration,
-        hostConfiguration,
-        ruleClassProvider.getPrerequisiteValidator(),
-        aspect.getDefinition().getConfigurationFragmentPolicy());
+    RuleContext.Builder builder =
+        new RuleContext.Builder(
+            env,
+            associatedTarget.getTarget().getAssociatedRule(),
+            aspectPath,
+            aspectConfiguration,
+            hostConfiguration,
+            ruleClassProvider.getPrerequisiteValidator(),
+            aspect.getDefinition().getConfigurationFragmentPolicy());
+
+    Map<String, Attribute> aspectAttributes = mergeAspectAttributes(aspectPath);
+
     RuleContext ruleContext =
         builder
             .setVisibility(
                 convertVisibility(
                     prerequisiteMap, env.getEventHandler(), associatedTarget.getTarget(), null))
             .setPrerequisites(prerequisiteMap)
-            .setAspectAttributes(aspect.getDefinition().getAttributes())
+            .setAspectAttributes(aspectAttributes)
             .setConfigConditions(configConditions)
             .setUniversalFragment(ruleClassProvider.getUniversalFragment())
             .setToolchainContext(toolchainContext)
@@ -453,6 +458,27 @@ public final class ConfiguredTargetFactory {
     return configuredAspect;
   }
 
+  private Map<String, Attribute> mergeAspectAttributes(ImmutableList<Aspect> aspectPath) {
+    if (aspectPath.isEmpty()) {
+      return ImmutableMap.of();
+    } else if (aspectPath.size() == 1) {
+      return aspectPath.get(0).getDefinition().getAttributes();
+    } else {
+
+      LinkedHashMap<String, Attribute> aspectAttributes = new LinkedHashMap<>();
+      for (Aspect underlyingAspect : aspectPath) {
+        ImmutableMap<String, Attribute> currentAttributes = underlyingAspect.getDefinition()
+            .getAttributes();
+        for (Entry<String, Attribute> kv : currentAttributes.entrySet()) {
+          if (!aspectAttributes.containsKey(kv.getKey())) {
+            aspectAttributes.put(kv.getKey(), kv.getValue());
+          }
+        }
+      }
+      return aspectAttributes;
+    }
+  }
+
   private void validateAdvertisedProviders(
       ConfiguredAspect configuredAspect,
       AdvertisedProviderSet advertisedProviders, Target target,
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
index 6a027834fb..8fbdf4bf32 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
@@ -18,7 +18,6 @@ import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 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;
@@ -576,24 +575,31 @@ public abstract class DependencyResolver {
   }
 
   /**
-   * Collects into {@code filteredAspectPath}
-   * aspects from {@code aspectPath} that propagate along {@code attribute}
-   * and apply to a given {@code target}.
+   * Collects into {@code filteredAspectPath} aspects from {@code aspectPath} that propagate along
+   * {@code attributeAndOwner} and apply to a given {@code target}.
    *
-   * The last aspect in {@code aspectPath} is (potentially) visible and recorded
-   * in {@code visibleAspects}.
+   * <p>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,
+  private static void collectPropagatingAspects(
+      Iterable<Aspect> aspectPath,
+      AttributeAndOwner attributeAndOwner,
+      Rule target,
       ImmutableList.Builder<Aspect> filteredAspectPath,
       ImmutableSet.Builder<AspectDescriptor> visibleAspects) {
 
     Aspect lastAspect = null;
     for (Aspect aspect : aspectPath) {
+      if (aspect.getAspectClass().equals(attributeAndOwner.ownerAspect)) {
+        // Do not propagate over the aspect's own attributes.
+        continue;
+      }
       lastAspect = aspect;
-      if (aspect.getDefinition().propagateAlong(attribute)
-          && aspect.getDefinition().getRequiredProviders()
-                  .isSatisfiedBy(target.getRuleClassObject().getAdvertisedProviders())) {
+      if (aspect.getDefinition().propagateAlong(attributeAndOwner.attribute)
+          && aspect
+              .getDefinition()
+              .getRequiredProviders()
+              .isSatisfiedBy(target.getRuleClassObject().getAdvertisedProviders())) {
         filteredAspectPath.add(aspect);
       } else {
         lastAspect = null;
@@ -682,22 +688,24 @@ public abstract class DependencyResolver {
       this.rootCauses = rootCauses;
       this.outgoingEdges = outgoingEdges;
 
-      this.attributes = getAttributes(rule,
-          // These are attributes that the application of `aspects` "path"
-          // to the rule will see. Application of path is really the
-          // application of the last aspect in the path, so we only let it see
-          // it's own attributes.
-          Iterables.getLast(aspects, null));
+      this.attributes =
+          getAttributes(
+              rule,
+              // These are attributes that the application of `aspects` "path"
+              // to the rule will see. Application of path is really the
+              // application of the last aspect in the path, so we only let it see
+              // it's own attributes.
+              aspects);
     }
 
     /** Returns the attributes that should be visited for this rule/aspect combination. */
-    private List<AttributeAndOwner> getAttributes(Rule rule, @Nullable Aspect aspect) {
+    private List<AttributeAndOwner> getAttributes(Rule rule, Iterable<Aspect> aspects) {
       ImmutableList.Builder<AttributeAndOwner> result = ImmutableList.builder();
       List<Attribute> ruleDefs = rule.getRuleClassObject().getAttributes();
       for (Attribute attribute : ruleDefs) {
         result.add(new AttributeAndOwner(attribute));
       }
-      if (aspect != null) {
+      for (Aspect aspect : aspects) {
         for (Attribute attribute : aspect.getDefinition().getAttributes().values()) {
           result.add(new AttributeAndOwner(attribute, aspect.getAspectClass()));
         }
@@ -754,25 +762,21 @@ public abstract class DependencyResolver {
         return AspectCollection.EMPTY;
       }
 
-      if (attributeAndOwner.ownerAspect != null) {
-        // Do not propagate aspects along aspect attributes.
-        return AspectCollection.EMPTY;
-      }
 
       ImmutableList.Builder<Aspect> filteredAspectPath = ImmutableList.builder();
       ImmutableSet.Builder<AspectDescriptor> visibleAspects = ImmutableSet.builder();
 
-      Attribute attribute = attributeAndOwner.attribute;
-      collectOriginatingAspects(rule, attribute, (Rule) target,
-          filteredAspectPath, visibleAspects);
+      if (attributeAndOwner.ownerAspect == null) {
+        collectOriginatingAspects(
+            rule, attributeAndOwner.attribute, (Rule) target, filteredAspectPath, visibleAspects);
+      }
 
-      collectPropagatingAspects(aspects,
-          attribute,
-          (Rule) target, filteredAspectPath, visibleAspects);
+      collectPropagatingAspects(
+          aspects, attributeAndOwner, (Rule) target, filteredAspectPath, visibleAspects);
       try {
         return AspectCollection.create(filteredAspectPath.build(), visibleAspects.build());
       } catch (AspectCycleOnPathException e) {
-        throw new InconsistentAspectOrderException(rule, attribute, target, e);
+        throw new InconsistentAspectOrderException(rule, attributeAndOwner.attribute, target, e);
       }
     }
   }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
index 755ccd08d9..86339dab1b 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -60,6 +60,7 @@ import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.packages.AbstractRuleErrorConsumer;
+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.Attribute.ConfigurationTransition;
@@ -101,6 +102,7 @@ import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 
 /**
@@ -157,6 +159,14 @@ public final class RuleContext extends TargetContext
   private static final String HOST_CONFIGURATION_PROGRESS_TAG = "for host";
 
   private final Rule rule;
+  /**
+   * A list of all aspects applied to the target. If this <code>RuleContext</code>
+   * is for a rule implementation, <code>aspects</code> is an empty list.
+   *
+   * Otherwise, the last aspect in <code>aspects</code> list is the aspect which
+   * this <code>RuleCointext</code> is for.
+   */
+  private final ImmutableList<Aspect> aspects;
   private final ImmutableList<AspectDescriptor> aspectDescriptors;
   private final ListMultimap<String, ConfiguredTarget> targetMap;
   private final ListMultimap<String, ConfiguredFilesetEntry> filesetEntryMap;
@@ -188,7 +198,13 @@ public final class RuleContext extends TargetContext
     super(builder.env, builder.rule, builder.configuration, builder.prerequisiteMap.get(null),
         builder.visibility);
     this.rule = builder.rule;
-    this.aspectDescriptors = builder.aspectDescriptors;
+    this.aspects = builder.aspects;
+    this.aspectDescriptors =
+        builder
+            .aspects
+            .stream()
+            .map(a -> a.getDescriptor())
+            .collect(ImmutableList.toImmutableList());
     this.configurationFragmentPolicy = builder.configurationFragmentPolicy;
     this.universalFragment = universalFragment;
     this.targetMap = targetMap;
@@ -259,6 +275,21 @@ public final class RuleContext extends TargetContext
     return rule;
   }
 
+  public ImmutableList<Aspect> getAspects() {
+    return aspects;
+  }
+
+  /**
+   * If this <code>RuleContext</code> is for an aspect implementation, returns that aspect.
+   * (it is the last aspect in the list of aspects applied to a target; all other aspects
+   * are the ones main aspect sees as specified by its "required_aspect_providers")
+   * Otherwise returns <code>null</code>.
+   */
+  @Nullable
+  public Aspect getMainAspect() {
+    return aspects.isEmpty() ? null : aspects.get(aspects.size() - 1);
+  }
+
   /**
    * Returns a rule class name suitable for log messages, including an aspect name if applicable.
    */
@@ -1353,20 +1384,20 @@ public final class RuleContext extends TargetContext
     private ImmutableMap<Label, ConfigMatchingProvider> configConditions;
     private NestedSet<PackageGroupContents> visibility;
     private ImmutableMap<String, Attribute> aspectAttributes;
-    private ImmutableList<AspectDescriptor> aspectDescriptors;
+    private ImmutableList<Aspect> aspects;
     private ToolchainContext toolchainContext;
 
     Builder(
         AnalysisEnvironment env,
         Rule rule,
-        ImmutableList<AspectDescriptor> aspectDescriptors,
+        ImmutableList<Aspect> aspects,
         BuildConfiguration configuration,
         BuildConfiguration hostConfiguration,
         PrerequisiteValidator prerequisiteValidator,
         ConfigurationFragmentPolicy configurationFragmentPolicy) {
       this.env = Preconditions.checkNotNull(env);
       this.rule = Preconditions.checkNotNull(rule);
-      this.aspectDescriptors = aspectDescriptors;
+      this.aspects = aspects;
       this.configurationFragmentPolicy = Preconditions.checkNotNull(configurationFragmentPolicy);
       this.configuration = Preconditions.checkNotNull(configuration);
       this.hostConfiguration = Preconditions.checkNotNull(hostConfiguration);
@@ -1684,7 +1715,7 @@ public final class RuleContext extends TargetContext
 
     /** Returns whether the context being constructed is for the evaluation of an aspect. */
     public boolean forAspect() {
-      return !aspectDescriptors.isEmpty();
+      return !aspects.isEmpty();
     }
 
     public Rule getRule() {
@@ -1695,11 +1726,14 @@ public final class RuleContext extends TargetContext
      * Returns a rule class name suitable for log messages, including an aspect name if applicable.
      */
     public String getRuleClassNameForLogging() {
-      if (aspectDescriptors.isEmpty()) {
+      if (aspects.isEmpty()) {
         return rule.getRuleClass();
       }
 
-      return Joiner.on(",").join(aspectDescriptors) + " aspect on " + rule.getRuleClass();
+      return Joiner.on(",")
+              .join(aspects.stream().map(a -> a.getDescriptor()).collect(Collectors.toList()))
+          + " aspect on "
+          + rule.getRuleClass();
     }
 
     public BuildConfiguration getConfiguration() {
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttributesCollection.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttributesCollection.java
new file mode 100644
index 0000000000..77d82b6622
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttributesCollection.java
@@ -0,0 +1,267 @@
+// Copyright 2017 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.analysis.skylark;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.analysis.AliasProvider;
+import com.google.devtools.build.lib.analysis.FilesToRunProvider;
+import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
+import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode;
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.BuildType;
+import com.google.devtools.build.lib.packages.Info;
+import com.google.devtools.build.lib.packages.NativeProvider;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
+import com.google.devtools.build.lib.syntax.EvalException;
+import com.google.devtools.build.lib.syntax.Runtime;
+import com.google.devtools.build.lib.syntax.SkylarkList;
+import com.google.devtools.build.lib.syntax.SkylarkType;
+import com.google.devtools.build.lib.syntax.Type;
+import com.google.devtools.build.lib.syntax.Type.LabelClass;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+
+@SkylarkModule(
+  name = "rule_attributes",
+  category = SkylarkModuleCategory.NONE,
+  doc = "Information about attributes of a rule an aspect is applied to."
+)
+class SkylarkAttributesCollection implements SkylarkValue {
+  private final SkylarkRuleContext skylarkRuleContext;
+  private final Info attrObject;
+  private final Info executableObject;
+  private final Info fileObject;
+  private final Info filesObject;
+  private final ImmutableMap<Artifact, FilesToRunProvider> executableRunfilesMap;
+  private final String ruleClassName;
+
+  private SkylarkAttributesCollection(
+      SkylarkRuleContext skylarkRuleContext,
+      String ruleClassName,
+      Map<String, Object> attrs,
+      Map<String, Object> executables,
+      Map<String, Object> singleFiles,
+      Map<String, Object> files,
+      ImmutableMap<Artifact, FilesToRunProvider> executableRunfilesMap) {
+    this.skylarkRuleContext = skylarkRuleContext;
+    this.ruleClassName = ruleClassName;
+    attrObject =
+        NativeProvider.STRUCT.create(
+            attrs,
+            "No attribute '%s' in attr. Make sure you declared a rule attribute with this name.");
+    executableObject =
+        NativeProvider.STRUCT.create(
+            executables,
+            "No attribute '%s' in executable. Make sure there is a label type attribute marked "
+                + "as 'executable' with this name");
+    fileObject =
+        NativeProvider.STRUCT.create(
+            singleFiles,
+            "No attribute '%s' in file. Make sure there is a label type attribute marked "
+                + "as 'single_file' with this name");
+    filesObject =
+        NativeProvider.STRUCT.create(
+            files,
+            "No attribute '%s' in files. Make sure there is a label or label_list type attribute "
+                + "with this name");
+    this.executableRunfilesMap = executableRunfilesMap;
+  }
+
+  private void checkMutable(String attrName) throws EvalException {
+    skylarkRuleContext.checkMutable("rule." + attrName);
+  }
+
+  @SkylarkCallable(name = "attr", structField = true, doc = SkylarkRuleContext.ATTR_DOC)
+  public Info getAttr() throws EvalException {
+    checkMutable("attr");
+    return attrObject;
+  }
+
+  @SkylarkCallable(name = "executable", structField = true, doc = SkylarkRuleContext.EXECUTABLE_DOC)
+  public Info getExecutable() throws EvalException {
+    checkMutable("executable");
+    return executableObject;
+  }
+
+  @SkylarkCallable(name = "file", structField = true, doc = SkylarkRuleContext.FILE_DOC)
+  public Info getFile() throws EvalException {
+    checkMutable("file");
+    return fileObject;
+  }
+
+  @SkylarkCallable(name = "files", structField = true, doc = SkylarkRuleContext.FILES_DOC)
+  public Info getFiles() throws EvalException {
+    checkMutable("files");
+    return filesObject;
+  }
+
+  @SkylarkCallable(
+    name = "kind",
+    structField = true,
+    doc = "The kind of a rule, such as 'cc_library'"
+  )
+  public String getRuleClassName() throws EvalException {
+    checkMutable("kind");
+    return ruleClassName;
+  }
+
+  public ImmutableMap<Artifact, FilesToRunProvider> getExecutableRunfilesMap() {
+    return executableRunfilesMap;
+  }
+
+  @Override
+  public boolean isImmutable() {
+    return skylarkRuleContext.isImmutable();
+  }
+
+  @Override
+  public void repr(SkylarkPrinter printer) {
+    printer.append("<rule collection for " + skylarkRuleContext.getRuleLabelCanonicalName() + ">");
+  }
+
+  public static Builder builder(SkylarkRuleContext ruleContext) {
+    return new Builder(ruleContext);
+  }
+
+  public static class Builder {
+    private final SkylarkRuleContext context;
+    private final LinkedHashMap<String, Object> attrBuilder = new LinkedHashMap<>();
+    private final LinkedHashMap<String, Object> executableBuilder = new LinkedHashMap<>();
+    private final ImmutableMap.Builder<Artifact, FilesToRunProvider> executableRunfilesbuilder =
+        ImmutableMap.builder();
+    private final LinkedHashMap<String, Object> fileBuilder = new LinkedHashMap<>();
+    private final LinkedHashMap<String, Object> filesBuilder = new LinkedHashMap<>();
+    private final HashSet<Artifact> seenExecutables = new HashSet<>();
+
+    private Builder(SkylarkRuleContext ruleContext) {
+      this.context = ruleContext;
+    }
+
+    public void addAttribute(Attribute a, Object val) {
+      Type<?> type = a.getType();
+      String skyname = a.getPublicName();
+
+      // The first attribute with the same name wins.
+      if (attrBuilder.containsKey(skyname)) {
+        return;
+      }
+
+      // TODO(mstaib): Remove the LABEL_DICT_UNARY special case of this conditional
+      // LABEL_DICT_UNARY was previously not treated as a dependency-bearing type, and was put into
+      // Skylark as a Map<String, Label>; this special case preserves that behavior temporarily.
+      if (type.getLabelClass() != LabelClass.DEPENDENCY || type == BuildType.LABEL_DICT_UNARY) {
+        attrBuilder.put(
+            skyname,
+            val == null
+                ? Runtime.NONE
+                // Attribute values should be type safe
+                : SkylarkType.convertToSkylark(val, null));
+        return;
+      }
+      if (a.isExecutable()) {
+        // In Skylark only label (not label list) type attributes can have the Executable flag.
+        FilesToRunProvider provider =
+            context.getRuleContext().getExecutablePrerequisite(a.getName(), Mode.DONT_CHECK);
+        if (provider != null && provider.getExecutable() != null) {
+          Artifact executable = provider.getExecutable();
+          executableBuilder.put(skyname, executable);
+          if (!seenExecutables.contains(executable)) {
+            // todo(dslomov,laurentlb): In general, this is incorrect.
+            // We associate the first encountered FilesToRunProvider with
+            // the executable (this provider is later used to build the spawn).
+            // However ideally we should associate a provider with the attribute name,
+            // and pass the correct FilesToRunProvider to the spawn depending on
+            // what attribute is used to access the executable.
+            executableRunfilesbuilder.put(executable, provider);
+            seenExecutables.add(executable);
+          }
+        } else {
+          executableBuilder.put(skyname, Runtime.NONE);
+        }
+      }
+      if (a.isSingleArtifact()) {
+        // In Skylark only label (not label list) type attributes can have the SingleArtifact flag.
+        Artifact artifact =
+            context.getRuleContext().getPrerequisiteArtifact(a.getName(), Mode.DONT_CHECK);
+        if (artifact != null) {
+          fileBuilder.put(skyname, artifact);
+        } else {
+          fileBuilder.put(skyname, Runtime.NONE);
+        }
+      }
+      filesBuilder.put(
+          skyname,
+          context.getRuleContext().getPrerequisiteArtifacts(a.getName(), Mode.DONT_CHECK).list());
+
+      if (type == BuildType.LABEL && !a.hasSplitConfigurationTransition()) {
+        Object prereq = context.getRuleContext().getPrerequisite(a.getName(), Mode.DONT_CHECK);
+        if (prereq == null) {
+          prereq = Runtime.NONE;
+        }
+        attrBuilder.put(skyname, prereq);
+      } else if (type == BuildType.LABEL_LIST
+          || (type == BuildType.LABEL && a.hasSplitConfigurationTransition())) {
+        List<?> allPrereq = context.getRuleContext().getPrerequisites(a.getName(), Mode.DONT_CHECK);
+        attrBuilder.put(skyname, SkylarkList.createImmutable(allPrereq));
+      } else if (type == BuildType.LABEL_KEYED_STRING_DICT) {
+        ImmutableMap.Builder<TransitiveInfoCollection, String> builder = ImmutableMap.builder();
+        Map<Label, String> original = BuildType.LABEL_KEYED_STRING_DICT.cast(val);
+        List<? extends TransitiveInfoCollection> allPrereq =
+            context.getRuleContext().getPrerequisites(a.getName(), Mode.DONT_CHECK);
+        for (TransitiveInfoCollection prereq : allPrereq) {
+          builder.put(prereq, original.get(AliasProvider.getDependencyLabel(prereq)));
+        }
+        attrBuilder.put(skyname, SkylarkType.convertToSkylark(builder.build(), null));
+      } else if (type == BuildType.LABEL_DICT_UNARY) {
+        Map<Label, TransitiveInfoCollection> prereqsByLabel = new LinkedHashMap<>();
+        for (TransitiveInfoCollection target :
+            context.getRuleContext().getPrerequisites(a.getName(), Mode.DONT_CHECK)) {
+          prereqsByLabel.put(target.getLabel(), target);
+        }
+        ImmutableMap.Builder<String, TransitiveInfoCollection> attrValue = ImmutableMap.builder();
+        for (Entry<String, Label> entry : ((Map<String, Label>) val).entrySet()) {
+          attrValue.put(entry.getKey(), prereqsByLabel.get(entry.getValue()));
+        }
+        attrBuilder.put(skyname, attrValue.build());
+      } else {
+        throw new IllegalArgumentException(
+            "Can't transform attribute "
+                + a.getName()
+                + " of type "
+                + type
+                + " to a Skylark object");
+      }
+    }
+
+    public SkylarkAttributesCollection build() {
+      return new SkylarkAttributesCollection(
+          context,
+          context.getRuleContext().getRule().getRuleClass(),
+          attrBuilder,
+          executableBuilder,
+          fileBuilder,
+          filesBuilder,
+          executableRunfilesbuilder.build());
+    }
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java
index 8388391361..7910b80410 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java
@@ -27,7 +27,6 @@ import com.google.common.collect.Ordering;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Root;
 import com.google.devtools.build.lib.analysis.ActionsProvider;
-import com.google.devtools.build.lib.analysis.AliasProvider;
 import com.google.devtools.build.lib.analysis.ConfigurationMakeVariableContext;
 import com.google.devtools.build.lib.analysis.DefaultInfo;
 import com.google.devtools.build.lib.analysis.FilesToRunProvider;
@@ -43,6 +42,7 @@ import com.google.devtools.build.lib.analysis.test.InstrumentedFilesCollector;
 import com.google.devtools.build.lib.analysis.test.InstrumentedFilesProvider;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.Location;
+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.Attribute.ConfigurationTransition;
@@ -71,14 +71,12 @@ import com.google.devtools.build.lib.syntax.SkylarkDict;
 import com.google.devtools.build.lib.syntax.SkylarkIndexable;
 import com.google.devtools.build.lib.syntax.SkylarkList;
 import com.google.devtools.build.lib.syntax.SkylarkSemantics;
-import com.google.devtools.build.lib.syntax.SkylarkType;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.syntax.Type.LabelClass;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.LinkedList;
 import java.util.List;
@@ -205,8 +203,8 @@ public final class SkylarkRuleContext implements SkylarkValue {
   private final SkylarkSemantics skylarkSemantics;
 
   private SkylarkDict<String, String> makeVariables;
-  private SkylarkRuleAttributesCollection attributesCollection;
-  private SkylarkRuleAttributesCollection ruleAttributesCollection;
+  private SkylarkAttributesCollection attributesCollection;
+  private SkylarkAttributesCollection ruleAttributesCollection;
   private Info splitAttributes;
 
   // TODO(bazel-team): we only need this because of the css_binary rule.
@@ -285,28 +283,46 @@ public final class SkylarkRuleContext implements SkylarkValue {
       this.artifactsLabelMap = artifactLabelMapBuilder.build();
       this.outputsObject = outputs;
 
-      this.attributesCollection =
-          buildAttributesCollection(
-              this, attributes, ruleContext, attributeValueExtractorForRule(ruleContext));
+      SkylarkAttributesCollection.Builder builder = SkylarkAttributesCollection.builder(this);
+      for (Attribute attribute : ruleContext.getRule().getAttributes()) {
+        Object value = ruleContext.attributes().get(attribute.getName(), attribute.getType());
+        builder.addAttribute(attribute, value);
+      }
+
+      this.attributesCollection = builder.build();
       this.splitAttributes = buildSplitAttributeInfo(attributes, ruleContext);
       this.ruleAttributesCollection = null;
     } else { // ASPECT
       this.isForAspect = true;
       this.artifactsLabelMap = ImmutableMap.of();
       this.outputsObject = null;
-      this.attributesCollection =
-          buildAttributesCollection(
-              this,
-              ruleContext.getAspectAttributes().values(),
-              ruleContext,
-              ATTRIBUTE_VALUE_EXTRACTOR_FOR_ASPECT);
+
+      ImmutableCollection<Attribute> attributes =
+          ruleContext.getMainAspect().getDefinition().getAttributes().values();
+      SkylarkAttributesCollection.Builder aspectBuilder = SkylarkAttributesCollection.builder(this);
+      for (Attribute attribute : attributes) {
+        aspectBuilder.addAttribute(attribute, attribute.getDefaultValue(null));
+      }
+      this.attributesCollection = aspectBuilder.build();
+
       this.splitAttributes = null;
-      this.ruleAttributesCollection =
-          buildAttributesCollection(
-              this,
-              ruleContext.getRule().getAttributes(),
-              ruleContext,
-              attributeValueExtractorForRule(ruleContext));
+      SkylarkAttributesCollection.Builder ruleBuilder = SkylarkAttributesCollection.builder(this);
+
+      for (Attribute attribute : ruleContext.getRule().getAttributes()) {
+        Object value = ruleContext.attributes().get(attribute.getName(), attribute.getType());
+        ruleBuilder.addAttribute(attribute, value);
+      }
+      for (Aspect aspect : ruleContext.getAspects()) {
+        if (aspect.equals(ruleContext.getMainAspect())) {
+          // Aspect's own attributes are in <code>attributesCollection</code>.
+          continue;
+        }
+        for (Attribute attribute : aspect.getDefinition().getAttributes().values()) {
+          ruleBuilder.addAttribute(attribute, attribute.getDefaultValue(null));
+        }
+      }
+
+      this.ruleAttributesCollection = ruleBuilder.build();
     }
 
     makeVariables = ruleContext.getConfigurationMakeVariableContext().collectMakeVariables();
@@ -366,7 +382,7 @@ public final class SkylarkRuleContext implements SkylarkValue {
       if (context.isExecutable() && EXECUTABLE_OUTPUT_NAME.equals(name)) {
         executableCreated = true;
         // createOutputArtifact() will cache the created artifact.
-        return context.ruleContext.createOutputArtifact();
+        return context.getRuleContext().createOutputArtifact();
       }
 
       return outputs.get(name);
@@ -461,121 +477,8 @@ public final class SkylarkRuleContext implements SkylarkValue {
     return aspectDescriptor;
   }
 
-  private Function<Attribute, Object> attributeValueExtractorForRule(
-      final RuleContext ruleContext) {
-    return new Function<Attribute, Object>() {
-      @Nullable
-      @Override
-      public Object apply(Attribute attribute) {
-        return ruleContext.attributes().get(attribute.getName(), attribute.getType());
-      }
-    };
-  }
-
-  private static SkylarkRuleAttributesCollection buildAttributesCollection(
-      SkylarkRuleContext skylarkRuleContext,
-      Collection<Attribute> attributes,
-      RuleContext ruleContext,
-      Function<Attribute, Object> attributeValueExtractor) {
-    Builder<String, Object> attrBuilder = new Builder<>();
-    Builder<String, Object> executableBuilder = new Builder<>();
-    Builder<Artifact, FilesToRunProvider> executableRunfilesbuilder = new Builder<>();
-    Builder<String, Object> fileBuilder = new Builder<>();
-    Builder<String, Object> filesBuilder = new Builder<>();
-    HashSet<Artifact> seenExecutables = new HashSet<>();
-    for (Attribute a : attributes) {
-      Type<?> type = a.getType();
-      Object val = attributeValueExtractor.apply(a);
-      // TODO(mstaib): Remove the LABEL_DICT_UNARY special case of this conditional
-      // LABEL_DICT_UNARY was previously not treated as a dependency-bearing type, and was put into
-      // Skylark as a Map<String, Label>; this special case preserves that behavior temporarily.
-      if (type.getLabelClass() != LabelClass.DEPENDENCY || type == BuildType.LABEL_DICT_UNARY) {
-        attrBuilder.put(a.getPublicName(), val == null ? Runtime.NONE
-            // Attribute values should be type safe
-            : SkylarkType.convertToSkylark(val, null));
-        continue;
-      }
-      String skyname = a.getPublicName();
-      if (a.isExecutable()) {
-        // In Skylark only label (not label list) type attributes can have the Executable flag.
-        FilesToRunProvider provider =
-            ruleContext.getExecutablePrerequisite(a.getName(), Mode.DONT_CHECK);
-        if (provider != null && provider.getExecutable() != null) {
-          Artifact executable = provider.getExecutable();
-          executableBuilder.put(skyname, executable);
-          if (!seenExecutables.contains(executable)) {
-            // todo(dslomov,laurentlb): In general, this is incorrect.
-            // We associate the first encountered FilesToRunProvider with
-            // the executable (this provider is later used to build the spawn).
-            // However ideally we should associate a provider with the attribute name,
-            // and pass the correct FilesToRunProvider to the spawn depending on
-            // what attribute is used to access the executable.
-            executableRunfilesbuilder.put(executable, provider);
-            seenExecutables.add(executable);
-          }
-        } else {
-          executableBuilder.put(skyname, Runtime.NONE);
-        }
-      }
-      if (a.isSingleArtifact()) {
-        // In Skylark only label (not label list) type attributes can have the SingleArtifact flag.
-        Artifact artifact = ruleContext.getPrerequisiteArtifact(a.getName(), Mode.DONT_CHECK);
-        if (artifact != null) {
-          fileBuilder.put(skyname, artifact);
-        } else {
-          fileBuilder.put(skyname, Runtime.NONE);
-        }
-      }
-      filesBuilder.put(
-          skyname, ruleContext.getPrerequisiteArtifacts(a.getName(), Mode.DONT_CHECK).list());
-
-      if (type == BuildType.LABEL && !a.hasSplitConfigurationTransition()) {
-        Object prereq = ruleContext.getPrerequisite(a.getName(), Mode.DONT_CHECK);
-        if (prereq == null) {
-          prereq = Runtime.NONE;
-        }
-        attrBuilder.put(skyname, prereq);
-      } else if (type == BuildType.LABEL_LIST
-          || (type == BuildType.LABEL && a.hasSplitConfigurationTransition())) {
-        List<?> allPrereq = ruleContext.getPrerequisites(a.getName(), Mode.DONT_CHECK);
-        attrBuilder.put(skyname, SkylarkList.createImmutable(allPrereq));
-      } else if (type == BuildType.LABEL_KEYED_STRING_DICT) {
-        ImmutableMap.Builder<TransitiveInfoCollection, String> builder =
-            new ImmutableMap.Builder<>();
-        Map<Label, String> original = BuildType.LABEL_KEYED_STRING_DICT.cast(val);
-        List<? extends TransitiveInfoCollection> allPrereq =
-            ruleContext.getPrerequisites(a.getName(), Mode.DONT_CHECK);
-        for (TransitiveInfoCollection prereq : allPrereq) {
-          builder.put(prereq, original.get(AliasProvider.getDependencyLabel(prereq)));
-        }
-        attrBuilder.put(skyname, SkylarkType.convertToSkylark(builder.build(), null));
-      } else if (type == BuildType.LABEL_DICT_UNARY) {
-        Map<Label, TransitiveInfoCollection> prereqsByLabel = new LinkedHashMap<>();
-        for (TransitiveInfoCollection target
-              : ruleContext.getPrerequisites(a.getName(), Mode.DONT_CHECK)) {
-          prereqsByLabel.put(target.getLabel(), target);
-        }
-        ImmutableMap.Builder<String, TransitiveInfoCollection> attrValue =
-            new ImmutableMap.Builder<>();
-        for (Map.Entry<String, Label> entry : ((Map<String, Label>) val).entrySet()) {
-          attrValue.put(entry.getKey(), prereqsByLabel.get(entry.getValue()));
-        }
-        attrBuilder.put(skyname, attrValue.build());
-      } else {
-        throw new IllegalArgumentException(
-            "Can't transform attribute " + a.getName() + " of type " + type
-            + " to a Skylark object");
-      }
-    }
-
-    return new SkylarkRuleAttributesCollection(
-        skylarkRuleContext,
-        ruleContext.getRule().getRuleClass(),
-        attrBuilder.build(),
-        executableBuilder.build(),
-        fileBuilder.build(),
-        filesBuilder.build(),
-        executableRunfilesbuilder.build());
+  public String getRuleLabelCanonicalName() {
+    return ruleLabelCanonicalName;
   }
 
   private static Info buildSplitAttributeInfo(
@@ -624,101 +527,6 @@ public final class SkylarkRuleContext implements SkylarkValue {
             + "split configuration.");
   }
 
-  @SkylarkModule(
-    name = "rule_attributes",
-    category = SkylarkModuleCategory.NONE,
-    doc = "Information about attributes of a rule an aspect is applied to."
-  )
-  private static class SkylarkRuleAttributesCollection implements SkylarkValue {
-    private final SkylarkRuleContext skylarkRuleContext;
-    private final Info attrObject;
-    private final Info executableObject;
-    private final Info fileObject;
-    private final Info filesObject;
-    private final ImmutableMap<Artifact, FilesToRunProvider> executableRunfilesMap;
-    private final String ruleClassName;
-
-    private SkylarkRuleAttributesCollection(
-        SkylarkRuleContext skylarkRuleContext,
-        String ruleClassName, ImmutableMap<String, Object> attrs,
-        ImmutableMap<String, Object> executables,
-        ImmutableMap<String, Object> singleFiles,
-        ImmutableMap<String, Object> files,
-        ImmutableMap<Artifact, FilesToRunProvider> executableRunfilesMap) {
-      this.skylarkRuleContext = skylarkRuleContext;
-      this.ruleClassName = ruleClassName;
-      attrObject =
-          NativeProvider.STRUCT.create(
-              attrs,
-              "No attribute '%s' in attr. Make sure you declared a rule attribute with this name.");
-      executableObject =
-          NativeProvider.STRUCT.create(
-              executables,
-              "No attribute '%s' in executable. Make sure there is a label type attribute marked "
-                  + "as 'executable' with this name");
-      fileObject =
-          NativeProvider.STRUCT.create(
-              singleFiles,
-              "No attribute '%s' in file. Make sure there is a label type attribute marked "
-                  + "as 'single_file' with this name");
-      filesObject =
-          NativeProvider.STRUCT.create(
-              files,
-              "No attribute '%s' in files. Make sure there is a label or label_list type attribute "
-                  + "with this name");
-      this.executableRunfilesMap = executableRunfilesMap;
-    }
-
-    private void checkMutable(String attrName) throws EvalException {
-      skylarkRuleContext.checkMutable("rule." + attrName);
-    }
-
-    @SkylarkCallable(name = "attr", structField = true, doc = ATTR_DOC)
-    public Info getAttr() throws EvalException {
-      checkMutable("attr");
-      return attrObject;
-    }
-
-    @SkylarkCallable(name = "executable", structField = true, doc = EXECUTABLE_DOC)
-    public Info getExecutable() throws EvalException {
-      checkMutable("executable");
-      return executableObject;
-    }
-
-    @SkylarkCallable(name = "file", structField = true, doc = FILE_DOC)
-    public Info getFile() throws EvalException {
-      checkMutable("file");
-      return fileObject;
-    }
-
-    @SkylarkCallable(name = "files", structField = true, doc = FILES_DOC)
-    public Info getFiles() throws EvalException {
-      checkMutable("files");
-      return filesObject;
-    }
-
-    @SkylarkCallable(name = "kind", structField = true, doc =
-        "The kind of a rule, such as 'cc_library'")
-    public String getRuleClassName() throws EvalException {
-      checkMutable("kind");
-      return ruleClassName;
-    }
-
-    public ImmutableMap<Artifact, FilesToRunProvider> getExecutableRunfilesMap() {
-      return executableRunfilesMap;
-    }
-
-    @Override
-    public boolean isImmutable() {
-      return skylarkRuleContext.isImmutable();
-    }
-
-    @Override
-    public void repr(SkylarkPrinter printer) {
-      printer.append("<rule collection for " + skylarkRuleContext.ruleLabelCanonicalName + ">");
-    }
-  }
-
   @Override
   public boolean isImmutable() {
     return ruleContext == null;
@@ -922,10 +730,13 @@ public final class SkylarkRuleContext implements SkylarkValue {
     return outputsObject;
   }
 
-  @SkylarkCallable(structField = true,
-      doc = "Returns rule attributes descriptor for the rule that aspect is applied to."
-          + " Only available in aspect implementation functions.")
-  public SkylarkRuleAttributesCollection rule() throws EvalException {
+  @SkylarkCallable(
+    structField = true,
+    doc =
+        "Returns rule attributes descriptor for the rule that aspect is applied to."
+            + " Only available in aspect implementation functions."
+  )
+  public SkylarkAttributesCollection rule() throws EvalException {
     checkMutable("rule");
     if (!isForAspect) {
       throw new EvalException(
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
index 1d0abfb662..1ad16997e0 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
@@ -2317,7 +2317,182 @@ public class SkylarkAspectsTest extends AnalysisTestCase {
             "//test:r0+PAspect");
   }
 
+  @Test
+  public void aspectSeesOtherAspectAttributes() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "PAspect = provider(fields = [])",
+        "PCollector = provider(fields = ['aspect_attr'])",
+        "def _a_impl(target, ctx):",
+        "  return [PAspect()]",
+        "a = aspect(_a_impl, ",
+        "           provides = [PAspect],",
+        "           attrs = {'_a_attr' : attr.label(default = '//test:foo')})",
+        "def _rcollect(target, ctx):",
+        "  if hasattr(ctx.rule.attr, '_a_attr'):",
+        "     return [PCollector(aspect_attr = ctx.rule.attr._a_attr.label)]",
+        "  if hasattr(ctx.rule.attr, 'dep'):",
+        "     return [ctx.rule.attr.dep[PCollector]]",
+        "  return [PCollector()]",
+        "acollect = aspect(_rcollect, attr_aspects = ['*'], required_aspect_providers = [PAspect])",
+        "def _rimpl(ctx):",
+        "  pass",
+        "r0 = rule(_rimpl)",
+        "r = rule(_rimpl, attrs = { 'dep' : attr.label(aspects = [a]) })");
+    scratch.file(
+        "test/BUILD",
+        "load(':aspect.bzl', 'r0', 'r')",
+        "r0(name = 'foo')",
+        "r0(name = 'bar')",
+        "r(name = 'baz', dep = ':bar')");
+    AnalysisResult analysisResult =
+        update(ImmutableList.of("//test:aspect.bzl%acollect"), "//test:baz");
+    ConfiguredAspect configuredAspect =
+        Iterables.getOnlyElement(analysisResult.getAspects()).getConfiguredAspect();
+    SkylarkKey pCollector = new SkylarkKey(Label.parseAbsolute("//test:aspect.bzl"), "PCollector");
+    Info collector = configuredAspect.get(pCollector);
+    assertThat(collector.getValue("aspect_attr")).isEqualTo(Label.parseAbsolute("//test:foo"));
+  }
+
+  @Test
+  public void ruleAttributesWinOverAspects() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "PAspect = provider(fields = [])",
+        "PCollector = provider(fields = ['attr_value'])",
+        "def _a_impl(target, ctx):",
+        "  return [PAspect()]",
+        "a = aspect(_a_impl, ",
+        "           provides = [PAspect],",
+        "           attrs = {'_same_attr' : attr.int(default = 239)})",
+        "def _rcollect(target, ctx):",
+        "  if hasattr(ctx.rule.attr, '_same_attr'):",
+        "     return [PCollector(attr_value = ctx.rule.attr._same_attr)]",
+        "  if hasattr(ctx.rule.attr, 'dep'):",
+        "     return [ctx.rule.attr.dep[PCollector]]",
+        "  return [PCollector()]",
+        "acollect = aspect(_rcollect, attr_aspects = ['*'], required_aspect_providers = [PAspect])",
+        "def _rimpl(ctx):",
+        "  pass",
+        "r0 = rule(_rimpl)",
+        "r = rule(_rimpl, ",
+        "          attrs = { ",
+        "                  'dep' : attr.label(aspects = [a]), ",
+        "                  '_same_attr' : attr.int(default = 30)",
+        "          })");
+    scratch.file(
+        "test/BUILD",
+        "load(':aspect.bzl', 'r0', 'r')",
+        "r0(name = 'foo')",
+        "r0(name = 'bar')",
+        "r(name = 'baz', dep = ':bar')");
+    AnalysisResult analysisResult =
+        update(ImmutableList.of("//test:aspect.bzl%acollect"), "//test:baz");
+    ConfiguredAspect configuredAspect =
+        Iterables.getOnlyElement(analysisResult.getAspects()).getConfiguredAspect();
+    SkylarkKey pCollector = new SkylarkKey(Label.parseAbsolute("//test:aspect.bzl"), "PCollector");
+    Info collector = configuredAspect.get(pCollector);
+    assertThat(collector.getValue("attr_value")).isEqualTo(30);
+  }
 
+  @Test
+  public void earlyAspectAttributesWin() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "PAspect1 = provider(fields = [])",
+        "PAspect2 = provider(fields = [])",
+        "PCollector = provider(fields = ['attr_value'])",
+        "def _a1_impl(target, ctx):",
+        "  return [PAspect1()]",
+        "def _a2_impl(target, ctx):",
+        "  return [PAspect2()]",
+        "a1 = aspect(_a1_impl, ",
+        "            provides = [PAspect1],",
+        "            attrs = {'_same_attr' : attr.int(default = 30)})",
+        "a2 = aspect(_a2_impl, ",
+        "            provides = [PAspect2],",
+        "            attrs = {'_same_attr' : attr.int(default = 239)})",
+        "def _rcollect(target, ctx):",
+        "  if hasattr(ctx.rule.attr, 'dep'):",
+        "     return [ctx.rule.attr.dep[PCollector]]",
+        "  if hasattr(ctx.rule.attr, '_same_attr'):",
+        "     return [PCollector(attr_value = ctx.rule.attr._same_attr)]",
+        "  fail('???')",
+        "  return [PCollector()]",
+        "acollect = aspect(_rcollect, attr_aspects = ['*'], ",
+        "                  required_aspect_providers = [[PAspect1], [PAspect2]])",
+        "def _rimpl(ctx):",
+        "  pass",
+        "r0 = rule(_rimpl)",
+        "r1 = rule(_rimpl, ",
+        "          attrs = { ",
+        "                  'dep' : attr.label(aspects = [a1]), ",
+        "          })",
+        "r2 = rule(_rimpl, ",
+        "          attrs = { ",
+        "                  'dep' : attr.label(aspects = [a2]), ",
+        "          })"
+    );
+    scratch.file(
+        "test/BUILD",
+        "load(':aspect.bzl', 'r0', 'r1', 'r2')",
+        "r0(name = 'bar')",
+        "r1(name = 'baz', dep = ':bar')",
+        "r2(name = 'quux', dep = ':baz')"
+    );
+
+    AnalysisResult analysisResult =
+        update(ImmutableList.of("//test:aspect.bzl%acollect"), "//test:quux");
+    ConfiguredAspect configuredAspect =
+        Iterables.getOnlyElement(analysisResult.getAspects()).getConfiguredAspect();
+    SkylarkKey pCollector = new SkylarkKey(Label.parseAbsolute("//test:aspect.bzl"), "PCollector");
+    Info collector = configuredAspect.get(pCollector);
+    assertThat(collector.getValue("attr_value")).isEqualTo(30);
+  }
+
+
+  @Test
+  public void aspectPropagatesOverOtherAspectAttributes() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "PAspect = provider(fields = [])",
+        "PCollector = provider(fields = ['visited'])",
+        "def _a_impl(target, ctx):",
+        "  return [PAspect()]",
+        "a = aspect(_a_impl, ",
+        "       provides = [PAspect],",
+        "       attrs = {'_a_attr' : attr.label(default = '//test:referenced_from_aspect_only')})",
+        "def _rcollect(target, ctx):",
+        "  transitive = []",
+        "  if hasattr(ctx.rule.attr, 'dep') and ctx.rule.attr.dep:",
+        "     transitive += [ctx.rule.attr.dep[PCollector].visited]",
+        "  if hasattr(ctx.rule.attr, '_a_attr') and ctx.rule.attr._a_attr:",
+        "     transitive += [ctx.rule.attr._a_attr[PCollector].visited] ",
+        "  visited = depset([target.label], transitive = transitive, )",
+        "  return [PCollector(visited = visited)]",
+        "acollect = aspect(_rcollect, attr_aspects = ['*'], required_aspect_providers = [PAspect])",
+        "def _rimpl(ctx):",
+        "  pass",
+        "r0 = rule(_rimpl)",
+        "r = rule(_rimpl, attrs = { 'dep' : attr.label(aspects = [a]) })");
+    scratch.file(
+        "test/BUILD",
+        "load(':aspect.bzl', 'r0', 'r')",
+        "r0(name = 'referenced_from_aspect_only')",
+        "r0(name = 'bar')",
+        "r(name = 'baz', dep = ':bar')");
+    AnalysisResult analysisResult =
+        update(ImmutableList.of("//test:aspect.bzl%acollect"), "//test:baz");
+    ConfiguredAspect configuredAspect =
+        Iterables.getOnlyElement(analysisResult.getAspects()).getConfiguredAspect();
+    SkylarkKey pCollector = new SkylarkKey(Label.parseAbsolute("//test:aspect.bzl"), "PCollector");
+    Info collector = configuredAspect.get(pCollector);
+    assertThat(((SkylarkNestedSet) collector.getValue("visited")).toCollection())
+        .containsExactly(
+            Label.parseAbsolute("//test:referenced_from_aspect_only"),
+            Label.parseAbsolute("//test:bar"),
+            Label.parseAbsolute("//test:baz"));
+  }
 
   /** SkylarkAspectTest with "keep going" flag */
   @RunWith(JUnit4.class)