When creating RuleContext, explicitly pass the set of attributes an attached Aspect provides.

--
MOS_MIGRATED_REVID=106882046

6 files changed, 112 insertions, 32 deletions

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 067f370f2c..91c4299c79 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
@@ -55,6 +55,7 @@ import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.ArrayList;
 import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 
 import javax.annotation.Nullable;
@@ -290,10 +291,11 @@ public final class ConfiguredTargetFactory {
       RuleConfiguredTarget associatedTarget,
       ConfiguredAspectFactory aspectFactory,
       AspectParameters aspectParameters,
+      Map<String, Attribute> aspectAttributes,
       ListMultimap<Attribute, ConfiguredTarget> prerequisiteMap,
       Set<ConfigMatchingProvider> configConditions,
       BuildConfiguration hostConfiguration)
-      throws InterruptedException {
+          throws InterruptedException {
     RuleContext.Builder builder = new RuleContext.Builder(env,
         associatedTarget.getTarget(),
         associatedTarget.getConfiguration(),
@@ -303,6 +305,7 @@ public final class ConfiguredTargetFactory {
         .setVisibility(convertVisibility(
             prerequisiteMap, env.getEventHandler(), associatedTarget.getTarget(), null))
         .setPrerequisites(prerequisiteMap)
+        .setAspectAttributes(aspectAttributes)
         .setConfigConditions(configConditions)
         .build();
     if (ruleContext.hasErrors()) {
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 12916b3253..e1ccc44a9d 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
@@ -136,7 +136,7 @@ public final class RuleContext extends TargetContext
   private final Set<ConfigMatchingProvider> configConditions;
   private final AttributeMap attributes;
   private final ImmutableSet<String> features;
-  private final Map<String, Attribute> attributeMap;
+  private final Map<String, Attribute> aspectAttributes;
   private final BuildConfiguration hostConfiguration;
   private final ConfigurationFragmentPolicy configurationFragmentPolicy;
   private final ErrorReporter reporter;
@@ -148,7 +148,7 @@ public final class RuleContext extends TargetContext
 
   private RuleContext(Builder builder, ListMultimap<String, ConfiguredTarget> targetMap,
       ListMultimap<String, ConfiguredFilesetEntry> filesetEntryMap,
-      Set<ConfigMatchingProvider> configConditions, Map<String, Attribute> attributeMap) {
+      Set<ConfigMatchingProvider> configConditions, Map<String, Attribute> aspectAttributes) {
     super(builder.env, builder.rule, builder.configuration, builder.prerequisiteMap.get(null),
         builder.visibility);
     this.rule = builder.rule;
@@ -159,7 +159,7 @@ public final class RuleContext extends TargetContext
     this.attributes =
         ConfiguredAttributeMapper.of(builder.rule, configConditions);
     this.features = getEnabledFeatures();
-    this.attributeMap = attributeMap;
+    this.aspectAttributes = aspectAttributes;
     this.hostConfiguration = builder.hostConfiguration;
     reporter = builder.reporter;
   }
@@ -561,16 +561,11 @@ public final class RuleContext extends TargetContext
   }
 
   private Attribute getAttribute(String attributeName) {
-    // TODO(bazel-team): We should check original rule for such attribute first, because aspect
-    // can't contain empty attribute. Consider changing type of prerequisiteMap from
-    // ListMultimap<Attribute, ConfiguredTarget> to Map<Attribute, List<ConfiguredTarget>>. This can
-    // also simplify logic in DependencyResolver#resolveExplicitAttributes.
     Attribute result = getRule().getAttributeDefinition(attributeName);
     if (result != null) {
       return result;
     }
-    // Also this attribute can come from aspects, so we also have to check attributeMap.
-    return attributeMap.get(attributeName);
+    return aspectAttributes.get(attributeName);
   }
 
   /**
@@ -1239,6 +1234,7 @@ public final class RuleContext extends TargetContext
     private ListMultimap<Attribute, ConfiguredTarget> prerequisiteMap;
     private Set<ConfigMatchingProvider> configConditions;
     private NestedSet<PackageSpecification> visibility;
+    private Map<String, Attribute> aspectAttributes;
 
     Builder(AnalysisEnvironment env, Rule rule, BuildConfiguration configuration,
         BuildConfiguration hostConfiguration,
@@ -1259,14 +1255,8 @@ public final class RuleContext extends TargetContext
       ListMultimap<String, ConfiguredTarget> targetMap = createTargetMap();
       ListMultimap<String, ConfiguredFilesetEntry> filesetEntryMap =
           createFilesetEntryMap(rule, configConditions);
-      Map<String, Attribute> attributeMap = new HashMap<>();
-      for (Attribute attribute : prerequisiteMap.keySet()) {
-        if (attribute == null) {
-          continue;
-        }
-        attributeMap.put(attribute.getName(), attribute);
-      }
-      return new RuleContext(this, targetMap, filesetEntryMap, configConditions, attributeMap);
+      return new RuleContext(this, targetMap, filesetEntryMap, configConditions,
+          aspectAttributes != null ? aspectAttributes : ImmutableMap.<String, Attribute>of());
     }
 
     Builder setVisibility(NestedSet<PackageSpecification> visibility) {
@@ -1284,6 +1274,14 @@ public final class RuleContext extends TargetContext
     }
 
     /**
+     * Adds attributes which are defined by an Aspect (and not by RuleClass).
+     */
+    Builder setAspectAttributes(Map<String, Attribute> aspectAttributes) {
+      this.aspectAttributes = aspectAttributes;
+      return this;
+    }
+
+    /**
      * Sets the configuration conditions needed to determine which paths to follow for this
      * rule's configurable attributes.
      */
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 1ddd626164..41af5ff3f2 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
@@ -28,6 +28,7 @@ import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.events.StoredEventHandler;
+import com.google.devtools.build.lib.packages.AspectParameters;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
 import com.google.devtools.build.lib.packages.NativeAspectClass;
@@ -223,9 +224,10 @@ public final class AspectFunction implements SkyFunction {
       return null;
     }
 
+    AspectParameters aspectParams = key.getParameters();
     Aspect aspect = view.createAspect(
         analysisEnvironment, associatedTarget, aspectFactory, directDeps, configConditions,
-        key.getParameters());
+        aspectParams, key.getAspect().getDefinition(aspectParams).getAttributes());
 
     events.replayOn(env.getListener());
     if (events.hasErrors()) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
index e1dc3a025a..6912dc4423 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -528,10 +528,11 @@ public final class SkyframeBuildView {
       ConfiguredAspectFactory aspectFactory,
       ListMultimap<Attribute, ConfiguredTarget> prerequisiteMap,
       Set<ConfigMatchingProvider> configConditions,
-      AspectParameters aspectParameters)
-      throws InterruptedException {
+      AspectParameters aspectParameters,
+      Map<String, Attribute> aspectAttributes)
+          throws InterruptedException {
     return factory.createAspect(env, associatedTarget, aspectFactory, aspectParameters,
-        prerequisiteMap, configConditions,
+        aspectAttributes, prerequisiteMap, configConditions,
         getHostConfiguration(associatedTarget.getConfiguration()));
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
index 33881a83e8..7341ba1643 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
@@ -14,10 +14,24 @@
 package com.google.devtools.build.lib.analysis;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode.TARGET;
+import static com.google.devtools.build.lib.analysis.util.TestAspects.EMPTY_LATE_BOUND_LABEL;
+import static com.google.devtools.build.lib.packages.Attribute.attr;
+import static com.google.devtools.build.lib.packages.BuildType.LABEL;
+import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
 
 import com.google.devtools.build.lib.analysis.util.AnalysisTestCase;
 import com.google.devtools.build.lib.analysis.util.TestAspects;
+import com.google.devtools.build.lib.analysis.util.TestAspects.AspectInfo;
 import com.google.devtools.build.lib.analysis.util.TestAspects.AspectRequiringRule;
+import com.google.devtools.build.lib.analysis.util.TestAspects.BaseRule;
+import com.google.devtools.build.lib.analysis.util.TestAspects.DummyRuleFactory;
+import com.google.devtools.build.lib.analysis.util.TestAspects.RuleInfo;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.collect.nestedset.Order;
+import com.google.devtools.build.lib.packages.AspectDefinition;
+import com.google.devtools.build.lib.packages.AspectParameters;
+import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
 
 import org.junit.After;
@@ -46,7 +60,6 @@ public class AspectTest extends AnalysisTestCase {
     super.tearDown();
   }
 
-  @SafeVarargs
   private final void setRules(RuleDefinition... rules) throws Exception {
     ConfiguredRuleClassProvider.Builder builder =
         new ConfiguredRuleClassProvider.Builder();
@@ -71,7 +84,7 @@ public class AspectTest extends AnalysisTestCase {
         "aspect(name='b', foo=[])");
 
     ConfiguredTarget a = getConfiguredTarget("//a:a");
-    assertThat(a.getProvider(TestAspects.RuleInfo.class).getData())
+    assertThat(a.getProvider(RuleInfo.class).getData())
         .containsExactly("aspect //a:b", "rule //a:a");
   }
 
@@ -85,8 +98,7 @@ public class AspectTest extends AnalysisTestCase {
         "liar(name='b', foo=[])");
 
     ConfiguredTarget a = getConfiguredTarget("//a:a");
-    assertThat(a.getProvider(TestAspects.RuleInfo.class).getData())
-        .containsExactly("rule //a:a");
+    assertThat(a.getProvider(RuleInfo.class).getData()).containsExactly("rule //a:a");
   }
 
   @Test
@@ -99,7 +111,7 @@ public class AspectTest extends AnalysisTestCase {
         "honest(name='b', foo=[])");
 
     ConfiguredTarget a = getConfiguredTarget("//a:a");
-    assertThat(a.getProvider(TestAspects.RuleInfo.class).getData())
+    assertThat(a.getProvider(RuleInfo.class).getData())
         .containsExactly("rule //a:a", "aspect //a:b");
   }
 
@@ -179,7 +191,7 @@ public class AspectTest extends AnalysisTestCase {
         "aspect(name='b', foo=[])");
 
     ConfiguredTarget a = getConfiguredTarget("//a:a");
-    assertThat(a.getProvider(TestAspects.RuleInfo.class).getData())
+    assertThat(a.getProvider(RuleInfo.class).getData())
         .containsExactly("aspect //a:b", "rule //a:a");
   }
 
@@ -193,10 +205,67 @@ public class AspectTest extends AnalysisTestCase {
         "honest(name='b', foo=[])");
 
     ConfiguredTarget a = getConfiguredTarget("//a:a");
-    assertThat(a.getProvider(TestAspects.RuleInfo.class).getData())
+    assertThat(a.getProvider(RuleInfo.class).getData())
         .containsExactly("rule //a:a", "aspect //a:b data hello");
   }
 
