Force the default value of symbolic macro's optional inherited attrib…

…utes to None

This fixes two problems:

* Various attributes (e.g. compatible_with, restricted_to, shard_count,
  genrule's cmd and cmd_bat, etc.) have hard-coded analysis-time behavior in
  Bazel which differs depending on whether the attribute was unset (or set to
  None) or set to any other value (including the attribute's non-None
  default!). Using the original default value for such an inherited attribute
  and passing it to the wrapped rule would in many cases break the build.
* The fact that an attribute was set explicitly is reflected in query proto
  and xml output. In a legacy macro that wraps a rule and passes the bulk of
  them via **kwargs, it is expected that most of the **kwargs will be empty
  and most of the wrapped rule's attributes will thus be not explicitly set.
  We want to preserve the same behavior in symbolic macros.

Working towards #24066

Fixes #24319

PiperOrigin-RevId: 697301269
Change-Id: I47563898c511a1f065d117f51a7a3a227e23260e

src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java

@@ -425,6 +425,12 @@ public MacroFunctionApi macro(
           && attr.isDocumented()
           && !MacroClass.RESERVED_MACRO_ATTR_NAMES.contains(attrName)
           && !attrs.containsKey(attrName)) {
+        // Force the default value of optional inherited attributes to None.
+        if (!attr.isMandatory()
+            && attr.getDefaultValueUnchecked() != null
+            && attr.getDefaultValueUnchecked() != Starlark.NONE) {
+          attr = attr.cloneBuilder().defaultValueNone().build();
+        }
         builder.addAttribute(attr);
       }
     }

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

@@ -710,6 +710,18 @@ public Builder<TYPE> defaultValue(Object defaultValue) throws ConversionExceptio
       return defaultValue(defaultValue, null, null);
     }
 
