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 bazelbuild#19922.

PiperOrigin-RevId: 669996144
Change-Id: Id05e7a53baed1cc704e807695a1fc93a519a2f5b

src/main/java/com/google/devtools/build/lib/packages/Attribute.java

@@ -1774,7 +1774,7 @@ public static LabelListLateBoundDefault<Void> 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<AttributeTransitionData> 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;
  }

src/main/java/com/google/devtools/build/lib/packages/MacroClass.java

@@ -196,7 +196,8 @@ private MacroInstance instantiateMacro(Package.Builder pkgBuilder, Map<String, O
  }
 
  // Setting an attr to None is the same as omitting it (except that it's still an error to set
- // an unknown attr to None).
+ // an unknown attr to None). If the attr is optional, skip adding it to the map now but put it
+ // in below when we realize it's missing.
  if (value == Starlark.NONE) {
  continue;
  }
@@ -221,26 +222,35 @@ private MacroInstance instantiateMacro(Package.Builder pkgBuilder, Map<String, O
  } else {
  // Already validated at schema creation time that the default is not a computed default or
  // late-bound default
- attrValues.put(attr.getName(), attr.getDefaultValueUnchecked());
+ Object defaultValue = attr.getDefaultValueUnchecked();
+ if (defaultValue == null) {
+ // Null values can occur for some types of attributes (e.g. LabelType).
+ defaultValue = Starlark.NONE;
+ }
+ attrValues.put(attr.getName(), defaultValue);
  }
  }
 
  // Normalize and validate all attr values. (E.g., convert strings to labels, fail if bool was
  // passed instead of label, ensure values are immutable.)
  for (Map.Entry<String, Object> 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.

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();
 
  /**

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",

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(