diff --git a/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java b/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java index 9a3751f138bc8a..6b0415da8fe243 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java +++ b/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java @@ -247,26 +247,25 @@ private MacroInstance instantiateMacro(Package.Builder pkgBuilder, Map entry : ImmutableMap.copyOf(attrValues).entrySet()) { String attrName = entry.getKey(); Object value = entry.getValue(); - // Skip auto-populated `None`s. They are not type-checked or lifted to select()s. - if (value != Starlark.NONE) { - Attribute attribute = attributes.get(attrName); - Object normalizedValue = - // copyAndLiftStarlarkValue ensures immutability. - BuildType.copyAndLiftStarlarkValue( - name, attribute, value, pkgBuilder.getLabelConverter()); - // TODO(#19922): Validate that LABEL_LIST type attributes don't contain duplicates, to match - // the behavior of rules. This probably requires factoring out logic from - // AggregatingAttributeMapper. - if (attribute.isConfigurable() && !(normalizedValue instanceof SelectorList)) { - normalizedValue = SelectorList.wrapSingleValue(normalizedValue); - } - attrValues.put(attrName, normalizedValue); + Attribute attribute = attributes.get(attrName); + Object normalizedValue = + // copyAndLiftStarlarkValue ensures immutability. + BuildType.copyAndLiftStarlarkValue( + name, attribute, value, pkgBuilder.getLabelConverter()); + // TODO(#19922): Validate that LABEL_LIST type attributes don't contain duplicates, to match + // the behavior of rules. This probably requires factoring out logic from + // AggregatingAttributeMapper. + if (attribute.isConfigurable() && !(normalizedValue instanceof SelectorList)) { + normalizedValue = SelectorList.wrapSingleValue(normalizedValue); } + attrValues.put(attrName, normalizedValue); } // Type and existence enforced by RuleClass.NAME_ATTRIBUTE. diff --git a/src/main/java/com/google/devtools/build/lib/packages/Type.java b/src/main/java/com/google/devtools/build/lib/packages/Type.java index b743202d288b59..36e496f2490689 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Type.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Type.java @@ -123,6 +123,11 @@ public abstract T convert(Object x, Object what, @Nullable LabelConverter labelC */ public Object copyAndLiftStarlarkValue( Object x, Object what, @Nullable LabelConverter labelConverter) throws ConversionException { + // Nones are valid as the Starlark representation of the internal null value (used for certain + // types' default values). + if (x == Starlark.NONE) { + return x; + } return convert(x, what, labelConverter); } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/BUILD index c8574d5ffa5a9d..e105c5346b1088 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/test/java/com/google/devtools/build/lib/analysis/BUILD @@ -368,7 +368,7 @@ java_test( java_test( name = "SymbolicMacroTest", srcs = ["SymbolicMacroTest.java"], - shard_count = 8, + shard_count = 12, deps = [ "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", "//src/main/java/com/google/devtools/build/lib/analysis:test/analysis_failure", diff --git a/src/test/java/com/google/devtools/build/lib/analysis/SymbolicMacroTest.java b/src/test/java/com/google/devtools/build/lib/analysis/SymbolicMacroTest.java index bd01ce9426ac2f..34ff471868ded2 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/SymbolicMacroTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/SymbolicMacroTest.java @@ -24,6 +24,7 @@ import com.google.devtools.build.lib.packages.MacroInstance; import com.google.devtools.build.lib.packages.NoSuchPackageException; import com.google.devtools.build.lib.packages.Package; +import com.google.devtools.build.lib.packages.Rule; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.testing.junit.testparameterinjector.TestParameterInjector; import com.google.testing.junit.testparameterinjector.TestParameters; @@ -40,7 +41,9 @@ public final class SymbolicMacroTest extends BuildViewTestCase { @Before public void setUp() throws Exception { - setBuildLanguageOptions("--experimental_enable_first_class_macros"); + setBuildLanguageOptions( + "--experimental_enable_first_class_macros", + "--incompatible_simplify_unconditional_selects_in_rule_attrs"); } /** @@ -724,13 +727,16 @@ public void hardcodedDefaultAttrValue_isUsedWhenNotOverriddenAndAttrHasNoUserSpe scratch.file( "pkg/foo.bzl", """ - def _impl(name, visibility, dep): - print("dep is %s" % dep) + def _impl(name, visibility, dep_nonconfigurable, dep_configurable): + print("dep_nonconfigurable is %s" % dep_nonconfigurable) + print("dep_configurable is %s" % dep_configurable) my_macro = macro( implementation=_impl, attrs = { # Test label type, since LabelType#getDefaultValue returns null. - "dep": attr.label(configurable=False) + "dep_nonconfigurable": attr.label(configurable=False), + # Try it again, this time in a select(). + "dep_configurable": attr.label(configurable=True), }, ) """); @@ -743,7 +749,8 @@ def _impl(name, visibility, dep): Package pkg = getPackage("pkg"); assertPackageNotInError(pkg); - assertContainsEvent("dep is None"); + assertContainsEvent("dep_nonconfigurable is None"); + assertContainsEvent("dep_configurable is select({\"//conditions:default\": None})"); } @Test @@ -827,6 +834,42 @@ def _impl(name, visibility, _xyz): assertContainsEvent("xyz is IMPLICIT"); } + @Test + public void defaultAttrValue_wrappingMacroTakesPrecedenceOverWrappedRule() throws Exception { + scratch.file( + "pkg/foo.bzl", + """ + def _rule_impl(ctx): + pass + + my_rule = rule( + implementation = _rule_impl, + attrs = {"dep": attr.label(default="//common:rule_default")}, + ) + + def _macro_impl(name, visibility, dep): + my_rule(name = name, dep = dep) + + my_macro = macro( + implementation = _macro_impl, + attrs = {"dep": attr.label(default="//common:macro_default")}, + ) + """); + scratch.file( + "pkg/BUILD", + """ + load(":foo.bzl", "my_macro") + my_macro(name="abc") + """); + + Package pkg = getPackage("pkg"); + assertPackageNotInError(pkg); + Rule rule = pkg.getRule("abc"); + assertThat(rule).isNotNull(); + assertThat(rule.getAttr("dep")) + .isEqualTo(Label.parseCanonicalUnchecked("//common:macro_default")); + } + @Test public void noneAttrValue_doesNotOverrideDefault() throws Exception { scratch.file(