+    /**
+     * Force the default value to be {@code None}. This method is meant only for usage by symbolic
+     * macro machinery.
+     */
+    @CanIgnoreReturnValue
+    public Builder<TYPE> defaultValueNone() {
+      value = null;
+      valueSet = true;
+      valueSource = AttributeValueSource.DIRECT;
+      return this;
+    }
+
     /** Returns where the value of this attribute comes from. Useful only for Starlark. */
     public AttributeValueSource getValueSource() {
       return valueSource;

src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java

@@ -271,9 +271,6 @@ site of the rule. Such attributes can be assigned a default value (as in
 A rule symbol, macro symbol, or the name of a built-in common attribute list (see below) from which
 the macro should inherit attributes.
 
-<p>If <code>inherit_attrs</code> is set, the macro's implementation function <em>must</em> have a
-<code>**kwargs</code> residual keyword parameter.
-
 <p>If <code>inherit_attrs</code> is set to the string <code>"common"</code>, the macro will inherit
 <a href="/reference/be/common-definitions#common-attributes">common rule attribute definitions</a>
 used by all Starlark rules.
@@ -282,29 +279,41 @@ site of the rule. Such attributes can be assigned a default value (as in
 a global variable in a .bzl file, then such a value has not been registered as a rule or macro
 symbol, and therefore cannot be used for <code>inherit_attrs</code>.
 
-<p>By convention, a macro should pass inherited, non-overridden attributes unchanged to the "main"
-rule or macro symbol which the macro is wrapping. Typically, most inherited attributes will not have
-a parameter in the implementation function's parameter list, and will simply be passed via
-<code>**kwargs</code>. However, it may be convenient for the implementation function to have
-explicit parameters for some inherited attributes (most commonly, <code>tags</code> and
-<code>testonly</code>) if the macro needs to pass those attributes to both "main" and non-"main"
-targets.
-
 <p>The inheritance mechanism works as follows:</p>
 <ol>
   <li>The special <code>name</code> and <code>visibility</code> attributes are never inherited;
   <li>Hidden attributes (ones whose name starts with <code>"_"</code>) are never inherited;
-  <li>The remaining inherited attributes are merged with the <code>attrs</code> dictionary, with
-    the entries in <code>attrs</code> dictionary taking precedence in case of conflicts.
+  <li>Attributes whose names are already defined in the <code>attrs</code> dictionary are never
+    inherited (the entry in <code>attrs</code> takes precedence; note that an entry may be set to
+    <code>None</code> to ensure that no attribute by that name gets defined on the macro);
+  <li>All other attributes are inherited from the rule or macro and effectively merged into the
+    <code>attrs</code> dict.
 </ol>
 
-<p>For example, the following macro inherits all attributes from <code>native.cc_library</code>, except
-for <code>cxxopts</code> (which is removed from the attribute list) and <code>copts</code> (which is
-given a new definition):
+<p>When a non-mandatory attribute is inherited, the default value of the attribute is overridden
+to be <code>None</code>, regardless of what it was specified in the original rule or macro. This
+ensures that when the macro forwards the attribute's value to an instance of the wrapped rule or
+macro &ndash; such as by passing in the unmodified <code>**kwargs</code> &ndash; a value that was
+absent from the outer macro's call will also be absent in the inner rule or macro's call (since
+passing <code>None</code> to an attribute is treated the same as omitting the attribute).
+This is important because omitting an attribute has subtly different semantics from passing
+its apparent default value. In particular, omitted attributes are not shown in some <code>bazel
+query</code> output formats, and computed defaults only execute when the value is omitted. If the
+macro needs to examine or modify an inherited attribute &ndash; for example, to add a value to an
+inherited <code>tags</code> attribute &ndash; you must make sure to handle the <code>None</code>
+case in the macro's implementation function.
+
+<p>For example, the following macro inherits all attributes from <code>native.cc_library</code>,
+except for <code>cxxopts</code> (which is removed from the attribute list) and <code>copts</code>
+(which is given a new definition). It also takes care to checks for the default <code>None</code>
+value of the inherited <code>tags</code> attribute before appending an additional tag.
 
 <pre class="language-python">
-def _my_cc_library_impl(name, visibility, **kwargs):
-    ...
+def _my_cc_library_impl(name, visibility, tags, **kwargs):
+    # Append a tag; tags attr is inherited from native.cc_library, and therefore is None unless
+    # explicitly set by the caller of my_cc_library()
+    my_tags = (tags or []) + ["my_custom_tag"]
+    native.cc_library(name = name, visibility = visibility, tags = my_tags, **kwargs)
 
 my_cc_library = macro(
     implementation = _my_cc_library_impl,
@@ -316,12 +325,17 @@ def _my_cc_library_impl(name, visibility, **kwargs):
 )
 </pre>
 
-<p>Note that a macro may inherit a non-hidden attribute with a computed default (for example,
-<a href="/reference/be/common-definitions#common.testonly"><code>testonly</code></a>); normally,
-macros do not allow attributes with computed defaults. If such an attribute is unset in a macro
-invocation, the value passed to the implementation function will be <code>None</code>, and the
-<code>None</code> may be safely passed on to the corresponding attribute of a rule target, causing
-the rule to compute the default as expected.
+<p>If <code>inherit_attrs</code> is set, the macro's implementation function <em>must</em> have a
+<code>**kwargs</code> residual keyword parameter.
+
+<p>By convention, a macro should pass inherited, non-overridden attributes unchanged to the "main"
+rule or macro symbol which the macro is wrapping. Typically, most inherited attributes will not have
+a parameter in the implementation function's parameter list, and will simply be passed via
+<code>**kwargs</code>. It can be convenient for the implementation function to have explicit
+parameters for some inherited attributes (most commonly, <code>tags</code> and
+<code>testonly</code>) if the macro needs to pass those attributes to both "main" and non-"main"
+targets &ndash; but if the macro also needs to examine or manipulate those attributes, you must take
+care to handle the <code>None</code> default value of non-mandatory inherited attributes.
 """),
         // TODO: #19922 - Make a concepts page for symbolic macros, migrate some details like the
         // list of disallowed APIs to there.

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

@@ -1728,7 +1728,9 @@ public void inheritAttrs_fromAnyNativeRule() throws Exception {
     // wrapping of all builtin rule classes which are accessible from Starlark. We do not test rule
     // classes which are exposed to Starlark via macro wrappers in @_builtins, because Starlark code
     // typically cannot get at the wrapped native rule's rule class symbol from which to inherit
-    // attributes.
+    // attributes. We also do not test rule target instantiation (and thus, do not test whether such
+    // a target would pass analysis) because declaring arbitrary native rule targets is difficult to
+    // automate.
     //
     // This test is expected to fail if:
     // * a native rule adds a mandatory attribute of a type which is not supported by this test's
@@ -1812,6 +1814,47 @@ def _impl(name, visibility, **kwargs):
     }
   }
 
+  @Test
+  public void inheritAttrs_fromGenrule_producesTargetThatPassesAnalysis() throws Exception {
+    // inheritAttrs_fromAnyNativeRule() above is loading-phase only; by contrast, this test verifies
+    // that we can wrap a native rule (in this case, genrule) in a macro with inherit_attrs, and
+    // that the macro-wrapped rule target passes analysis.
+    setBuildLanguageOptions("--experimental_enable_macro_inherit_attrs");
+    scratch.file(
+        "pkg/my_genrule.bzl",
+        """
+def _my_genrule_impl(name, visibility, tags, **kwargs):
+    print("my_genrule: tags = %s" % tags)
+    for k in kwargs:
+        print("my_genrule: kwarg %s = %s" % (k, kwargs[k]))
+    native.genrule(name = name + "_wrapped_genrule", visibility = visibility, **kwargs)
+
+my_genrule = macro(
+    implementation = _my_genrule_impl,
+    inherit_attrs = native.genrule,
+)
+""");
+    scratch.file(
+        "pkg/BUILD",
+        """
+        load(":my_genrule.bzl", "my_genrule")
+        my_genrule(
+            name = "abc",
+            outs = ["out.txt"],
+            cmd = "touch $@",
+        )
+        """);
+
+    Package pkg = getPackage("pkg");
+    assertPackageNotInError(pkg);
+    assertThat(getConfiguredTarget("//pkg:abc_wrapped_genrule")).isNotNull();
+    assertContainsEvent("my_genrule: tags = None"); // Not []
+    assertContainsEvent(
+        "my_genrule: kwarg srcs = None"); // Not select({"//conditions:default": []})
+    assertContainsEvent(
+        "my_genrule: kwarg testonly = None"); // Not select({"//conditions:default": False})
+  }
+
   @Test
   public void inheritAttrs_fromExportedStarlarkRule() throws Exception {
     setBuildLanguageOptions("--experimental_enable_macro_inherit_attrs");
@@ -1895,24 +1938,28 @@ public void inheritAttrs_fromExportedMacro() throws Exception {
     scratch.file(
         "pkg/my_macro.bzl",
         """
-        def _other_macro_impl(name, visibility, **kwargs):
-            pass
+def _other_macro_impl(name, visibility, **kwargs):
+    pass
 
-        _other_macro = macro(
-            implementation = _other_macro_impl,
-            attrs = {
-                "srcs": attr.label_list(),
-            },
-        )
+_other_macro = macro(
+    implementation = _other_macro_impl,
+    attrs = {
+        "srcs": attr.label_list(),
+        "tags": attr.string_list(configurable = False),
+    },
+)
 
-        def _my_macro_impl(name, visibility, **kwargs):
-            _other_macro(name = name + "_other_macro", visibility = visibility, **kwargs)
+def _my_macro_impl(name, visibility, tags, **kwargs):
+    print("my_macro: tags = %s" % tags)
+    for k in kwargs:
+        print("my_macro: kwarg %s = %s" % (k, kwargs[k]))
+    _other_macro(name = name + "_other_macro", visibility = visibility, tags = tags, **kwargs)
 
-        my_macro = macro(
-            implementation = _my_macro_impl,
-            inherit_attrs = _other_macro,
-        )
-        """);
+my_macro = macro(
+    implementation = _my_macro_impl,
+    inherit_attrs = _other_macro,
+)
+""");
     scratch.file(
         "pkg/BUILD",
         """
@@ -1922,7 +1969,9 @@ def _my_macro_impl(name, visibility, **kwargs):
     Package pkg = getPackage("pkg");
     assertPackageNotInError(pkg);
     assertThat(getMacroById(pkg, "abc:1").getMacroClass().getAttributes().keySet())
-        .containsExactly("name", "visibility", "srcs");
+        .containsExactly("name", "visibility", "srcs", "tags");
+    assertContainsEvent("my_macro: tags = None"); // Not []
+    assertContainsEvent("my_macro: kwarg srcs = None"); // Not select({"//conditions:default": []})
   }
 
   @Test

src/test/java/com/google/devtools/build/lib/starlarkdocextract/ModuleInfoExtractorTest.java

@@ -972,6 +972,47 @@ def _my_impl(name):
             );
   }
 
+  @Test
+  public void macroInheritedAttributes() throws Exception {
+    Module module =
+        execWithOptions(
+            ImmutableList.of("--experimental_enable_macro_inherit_attrs"),
+            """
+def _my_rule_impl(ctx):
+    pass
+
+_my_rule = rule(
+    implementation = _my_rule_impl,
+    attrs = {
+       "srcs": attr.label_list(doc = "My rule sources"),
+    },
+)
+
+def _my_macro_impl(name, visibility, srcs, **kwargs):
+    _my_rule(name = name, visibility = visibility, srcs = srcs, **kwargs)
+
+my_macro = macro(
+    inherit_attrs = _my_rule,
+    implementation = _my_macro_impl,
+)
+""");
+    ModuleInfo moduleInfo = getExtractor().extractFrom(module);
+    assertThat(moduleInfo.getMacroInfoList().get(0).getAttributeList())
+        .containsExactly(
+            IMPLICIT_MACRO_NAME_ATTRIBUTE_INFO, // name comes first
+            // TODO(arostovtsev): for macros, we ought to also document the visibility attr
+            AttributeInfo.newBuilder()
+                .setName("srcs")
+                .setType(AttributeType.LABEL_LIST)
+                .setDocString("My rule sources")
+                .setDefaultValue("None") // Default value of inherited attributes is always None
+                .build()
+            // TODO(arostovtsev): currently, non-Starlark-defined attributes don't get documented.
+            // This is a reasonable behavior for rules, but we probably ought to document them in
+            // macros with inherited attributes.
+            );
+  }
+
   @Test
   public void unexportedMacro_notDocumented() throws Exception {
     Module module =