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 1ce27a92901db2..c9622fce14cc24 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 @@ -109,6 +109,7 @@ import com.google.devtools.build.lib.skyframe.BzlLoadValue; import com.google.devtools.build.lib.skyframe.serialization.AbstractExportedStarlarkSymbolCodec; import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant; +import com.google.devtools.build.lib.starlarkbuildapi.MacroFunctionApi; import com.google.devtools.build.lib.starlarkbuildapi.StarlarkRuleFunctionsApi; import com.google.devtools.build.lib.starlarkbuildapi.StarlarkSubruleApi; import com.google.devtools.build.lib.starlarkbuildapi.config.ConfigurationTransitionApi; @@ -215,6 +216,8 @@ public class StarlarkRuleClassFunctions implements StarlarkRuleFunctionsApi { public static final ImmutableSet ALLOWLIST_RULE_EXTENSION_API_EXPERIMENTAL = ImmutableSet.of(allowlistEntry("", "initializer_testing/builtins")); + private static final String COMMON_ATTRIBUTES_NAME = "common"; + /** Parent rule class for test Starlark rules. */ public static RuleClass getTestBaseRule(RuleDefinitionEnvironment env) { RepositoryName toolsRepository = env.getToolsRepository(); @@ -356,9 +359,10 @@ private static void failIf(boolean condition, String message, Object... args) } @Override - public StarlarkCallable macro( + public MacroFunctionApi macro( StarlarkFunction implementation, Dict attrs, + Object inheritAttrs, boolean finalizer, Object doc, StarlarkThread thread) @@ -373,17 +377,38 @@ public StarlarkCallable macro( } MacroClass.Builder builder = new MacroClass.Builder(implementation); - // "name" and "visibility" attributes are added automatically by the builder. - for (Map.Entry descriptorEntry : - Dict.cast(attrs, String.class, Descriptor.class, "attrs").entrySet()) { - String attrName = descriptorEntry.getKey(); - Descriptor descriptor = descriptorEntry.getValue(); + for (Map.Entry uncheckedEntry : attrs.entrySet()) { + String attrName; + @Nullable Descriptor descriptor; + try { + // Dict.cast() does not support none-able values - so we type-check manually, and translate + // Starlark None to Java null. + attrName = (String) uncheckedEntry.getKey(); + checkAttributeName(attrName); + descriptor = + uncheckedEntry.getValue() != Starlark.NONE + ? (Descriptor) uncheckedEntry.getValue() + : null; + } catch ( + @SuppressWarnings("UnusedException") + ClassCastException e) { + throw Starlark.errorf( + "got dict<%s, %s> for 'attrs', want dict", + Starlark.type(uncheckedEntry.getKey()), Starlark.type(uncheckedEntry.getValue())); + } + // "name" and "visibility" attributes are added automatically by the builder. if (MacroClass.RESERVED_MACRO_ATTR_NAMES.contains(attrName)) { throw Starlark.errorf("Cannot declare a macro attribute named '%s'", attrName); } + if (descriptor == null) { + // a None descriptor should ignored. + continue; + } + if (!descriptor.getValueSource().equals(AttributeValueSource.DIRECT)) { + // Note that inherited native attributes may have a computed default, e.g. testonly. throw Starlark.errorf( "In macro attribute '%s': Macros do not support computed defaults or late-bound" + " defaults", @@ -393,6 +418,20 @@ public StarlarkCallable macro( Attribute attr = descriptor.build(attrName); builder.addAttribute(attr); } + for (Attribute attr : getInheritedAttrs(inheritAttrs)) { + String attrName = attr.getName(); + if (attr.isPublic() + // isDocumented() is false only for generator_* magic attrs (for which isPublic() is true) + && attr.isDocumented() + && !MacroClass.RESERVED_MACRO_ATTR_NAMES.contains(attrName) + && !attrs.containsKey(attrName)) { + builder.addAttribute(attr); + } + } + if (inheritAttrs != Starlark.NONE && !implementation.hasKwargs()) { + throw Starlark.errorf( + "If inherit_attrs is set, implementation function must have a **kwargs parameter"); + } if (finalizer) { builder.setIsFinalizer(); @@ -404,6 +443,22 @@ public StarlarkCallable macro( getBzlKeyToken(thread, "Macros")); } + private static ImmutableList getInheritedAttrs(Object inheritAttrs) + throws EvalException { + if (inheritAttrs == Starlark.NONE) { + return ImmutableList.of(); + } else if (inheritAttrs instanceof RuleFunction ruleFunction) { + return ruleFunction.getRuleClass().getAttributes(); + } else if (inheritAttrs instanceof MacroFunction macroFunction) { + return macroFunction.getMacroClass().getAttributes().values().asList(); + } else if (inheritAttrs.equals(COMMON_ATTRIBUTES_NAME)) { + return baseRule.getAttributes(); + } + throw Starlark.errorf( + "Invalid 'inherit_attrs' value %s; expected a rule, a macro, or \"common\"", + Starlark.repr(inheritAttrs)); + } + private static Symbol getBzlKeyToken(StarlarkThread thread, String onBehalfOf) { Symbol untypedToken = thread.getNextIdentityToken(); checkState( @@ -1317,7 +1372,9 @@ private static ImmutableSet getLegacyAnyTypeAttrs(RuleClass ruleClass) { *

This object is not usable until it has been {@link #export exported}. Calling an unexported * macro function results in an {@link EvalException}. */ - public static final class MacroFunction implements StarlarkExportable, StarlarkCallable { + // Ideally, we'd want to merge this with {@link MacroFunctionApi}, but that would cause a circular + // dependency between packages and starlarkbuildapi. + public static final class MacroFunction implements StarlarkExportable, MacroFunctionApi { // Initially non-null, then null once exported. @Nullable private MacroClass.Builder builder; diff --git a/src/main/java/com/google/devtools/build/lib/packages/BUILD b/src/main/java/com/google/devtools/build/lib/packages/BUILD index 4f72aa5d3f766a..16ee054ac0a134 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/BUILD +++ b/src/main/java/com/google/devtools/build/lib/packages/BUILD @@ -46,15 +46,16 @@ java_library( srcs = glob( ["*.java"], exclude = [ + "AutoloadSymbols.java", "BuilderFactoryForTesting.java", # see builder_factory_for_testing "BzlVisibility.java", + "ConfiguredAttributeMapper.java", + "ExecGroup.java", "Globber.java", "GlobberUtils.java", - "ExecGroup.java", - "ConfiguredAttributeMapper.java", "LabelPrinter.java", "PackageSpecification.java", - "AutoloadSymbols.java", + "RuleClassUtils.java", ], ), deps = [ @@ -209,3 +210,16 @@ java_library( "//third_party:auto_value", ], ) + +java_library( + name = "rule_class_utils", + srcs = ["RuleClassUtils.java"], + deps = [ + ":packages", + "//src/main/java/com/google/devtools/build/lib/packages/semantics", + "//src/main/java/com/google/devtools/build/lib/skyframe:starlark_builtins_value", + "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi", + "//src/main/java/net/starlark/java/eval", + "//third_party:guava", + ], +) diff --git a/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java b/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java index 3f54d1fccd46b8..506cd2d3be9724 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java +++ b/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java @@ -266,11 +266,14 @@ private MacroInstance instantiateMacro(Package.Builder pkgBuilder, MapThis 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) { + return attr.hasComputedDefault() + || attr.isLateBound() + || attr.isMaterializing() + || attr.getType() == BuildType.LICENSE + || attr.getType() == BuildType.DISTRIBUTIONS; + } + /** * Constructs a new {@link MacroInstance} associated with this {@code MacroClass}, adds it to the * package, and returns it. diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java index 2079659ba97ca7..c86221a5bff433 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java @@ -2083,7 +2083,7 @@ int getAttributeCount() { * Returns an (immutable) list of all Attributes defined for this class of rule, ordered by * increasing index. */ - public List getAttributes() { + public ImmutableList getAttributes() { return attributes; } diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClassUtils.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClassUtils.java new file mode 100644 index 00000000000000..73c2d42681d157 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClassUtils.java @@ -0,0 +1,79 @@ +// Copyright 2024 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.packages; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; +import com.google.devtools.build.lib.skyframe.StarlarkBuiltinsValue; +import com.google.devtools.build.lib.starlarkbuildapi.MacroFunctionApi; +import java.util.ArrayList; +import java.util.Comparator; +import java.util.Map; +import net.starlark.java.eval.StarlarkFunction; + +/** Rule class utilities. */ +public final class RuleClassUtils { + + /** + * Returns the sorted list of all builtin rule classes. + * + *

Unlike {@link RuleClassProvider#getRuleClassMap}, this method returns real Starlark builtins + * instead of stub overridden native rules. + * + * @param includeMacroWrappedRules if true, include rule classes for rules wrapped in macros. + */ + public static ImmutableList getBuiltinRuleClasses( + StarlarkBuiltinsValue builtins, + RuleClassProvider ruleClassProvider, + boolean includeMacroWrappedRules) { + ImmutableMap nativeRuleClasses = ruleClassProvider.getRuleClassMap(); + // The conditional for selecting whether or not to load symbols from @_builtins is the same as + // in PackageFunction.compileBuildFile + if (builtins + .starlarkSemantics + .get(BuildLanguageOptions.EXPERIMENTAL_BUILTINS_BZL_PATH) + .isEmpty()) { + return ImmutableList.sortedCopyOf( + Comparator.comparing(RuleClass::getName), nativeRuleClasses.values()); + } else { + ArrayList ruleClasses = new ArrayList<>(builtins.predeclaredForBuild.size()); + for (Map.Entry entry : builtins.predeclaredForBuild.entrySet()) { + if (entry.getValue() instanceof RuleFunction) { + ruleClasses.add(((RuleFunction) entry.getValue()).getRuleClass()); + } else if ((entry.getValue() instanceof StarlarkFunction + || entry.getValue() instanceof MacroFunctionApi) + && includeMacroWrappedRules) { + // entry.getValue() is a macro in @_builtins which overrides a native rule and wraps a + // instantiation of a rule target. We cannot get at that main target's rule class + // directly, so we attempt heuristics. + // Note that we do not rely on the StarlarkFunction or MacroFunction object's name because + // the name under which the macro was defined may not match the name under which + // @_builtins re-exported it. + if (builtins.exportedToJava.containsKey(entry.getKey() + "_rule_function")) { + ruleClasses.add( + ((RuleFunction) builtins.exportedToJava.get(entry.getKey() + "_rule_function")) + .getRuleClass()); + } else if (nativeRuleClasses.containsKey(entry.getKey())) { + ruleClasses.add(nativeRuleClasses.get(entry.getKey())); + } + } + } + return ImmutableList.sortedCopyOf(Comparator.comparing(RuleClass::getName), ruleClasses); + } + } + + private RuleClassUtils() {} +} diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleFunction.java b/src/main/java/com/google/devtools/build/lib/packages/RuleFunction.java index e8f00d3144c4f0..0f351c3e1f7685 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RuleFunction.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RuleFunction.java @@ -14,22 +14,11 @@ package com.google.devtools.build.lib.packages; -import com.google.devtools.build.docgen.annot.DocCategory; -import net.starlark.java.annot.StarlarkBuiltin; -import net.starlark.java.eval.StarlarkCallable; +import com.google.devtools.build.lib.starlarkbuildapi.RuleFunctionApi; /** Interface for a native or Starlark rule function. */ -@StarlarkBuiltin( - name = "rule", - category = DocCategory.BUILTIN, - doc = - """ - A callable value representing the type of a native or Starlark rule (created by \ - rule()). Calling the value during \ - evaluation of a package's BUILD file creates an instance of the rule and adds it to the \ - package's target set. For more information, visit this page about \ - Rules. - """) -public interface RuleFunction extends StarlarkCallable { +// Ideally, this interface should be merged with RuleFunctionApi, but that would cause a circular +// dependency between packages and starlarkbuildapi. +public interface RuleFunction extends RuleFunctionApi { RuleClass getRuleClass(); } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/info/BUILD b/src/main/java/com/google/devtools/build/lib/runtime/commands/info/BUILD index 38a050635262be..ef89912b8f2a39 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/info/BUILD +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/info/BUILD @@ -24,6 +24,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages:rule_class_utils", "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/pkgcache", "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster", diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/info/BuildLanguageInfoItem.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/info/BuildLanguageInfoItem.java index cabec2d1b049b5..06238b96f6f8d9 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/info/BuildLanguageInfoItem.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/info/BuildLanguageInfoItem.java @@ -20,7 +20,6 @@ import com.google.common.base.Predicates; import com.google.common.base.Supplier; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; import com.google.devtools.build.lib.packages.Attribute; @@ -28,10 +27,9 @@ import com.google.devtools.build.lib.packages.BuildType; import com.google.devtools.build.lib.packages.ProtoUtils; import com.google.devtools.build.lib.packages.RuleClass; -import com.google.devtools.build.lib.packages.RuleFunction; +import com.google.devtools.build.lib.packages.RuleClassUtils; import com.google.devtools.build.lib.packages.TriState; import com.google.devtools.build.lib.packages.Type; -import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import com.google.devtools.build.lib.query2.proto.proto2api.Build.AllowedRuleClassInfo; import com.google.devtools.build.lib.query2.proto.proto2api.Build.AttributeDefinition; import com.google.devtools.build.lib.query2.proto.proto2api.Build.AttributeValue; @@ -49,7 +47,6 @@ import java.util.Comparator; import java.util.List; import java.util.Map; -import net.starlark.java.eval.StarlarkFunction; import net.starlark.java.eval.StarlarkInt; /** @@ -76,7 +73,12 @@ public byte[] get(Supplier configurationSupplier, Comma throws AbruptExitException { checkNotNull(env); StarlarkBuiltinsValue builtins = loadStarlarkBuiltins(env); - return print(getBuildLanguageDefinition(getRuleClasses(builtins, env))); + return print( + getBuildLanguageDefinition( + RuleClassUtils.getBuiltinRuleClasses( + builtins, + env.getRuntime().getRuleClassProvider(), + /* includeMacroWrappedRules= */ true))); } private StarlarkBuiltinsValue loadStarlarkBuiltins(CommandEnvironment env) @@ -98,46 +100,6 @@ private StarlarkBuiltinsValue loadStarlarkBuiltins(CommandEnvironment env) return (StarlarkBuiltinsValue) result.get(StarlarkBuiltinsValue.key()); } - private ImmutableList getRuleClasses( - StarlarkBuiltinsValue builtins, CommandEnvironment env) { - ImmutableMap nativeRuleClasses = - env.getRuntime().getRuleClassProvider().getRuleClassMap(); - - // The conditional for selecting whether or not to load symbols from @_builtins is the same as - // in PackageFunction.compileBuildFile - if (builtins - .starlarkSemantics - .get(BuildLanguageOptions.EXPERIMENTAL_BUILTINS_BZL_PATH) - .isEmpty()) { - return ImmutableList.sortedCopyOf( - Comparator.comparing(RuleClass::getName), nativeRuleClasses.values()); - } else { - ImmutableList.Builder ruleClasses = ImmutableList.builder(); - for (Map.Entry entry : builtins.predeclaredForBuild.entrySet()) { - if (entry.getValue() instanceof RuleFunction) { - ruleClasses.add(((RuleFunction) entry.getValue()).getRuleClass()); - } else if (entry.getValue() instanceof StarlarkFunction) { - // entry.getValue() is a Starlark macro in @_builtins overriding a native rule. We - // cannot extract the macro's metadata (other than by, perhaps, parsing its Starlark - // docstring via starlark_doc_extract, but that does not have sufficient fidelity to - // get rule attribute metadata), so we try to find the rule function if it's exposed to - // native, else extract it from the legacy rule instead. - // Note that we *cannot* rely on StarlarkFunction.getName() because under which the - // macro is defined may not match the name under which @_builtins exports it. - if (builtins.exportedToJava.containsKey(entry.getKey() + "_rule_function")) { - ruleClasses.add( - ((RuleFunction) builtins.exportedToJava.get(entry.getKey() + "_rule_function")) - .getRuleClass()); - } else if (nativeRuleClasses.containsKey(entry.getKey())) { - ruleClasses.add(nativeRuleClasses.get(entry.getKey())); - } - } - } - return ImmutableList.sortedCopyOf( - Comparator.comparing(RuleClass::getName), ruleClasses.build()); - } - } - /** * Returns a byte array containing a proto-buffer describing the build language. * diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/MacroFunctionApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/MacroFunctionApi.java new file mode 100644 index 00000000000000..c6795879cc04a5 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/MacroFunctionApi.java @@ -0,0 +1,36 @@ +// Copyright 2024 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.starlarkbuildapi; + +import com.google.devtools.build.docgen.annot.DocCategory; +import net.starlark.java.annot.StarlarkBuiltin; +import net.starlark.java.eval.StarlarkCallable; + +/** The interface for a Starlark symbolic macro. */ +@StarlarkBuiltin( + name = "macro", + category = DocCategory.BUILTIN, + doc = + """ +A callable Starlark value representing a symbolic macro; in other words, the return value of +macro(). Invoking this value during package +construction time will instantiate the macro, and cause the macro's implementation function to be +evaluated (in a separate context, different from the context in which the macro value was invoked), +in most cases causing targets to be added to the package's target set. For more information, see +Macros. +""") +// Ideally, this interface should be merged with MacroFunction, but that would cause a circular +// dependency between packages and starlarkbuildapi. +public interface MacroFunctionApi extends StarlarkCallable {} diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/RuleFunctionApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/RuleFunctionApi.java new file mode 100644 index 00000000000000..7f892d42140ace --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/RuleFunctionApi.java @@ -0,0 +1,35 @@ +// Copyright 2024 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.starlarkbuildapi; + +import com.google.devtools.build.docgen.annot.DocCategory; +import net.starlark.java.annot.StarlarkBuiltin; +import net.starlark.java.eval.StarlarkCallable; + +/** Interface for a native or Starlark rule function. */ +@StarlarkBuiltin( + name = "rule", + category = DocCategory.BUILTIN, + doc = + """ + A callable value representing the type of a native or Starlark rule (created by + rule()). Calling the value during + evaluation of a package's BUILD file creates an instance of the rule and adds it to the + package's target set. For more information, visit this page about + Rules. + """) +// Ideally, this interface should be merged with RuleFunction, but that would cause a circular +// dependency between packages and starlarkbuildapi. +public interface RuleFunctionApi extends StarlarkCallable {} 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 d2824b8b54c1c6..2a7123bb9d73cf 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 @@ -219,9 +219,12 @@ Object provider(Object doc, Object fields, Object init, StarlarkThread thread) defaultValue = "{}", doc = """ -A dictionary of the attributes this macro supports, analogous to rule.attrs -. Keys are attribute names, and values are attribute objects like attr.label_list(...) - (see the attr module). +A dictionary of the attributes this macro supports, analogous to +rule.attrs. Keys are attribute names, and values are either attribute +objects like attr.label_list(...) (see the attr +module), or None. A None entry means that the macro does not have an +attribute by that name, even if it would have otherwise inherited one via inherit_attrs +(see below).

The special name attribute is predeclared and must not be included in the dictionary. The visibility attribute name is reserved and must not be included in the @@ -249,6 +252,70 @@ site of the rule. Such attributes can be assigned a default value (as in

To limit memory usage, there is a cap on the number of attributes that may be declared. +"""), + @Param( + name = "inherit_attrs", + allowedTypes = { + @ParamType(type = RuleFunctionApi.class), + @ParamType(type = MacroFunctionApi.class), + @ParamType(type = String.class), + @ParamType(type = NoneType.class) + }, + positional = false, + named = true, + defaultValue = "None", + doc = + """ +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. + +

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. +
+ +

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

+def _my_cc_library_impl(name, visibility, **kwargs):
+    ...
+
+my_cc_library = macro(
+    implementation = _my_cc_library_impl,
+    inherit_attrs = native.cc_library,
+    attrs = {
+        "cxxopts": None,
+        "copts": attr.string_list(default = ["-D_FOO"]),
+    },
+)
+
+ +

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. """), // TODO: #19922 - Make a concepts page for symbolic macros, migrate some details like the // list of disallowed APIs to there. @@ -289,9 +356,10 @@ site of the rule. Such attributes can be assigned a default value (as in + "tools.") }, useStarlarkThread = true) - StarlarkCallable macro( + MacroFunctionApi macro( StarlarkFunction implementation, Dict attrs, + Object inheritAttrs, boolean finalizer, Object doc, StarlarkThread thread) diff --git a/src/test/java/com/google/devtools/build/docgen/StarlarkDocumentationTest.java b/src/test/java/com/google/devtools/build/docgen/StarlarkDocumentationTest.java index cc1e1ade1ccf04..7e8928d794c0c0 100644 --- a/src/test/java/com/google/devtools/build/docgen/StarlarkDocumentationTest.java +++ b/src/test/java/com/google/devtools/build/docgen/StarlarkDocumentationTest.java @@ -54,8 +54,7 @@ public class StarlarkDocumentationTest { private static final ImmutableList - DEPRECATED_OR_EXPERIMENTAL_UNDOCUMENTED_TOP_LEVEL_SYMBOLS = - ImmutableList.of("Actions", "macro"); + DEPRECATED_OR_EXPERIMENTAL_UNDOCUMENTED_TOP_LEVEL_SYMBOLS = ImmutableList.of("Actions"); private static final StarlarkDocExpander expander = new StarlarkDocExpander(null) { 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 af55a4adc5deca..917a33847de243 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 @@ -16,6 +16,7 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assertWithMessage; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.analysis.test.AnalysisFailure; @@ -23,11 +24,16 @@ import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.PackageIdentifier; +import com.google.devtools.build.lib.packages.Attribute; +import com.google.devtools.build.lib.packages.BuildType; 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.devtools.build.lib.packages.RuleClass; import com.google.devtools.build.lib.packages.Target; +import com.google.devtools.build.lib.packages.Type; +import com.google.devtools.build.lib.packages.Types; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.testing.junit.testparameterinjector.TestParameterInjector; import com.google.testing.junit.testparameterinjector.TestParameters; @@ -1459,4 +1465,442 @@ def _impl(name, visibility): assertThat(getMacroVisibility(pkg, "macro_without_explicit_vis_sub:1")) .containsExactly("//lib:__pkg__"); } + + @Test + public void wrongKeyTypeInAttrsDict_detected() throws Exception { + scratch.file( + "pkg/foo.bzl", + """ + def _impl(name, visibility, **kwargs): + pass + + my_macro = macro( + implementation = _impl, + attrs = {123: attr.string()}, + ) + """); + scratch.file( + "pkg/BUILD", + """ + load(":foo.bzl", "my_macro") + my_macro(name = "abc") + """); + reporter.removeHandler(failFastHandler); + assertThat(getPackage("pkg")).isNull(); + assertContainsEvent("got dict for 'attrs', want dict"); + } + + @Test + public void wrongKeyValueInAttrsDict_detected() throws Exception { + scratch.file( + "pkg/foo.bzl", + """ + def _impl(name, visibility, **kwargs): + pass + + my_macro = macro( + implementation = _impl, + attrs = {"bad attr": None}, + ) + """); + scratch.file( + "pkg/BUILD", + """ + load(":foo.bzl", "my_macro") + my_macro(name = "abc") + """); + reporter.removeHandler(failFastHandler); + assertThat(getPackage("pkg")).isNull(); + assertContainsEvent("attribute name `bad attr` is not a valid identifier"); + } + + @Test + public void wrongValueTypeInAttrsDict_detected() throws Exception { + scratch.file( + "pkg/foo.bzl", + """ + def _impl(name, visibility, **kwargs): + pass + + my_macro = macro( + implementation = _impl, + attrs = {"bad": 123}, + ) + """); + scratch.file( + "pkg/BUILD", + """ + load(":foo.bzl", "my_macro") + my_macro(name = "abc") + """); + reporter.removeHandler(failFastHandler); + assertThat(getPackage("pkg")).isNull(); + assertContainsEvent("got dict for 'attrs', want dict"); + } + + @Test + public void noneValueInAttrsDict_ignored() throws Exception { + scratch.file( + "pkg/foo.bzl", + """ + def _impl(name, visibility, **kwargs): + pass + + my_macro = macro( + implementation = _impl, + attrs = {"disabled_attr": None}, + ) + """); + scratch.file( + "pkg/BUILD", + """ + load(":foo.bzl", "my_macro") + my_macro(name = "abc") + """); + Package pkg = getPackage("pkg"); + assertPackageNotInError(pkg); + assertThat(getMacroById(pkg, "abc:1").getMacroClass().getAttributes()) + .doesNotContainKey("disabled_attr"); + } + + @Test + public void inheritAttrs_fromInvalidSource_fails() throws Exception { + scratch.file( + "pkg/foo.bzl", + """ + def _my_macro_impl(name, visibility, **kwargs): + pass + + my_macro = macro( + implementation = _my_macro_impl, + inherit_attrs = "???", + ) + """); + scratch.file( + "pkg/BUILD", + """ + load(":foo.bzl", "my_macro") + """); + reporter.removeHandler(failFastHandler); + assertThat(getPackage("pkg")).isNull(); + assertContainsEvent( + "Invalid 'inherit_attrs' value \"???\"; expected a rule, a macro, or \"common\""); + } + + @Test + public void inheritAttrs_withoutKwargsInImplementation_fails() throws Exception { + scratch.file( + "pkg/foo.bzl", + """ + def _my_macro_impl(name, visibility, tags): + pass + + my_macro = macro( + implementation = _my_macro_impl, + inherit_attrs = "common" + ) + """); + scratch.file( + "pkg/BUILD", + """ + load(":foo.bzl", "my_macro") + """); + reporter.removeHandler(failFastHandler); + assertThat(getPackage("pkg")).isNull(); + assertContainsEvent( + "If inherit_attrs is set, implementation function must have a **kwargs parameter"); + } + + @Test + public void inheritAttrs_fromCommon_withOverrides() throws Exception { + scratch.file( + "pkg/my_macro.bzl", + """ + def _my_macro_impl(name, visibility, **kwargs): + pass + + my_macro = macro( + implementation = _my_macro_impl, + attrs = { + # add a new attr + "new_attr": attr.string(), + # override an inherited attr + "tags": attr.string_list(default = ["foo"]), + # remove an inherited attr + "features": None, + }, + inherit_attrs = "common", + ) + """); + scratch.file( + "pkg/BUILD", + """ + load(":my_macro.bzl", "my_macro") + my_macro(name = "abc") + """); + Package pkg = getPackage("pkg"); + assertPackageNotInError(pkg); + // inherited attrs + assertThat(getMacroById(pkg, "abc:1").getMacroClass().getAttributes().keySet()) + .containsAtLeast("compatible_with", "testonly", "toolchains"); + // overridden attr + assertThat( + getMacroById(pkg, "abc:1") + .getMacroClass() + .getAttributes() + .get("tags") + .getDefaultValueUnchecked()) + .isEqualTo(ImmutableList.of("foo")); + // non-inherited attr + assertThat(getMacroById(pkg, "abc:1").getMacroClass().getAttributes()) + .doesNotContainKey("features"); + // internal public attrs which macro machinery must avoid inheriting + assertThat(getMacroById(pkg, "abc:1").getMacroClass().getAttributes().keySet()) + .containsNoneOf("generator_name", "generator_location", "generator_function"); + } + + @Test + public void inheritAttrs_fromAnyNativeRule() throws Exception { + // Ensure that a symbolic macro can inherit attributes from (and thus, can conveniently wrap) + // any native rule. Native rules may use attribute definitions which are unavailable to Starlark + // rules, so to verify that we handle the native attribute corner cases, we exhaustively test + // 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. + // + // 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 + // fakeMandatoryArgs mechanism below (to fix, add support for it to fakeMandatoryArgs); or + // * a new AttributeValueSource or a new attribute type is introduced, and symbolic macros + // cannot inherit an attribute with a default with this source or of such a type (to fix, add + // a check for it in MacroClass#forceDefaultToNone). + for (RuleClass ruleClass : getBuiltinRuleClasses(false)) { + if (ruleClass.getAttributes().isEmpty()) { + continue; + } + if (!(ruleClass.getRuleClassType().equals(RuleClass.Builder.RuleClassType.NORMAL) + || ruleClass.getRuleClassType().equals(RuleClass.Builder.RuleClassType.TEST))) { + continue; + } + String pkgName = "pkg_" + ruleClass.getName(); + String macroName = "my_" + ruleClass.getName(); + // Provide fake values for mandatory attributes in macro invocation + StringBuilder fakeMandatoryArgs = new StringBuilder(); + for (Attribute attr : ruleClass.getAttributes()) { + String fakeValue = null; + if (attr.isPublic() && attr.isMandatory() && !attr.getName().equals("name")) { + Type type = attr.getType(); + if (type.equals(Type.STRING) + || type.equals(BuildType.OUTPUT) + || type.equals(BuildType.LABEL) + || type.equals(BuildType.NODEP_LABEL) + || type.equals(BuildType.DORMANT_LABEL) + || type.equals(BuildType.GENQUERY_SCOPE_TYPE)) { + fakeValue = "\":fake\""; + } else if (type.equals(Types.STRING_LIST) + || type.equals(BuildType.OUTPUT_LIST) + || type.equals(BuildType.LABEL_LIST) + || type.equals(BuildType.NODEP_LABEL_LIST) + || type.equals(BuildType.DORMANT_LABEL_LIST) + || type.equals(BuildType.GENQUERY_SCOPE_TYPE_LIST)) { + fakeValue = "[\":fake\"]"; + } else if (type.equals(BuildType.LABEL_DICT_UNARY) + || type.equals(BuildType.LABEL_KEYED_STRING_DICT)) { + fakeValue = "{\":fake\": \":fake\"}"; + } + } + if (fakeValue != null) { + fakeMandatoryArgs.append(", ").append(attr.getName()).append(" = ").append(fakeValue); + } + } + + scratch.file( + pkgName + "/macro.bzl", + String.format( + """ + def _impl(name, visibility, **kwargs): + pass + + %s = macro( + implementation = _impl, + inherit_attrs = native.%s, + ) + """, + macroName, ruleClass.getName())); + scratch.file( + pkgName + "/BUILD", + String.format( + """ + load(":macro.bzl", "%s") + %s(name = "abc"%s) + """, + macroName, macroName, fakeMandatoryArgs)); + Package pkg = getPackage(pkgName); + assertPackageNotInError(pkg); + assertWithMessage("rule '%s'", ruleClass.getName()) + .that(getMacroById(pkg, "abc:1").getMacroClass().getAttributes().keySet()) + .containsAtLeastElementsIn( + ruleClass.getAttributes().stream() + .filter(a -> a.isPublic() && a.isDocumented()) + .map(Attribute::getName) + .collect(toImmutableList())); + assertThat(getMacroById(pkg, "abc:1").getMacroClass().getAttributes().keySet()) + .containsNoneOf("generator_name", "generator_location", "generator_function"); + } + } + + @Test + public void inheritAttrs_fromExportedStarlarkRule() throws Exception { + scratch.file( + "pkg/my_rule.bzl", + """ + def _my_rule_impl(ctx): + pass + + my_rule = rule( + implementation = _my_rule_impl, + attrs = { + "srcs": attr.label_list(), + }, + ) + """); + scratch.file( + "pkg/my_macro.bzl", + """ + load(":my_rule.bzl", "my_rule") + + def _my_macro_impl(name, visibility, **kwargs): + my_rule(name = name + "_my_rule", visibility = visibility, **kwargs) + + my_macro = macro( + implementation = _my_macro_impl, + inherit_attrs = my_rule, + ) + """); + scratch.file( + "pkg/BUILD", + """ + load(":my_macro.bzl", "my_macro") + my_macro(name = "abc") + """); + Package pkg = getPackage("pkg"); + assertPackageNotInError(pkg); + assertThat(getMacroById(pkg, "abc:1").getMacroClass().getAttributes().keySet()) + .containsAtLeast("srcs", "tags"); + assertThat(getMacroById(pkg, "abc:1").getMacroClass().getAttributes().keySet()) + .containsNoneOf("generator_name", "generator_location", "generator_function"); + } + + @Test + public void inheritAttrs_fromUnexportedStarlarkRule() throws Exception { + scratch.file( + "pkg/my_macro.bzl", + """ + def _my_rule_impl(ctx): + pass + + _my_rule = rule( + implementation = _my_rule_impl, + attrs = { + "srcs": attr.label_list(), + }, + ) + + def _my_macro_impl(name, visibility, **kwargs): + _my_rule(name = name + "_my_rule", visibility = visibility, **kwargs) + + my_macro = macro( + implementation = _my_macro_impl, + inherit_attrs = _my_rule, + ) + """); + scratch.file( + "pkg/BUILD", + """ + load(":my_macro.bzl", "my_macro") + my_macro(name = "abc") + """); + Package pkg = getPackage("pkg"); + assertPackageNotInError(pkg); + assertThat(getMacroById(pkg, "abc:1").getMacroClass().getAttributes().keySet()) + .containsAtLeast("srcs", "tags"); + assertThat(getMacroById(pkg, "abc:1").getMacroClass().getAttributes().keySet()) + .containsNoneOf("generator_name", "generator_location", "generator_function"); + } + + @Test + public void inheritAttrs_fromExportedMacro() throws Exception { + scratch.file( + "pkg/other_macro.bzl", + """ + def _other_macro_impl(name, visibility, **kwargs): + pass + + other_macro = macro( + implementation = _other_macro_impl, + attrs = { + "srcs": attr.label_list(), + }, + ) + """); + scratch.file( + "pkg/my_macro.bzl", + """ + load(":other_macro.bzl", "other_macro") + + def _my_macro_impl(name, visibility, **kwargs): + other_macro(name = name + "_other_macro", visibility = visibility, **kwargs) + + my_macro = macro( + implementation = _my_macro_impl, + inherit_attrs = other_macro, + ) + """); + scratch.file( + "pkg/BUILD", + """ + load(":my_macro.bzl", "my_macro") + my_macro(name = "abc") + """); + Package pkg = getPackage("pkg"); + assertPackageNotInError(pkg); + assertThat(getMacroById(pkg, "abc:1").getMacroClass().getAttributes().keySet()) + .containsExactly("name", "visibility", "srcs"); + } + + @Test + public void inheritAttrs_fromUnexportedMacro() throws Exception { + scratch.file( + "pkg/my_macro.bzl", + """ + def _other_macro_impl(name, visibility, **kwargs): + pass + + _other_macro = macro( + implementation = _other_macro_impl, + attrs = { + "srcs": attr.label_list(), + }, + ) + + def _my_macro_impl(name, visibility, **kwargs): + _other_macro(name = name + "_other_macro", visibility = visibility, **kwargs) + + my_macro = macro( + implementation = _my_macro_impl, + inherit_attrs = _other_macro, + ) + """); + scratch.file( + "pkg/BUILD", + """ + load(":my_macro.bzl", "my_macro") + my_macro(name = "abc") + """); + Package pkg = getPackage("pkg"); + assertPackageNotInError(pkg); + assertThat(getMacroById(pkg, "abc:1").getMacroClass().getAttributes().keySet()) + .containsExactly("name", "visibility", "srcs"); + } } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD index 4c8df2c63e268e..5b76a5f2b529a8 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD @@ -95,6 +95,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/exec:execution_options", "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/packages:configured_attribute_mapper", + "//src/main/java/com/google/devtools/build/lib/packages:rule_class_utils", "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/pkgcache", "//src/main/java/com/google/devtools/build/lib/rules:repository/local_repository_rule", diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java index 58406925cd460a..aaa5f17f4d7063 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java @@ -13,6 +13,7 @@ // limitations under the License. package com.google.devtools.build.lib.analysis.util; +import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.MoreCollectors.onlyElement; import static com.google.common.truth.Truth.assertThat; @@ -131,7 +132,9 @@ import com.google.devtools.build.lib.packages.PackageValidator; import com.google.devtools.build.lib.packages.RawAttributeMapper; import com.google.devtools.build.lib.packages.Rule; +import com.google.devtools.build.lib.packages.RuleClass; import com.google.devtools.build.lib.packages.RuleClassProvider; +import com.google.devtools.build.lib.packages.RuleClassUtils; import com.google.devtools.build.lib.packages.StarlarkInfo; import com.google.devtools.build.lib.packages.StarlarkProvider; import com.google.devtools.build.lib.packages.Target; @@ -706,6 +709,21 @@ public SkyFunctionName functionName() { starlarkBuiltinsValue); } + /** + * Returns the sorted list of all rule classes available in builtins, following the logic of + * {@code bazel info build-language}. + * + * @param includeMacroWrappedRules if true, include rule classes for rules wrapped in macros. + */ + protected ImmutableList getBuiltinRuleClasses(boolean includeMacroWrappedRules) + throws Exception { + SkyFunction.Environment env = new SkyFunctionEnvironmentForTesting(reporter, skyframeExecutor); + StarlarkBuiltinsValue builtins = + (StarlarkBuiltinsValue) checkNotNull(env.getValue(StarlarkBuiltinsValue.key())); + return RuleClassUtils.getBuiltinRuleClasses( + builtins, ruleClassProvider, includeMacroWrappedRules); + } + /** * Allows access to the prerequisites of a configured target. This is currently used in some tests * to reach into the internals of RuleCT for white box testing. In principle, this should not be