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(