Don't select()-promote None values (#24368)

Merge fix for #24066 into 8.0.0.

Closes tracking issue #24320.

I'm not sure why the automatic merge failed but it works locally, so
here goes.

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

@@ -270,13 +270,15 @@ private MacroInstance instantiateMacro(Package.Builder pkgBuilder, Map<String, O
             "missing value for mandatory attribute '%s' in '%s' macro", attr.getName(), name);
       } else {
         Object defaultValue = attr.getDefaultValueUnchecked();
-        if (defaultValue == null || forceDefaultToNone(attr)) {
+        // For attributes defined directly in this macro's `attrs` param, we've already validated
+        // that the default is not a computed default, late-bound default, etc. But these may still
+        // appear in inherited attributes. Those should be replaced by None as the default.
+        //
+        // We may also see a null default for LabelType, which also should be replaced by None.
+        //
+        // TODO(arostovtsev): All inherited attributes should be forced to have None defaults.
+        if (defaultValue == null || shouldForceDefaultToNone(attr)) {
           // Set the default value as None if:
-          // 1. The native attribute value is null (e.g. LabelType); or
-          // 2. The attribute is an inherited non-Starlark-defined attribute and with a non-direct
-          // default or is a legacy native type.
-          // Note that for Starlark-defined attributes, we already validated at schema creation time
-          // that the default is not a computed default or late-bound default.
           defaultValue = Starlark.NONE;
         }
         attrValues.put(attr.getName(), defaultValue);
@@ -285,14 +287,14 @@ private MacroInstance instantiateMacro(Package.Builder pkgBuilder, Map<String, O
 
     // Normalize and validate all attr values. (E.g., convert strings to labels, promote
     // configurable attribute values to select()s, fail if bool was passed instead of label, ensure
-    // values are immutable.) This applies to default values, even Nones (default value of
-    // LabelType).
+    // values are immutable.) Note that this applies even to default values, although as a special
+    // case None is never promoted to select().
     for (Map.Entry<String, Object> entry : ImmutableMap.copyOf(attrValues).entrySet()) {
       String attrName = entry.getKey();
       Object value = entry.getValue();
       Attribute attribute = attributes.get(attrName);
-      if (value.equals(Starlark.NONE) && forceDefaultToNone(attribute)) {
-        // Skip normalization, since None may violate the attribute's type checking.
+      if (value.equals(Starlark.NONE)) {
+        // Don't promote None to select({"//conditions:default": None}).
         continue;
       }
       Object normalizedValue =
@@ -321,15 +323,14 @@ private MacroInstance instantiateMacro(Package.Builder pkgBuilder, Map<String, O
   }
 
   /**
-   * Returns true if the given inherited attribute should be forced to have a default value of
-   * {@code None}, skipping usual normalization.
+   * Returns true if the given attribute's default value should be considered {@code None}.
    *
    * <p>This is the case for non-direct defaults and legacy licenses and distribs attributes,
    * because None may (depending on attribute type) violate type checking - and that is ok, since
    * the macro implementation will pass the None to the rule function, which will then set the
    * default as expected.
    */
-  private static boolean forceDefaultToNone(Attribute attr) {
+  private static boolean shouldForceDefaultToNone(Attribute attr) {
     return attr.hasComputedDefault()
         || attr.isLateBound()
         || attr.isMaterializing()

src/test/java/com/google/devtools/build/lib/analysis/SymbolicMacroTest.java

@@ -803,16 +803,19 @@ public void hardcodedDefaultAttrValue_isUsedWhenNotOverriddenAndAttrHasNoUserSpe
     scratch.file(
         "pkg/foo.bzl",
         """
-        def _impl(name, visibility, dep_nonconfigurable, dep_configurable):
+        def _impl(name, visibility, dep_nonconfigurable, dep_configurable, xyz_configurable):
             print("dep_nonconfigurable is %s" % dep_nonconfigurable)
             print("dep_configurable is %s" % dep_configurable)
+            print("xyz_configurable is %s" % xyz_configurable)
         my_macro = macro(
             implementation = _impl,
             attrs = {
               # Test label type, since LabelType#getDefaultValue returns null.
               "dep_nonconfigurable": attr.label(configurable=False),
-              # Try it again, this time in a select().
-              "dep_configurable": attr.label(configurable=True),
+              # Try it again, this time configurable. Select()-promotion doesn't apply to None.
+              "dep_configurable": attr.label(),
+              # Now do it for a value besides None. Select()-promotion applies.
+              "xyz_configurable": attr.string(),
             },
         )
         """);
@@ -826,7 +829,8 @@ def _impl(name, visibility, dep_nonconfigurable, dep_configurable):
     Package pkg = getPackage("pkg");
     assertPackageNotInError(pkg);
     assertContainsEvent("dep_nonconfigurable is None");
-    assertContainsEvent("dep_configurable is select({\"//conditions:default\": None})");
+    assertContainsEvent("dep_configurable is None");
+    assertContainsEvent("xyz_configurable is select({\"//conditions:default\": \"\"})");
   }
 
   @Test
@@ -1195,17 +1199,21 @@ public void configurableAttrValuesArePromotedToSelects() throws Exception {
     scratch.file(
         "pkg/foo.bzl",
         """
-        def _impl(name, visibility, configurable_xyz, nonconfigurable_xyz):
+        def _impl(name, visibility,
+                  configurable_xyz, nonconfigurable_xyz, configurable_default_xyz):
             print("configurable_xyz is '%s' (type %s)" %
                 (str(configurable_xyz), type(configurable_xyz)))
             print("nonconfigurable_xyz is '%s' (type %s)" %
                     (str(nonconfigurable_xyz), type(nonconfigurable_xyz)))
+            print("configurable_default_xyz is '%s' (type %s)" %
+                (str(configurable_default_xyz), type(configurable_default_xyz)))
 
         my_macro = macro(
             implementation = _impl,
             attrs = {
               "configurable_xyz": attr.string(),
               "nonconfigurable_xyz": attr.string(configurable=False),
+              "configurable_default_xyz": attr.string(default = "xyz"),
             },
         )
         """);
@@ -1217,6 +1225,7 @@ def _impl(name, visibility, configurable_xyz, nonconfigurable_xyz):
             name = "abc",
             configurable_xyz = "configurable",
             nonconfigurable_xyz = "nonconfigurable",
+            # configurable_default_xyz not set
         )
         """);
 
@@ -1225,6 +1234,8 @@ def _impl(name, visibility, configurable_xyz, nonconfigurable_xyz):
     assertContainsEvent(
         "configurable_xyz is 'select({\"//conditions:default\": \"configurable\"})' (type select)");
     assertContainsEvent("nonconfigurable_xyz is 'nonconfigurable' (type string)");
+    assertContainsEvent(
+        "configurable_default_xyz is 'select({\"//conditions:default\": \"xyz\"})' (type select)");
   }
 
   @Test
@@ -1330,7 +1341,7 @@ def _impl(name, visibility, **kwargs):
                 "//Q:cond2": None,
                 "//conditions:default": "//D:D",
             }),
-            configurable_withdefault = select({"//conditions:default": None}),
+            configurable_withdefault = select({"//Q:cond": "//E:E", "//conditions:default": None}),
             visibility = ["//common:my_package_group"],
         )
         """);
@@ -1350,6 +1361,7 @@ def _impl(name, visibility, **kwargs):
             "//C:C",
             // //Q:cond2 maps to default, which doesn't exist for that attr
             "//D:D",
+            "//E:E",
             "//common:configurable_withdefault", // from attr default
             // `omitted` ignored, it has no default
             // `_implicit_default` ignored because it's implicit