Skip to content

Commit

Permalink
Copy and lift labels in arguments of initializers
Browse files Browse the repository at this point in the history 
The arguments are copied into immutable Starlark values. They are type checked.  Strings are converted to labels (in the right context).

I was worried copying would cause memory regression, but it doesn't. (There's initializer on cc_binary).

There's a potential optimisation to use Starlark types everywhere. StarlarkList, Dict are already as good as ImmutableList and ImmutableMap (I think). We'd need to converge Selectors to a single type. Then the convert operation becomes the same as copyAndLiftStarlarkValue. And it could be optimised to a no-op when called twice.

PiperOrigin-RevId: 582324735
Change-Id: I64ee4f70fa12f9818d57474ba33d085aeed347ad
  • Loading branch information
@comius @copybara-github
comius authored and copybara-github committed Nov 14, 2023
1 parent c042c46 commit 82a469f
Show file tree
Hide file tree
Showing 5 changed files with 144 additions and 20 deletions.
40 changes: 29 additions & 11 deletions ...main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -496,7 +496,7 @@ public static StarlarkRuleFunction createRule(
builder = new RuleClass.Builder("", type, true, baseParent);
}

builder.initializer(initializer);
builder.initializer(initializer, labelConverter);

builder.setDefaultExtendableAllowlist(
ruleDefinitionEnvironment.getToolsLabel("//tools/allowlists/extend_rule_allowlist"));
Expand Down Expand Up @@ -1030,6 +1030,12 @@ public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> kwarg
if (ruleClass == null) {
throw new EvalException("Invalid rule class hasn't been exported by a bzl file");
}
PackageContext pkgContext = thread.getThreadLocal(PackageContext.class);
if (pkgContext == null) {
throw new EvalException(
"Cannot instantiate a rule when loading a .bzl file. "
+ "Rules may be instantiated only in a BUILD thread.");
}

validateRulePropagatedAspects(ruleClass);

Expand All @@ -1052,8 +1058,6 @@ public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> kwarg
continue;
}