+  /**
+   * Rule definitions to be used in emptyAspectAttributesAreAvailableInRuleContext().
+   */
+  public static class EmptyAspectAttributesAreAvailableInRuleContext {
+    public static class TestRule implements RuleDefinition {
+      @Override
+      public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment environment) {
+        return builder
+            .add(attr("foo", LABEL_LIST).legacyAllowAnyFileType()
+                .aspect(AspectWithEmptyLateBoundAttribute.class))
+            .build();
+      }
+
+      @Override
+      public Metadata getMetadata() {
+        return RuleDefinition.Metadata.builder().name("testrule")
+            .factoryClass(DummyRuleFactory.class).ancestors(BaseRule.class).build();
+      }
+    }
+
+    public static class AspectWithEmptyLateBoundAttribute implements ConfiguredNativeAspectFactory {
+      @Override
+      public AspectDefinition getDefinition(AspectParameters params) {
+        return new AspectDefinition.Builder("testaspect")
+            .add(attr(":late", LABEL).value(EMPTY_LATE_BOUND_LABEL)).build();
+      }
+
+      @Override
+      public Aspect create(ConfiguredTarget base,
+          RuleContext ruleContext, AspectParameters parameters) throws InterruptedException {
+        Object lateBoundPrereq = ruleContext.getPrerequisite(":late", TARGET);
+        return new Aspect.Builder("testaspect")
+            .addProvider(
+                new AspectInfo(NestedSetBuilder.create(
+                    Order.STABLE_ORDER, lateBoundPrereq != null ? "non-empty" : "empty")))
+            .build();
+      }
+    }
+  }
+
+  /**
+   * An Aspect has a late-bound attribute with no value (that is, a LateBoundLabel whose
+   * getDefault() returns `null`).
+   * Test that this attribute is available in the RuleContext which is provided to the Aspect's
+   * `create()` method.
+   */
+  @Test
+  public void emptyAspectAttributesAreAvailableInRuleContext() throws Exception {
+    setRules(new TestAspects.BaseRule(),
+        new EmptyAspectAttributesAreAvailableInRuleContext.TestRule());
+    pkg("a",
+        "testrule(name='a', foo=[':b'])",
+        "testrule(name='b')");
+    ConfiguredTarget a = getConfiguredTarget("//a:a");
+    assertThat(a.getProvider(RuleInfo.class).getData()).contains("empty");
+  }
+
   @RunWith(JUnit4.class)
   public static class AspectTestWithoutLoading extends AspectTest {
     @Override
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java b/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java
index 0551f11058..0a67c921b1 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java
@@ -46,7 +46,7 @@ import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.packages.AspectDefinition;
 import com.google.devtools.build.lib.packages.AspectParameters;
-import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.Attribute.LateBoundLabel;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.packages.RuleClass.Builder;
@@ -62,6 +62,14 @@ import java.util.List;
  * and {@link com.google.devtools.build.lib.analysis.AspectTest}.
  */
 public class TestAspects {
+
+  public static final LateBoundLabel EMPTY_LATE_BOUND_LABEL = new LateBoundLabel<Object>() {
+    @Override
+    public Label getDefault(Rule rule, Object configuration) {
+      return null;
+    }
+  };
+
   /**
    * A transitive info provider for collecting aspects in the transitive closure. Created by
    * aspects.
@@ -233,8 +241,7 @@ public class TestAspects {
       ImmutableCollection<String> baz = aspectParameters.getAttribute("baz");
       if (baz != null) {
         try {
-          builder.add(
-              Attribute.attr("$dep", LABEL).value(Label.parseAbsolute(baz.iterator().next())));
+          builder.add(attr("$dep", LABEL).value(Label.parseAbsolute(baz.iterator().next())));
         } catch (LabelSyntaxException e) {
           throw new IllegalStateException();
         }