Implement Skylark aspects originating from Skylark rules.

--
MOS_MIGRATED_REVID=108777120

5 files changed, 228 insertions, 42 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
index c2b3eea933..9898294377 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
@@ -725,11 +725,36 @@ public final class Attribute implements Comparable<Attribute> {
      */
     public <T extends NativeAspectFactory> Builder<TYPE> aspect(
         Class<T> aspect, Function<Rule, AspectParameters> evaluator) {
-      this.aspects.add(new RuleAspect(new NativeAspectClass<T>(aspect), evaluator));
+      return this.aspect(new NativeAspectClass<T>(aspect), evaluator);
+    }
+
+    /**
+     * Asserts that a particular parameterized aspect probably needs to be computed for all direct
+     * dependencies through this attribute.
+     *
+     * @param evaluator function that extracts aspect parameters from rule.
+     */
+    public Builder<TYPE> aspect(AspectClass aspect, Function<Rule, AspectParameters> evaluator) {
+      this.aspects.add(new RuleAspect(aspect, evaluator));
       return this;
     }
 
     /**
+     * Asserts that a particular parameterized aspect probably needs to be computed for all direct
+     * dependencies through this attribute.
+     */
+    public Builder<TYPE> aspect(AspectClass aspect) {
+      Function<Rule, AspectParameters> noParameters =
+          new Function<Rule, AspectParameters>() {
+            @Override
+            public AspectParameters apply(Rule input) {
+              return AspectParameters.EMPTY;
+            }
+          };
+      return this.aspect(aspect, noParameters);
+    }
+
+    /**
      * Sets the predicate-like edge validity checker.
      */
     public Builder<TYPE> validityPredicate(ValidityPredicate validityPredicate) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java
index f445eb6699..58b9386ac7 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java
@@ -14,6 +14,8 @@
 
 package com.google.devtools.build.lib.rules;
 
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.Attribute;
@@ -21,6 +23,7 @@ import com.google.devtools.build.lib.packages.Attribute.AllowedValueSet;
 import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition;
 import com.google.devtools.build.lib.packages.Attribute.SkylarkLateBound;
 import com.google.devtools.build.lib.packages.BuildType;
+import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions.SkylarkAspect;
 import com.google.devtools.build.lib.syntax.BuiltinFunction;
 import com.google.devtools.build.lib.syntax.Environment;
 import com.google.devtools.build.lib.syntax.EvalException;
@@ -68,6 +71,10 @@ public final class SkylarkAttr {
       "which rule targets (name of the classes) are allowed. This is deprecated (kept only for "
           + "compatiblity), use providers instead.";
 
+  private static final String ASPECTS_ARG = "aspects";
+  private static final String ASPECT_ARG_DOC =
+      "aspects that should be applied to dependencies specified by this attribute";
+
   private static final String CONFIGURATION_ARG = "cfg";
   private static final String CONFIGURATION_DOC =
       "configuration of the attribute. " + "For example, use DATA_CFG or HOST_CFG.";
@@ -184,7 +191,7 @@ public final class SkylarkAttr {
     return builder;
   }
 
-  private static Descriptor createAttribute(
+  private static Descriptor createAttrDescriptor(
       Map<String, Object> kwargs, Type<?> type, FuncallExpression ast, Environment env)
       throws EvalException {
     try {
@@ -230,7 +237,7 @@ public final class SkylarkAttr {
             throws EvalException {
           // TODO(bazel-team): Replace literal strings with constants.
           env.checkLoadingPhase("attr.int", ast.getLocation());
-          return createAttribute(
+          return createAttrDescriptor(
               EvalUtils.optionMap(
                   DEFAULT_ARG, defaultInt, MANDATORY_ARG, mandatory, VALUES_ARG, values),
               Type.INTEGER,
@@ -274,7 +281,7 @@ public final class SkylarkAttr {
             Environment env)
             throws EvalException {
           env.checkLoadingPhase("attr.string", ast.getLocation());
-          return createAttribute(
+          return createAttrDescriptor(
               EvalUtils.optionMap(
                   DEFAULT_ARG, defaultString, MANDATORY_ARG, mandatory, VALUES_ARG, values),
               Type.STRING,
@@ -362,7 +369,7 @@ public final class SkylarkAttr {
             Environment env)
             throws EvalException {
           env.checkLoadingPhase("attr.label", ast.getLocation());
-          return createAttribute(
+          return createAttrDescriptor(
               EvalUtils.optionMap(
                   DEFAULT_ARG,
                   defaultO,
@@ -417,7 +424,7 @@ public final class SkylarkAttr {
             Environment env)
             throws EvalException {
           env.checkLoadingPhase("attr.string_list", ast.getLocation());
-          return createAttribute(
+          return createAttrDescriptor(
               EvalUtils.optionMap(
                   DEFAULT_ARG, defaultList, MANDATORY_ARG, mandatory, NON_EMPTY_ARG, nonEmpty),
               Type.STRING_LIST,
@@ -457,7 +464,7 @@ public final class SkylarkAttr {
             Environment env)
             throws EvalException {
           env.checkLoadingPhase("attr.int_list", ast.getLocation());
-          return createAttribute(
+          return createAttrDescriptor(
               EvalUtils.optionMap(
                   DEFAULT_ARG, defaultList, MANDATORY_ARG, mandatory, NON_EMPTY_ARG, nonEmpty),
               Type.INTEGER_LIST,
@@ -522,6 +529,13 @@ public final class SkylarkAttr {
         noneable = true,
         defaultValue = "None",
         doc = CONFIGURATION_DOC
+      ),
+      @Param(
+        name = ASPECTS_ARG,
+        type = SkylarkList.class,
+        generic1 = SkylarkAspect.class,
+        defaultValue = "[]",
+        doc = ASPECT_ARG_DOC
       )
     },
     useAst = true,
@@ -538,31 +552,48 @@ public final class SkylarkAttr {
             Boolean mandatory,
             Boolean nonEmpty,
             Object cfg,
+            SkylarkList aspects,
             FuncallExpression ast,
             Environment env)
             throws EvalException {
           env.checkLoadingPhase("attr.label_list", ast.getLocation());
-          return createAttribute(
-              EvalUtils.optionMap(
-                  DEFAULT_ARG,
-                  defaultList,
-                  ALLOW_FILES_ARG,
-                  allowFiles,
-                  ALLOW_RULES_ARG,
-                  allowRules,
-                  PROVIDERS_ARG,
-                  providers,
-                  FLAGS_ARG,
-                  flags,
-                  MANDATORY_ARG,
-                  mandatory,
-                  NON_EMPTY_ARG,
-                  nonEmpty,
-                  CONFIGURATION_ARG,
-                  cfg),
-              BuildType.LABEL_LIST,
-              ast,
-              env);
+          ImmutableMap<String, Object> kwargs = EvalUtils.optionMap(
+              DEFAULT_ARG, defaultList,
+              ALLOW_FILES_ARG,
+              allowFiles,
+              ALLOW_RULES_ARG,
+              allowRules,
+              PROVIDERS_ARG,
+              providers,
+              FLAGS_ARG,
+              flags,
+              MANDATORY_ARG,
+              mandatory,
+              NON_EMPTY_ARG,
+              nonEmpty,
+              CONFIGURATION_ARG,
+              cfg);
+          try {
+            Attribute.Builder<?> attribute = createAttribute(
+                BuildType.LABEL_LIST, kwargs, ast, env);
+
+            ImmutableList<SkylarkAspect> skylarkAspects = getSkylarkAspects(aspects);
+            return new Descriptor(attribute, skylarkAspects);
+          } catch (ConversionException e) {
+            throw new EvalException(ast.getLocation(), e.getMessage());
+          }
+        }
+
+        protected ImmutableList<SkylarkAspect> getSkylarkAspects(SkylarkList aspects)
+            throws ConversionException {
+          ImmutableList.Builder<SkylarkAspect> aspectBuilder = ImmutableList.builder();
+          for (Object aspect : aspects) {
+            if (!(aspect instanceof SkylarkAspect)) {
+              throw new ConversionException("Expected a list of aspects for 'aspects'");
+            }
+            aspectBuilder.add((SkylarkAspect) aspect);
+          }
+          return aspectBuilder.build();
         }
       };
 
@@ -585,7 +616,7 @@ public final class SkylarkAttr {
             Boolean defaultO, Boolean mandatory, FuncallExpression ast, Environment env)
             throws EvalException {
           env.checkLoadingPhase("attr.bool", ast.getLocation());
-          return createAttribute(
+          return createAttrDescriptor(
               EvalUtils.optionMap(DEFAULT_ARG, defaultO, MANDATORY_ARG, mandatory),
               Type.BOOLEAN,
               ast,
@@ -621,7 +652,7 @@ public final class SkylarkAttr {
             Object defaultO, Boolean mandatory, FuncallExpression ast, Environment env)
             throws EvalException {
           env.checkLoadingPhase("attr.output", ast.getLocation());
-          return createAttribute(
+          return createAttrDescriptor(
               EvalUtils.optionMap(DEFAULT_ARG, defaultO, MANDATORY_ARG, mandatory),
               BuildType.OUTPUT,
               ast,
@@ -662,7 +693,7 @@ public final class SkylarkAttr {
             Environment env)
             throws EvalException {
           env.checkLoadingPhase("attr.output_list", ast.getLocation());
-          return createAttribute(
+          return createAttrDescriptor(
               EvalUtils.optionMap(
                   DEFAULT_ARG, defaultList, MANDATORY_ARG, mandatory, NON_EMPTY_ARG, nonEmpty),
               BuildType.OUTPUT_LIST,
@@ -698,7 +729,7 @@ public final class SkylarkAttr {
             Environment env)
             throws EvalException {
           env.checkLoadingPhase("attr.string_dict", ast.getLocation());
-          return createAttribute(
+          return createAttrDescriptor(
               EvalUtils.optionMap(
                   DEFAULT_ARG, defaultO, MANDATORY_ARG, mandatory, NON_EMPTY_ARG, nonEmpty),
               Type.STRING_DICT,
@@ -734,7 +765,7 @@ public final class SkylarkAttr {
             Environment env)
             throws EvalException {
           env.checkLoadingPhase("attr.string_list_dict", ast.getLocation());
-          return createAttribute(
+          return createAttrDescriptor(
               EvalUtils.optionMap(
                   DEFAULT_ARG, defaultO, MANDATORY_ARG, mandatory, NON_EMPTY_ARG, nonEmpty),
               Type.STRING_LIST_DICT,
@@ -764,7 +795,7 @@ public final class SkylarkAttr {
             Object defaultO, Boolean mandatory, FuncallExpression ast, Environment env)
             throws EvalException {
           env.checkLoadingPhase("attr.license", ast.getLocation());
-          return createAttribute(
+          return createAttrDescriptor(
               EvalUtils.optionMap(DEFAULT_ARG, defaultO, MANDATORY_ARG, mandatory),
               BuildType.LICENSE,
               ast,
@@ -777,14 +808,24 @@ public final class SkylarkAttr {
    */
   public static final class Descriptor {
     private final Attribute.Builder<?> attributeBuilder;
+    private final ImmutableList<SkylarkAspect> aspects;
 
     public Descriptor(Attribute.Builder<?> attributeBuilder) {
+      this(attributeBuilder, ImmutableList.<SkylarkAspect>of());
+    }
+
+    public Descriptor(Attribute.Builder<?> attributeBuilder, ImmutableList<SkylarkAspect> aspects) {
       this.attributeBuilder = attributeBuilder;
+      this.aspects = aspects;
     }
 
     public Attribute.Builder<?> getAttributeBuilder() {
       return attributeBuilder;
     }
+
+    public ImmutableList<SkylarkAspect> getAspects() {
+      return aspects;
+    }
   }
 
   static {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
index 63b6307525..df27c88169 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
@@ -89,6 +89,7 @@ import com.google.devtools.build.lib.util.Pair;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Set;
 import java.util.concurrent.ExecutionException;
 
 /**
@@ -423,8 +424,17 @@ public class SkylarkRuleClassFunctions {
             + "', test rule class names must end with '_test' and other rule classes must not");
       }
       for (Pair<String, SkylarkAttr.Descriptor> attribute : attributes) {
+        SkylarkAttr.Descriptor descriptor = attribute.getSecond();
+        Attribute.Builder<?> attributeBuilder = descriptor.getAttributeBuilder();
+        for (SkylarkAspect skylarkAspect : descriptor.getAspects()) {
+          if (!skylarkAspect.isExported()) {
+            throw new EvalException(definitionLocation,
+                "All aspects applied to rule dependencies must be top-level values");
+          }
+          attributeBuilder.aspect(new SkylarkAspectClass(skylarkAspect));
+        }
         addAttribute(definitionLocation, builder,
-            attribute.getSecond().getAttributeBuilder().build(attribute.getFirst()));
+            descriptor.getAttributeBuilder().build(attribute.getFirst()));
       }
       this.ruleClass = builder.build(ruleClassName);
 
@@ -441,7 +451,25 @@ public class SkylarkRuleClassFunctions {
 
   public static void exportRuleFunctionsAndAspects(Environment env, Label skylarkLabel)
       throws EvalException {
-    for (String name : env.getGlobals().getDirectVariableNames()) {
+    Set<String> globalNames = env.getGlobals().getDirectVariableNames();
+
+    // Export aspects first since rules can depend on aspects.
+    for (String name : globalNames) {
+      Object value;
+      try {
+        value = env.lookup(name);
+      } catch (NoSuchVariableException e) {
+        throw new AssertionError(e);
+      }
+      if (value instanceof SkylarkAspect) {
+        SkylarkAspect skylarkAspect = (SkylarkAspect) value;
+        if (!skylarkAspect.isExported()) {
+          skylarkAspect.export(skylarkLabel, name);
+        }
+      }
+    }
+
+    for (String name : globalNames) {
       try {
         Object value = env.lookup(name);
         if (value instanceof RuleFunction) {
@@ -450,12 +478,6 @@ public class SkylarkRuleClassFunctions {
             function.export(skylarkLabel, name);
           }
         }
-        if (value instanceof SkylarkAspect) {
-          SkylarkAspect skylarkAspect = (SkylarkAspect) value;
-          if (!skylarkAspect.isExported()) {
-            skylarkAspect.export(skylarkLabel, name);
-          }
-        }
       } catch (NoSuchVariableException e) {
         throw new AssertionError(e);
       }
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 b12ccd04a6..b3b6bc99ba 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
@@ -141,6 +141,80 @@ public class SkylarkAspectsTest extends BuildViewTestCase {
         .containsExactly("//test:xxx", "//test:yyy");
   }
 
+
+  public void testAspectsFromSkylarkRules() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "def _impl(target, ctx):",
+        "   s = set([target.label])",
+        "   for i in ctx.attr.deps:",
+        "       s += i.target_labels",
+        "   return struct(target_labels = s)",
+        "def _rule_impl(ctx):",
+        "   s = set([])",
+        "   for i in ctx.attr.attr:",
+        "       s += i.target_labels",
+        "   return struct(rule_deps = s)",
+        "",
+        "MyAspect = aspect(",
+        "   implementation=_impl,",
+        "   attr_aspects=['deps'],",
+        ")",
+        "my_rule = rule(",
+        "   implementation=_rule_impl,",
+        "   attrs = { 'attr' : ",
+        "             attr.label_list(mandatory=True, allow_files=True, aspects = [MyAspect]) },",
+        ")");
+
+    scratch.file(
+        "test/BUILD",
+        "load('/test/aspect', 'my_rule')",
+        "java_library(",
+        "     name = 'yyy',",
+        ")",
+        "my_rule(",
+        "     name = 'xxx',",
+        "     attr = [':yyy'],",
+        ")");
+
+    AnalysisResult analysisResult =
+        update(
+            ImmutableList.of("//test:xxx"),
+            ImmutableList.<String>of(),
+            false,
+            LOADING_PHASE_THREADS,
+            true,
+            new EventBus());
+    assertThat(
+        transform(
+            analysisResult.getTargetsToBuild(),
+            new Function<ConfiguredTarget, String>() {
+              @Nullable
+              @Override
+              public String apply(ConfiguredTarget configuredTarget) {
+                return configuredTarget.getLabel().toString();
+              }
+            }))
+        .containsExactly("//test:xxx");
+    ConfiguredTarget target = analysisResult.getTargetsToBuild().iterator().next();
+    SkylarkProviders skylarkProviders = target.getProvider(SkylarkProviders.class);
+    assertThat(skylarkProviders).isNotNull();
+    Object names = skylarkProviders.getValue("rule_deps");
+    assertThat(names).isInstanceOf(SkylarkNestedSet.class);
+    assertThat(
+        transform(
+            (SkylarkNestedSet) names,
+            new Function<Object, String>() {
+              @Nullable
+              @Override
+              public String apply(Object o) {
+                assertThat(o).isInstanceOf(Label.class);
+                return o.toString();
+              }
+            }))
+        .containsExactly("//test:yyy");
+  }
+
   public void testAspectFailingExecution() throws Exception {
     scratch.file(
         "test/aspect.bzl",
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
index bca93ac205..83ea26a7f6 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
@@ -178,6 +178,30 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase {
   }
 
   @Test
+  public void testLabelListWithAspects() throws Exception {
+    SkylarkAttr.Descriptor attr =
+        (SkylarkAttr.Descriptor) evalRuleClassCode(
+          "def _impl(target, ctx):",
+          "   pass",
+          "my_aspect = aspect(implementation = _impl)",
+          "attr.label_list(aspects = [my_aspect])");
+    Object aspect = ev.lookup("my_aspect");
+    assertThat(aspect).isNotNull();
+    assertThat(attr.getAspects()).containsExactly(aspect);
+  }
+
+  @Test
+  public void testLabelListWithAspectsError() throws Exception {
+    checkErrorContains(
+        "Expected a list of aspects for 'aspects'",
+        "def _impl(target, ctx):",
+        "   pass",
+        "my_aspect = aspect(implementation = _impl)",
+        "attr.label_list(aspects = [my_aspect, 123])"
+    );
+  }
+
+  @Test
   public void testNonLabelAttrWithProviders() throws Exception {
     checkErrorContains(
         "unexpected keyword 'providers' in call to string", "attr.string(providers = ['a'])");