// TODO: b/298561048 - lift parameters to more accurate type - for example strings to
// Labels
// You might feel tempted to inspect the signature of the initializer function. The
// temptation might come from handling default values, making them work for better for the
// users.
Expand All @@ -1062,7 +1066,17 @@ public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> kwarg
for (var attr : currentRuleClass.getAttributes()) {
if (attr.isPublic() && attr.starlarkDefined()) {
if (kwargs.containsKey(attr.getName())) {
initializerKwargs.put(attr.getName(), kwargs.get(attr.getName()));
Object value = kwargs.get(attr.getName());
if (value == Starlark.NONE) {
continue;
}
Object reifiedValue =
BuildType.copyAndLiftStarlarkValue(
currentRuleClass.getName(),
attr,
value,
pkgContext.getBuilder().getLabelConverter());
initializerKwargs.put(attr.getName(), reifiedValue);
}
}
}
Expand Down Expand Up @@ -1093,7 +1107,17 @@ public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> kwarg
throw Starlark.errorf(
"Initializer can only set Starlark defined attributes, not '%s'", arg);
}
kwargs.putEntry(nativeName, newKwargs.get(arg));
Object value = newKwargs.get(arg);
Object reifiedValue =
attr == null || value == Starlark.NONE
? value
: BuildType.copyAndLiftStarlarkValue(
currentRuleClass.getName(),
attr,
value,
// Reify to the location of the initializer definition
currentRuleClass.getLabelConverterForInitializer());
kwargs.putEntry(nativeName, reifiedValue);
}
}
} finally {
Expand All @@ -1103,12 +1127,6 @@ public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> kwarg
BuildLangTypedAttributeValuesMap attributeValues =
new BuildLangTypedAttributeValuesMap(kwargs);
try {
PackageContext pkgContext = thread.getThreadLocal(PackageContext.class);
if (pkgContext == null) {
throw new EvalException(
"Cannot instantiate a rule when loading a .bzl file. "
+ "Rules may be instantiated only in a BUILD thread.");
}
RuleFactory.createAndAddRule(
pkgContext.getBuilder(),
ruleClass,
Expand Down
6 changes: 6 additions & 0 deletions src/main/java/com/google/devtools/build/lib/packages/BuildType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,12 @@ public License convert(Object x, Object what, LabelConverter labelConverter)
}
}

@Override
public Object copyAndLiftStarlarkValue(
Object x, Object what, @Nullable LabelConverter labelConverter) throws ConversionException {
return STRING_LIST.copyAndLiftStarlarkValue(x, what, labelConverter);
}

@Override
public License getDefaultValue() {
return License.NO_LICENSE;
Expand Down
15 changes: 14 additions & 1 deletion src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -753,6 +753,8 @@ public String toString() {
private final RuleClassType type;
@Nullable private RuleClass starlarkParent = null;
@Nullable private StarlarkFunction initializer = null;
@Nullable private LabelConverter labelConverterForInitializer = null;

// The extendable may take 3 value, null means that the default allowlist should be use when
// rule is extendable in practice.
@Nullable private Boolean extendable = null;
Expand Down Expand Up @@ -961,6 +963,7 @@ public RuleClass build(String name, String key) {
type,
starlarkParent,
initializer,
labelConverterForInitializer,
starlark,
extendable,
extendableAllowlist,
Expand Down Expand Up @@ -1041,8 +1044,10 @@ private void assertStarlarkRuleClassHasEnvironmentLabel() {
}

@CanIgnoreReturnValue
public Builder initializer(StarlarkFunction initializer) {
public Builder initializer(
StarlarkFunction initializer, LabelConverter labelConverterForInitializer) {
this.initializer = initializer;
this.labelConverterForInitializer = labelConverterForInitializer;
return this;
}

Expand Down Expand Up @@ -1679,6 +1684,7 @@ public Attribute.Builder<?> copy(String name) {
private final RuleClassType type;
@Nullable private final RuleClass starlarkParent;
@Nullable private final StarlarkFunction initializer;
@Nullable private final LabelConverter labelConverterForInitializer;
private final boolean isStarlark;
private final boolean extendable;
@Nullable private final Label extendableAllowlist;
Expand Down Expand Up @@ -1816,6 +1822,7 @@ public Attribute.Builder<?> copy(String name) {
RuleClassType type,
RuleClass starlarkParent,
@Nullable StarlarkFunction initializer,
@Nullable LabelConverter labelConverterForInitializer,
boolean isStarlark,
boolean extendable,
@Nullable Label extendableAllowlist,
Expand Down Expand Up @@ -1855,6 +1862,7 @@ public Attribute.Builder<?> copy(String name) {
this.type = type;
this.starlarkParent = starlarkParent;
this.initializer = initializer;
this.labelConverterForInitializer = labelConverterForInitializer;
this.isStarlark = isStarlark;
this.extendable = extendable;
this.extendableAllowlist = extendableAllowlist;
Expand Down Expand Up @@ -1960,6 +1968,11 @@ public StarlarkFunction getInitializer() {
return initializer;
}

@Nullable
public LabelConverter getLabelConverterForInitializer() {
return labelConverterForInitializer;
}

/**
* Returns the stack of Starlark active function calls at the moment this rule class was created.
* Entries appear outermost first, and exclude the built-in itself ('rule' or 'repository_rule').
Expand Down
1 change: 1 addition & 0 deletions src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1026,6 +1026,7 @@ private static RuleClass newRuleClass(
RuleClassType.NORMAL,
/* starlarkParent= */ null,
/* initializer= */ null,
/* labelConverterForInitializer= */ null,
/* isStarlark= */ starlarkExecutable,
/* extendable= */ false,
/* extendableAllowlist= */ null,
Expand Down
102 changes: 94 additions & 8 deletions src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,6 @@
import org.junit.Test;
import org.junit.rules.ExpectedException;
import org.junit.runner.RunWith;

/** Tests for StarlarkRuleClassFunctions. */
@RunWith(TestParameterInjector.class)
public final class StarlarkRuleClassFunctionsTest extends BuildViewTestCase {
Expand Down Expand Up @@ -2821,6 +2820,67 @@ public void initializer_basic() throws Exception {
assertThat((List<String>) info.getValue("deps")).containsExactly("@@//:initial", "@@//:added");
}

@Test
public void initializer_wrongType() throws Exception {
scratch.file(
"initializer_testing/b.bzl",
"MyInfo = provider()",
"def initializer(srcs = []):",
" return {'srcs': ['a.ml']}",
"def impl(ctx): ",
" return [MyInfo(",
" srcs = [s.short_path for s in ctx.files.srcs])]",
"my_rule = rule(impl,",
" initializer = initializer,",
" attrs = {",
" 'srcs': attr.label_list(allow_files = ['ml']),",
" })");
scratch.file(
"initializer_testing/BUILD", //
"load(':b.bzl','my_rule')",
"my_rule(name = 'my_target', srcs = 'default_files')");

reporter.removeHandler(failFastHandler);
reporter.addHandler(ev.getEventCollector());
getConfiguredTarget("//initializer_testing:my_target");

ev.assertContainsError(
"expected value of type 'list(label)' for attribute 'srcs' in 'my_rule' rule, but got"
+ " \"default_files\" (string)");
}

@Test
@SuppressWarnings("unchecked")
public void initializer_withSelect() throws Exception {
scratch.file(
"initializer_testing/b.bzl",
"MyInfo = provider()",
"def initializer(srcs = []):",
" return {'srcs': srcs + ['b.ml']}",
"def impl(ctx): ",
" return [MyInfo(",
" srcs = [s.short_path for s in ctx.files.srcs])]",
"my_rule = rule(impl,",
" initializer = initializer,",
" attrs = {",
" 'srcs': attr.label_list(allow_files = ['ml']),",
" })");
scratch.file(
"initializer_testing/BUILD", //
"load(':b.bzl','my_rule')",
"my_rule(name = 'my_target', srcs = select({'//conditions:default': ['a.ml']}))");

ConfiguredTarget myTarget = getConfiguredTarget("//initializer_testing:my_target");
StructImpl info =
(StructImpl)
myTarget.get(
new StarlarkProvider.Key(
Label.parseCanonical("//initializer_testing:b.bzl"), "MyInfo"));

assertThat((List<String>) info.getValue("srcs"))
.containsExactly("initializer_testing/a.ml", "initializer_testing/b.ml");
}

@Test
public void initializer_passThrough() throws Exception {
scratch.file(
Expand Down Expand Up @@ -2923,8 +2983,7 @@ public void initializer_omittedValueIsNotPassed() throws Exception {
"def initializer(srcs):",
" return {'srcs': srcs}",
"def impl(ctx): ",
" return [MyInfo(",
" deps = [str(d.label) for d in ctx.attr.deps])]",
" pass",
"my_rule = rule(impl,",
" initializer = initializer,",
" attrs = {",
Expand All @@ -2934,8 +2993,6 @@ public void initializer_omittedValueIsNotPassed() throws Exception {
"initializer_testing/BUILD", //
"load(':b.bzl','my_rule')",
"my_rule(name = 'my_target')");
// TODO: b/298561048 - Behavior with `srcs=[]` or `srcs=None` is different. Fix that when
// lifting parameters types.

reporter.removeHandler(failFastHandler);
reporter.addHandler(ev.getEventCollector());
Expand All @@ -2945,6 +3002,32 @@ public void initializer_omittedValueIsNotPassed() throws Exception {
ev.assertContainsError("initializer() missing 1 required positional argument: srcs");
}

@Test
public void initializer_noneValueIsNotPassed() throws Exception {
scratch.file(
"initializer_testing/b.bzl",
"MyInfo = provider()",
"def initializer(srcs):",
" return {'srcs': srcs}",
"def impl(ctx): ",
" pass",
"my_rule = rule(impl,",
" initializer = initializer,",
" attrs = {",
" 'srcs': attr.label_list(),",
" })");
scratch.file(
"initializer_testing/BUILD", //
"load(':b.bzl','my_rule')",
"my_rule(name = 'my_target', srcs = None)");

reporter.removeHandler(failFastHandler);
reporter.addHandler(ev.getEventCollector());
getConfiguredTarget("//initializer_testing:my_target");

ev.assertContainsError("initializer() missing 1 required positional argument: srcs");
}

@Test
public void initializer_incorrectReturnType() throws Exception {
scratch.file(
Expand Down Expand Up @@ -3052,7 +3135,10 @@ public void initializer_failsSettingPrivateAttribute_outsideBuiltins() throws Ex

@Test
public void initializer_settingPrivateAttribute_insideBuiltins() throws Exception {
scratch.file("initializer_testing/builtins/BUILD");
// Because it's hard to test something that needs to be in builtins,
// this is also allowed in a special testing location: {@link
// StarlarkRuleClassFunctions.ALLOWLIST_RULE_EXTENSION_API_EXPERIMENTAL}
scratch.file("initializer_testing/builtins/BUILD", "filegroup(name='my_tool')");
scratch.file(
"initializer_testing/builtins/b.bzl",
"def initializer(srcs = [], deps = []):",
Expand All @@ -3069,7 +3155,6 @@ public void initializer_settingPrivateAttribute_insideBuiltins() throws Exceptio
scratch.file(
"initializer_testing/BUILD", //
"load('//initializer_testing/builtins:b.bzl','my_rule')",
"filegroup(name='my_tool')",
"my_rule(name = 'my_target', srcs = ['a.ml'])");

ConfiguredTarget myTarget = getConfiguredTarget("//initializer_testing:my_target");
Expand All @@ -3079,7 +3164,8 @@ public void initializer_settingPrivateAttribute_insideBuiltins() throws Exceptio
new StarlarkProvider.Key(
Label.parseCanonical("//initializer_testing/builtins:b.bzl"), "MyInfo"));

assertThat(info.getValue("_tool").toString()).isEqualTo("@@//initializer_testing:my_tool");
assertThat(info.getValue("_tool").toString())
.isEqualTo("@@//initializer_testing/builtins:my_tool");
}

@Test
Expand Down

0 comments on commit 82a469f

Please sign in to comment.