Handle mandatory aspect attributes This CL makes the rules propagating aspects required to specify parameters for aspect attributes marked as mandatory whether they have a default value in the aspect definition or not. PiperOrigin-RevId: 416531611
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java index fa7c661..c60f261 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
@@ -639,9 +639,7 @@ throw Starlark.errorf( "Aspect parameter attribute '%s' must have type 'string'.", nativeName); } - if (!hasDefault) { - requiredParams.add(nativeName); - } else if (attribute.checkAllowedValues()) { + if (hasDefault && attribute.checkAllowedValues()) { PredicateWithMessage<Object> allowed = attribute.getAllowedValues(); Object defaultVal = attribute.getDefaultValue(null); if (!allowed.apply(defaultVal)) { @@ -650,6 +648,9 @@ nativeName, allowed.getErrorReason(defaultVal)); } } + if (!hasDefault || attribute.isMandatory()) { + requiredParams.add(nativeName); + } } else if (!hasDefault) { // Implicit or late bound attribute String starlarkName = "_" + nativeName.substring(1); throw Starlark.errorf("Aspect attribute '%s' has no default value.", starlarkName);
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkDefinedAspectsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkDefinedAspectsTest.java index 086077e..b2e3ed6 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkDefinedAspectsTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkDefinedAspectsTest.java
@@ -7338,6 +7338,203 @@ + " 'string' and use the 'values' restriction."); } + @Test + public void testRuleAspectWithMandatoryParameterNotProvided() throws Exception { + scratch.file( + "test/defs.bzl", + "def _aspect_a_impl(target, ctx):", + " result = ['aspect_a on target {}, p = {}'.", + " format(target.label, ctx.attr.p)]", + " if ctx.rule.attr.dep:", + " result += ctx.rule.attr.dep.aspect_a_result", + " return struct(aspect_a_result = result)", + "", + "aspect_a = aspect(", + " implementation = _aspect_a_impl,", + " attr_aspects = ['dep'],", + " attrs = { 'p' : attr.string(default = 'p_v', values = ['p_v'], mandatory = True) },", + ")", + "", + "def _my_rule_impl(ctx):", + " pass", + "my_rule = rule(", + " implementation = _my_rule_impl,", + " attrs = { 'dep' : attr.label(aspects = [aspect_a]) },", + ")"); + scratch.file( + "test/BUILD", + "load('//test:defs.bzl', 'my_rule')", + "my_rule(", + " name = 'main_target',", + " dep = ':dep_target_1',", + ")", + "my_rule(", + " name = 'dep_target_1',", + " dep = ':dep_target_2',", + ")", + "my_rule(", + " name = 'dep_target_2',", + ")"); + reporter.removeHandler(failFastHandler); + + // This call succeeds if "--keep_going" was passed, which it does in the WithKeepGoing test + // suite. Otherwise, it fails and throws a TargetParsingException. + if (keepGoing()) { + AnalysisResult analysisResult = update("//test:main_target"); + assertThat(analysisResult.hasError()).isTrue(); + } else { + assertThrows(TargetParsingException.class, () -> update("//test:main_target")); + } + assertContainsEvent( + "Aspect //test:defs.bzl%aspect_a requires rule my_rule to specify attribute 'p' with type" + + " string"); + } + + @Test + public void testRuleAspectWithMandatoryParameterProvidedWrongType() throws Exception { + scratch.file( + "test/defs.bzl", + "def _aspect_a_impl(target, ctx):", + " result = ['aspect_a on target {}, p = {}'.", + " format(target.label, ctx.attr.p)]", + " if ctx.rule.attr.dep:", + " result += ctx.rule.attr.dep.aspect_a_result", + " return struct(aspect_a_result = result)", + "", + "aspect_a = aspect(", + " implementation = _aspect_a_impl,", + " attr_aspects = ['dep'],", + " attrs = { 'p' : attr.string(default = 'p_v', values = ['p_v'], mandatory = True) },", + ")", + "", + "def _my_rule_impl(ctx):", + " pass", + "my_rule = rule(", + " implementation = _my_rule_impl,", + " attrs = { 'dep' : attr.label(aspects = [aspect_a]),", + " 'p': attr.int() } ", + ")"); + scratch.file( + "test/BUILD", + "load('//test:defs.bzl', 'my_rule')", + "my_rule(", + " name = 'main_target',", + " dep = ':dep_target_1',", + ")", + "my_rule(", + " name = 'dep_target_1',", + " dep = ':dep_target_2',", + ")", + "my_rule(", + " name = 'dep_target_2',", + ")"); + reporter.removeHandler(failFastHandler); + + // This call succeeds if "--keep_going" was passed, which it does in the WithKeepGoing test + // suite. Otherwise, it fails and throws a TargetParsingException. + if (keepGoing()) { + AnalysisResult analysisResult = update("//test:main_target"); + assertThat(analysisResult.hasError()).isTrue(); + } else { + assertThrows(TargetParsingException.class, () -> update("//test:main_target")); + } + assertContainsEvent( + "Aspect //test:defs.bzl%aspect_a requires rule my_rule to specify attribute 'p' with type" + + " string"); + } + + @Test + public void testRuleAspectWithMandatoryParameter_useRuleDefault() throws Exception { + scratch.file( + "test/defs.bzl", + "def _aspect_a_impl(target, ctx):", + " result = 'aspect_a on target {}, p = {}'.format(target.label, ctx.attr.p)", + " return struct(aspect_a_result = result)", + "", + "aspect_a = aspect(", + " implementation = _aspect_a_impl,", + " attr_aspects = ['dep'],", + " attrs = { 'p' : attr.string(default = 'p_v1', values = ['p_v1', 'p_v2'],", + " mandatory = True) },", + ")", + "", + "def _main_rule_impl(ctx):", + " if ctx.attr.dep:", + " return struct(aspect_a_result = ctx.attr.dep.aspect_a_result)", + " pass", + "main_rule = rule(", + " implementation = _main_rule_impl,", + " attrs = {", + " 'dep': attr.label(aspects = [aspect_a]),", + " 'p' : attr.string(default = 'p_v2'),", + " },", + ")"); + scratch.file( + "test/BUILD", + "load('//test:defs.bzl', 'main_rule')", + "main_rule(", + " name = 'main',", + " dep = ':dep_target',", + ")", + "main_rule(", + " name = 'dep_target',", + ")"); + + AnalysisResult analysisResult = update("//test:main"); + + ConfiguredTarget configuredTarget = + Iterables.getOnlyElement(analysisResult.getTargetsToBuild()); + String aspectAResult = (String) configuredTarget.get("aspect_a_result"); + assertThat(aspectAResult).isEqualTo("aspect_a on target //test:dep_target, p = p_v2"); + } + + @Test + public void testRuleAspectWithMandatoryParameterProvided() throws Exception { + scratch.file( + "test/defs.bzl", + "def _aspect_a_impl(target, ctx):", + " result = 'aspect_a on target {}, p = {}'.format(target.label, ctx.attr.p)", + " return struct(aspect_a_result = result)", + "", + "aspect_a = aspect(", + " implementation = _aspect_a_impl,", + " attr_aspects = ['dep'],", + " attrs = { 'p' : attr.string(default = 'p_v2', values = ['p_v1', 'p_v2'],", + " mandatory = True) },", + ")", + "", + "def _main_rule_impl(ctx):", + " if ctx.attr.dep:", + " return struct(aspect_a_result = ctx.attr.dep.aspect_a_result)", + " pass", + "main_rule = rule(", + " implementation = _main_rule_impl,", + " attrs = {", + " 'dep': attr.label(aspects = [aspect_a]),", + " 'p' : attr.string(mandatory = True),", + " },", + ")"); + scratch.file( + "test/BUILD", + "load('//test:defs.bzl', 'main_rule')", + "main_rule(", + " name = 'main',", + " dep = ':dep_target',", + " p = 'p_v1',", + ")", + "main_rule(", + " name = 'dep_target',", + " p = 'p_v2',", + ")"); + + AnalysisResult analysisResult = update("//test:main"); + + ConfiguredTarget configuredTarget = + Iterables.getOnlyElement(analysisResult.getTargetsToBuild()); + String aspectAResult = (String) configuredTarget.get("aspect_a_result"); + assertThat(aspectAResult).isEqualTo("aspect_a on target //test:dep_target, p = p_v1"); + } + private ConfiguredAspect getConfiguredAspect( Map<AspectKey, ConfiguredAspect> aspectsMap, String aspectName) { for (Map.Entry<AspectKey, ConfiguredAspect> entry : aspectsMap.entrySet()) {