Skip to content

Commit

Permalink
Symbolic macro attribute inheritance
Browse files Browse the repository at this point in the history
Allows the following syntax:

```
my_cc_library = macro(
    # inherit all attributes from a given rule or macro symbol (or "common" for
    # the set of common Starlark rule attributes)
    inherit_attrs = native.cc_library,
    attrs = {
        # remove cxxopts from inherited attrs list
        "cxxopts": None,
        # override the copts attribute inherited from native.cc_library
        "copts": attr.string_list(default = ["-D_FOO"]),
    },
    ...
)
```

Fixes bazelbuild#24066

Tricky parts:
* Some common rule attributes ("testonly", "size", etc.) have computed defaults;
  native rule "license" and "distribs" attrs have defaults which fail type check if
  lifted. The only sane way of handling such attrs, if the attribute is unset in
  the macro function call, to pass the value as None to the implementation function
  so that the implementation function will pass None to the rule function, which
  will thus set the default appropriately.
* We need RuleFunctionApi and MacroFunctionApi interfaces (yet more interfaces,
  alas, or we get a circular dependency) to express the type of inherit_attrs param
* Iterating over all native rules (for testing inheritance from them) is tricky: we
  need to look at builtins (since the ConfiguredRuleClassProvider may have stubs for
  java rules overridden by builtins). This is already correctly handled by
  `bazel info build-language`, so let's move the logic into a utility class.

RELNOTES: Add inherit_attrs param to macro() to allow symbolic macros to
inherit attributes from rules or other symbolic macros.
PiperOrigin-RevId: 694154352
Change-Id: I849a7f16b4da8eb2829cdbc6a131d85a28bc4740
  • Loading branch information
tetromino authored and ramil-bitrise committed Dec 18, 2024
1 parent 90b051b commit 417fd24
Show file tree
Hide file tree
Showing 15 changed files with 808 additions and 81 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -215,6 +216,8 @@ public class StarlarkRuleClassFunctions implements StarlarkRuleFunctionsApi {
public static final ImmutableSet<AllowlistEntry> 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();
Expand Down Expand Up @@ -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)
Expand All @@ -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<String, Descriptor> 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<string, Attribute|None>",
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",
Expand All @@ -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();
Expand All @@ -404,6 +443,22 @@ public StarlarkCallable macro(
getBzlKeyToken(thread, "Macros"));
}

private static ImmutableList<Attribute> 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<BzlLoadValue.Key> getBzlKeyToken(StarlarkThread thread, String onBehalfOf) {
Symbol<?> untypedToken = thread.getNextIdentityToken();
checkState(
Expand Down Expand Up @@ -1317,7 +1372,9 @@ private static ImmutableSet<String> getLegacyAnyTypeAttrs(RuleClass ruleClass) {
* <p>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;
Expand Down
20 changes: 17 additions & 3 deletions src/main/java/com/google/devtools/build/lib/packages/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [
Expand Down Expand Up @@ -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",
],
)
Original file line number Diff line number Diff line change
Expand Up @@ -266,11 +266,14 @@ private MacroInstance instantiateMacro(Package.Builder pkgBuilder, Map<String, O
throw Starlark.errorf(
"missing value for mandatory attribute '%s' in '%s' macro", attr.getName(), name);
} else {
// Already validated at schema creation time that the default is not a computed default or
// late-bound default
Object defaultValue = attr.getDefaultValueUnchecked();
if (defaultValue == null) {
// Null values can occur for some types of attributes (e.g. LabelType).
if (defaultValue == null || forceDefaultToNone(attr)) {
// Set the default value as None if:
// 1. The native attribute value is null (e.g. LabelType); or
// 2. The attribute is an inherited non-Starlark-defined attribute and with a non-direct
// default or is a legacy native type.
// Note that for Starlark-defined attributes, we already validated at schema creation time
// that the default is not a computed default or late-bound default.
defaultValue = Starlark.NONE;
}
attrValues.put(attr.getName(), defaultValue);
Expand All @@ -285,6 +288,10 @@ private MacroInstance instantiateMacro(Package.Builder pkgBuilder, Map<String, O
String attrName = entry.getKey();
Object value = entry.getValue();
Attribute attribute = attributes.get(attrName);
if (value.equals(Starlark.NONE) && forceDefaultToNone(attribute)) {
// Skip normalization, since None may violate the attribute's type checking.
continue;
}
Object normalizedValue =
// copyAndLiftStarlarkValue ensures immutability.
BuildType.copyAndLiftStarlarkValue(
Expand All @@ -310,6 +317,23 @@ private MacroInstance instantiateMacro(Package.Builder pkgBuilder, Map<String, O
return pkgBuilder.createMacro(this, attrValues, sameNameDepth);
}

/**
* Returns true if the given inherited attribute should be forced to have a default value of
* {@code None}, skipping usual normalization.
*
* <p>This 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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<Attribute> getAttributes() {
public ImmutableList<Attribute> getAttributes() {
return attributes;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -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.
*
* <p>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<RuleClass> getBuiltinRuleClasses(
StarlarkBuiltinsValue builtins,
RuleClassProvider ruleClassProvider,
boolean includeMacroWrappedRules) {
ImmutableMap<String, RuleClass> 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<RuleClass> ruleClasses = new ArrayList<>(builtins.predeclaredForBuild.size());
for (Map.Entry<String, Object> 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() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
<a href="../globals/bzl.html#rule"><code>rule()</code></a>). 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 \
<a href ="https://bazel.build/extending/rules">Rules</a>.
""")
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();
}
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Loading

0 comments on commit 417fd24

Please sign in to comment.