8000 Force the default value of symbolic macro's optional inherited attrib… · coderabbit-test/bazel@a2f1f58 · GitHub
[go: up one dir, main page]
Skip to content

Commit a2f1f58

Browse files
tetrominocopybara-github
tetromino
authored and
copybara-github
committed
Force the default value of symbolic macro's optional inherited attributes 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 bazelbuild#24066 Fixes bazelbuild#24319 PiperOrigin-RevId: 697301269 Change-Id: I47563898c511a1f065d117f51a7a3a227e23260e
1 parent 09c848f commit a2f1f58

File tree

5 files changed

+163
-41
lines changed

5 files changed

+163
-41
lines changed

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -425,6 +425,12 @@ public MacroFunctionApi macro(
425425
&& attr.isDocumented()
426426
&& !MacroClass.RESERVED_MACRO_ATTR_NAMES.contains(attrName)
427427
&& !attrs.containsKey(attrName)) {
428+
// Force the default value of optional inherited attributes to None.
429+
if (!attr.isMandatory()
430+
&& attr.getDefaultValueUnchecked() != null
431+
&& attr.getDefaultValueUnchecked() != Starlark.NONE) {
432+
attr = attr.cloneBuilder().defaultValueNone().build();
433+
}
428434
builder.addAttribute(attr);
429435
}
430436
}

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -710,6 +710,18 @@ public Builder<TYPE> defaultValue(Object defaultValue) throws ConversionExceptio
710710
return defaultValue(defaultValue, null, null);
711711
}
712712

