Apply select() promotion to Nones

Previously we did not apply `select()` promotion if the attribute value was `None`, which occurs as the Starlark representation of the default value for certain types (most notably `attr.label()`). But this is inconsistent. The same argument for general `select()` promotion -- that you fail fast and find errors sooner -- applies here. For instance, the macro author might have written code that passes when the argument is `None` but not when it is `select("//conditions:default", None)`.

Fixing this exposed a weakness in `copyAndLiftStarlarkValue()`, that it did not handle Nones correctly. (This was probably why I avoided select() promotion in this case to begin with.)

Work toward bazelbuild#19922.

PiperOrigin-RevId: 680640681
Change-Id: I6983bc3e44d3cf629c40c6014a2873b6c62e8841

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

@@ -247,26 +247,25 @@ private MacroInstance instantiateMacro(Package.Builder pkgBuilder, Map<String, O
       }
     }
 
-    // Normalize and validate all attr values. (E.g., convert strings to labels, fail if bool was
-    // passed instead of label, ensure values are immutable.)
+    // 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).
     for (Map.Entry<String, Object> 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.

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);
   }
 

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

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(