From 100438c8e6d634c558efa079c296351b4ec77bb1 Mon Sep 17 00:00:00 2001 From: Googler Date: Sun, 1 Sep 2024 15:37:16 -0700 Subject: [PATCH] Fix macro default attribute logic Some BuildType attribute types have a universal default value, i.e. a default that applies when the rule author doesn't pass `default=...` to the attribute schema. For example, StringType has "" and IntegerType has 0. Others do not have a default, in particular LabelType, and for these types getDefaultValue() returns null. StarlarkAttributesCollection papers over this by turning null into None. We need to do the same in MacroClass#instantiateMacro so that symbolic macros can handle optional label attributes without a NPE crash. Work toward #19922. PiperOrigin-RevId: 669996144 Change-Id: Id05e7a53baed1cc704e807695a1fc93a519a2f5b --- .../build/lib/packages/Attribute.java | 3 +- .../build/lib/packages/MacroClass.java | 36 ++++++++++++------- .../devtools/build/lib/packages/Type.java | 1 + .../google/devtools/build/lib/analysis/BUILD | 1 + .../build/lib/analysis/SymbolicMacroTest.java | 29 +++++++++++++++ 5 files changed, 56 insertions(+), 14 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 a139c8413af150..8ff682ab12deb0 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 @@ -1774,7 +1774,7 @@ public static LabelListLateBoundDefault fromRuleAndAttributesOnly( // 5. defaultValue instanceof LateBoundDefault && // type.isValid(defaultValue.getDefault(configuration)) // (We assume a hypothetical Type.isValid(Object) predicate.) - private final Object defaultValue; + @Nullable private final Object defaultValue; private final TransitionFactory transitionFactory; @@ -2140,6 +2140,7 @@ public Object getDefaultValue(@Nullable Rule rule) { * Returns the default value of this attribute, even if it is a computed default, or a late-bound * default. */ + @Nullable public Object getDefaultValueUnchecked() { return defaultValue; } 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 75b2973cd4f0e9..11cc69ee969003 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 @@ -196,7 +196,8 @@ private MacroInstance instantiateMacro(Package.Builder pkgBuilder, Map entry : ImmutableMap.copyOf(attrValues).entrySet()) { String attrName = entry.getKey(); - Attribute attribute = attributes.get(attrName); - Object normalizedValue = - // copyAndLiftStarlarkValue ensures immutability. - BuildType.copyAndLiftStarlarkValue( - name, attribute, entry.getValue(), 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); + 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); } - 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 1c21e68c158819..b619ebee5451f8 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 @@ -179,6 +179,7 @@ public final T convertOptional(Object x, String what) throws ConversionException * Returns the default value for this type; may return null iff no default is defined for this * type. */ + @Nullable public abstract T getDefaultValue(); /** 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 1ccd4b5fa99e7a..491bc1e3837b03 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/test/java/com/google/devtools/build/lib/analysis/BUILD @@ -367,6 +367,7 @@ java_test( java_test( name = "SymbolicMacroTest", srcs = ["SymbolicMacroTest.java"], + shard_count = 5, 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 06bfd7de698c0f..dafe7836ce225c 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 @@ -713,6 +713,35 @@ def query(): assertContainsEvent("existing_rules() keys: [\"outer_target\"]"); } + @Test + public void hardcodedDefaultAttrValue_isUsedWhenNotOverriddenAndAttrHasNoUserSpecifiedDefault() + throws Exception { + + scratch.file( + "pkg/foo.bzl", + """ + def _impl(name, dep): + print("dep is %s" % dep) + my_macro = macro( + implementation=_impl, + attrs = { + # Test label type, since LabelType#getDefaultValue returns null. + "dep": attr.label(configurable=False) + }, + ) + """); + scratch.file( + "pkg/BUILD", + """ + load(":foo.bzl", "my_macro") + my_macro(name="abc") + """); + + Package pkg = getPackage("pkg"); + assertPackageNotInError(pkg); + assertContainsEvent("dep is None"); + } + @Test public void defaultAttrValue_isUsedWhenNotOverridden() throws Exception { scratch.file(