713+
/**
714+
* Force the default value to be {@code None}. This method is meant only for usage by symbolic
715+
* macro machinery.
716+
*/
717+
@CanIgnoreReturnValue
718+
public Builder<TYPE> defaultValueNone() {
719+
value = null;
720+
valueSet = true;
721+
valueSource = AttributeValueSource.DIRECT;
722+
return this;
723+
}
724+
713725
/** Returns where the value of this attribute comes from. Useful only for Starlark. */
714726
public AttributeValueSource getValueSource() {
715727
return valueSource;

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

Lines changed: 38 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -271,9 +271,6 @@ site of the rule. Such attributes can be assigned a default value (as in
271271
A rule symbol, macro symbol, or the name of a built-in common attribute list (see below) from which
272272
the macro should inherit attributes.
273273
274-
<p>If <code>inherit_attrs</code> is set, the macro's implementation function <em>must</em> have a
275-
<code>**kwargs</code> residual keyword parameter.
276-
277274
<p>If <code>inherit_attrs</code> is set to the string <code>"common"</code>, the macro will inherit
278275
<a href="/reference/be/common-definitions#common-attributes">common rule attribute definitions</a>
279276
used by all Starlark rules.
@@ -282,29 +279,41 @@ site of the rule. Such attributes can be assigned a default value (as in
282279
a global variable in a .bzl file, then such a value has not been registered as a rule or macro
283280
symbol, and therefore cannot be used for <code>inherit_attrs</code>.
284281
285-
<p>By convention, a macro should pass inherited, non-overridden attributes unchanged to the "main"
286-
rule or macro symbol which the macro is wrapping. Typically, most inherited attributes will not have
287-
a parameter in the implementation function's parameter list, and will simply be passed via
288-
<code>**kwargs</code>. However, it may be convenient for the implementation function to have
289-
explicit parameters for some inherited attributes (most commonly, <code>tags</code> and
290-
<code>testonly</code>) if the macro needs to pass those attributes to both "main" and non-"main"
291-
targets.
292-
293282
<p>The inheritance mechanism works as follows:</p>
294283
<ol>
295284
<li>The special <code>name</code> and <code>visibility</code> attributes are never inherited;
296285
<li>Hidden attributes (ones whose name starts with <code>"_"</code>) are never inherited;
297-
<li>The remaining inherited attributes are merged with the <code>attrs</code> dictionary, with
298-
the entries in <code>attrs</code> dictionary taking precedence in case of conflicts.
286+
<li>Attributes whose names are already defined in the <code>attrs</code> dictionary are never
287+
inherited (the entry in <code>attrs</code> takes precedence; note that an entry may be set to
288+
<code>None</code> to ensure that no attribute by that name gets defined on the macro);
289+
<li>All other attributes are inherited from the rule or macro and effectively merged into the
290+
<code>attrs</code> dict.
299291
</ol>
300292
301-
<p>For example, the following macro inherits all attributes from <code>native.cc_library</code>, except
302-
for <code>cxxopts</code> (which is removed from the attribute list) and <code>copts</code> (which is
303-
given a new definition):
293+
<p>When a non-mandatory attribute is inherited, the default value of the attribute is overridden
294+
to be <code>None</code>, regardless of what it was specified in the original rule or macro. This
295+
ensures that when the macro forwards the attribute's value to an instance of the wrapped rule or
296+
macro &ndash; such as by passing in the unmodified <code>**kwargs</code> &ndash; a value that was
297+
absent from the outer macro's call will also be absent in the inner rule or macro's call (since
298+
passing <code>None</code> to an attribute is treated the same as omitting the attribute).
299+
This is important because omitting an attribute has subtly different semantics from passing
300+
its apparent default value. In particular, omitted attributes are not shown in some <code>bazel
301+
query</code> output formats, and computed defaults only execute when the value is omitted. If the
302+
macro needs to examine or modify an inherited attribute &ndash; for example, to add a value to an
303+
inherited <code>tags</code> attribute &ndash; you must make sure to handle the <code>None</code>
304+
case in the macro's implementation function.
305+
306+
<p>For example, the following macro inherits all attributes from <code>native.cc_library</code>,
307+
except for <code>cxxopts</code> (which is removed from the attribute list) and <code>copts</code>
308+
(which is given a new definition). It also takes care to checks for the default <code>None</code>
309+
value of the inherited <code>tags</code> attribute before appending an additional tag.
304310
305311
<pre class="language-python">
306-
def _my_cc_library_impl(name, visibility, **kwargs):
307-
...
312+
def _my_cc_library_impl(name, visibility, tags, **kwargs):
313+
# Append a tag; tags attr is inherited from native.cc_library, and therefore is None unless
314+
# explicitly set by the caller of my_cc_library()
315+
my_tags = (tags or []) + ["my_custom_tag"]
316+
native.cc_library(name = name, visibility = visibility, tags = my_tags, **kwargs)
308317
309318
my_cc_library = macro(
310319
implementation = _my_cc_library_impl,
@@ -316,12 +325,17 @@ def _my_cc_library_impl(name, visibility, **kwargs):
316325
)
317326
</pre>
318327
319-
<p>Note that a macro may inherit a non-hidden attribute with a computed default (for example,
320-
<a href="/reference/be/common-definitions#common.testonly"><code>testonly</code></a>); normally,
321-
macros do not allow attributes with computed defaults. If such an attribute is unset in a macro
322-
invocation, the value passed to the implementation function will be <code>None</code>, and the
323-
<code>None</code> may be safely passed on to the corresponding attribute of a rule target, causing
324-
the rule to compute the default as expected.
328+
<p>If <code>inherit_attrs</code> is set, the macro's implementation function <em>must</em> have a
329+
<code>**kwargs</code> residual keyword parameter.
330+
331+
<p>By convention, a macro should pass inherited, non-overridden attributes unchanged to the "main"
332+
rule or macro symbol which the macro is wrapping. Typically, most inherited attributes will not have
333+
a parameter in the implementation function's parameter list, and will simply be passed via
334+
<code>**kwargs</code>. It can be convenient for the implementation function to have explicit
335+
parameters for some inherited attributes (most commonly, <code>tags</code> and
336+
<code>testonly</code>) if the macro needs to pass those attributes to both "main" and non-"main"
337+
targets &ndash; but if the macro also needs to examine or manipulate those attributes, you must take
338+
care to handle the <code>None</code> default value of non-mandatory inherited attributes.
325339
"""),
326340
// TODO: #19922 - Make a concepts page for symbolic macros, migrate some details like the
327341
// list of disallowed APIs to there.

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

Lines changed: 66 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1728,7 +1728,9 @@ public void inheritAttrs_fromAnyNativeRule() throws Exception {
17281728
// wrapping of all builtin rule classes which are accessible from Starlark. We do not test rule
17291729
// classes which are exposed to Starlark via macro wrappers in @_builtins, because Starlark code
17301730
// typically cannot get at the wrapped native rule's rule class symbol from which to inherit
1731-
// attributes.
1731+
// attributes. We also do not test rule target instantiation (and thus, do not test whether such
1732+
// a target would pass analysis) because declaring arbitrary native rule targets is difficult to
1733+
// automate.
17321734
//
17331735
// This test is expected to fail if:
17341736
// * 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):
18121814
}
18131815
}
18141816

1817+
@Test
1818+
public void inheritAttrs_fromGenrule_producesTargetThatPassesAnalysis() throws Exception {
1819+
// inheritAttrs_fromAnyNativeRule() above is loading-phase only; by contrast, this test verifies
1820+
// that we can wrap a native rule (in this case, genrule) in a macro with inherit_attrs, and
1821+
// that the macro-wrapped rule target passes analysis.
1822+
setBuildLanguageOptions("--experimental_enable_macro_inherit_attrs");
1823+
scratch.file(
1824+
"pkg/my_genrule.bzl",
1825+
"""
1826+
def _my_genrule_impl(name, visibility, tags, **kwargs):
1827+
print("my_genrule: tags = %s" % tags)
1828+
for k in kwargs:
1829+
print("my_genrule: kwarg %s = %s" % (k, kwargs[k]))
1830+
native.genrule(name = name + "_wrapped_genrule", visibility = visibility, **kwargs)
1831+
1832+
my_genrule = macro(
1833+
implementation = _my_genrule_impl,
1834+
inherit_attrs = native.genrule,
1835+
)
1836+
""");
1837+
scratch.file(
1838+
"pkg/BUILD",
1839+
"""
1840+
load(":my_genrule.bzl", "my_genrule")
1841+
my_genrule(
1842+
name = "abc",
1843+
outs = ["out.txt"],
1844+
cmd = "touch $@",
1845+
)
1846+
""");
1847+
1848+
Package pkg = getPackage("pkg");
1849+
assertPackageNotInError(pkg);
1850+
assertThat(getConfiguredTarget("//pkg:abc_wrapped_genrule")).isNotNull();
1851+
assertContainsEvent("my_genrule: tags = None"); // Not []
1852+
assertContainsEvent(
1853+
"my_genrule: kwarg srcs = None"); // Not select({"//conditions:default": []})
1854+
assertContainsEvent(
1855+
"my_genrule: kwarg testonly = None"); // Not select({"//conditions:default": False})
1856+
}
1857+
18151858
@Test
18161859
public void inheritAttrs_fromExportedStarlarkRule() throws Exception {
18171860
setBuildLanguageOptions("--experimental_enable_macro_inherit_attrs");
@@ -1895,24 +1938,28 @@ public void inheritAttrs_fromExportedMacro() throws Exception {
18951938
scratch.file(
18961939
"pkg/my_macro.bzl",
18971940
"""
1898-
def _other_macro_impl(name, visibility, **kwargs):
1899-
pass
1941+
def _other_macro_impl(name, visibility, **kwargs):
1942+
pass
19001943
1901-
_other_macro = macro(
1902-
implementation = _other_macro_impl,
1903-
attrs = {
1904-
"srcs": attr.label_list(),
1905-
},
1906-
)
1944+
_other_macro = macro(
1945+
implementation = _other_macro_impl,
1946+
attrs = {
1947+
"srcs": attr.label_list(),
1948+
"tags": attr.string_list(configurable = False),
1949+
},
1950+
)
19071951
1908-
def _my_macro_impl(name, visibility, **kwargs):
1909-
_other_macro(name = name + "_other_macro", visibility = visibility, **kwargs)
1952+
def _my_macro_impl(name, visibility, tags, **kwargs):
1953+
print("my_macro: tags = %s" % tags)
1954+
for k in kwargs:
1955+
print("my_macro: kwarg %s = %s" % (k, kwargs[k]))
1956+
_other_macro(name = name + "_other_macro", visibility = visibility, tags = tags, **kwargs)
19101957
1911-
my_macro = macro(
1912-
implementation = _my_macro_impl,
1913-
inherit_attrs = _other_macro,
1914-
)
1915-
""");
1958+
my_macro = macro(
1959+
implementation = _my_macro_impl,
1960+
inherit_attrs = _other_macro,
1961+
)
1962+
""");
19161963
scratch.file(
19171964
"pkg/BUILD",
19181965
"""
@@ -1922,7 +1969,9 @@ def _my_macro_impl(name, visibility, **kwargs):
19221969
Package pkg = getPackage("pkg");
19231970
assertPackageNotInError(pkg);
19241971
assertThat(getMacroById(pkg, "abc:1").getMacroClass().getAttributes().keySet())
1925-
.containsExactly("name", "visibility", "srcs");
1972+
.containsExactly("name", "visibility", "srcs", "tags");
1973+
assertContainsEvent("my_macro: tags = None"); // Not []
1974+
assertContainsEvent("my_macro: kwarg srcs = None"); // Not select({"//conditions:default": []})
19261975
}
19271976

19281977
@Test

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -972,6 +972,47 @@ def _my_impl(name):
972972
);
973973
}
974974

975+
@Test
976+
public void macroInheritedAttributes() throws Exception {
977+
Module module =
978+
execWithOptions(
979+
ImmutableList.of("--experimental_enable_macro_inherit_attrs"),
980+
"""
981+
def _my_rule_impl(ctx):
982+
pass
983+
984+
_my_rule = rule(
985+
implementation = _my_rule_impl,
986+
attrs = {
987+
"srcs": attr.label_list(doc = "My rule sources"),
988+
},
989+
)
990+
991+
def _my_macro_impl(name, visibility, srcs, **kwargs):
992+
_my_rule(name = name, visibility = visibility, srcs = srcs, **kwargs)
993+
994+
my_macro = macro(
995+
inherit_attrs = _my_rule,
996+
implementation = _my_macro_impl,
997+
)
998+
""");
999+
ModuleInfo moduleInfo = getExtractor().extractFrom(module);
1000+
assertThat(moduleInfo.getMacroInfoList().get(0).getAttributeList())
1001+
.containsExactly(
1002+
IMPLICIT_MACRO_NAME_ATTRIBUTE_INFO, // name comes first
1003+
// TODO(arostovtsev): for macros, we ought to also document the visibility attr
1004+
AttributeInfo.newBuilder()
1005+
.setName("srcs")
1006+
.setType(AttributeType.LABEL_LIST)
1007+
.setDocString("My rule sources")
1008+
.setDefaultValue("None") // Default value of inherited attributes is always None
1009+
.build()
1010+
// TODO(arostovtsev): currently, non-Starlark-defined attributes don't get documented.
1011+
// This is a reasonable behavior for rules, but we probably ought to document them in
1012+
// macros with inherited attributes.
1013+
);
1014+
}
1015+
9751016
@Test
9761017
public void unexportedMacro_notDocumented() throws Exception {
9771018
Module module =

0 commit comments

Comments
 (0)
0