Expose parameterized aspects to Skylark.

There are no syntactic changes within Skylark; the only difference is that aspects may have non-implicit attributes, so long as they have type 'string' and use the 'values' restriction. Such aspects may only be requested by rules which have attributes with types and names matching the non-implicit, non-defaulted attributes of the aspect.

This is not yet a complete match for native AspectParameters functionality since implicit attributes cannot yet be affected by attribute values, but that will be added later.

Implicit aspects are still required to have default values. Non-implicit aspect attributes are considered "required" unless they have a default value. An error will occur if they are applied to a rule that does not "cover" all required attributes by having attributes of matching name and type. While non-implicit aspect attributes with a default are not required, they will still take on the value of a rule attribute with the same name and type if it is present.

Aspects with non-implicit, non-defaulted ("required") attributes cannot be requested on the command line, only by a rule.

RELNOTES: Expose parameterized aspects to Skylark.

--
MOS_MIGRATED_REVID=121711715

2 files changed, 256 insertions, 9 deletions

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 4d0514873f..35514449e1 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
@@ -532,6 +532,215 @@ public class SkylarkAspectsTest extends AnalysisTestCase {
   }
 
   @Test
+  public void testAspectParametersUncovered() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "def _impl(target, ctx):",
+        "   return struct()",
+        "def _rule_impl(ctx):",
+        "   return struct()",
+        "MyAspectUncovered = aspect(",
+        "    implementation=_impl,",
+        "    attrs = { 'my_attr' : attr.string(values=['aaa']) },",
+        ")",
+        "my_rule = rule(",
+        "    implementation=_rule_impl,",
+        "    attrs = { 'deps' : attr.label_list(aspects=[MyAspectUncovered]) },",
+        ")");
+    scratch.file("test/BUILD",
+        "load('//test:aspect.bzl', 'my_rule')",
+        "my_rule(name = 'xxx')");
+
+    reporter.removeHandler(failFastHandler);
+    try {
+      AnalysisResult result = update(ImmutableList.<String>of(), "//test:xxx");
+      assertThat(keepGoing()).isTrue();
+      assertThat(result.hasError()).isTrue();
+    } catch (Exception e) {
+      // expect to fail.
+    }
+    assertContainsEvent(//"ERROR /workspace/test/aspect.bzl:9:11: "
+        "Aspect //test:aspect.bzl%MyAspectUncovered requires rule my_rule to specify attribute "
+        + "'my_attr' with type string.");
+  }
+
+  @Test
+  public void testAspectParametersTypeMismatch() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "def _impl(target, ctx):",
+        "   return struct()",
+        "def _rule_impl(ctx):",
+        "   return struct()",
+        "MyAspectMismatch = aspect(",
+        "    implementation=_impl,",
+        "    attrs = { 'my_attr' : attr.string(values=['aaa']) },",
+        ")",
+        "my_rule = rule(",
+        "    implementation=_rule_impl,",
+        "    attrs = { 'deps' : attr.label_list(aspects=[MyAspectMismatch]),",
+        "              'my_attr' : attr.int() },",
+       ")");
+    scratch.file("test/BUILD",
+        "load('//test:aspect.bzl', 'my_rule')",
+        "my_rule(name = 'xxx', my_attr = 4)");
+
+    reporter.removeHandler(failFastHandler);
+    try {
+      AnalysisResult result = update(ImmutableList.<String>of(), "//test:xxx");
+      assertThat(keepGoing()).isTrue();
+      assertThat(result.hasError()).isTrue();
+    } catch (Exception e) {
+      // expect to fail.
+    }
+    assertContainsEvent(
+        "Aspect //test:aspect.bzl%MyAspectMismatch requires rule my_rule to specify attribute "
+        + "'my_attr' with type string.");
+  }
+
+  @Test
+  public void testAspectParametersBadDefault() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "def _impl(target, ctx):",
+        "   return struct()",
+        "def _rule_impl(ctx):",
+        "   return struct()",
+        "MyAspectBadDefault = aspect(",
+        "    implementation=_impl,",
+        "    attrs = { 'my_attr' : attr.string(values=['a'], default='b') },",
+        ")",
+        "my_rule = rule(",
+        "    implementation=_rule_impl,",
+        "    attrs = { 'deps' : attr.label_list(aspects=[MyAspectBadDefault]) },",
+        ")");
+    scratch.file("test/BUILD",
+        "load('//test:aspect.bzl', 'my_rule')",
+        "my_rule(name = 'xxx')");
+
+    reporter.removeHandler(failFastHandler);
+    try {
+      AnalysisResult result = update(ImmutableList.<String>of(), "//test:xxx");
+      assertThat(keepGoing()).isTrue();
+      assertThat(result.hasError()).isTrue();
+    } catch (Exception e) {
+      // expect to fail.
+    }
+    assertContainsEvent("ERROR /workspace/test/aspect.bzl:5:22: "
+        + "Aspect parameter attribute 'my_attr' has a bad default value: has to be one of 'a' "
+        + "instead of 'b'");
+  }
+
+  @Test
+  public void testAspectParametersBadValue() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "def _impl(target, ctx):",
+        "   return struct()",
+        "def _rule_impl(ctx):",
+        "   return struct()",
+        "MyAspectBadValue = aspect(",
+        "    implementation=_impl,",
+        "    attrs = { 'my_attr' : attr.string(values=['a']) },",
+        ")",
+        "my_rule = rule(",
+        "    implementation=_rule_impl,",
+        "    attrs = { 'deps' : attr.label_list(aspects=[MyAspectBadValue]),",
+        "              'my_attr' : attr.string() },",
+        ")");
+    scratch.file("test/BUILD",
+        "load('//test:aspect.bzl', 'my_rule')",
+        "my_rule(name = 'xxx', my_attr='b')");
+
+    reporter.removeHandler(failFastHandler);
+    try {
+      AnalysisResult result = update(ImmutableList.<String>of(), "//test:xxx");
+      assertThat(keepGoing()).isTrue();
+      assertThat(result.hasError()).isTrue();
+    } catch (Exception e) {
+      // expect to fail.
+    }
+    assertContainsEvent("ERROR /workspace/test/BUILD:2:1: //test:xxx: invalid value in 'my_attr' "
+        + "attribute: has to be one of 'a' instead of 'b'");
+  }
+
+  @Test
+  public void testAspectParameters() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "def _impl(target, ctx):",
+        "   return struct()",
+        "def _rule_impl(ctx):",
+        "   return struct()",
+        "MyAspect = aspect(",
+        "    implementation=_impl,",
+        "    attrs = { 'my_attr' : attr.string(values=['aaa']) },",
+        ")",
+        "my_rule = rule(",
+        "    implementation=_rule_impl,",
+        "    attrs = { 'deps' : attr.label_list(aspects=[MyAspect]),",
+        "              'my_attr' : attr.string() },",
+        ")");
+    scratch.file("test/BUILD",
+        "load('//test:aspect.bzl', 'my_rule')",
+        "my_rule(name = 'xxx', my_attr = 'aaa')");
+
+    AnalysisResult result = update(ImmutableList.<String>of(), "//test:xxx");
+    assertThat(result.hasError()).isFalse();
+  }
+
+  @Test
+  public void testAspectParametersOptional() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "def _impl(target, ctx):",
+        "   return struct()",
+        "def _rule_impl(ctx):",
+        "   return struct()",
+        "MyAspectOptParam = aspect(",
+        "    implementation=_impl,",
+        "    attrs = { 'my_attr' : attr.string(values=['aaa'], default='aaa') },",
+        ")",
+        "my_rule = rule(",
+        "    implementation=_rule_impl,",
+        "    attrs = { 'deps' : attr.label_list(aspects=[MyAspectOptParam]) },",
+        ")");
+    scratch.file("test/BUILD",
+        "load('//test:aspect.bzl', 'my_rule')",
+        "my_rule(name = 'xxx')");
+
+    AnalysisResult result = update(ImmutableList.<String>of(), "//test:xxx");
+    assertThat(result.hasError()).isFalse();
+  }
+
+  @Test
+  public void testAspectParametersOptionalOverride() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "def _impl(target, ctx):",
+        "   if (ctx.attr.my_attr == 'a'):",
+        "       fail('Rule is not overriding default, still has value ' + ctx.attr.my_attr)",
+        "   return struct()",
+        "def _rule_impl(ctx):",
+        "   return struct()",
+        "MyAspectOptOverride = aspect(",
+        "    implementation=_impl,",
+        "    attrs = { 'my_attr' : attr.string(values=['a', 'b'], default='a') },",
+        ")",
+        "my_rule = rule(",
+        "    implementation=_rule_impl,",
+        "    attrs = { 'deps' : attr.label_list(aspects=[MyAspectOptOverride]),",
+        "              'my_attr' : attr.string() },",
+        ")");
+    scratch.file("test/BUILD",
+        "load('//test:aspect.bzl', 'my_rule')",
+        "my_rule(name = 'xxx', my_attr = 'b')");
+
+    AnalysisResult result = update(ImmutableList.<String>of(), "//test:xxx");
+    assertThat(result.hasError()).isFalse();
+  }
+
+  @Test
   public void multipleExecutablesInTarget() throws Exception {
     scratch.file("foo/extension.bzl",
         "def _aspect_impl(target, ctx):",
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 81636b1aeb..50bd25ac56 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
@@ -32,17 +32,15 @@ import com.google.devtools.build.lib.packages.ImplicitOutputsFunction;
 import com.google.devtools.build.lib.packages.PredicateWithMessage;
 import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
+import com.google.devtools.build.lib.packages.SkylarkAspect;
 import com.google.devtools.build.lib.rules.SkylarkAttr;
-import com.google.devtools.build.lib.rules.SkylarkAttr.Descriptor;
 import com.google.devtools.build.lib.rules.SkylarkFileType;
 import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions;
 import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions.RuleFunction;
-import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions.SkylarkAspect;
 import com.google.devtools.build.lib.skylark.util.SkylarkTestCase;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.util.FileTypeSet;
-import com.google.devtools.build.lib.util.Pair;
 
 import org.junit.Assert;
 import org.junit.Before;
@@ -237,24 +235,64 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase {
         "   attrs = { '_extra_deps' : attr.label(default = Label('//foo/bar:baz')) }",
         ")");
     SkylarkAspect aspect = (SkylarkAspect) ev.lookup("my_aspect");
-    Pair<String, Descriptor> pair = Iterables.getOnlyElement(aspect.getAttributes());
-    assertThat(pair.first).isEqualTo("$extra_deps");
-    assertThat(pair.second.getAttributeBuilder().build("$extra_deps").getDefaultValue(null))
+    Attribute attribute = Iterables.getOnlyElement(aspect.getAttributes());
+    assertThat(attribute.getName()).isEqualTo("$extra_deps");
+    assertThat(attribute.getDefaultValue(null))
         .isEqualTo(Label.parseAbsolute("//foo/bar:baz", false));
   }
 
   @Test
-  public void testAspectNonImplicitAttribute() throws Exception {
+  public void testAspectParameter() throws Exception {
+    evalAndExport(
+        "def _impl(target, ctx):",
+        "   pass",
+        "my_aspect = aspect(_impl,",
+        "   attrs = { 'param' : attr.string(values=['a', 'b']) }",
+        ")");
+    SkylarkAspect aspect = (SkylarkAspect) ev.lookup("my_aspect");
+    Attribute attribute = Iterables.getOnlyElement(aspect.getAttributes());
+    assertThat(attribute.getName()).isEqualTo("param");
+  }
+  
+  @Test
+  public void testAspectParameterRequiresValues() throws Exception {
     checkErrorContains(
-        "Aspect attribute 'extra_deps' must be implicit (its name should start with '_')",
+        "Aspect parameter attribute 'param' must have type 'string' and use the 'values' "
+        + "restriction.",
         "def _impl(target, ctx):",
         "   pass",
         "my_aspect = aspect(_impl,",
-        "   attrs = { 'extra_deps' : attr.label(default = Label('//foo/bar:baz')) }",
+        "   attrs = { 'param' : attr.string(default = 'c') }",
         ")");
   }
 
   @Test
+  public void testAspectParameterBadType() throws Exception {
+    checkErrorContains(
+        "Aspect parameter attribute 'param' must have type 'string' and use the 'values' "
+        + "restriction.",
+        "def _impl(target, ctx):",
+        "   pass",
+        "my_aspect = aspect(_impl,",
+        "   attrs = { 'param' : attr.label(default = Label('//foo/bar:baz')) }",
+        ")");
+  }
+
+  @Test
+  public void testAspectParameterAndExtraDeps() throws Exception {
+    evalAndExport(
+        "def _impl(target, ctx):",
+        "   pass",
+        "my_aspect = aspect(_impl,",
+        "   attrs = { 'param' : attr.string(values=['a', 'b']),",
+        "             '_extra' : attr.label(default = Label('//foo/bar:baz')) }",
+        ")");
+    SkylarkAspect aspect = (SkylarkAspect) ev.lookup("my_aspect");
+    assertThat(aspect.getAttributes()).hasSize(2);
+    assertThat(aspect.getParamAttributes()).containsExactly("param");
+  }
+
+  @Test
   public void testAspectNoDefaultValueAttribute() throws Exception {
     checkErrorContains(
         "Aspect attribute '_extra_deps' has no default value",