diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java index 11f180d3f9a094..bd470b15ef1dce 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java +++ b/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); } } diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java index 490cc5de14fd4d..bdac9e39c1365b 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java @@ -710,6 +710,18 @@ public Builder 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 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; diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java index 5d61c994e440f0..cd06f483ada456 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java +++ b/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. -

If inherit_attrs is set, the macro's implementation function must have a -**kwargs residual keyword parameter. -

If inherit_attrs is set to the string "common", the macro will inherit common rule attribute definitions 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 inherit_attrs. -

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 -**kwargs. However, it may be convenient for the implementation function to have -explicit parameters for some inherited attributes (most commonly, tags and -testonly) if the macro needs to pass those attributes to both "main" and non-"main" -targets. -

The inheritance mechanism works as follows:

  1. The special name and visibility attributes are never inherited;
  2. Hidden attributes (ones whose name starts with "_") are never inherited; -
  3. The remaining inherited attributes are merged with the attrs dictionary, with - the entries in attrs dictionary taking precedence in case of conflicts. +
  4. Attributes whose names are already defined in the attrs dictionary are never + inherited (the entry in attrs takes precedence; note that an entry may be set to + None to ensure that no attribute by that name gets defined on the macro); +
  5. All other attributes are inherited from the rule or macro and effectively merged into the + attrs dict.
-

For example, the following macro inherits all attributes from native.cc_library, except -for cxxopts (which is removed from the attribute list) and copts (which is -given a new definition): +

When a non-mandatory attribute is inherited, the default value of the attribute is overridden +to be None, 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 – such as by passing in the unmodified **kwargs – 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 None 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 bazel +query output formats, and computed defaults only execute when the value is omitted. If the +macro needs to examine or modify an inherited attribute – for example, to add a value to an +inherited tags attribute – you must make sure to handle the None +case in the macro's implementation function. + +

For example, the following macro inherits all attributes from native.cc_library, +except for cxxopts (which is removed from the attribute list) and copts +(which is given a new definition). It also takes care to checks for the default None +value of the inherited tags attribute before appending an additional tag. 

-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):
 )
-

Note that a macro may inherit a non-hidden attribute with a computed default (for example, -testonly); 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 None, and the -None may be safely passed on to the corresponding attribute of a rule target, causing -the rule to compute the default as expected. +

If inherit_attrs is set, the macro's implementation function must have a +**kwargs residual keyword parameter. + +

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 +**kwargs. It can be convenient for the implementation function to have explicit +parameters for some inherited attributes (most commonly, tags and +testonly) if the macro needs to pass those attributes to both "main" and non-"main" +targets – but if the macro also needs to examine or manipulate those attributes, you must take +care to handle the None 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. diff --git a/src/test/java/com/google/devtools/build/lib/analysis/SymbolicMacroTest.java b/src/test/java/com/google/devtools/build/lib/analysis/SymbolicMacroTest.java index f5aa500c1525ba..94c4ca77ea7bfd 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/SymbolicMacroTest.java +++ b/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 diff --git a/src/test/java/com/google/devtools/build/lib/starlarkdocextract/ModuleInfoExtractorTest.java b/src/test/java/com/google/devtools/build/lib/starlarkdocextract/ModuleInfoExtractorTest.java index cb89faf564580b..a083f26cbe8289 100644 --- a/src/test/java/com/google/devtools/build/lib/starlarkdocextract/ModuleInfoExtractorTest.java +++ b/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 =