Add support for allow_closure in args.add_all and args.add_joined
In some situations, it can be useful to (carefully) close over values from the
rule implementation. The error is still useful as the default experience
because it's not generally safe/efficient to pass arbitrary closures.
The intended use case is when `map_each` depends on simple primitive values
coming from the build attributes that can't be turned into a `format_each`
string because they affect the result of the `map_each` return values.
Fixes https://github.com/bazelbuild/bazel/issues/13730
RELNOTES[NEW]: Args.add_all and Args.add_joined can now accept closures in
map_each if explicitly enabled via allow_closure.
PiperOrigin-RevId: 390379968
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/Args.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/Args.java
index 6736069..1559b4b 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/Args.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/Args.java
@@ -197,6 +197,7 @@
Boolean uniquify,
Boolean expandDirectories,
Object terminateWith,
+ Boolean allowClosure,
StarlarkThread thread)
throws EvalException {
throw Starlark.errorf("cannot modify frozen value");
@@ -213,6 +214,7 @@
Boolean omitIfEmpty,
Boolean uniquify,
Boolean expandDirectories,
+ Boolean allowClosure,
StarlarkThread thread)
throws EvalException {
throw Starlark.errorf("cannot modify frozen value");
@@ -322,6 +324,7 @@
Boolean uniquify,
Boolean expandDirectories,
Object terminateWith,
+ Boolean allowClosure,
StarlarkThread thread)
throws EvalException {
Starlark.checkMutable(this);
@@ -337,7 +340,7 @@
addVectorArg(
values,
argName,
- validateMapEach(mapEach),
+ validateMapEach(mapEach, allowClosure),
formatEach != Starlark.NONE ? (String) formatEach : null,
beforeEach != Starlark.NONE ? (String) beforeEach : null,
/* joinWith= */ null,
@@ -351,7 +354,8 @@
}
@Nullable
- private static StarlarkCallable validateMapEach(Object fn) throws EvalException {
+ private static StarlarkCallable validateMapEach(Object fn, boolean allowClosure)
+ throws EvalException {
if (fn == Starlark.NONE) {
return null;
}
@@ -364,7 +368,7 @@
// This unfortunately disallows such trivially safe non-global
// functions as "lambda x: x".
// See https://github.com/bazelbuild/bazel/issues/12701.
- if (sfn.getModule().getGlobal(sfn.getName()) != sfn) {
+ if (sfn.getModule().getGlobal(sfn.getName()) != sfn && !allowClosure) {
throw Starlark.errorf(
"to avoid unintended retention of analysis data structures, "
+ "the map_each function (declared at %s) must be declared "
@@ -386,6 +390,7 @@
Boolean omitIfEmpty,
Boolean uniquify,
Boolean expandDirectories,
+ Boolean allowClosure,
StarlarkThread thread)
throws EvalException {
Starlark.checkMutable(this);
@@ -401,7 +406,7 @@
addVectorArg(
values,
argName,
- validateMapEach(mapEach),
+ validateMapEach(mapEach, allowClosure),
formatEach != Starlark.NONE ? (String) formatEach : null,
/* beforeEach= */ null,
joinWith,
diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/CommandLineArgsApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/CommandLineArgsApi.java
index ddc5f47..d88712a 100644
--- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/CommandLineArgsApi.java
+++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/CommandLineArgsApi.java
@@ -244,7 +244,7 @@
+ "<p>To avoid unintended retention of large analysis-phase data structures "
+ "into the execution phase, the <code>map_each</code> function must be "
+ "declared by a top-level <code>def</code> statement; it may not be a "
- + "nested function closure."
+ + "nested function closure by default."
+ ""
+ "<p><i>Warning:</i> <a href='globals.html#print'><code>print()</code></a> "
+ "statements that are executed during the call to <code>map_each</code> will "
@@ -318,6 +318,15 @@
+ "added if <code>omit_if_empty</code> is true (the default) and no other "
+ "items are appended (as happens if <code>values</code> is empty or all of "
+ "its items are filtered)."),
+ @Param(
+ name = "allow_closure",
+ named = true,
+ positional = false,
+ defaultValue = "False",
+ doc =
+ "If true, allows the use of closures in function parameters like "
+ + "<code>map_each</code>. Usually this isn't necessary and it risks retaining "
+ + "large analysis-phase data structures into the execution phase."),
},
useStarlarkThread = true)
CommandLineArgsApi addAll(
@@ -330,6 +339,7 @@
Boolean uniquify,
Boolean expandDirectories,
Object terminateWith,
+ Boolean allowClosure,
StarlarkThread thread)
throws EvalException;
@@ -434,7 +444,13 @@
named = true,
positional = false,
defaultValue = "True",
- doc = "Same as for <a href='#add_all.expand_directories'><code>add_all</code></a>.")
+ doc = "Same as for <a href='#add_all.expand_directories'><code>add_all</code></a>."),
+ @Param(
+ name = "allow_closure",
+ named = true,
+ positional = false,
+ defaultValue = "False",
+ doc = "Same as for <a href='#add_all.allow_closure'><code>add_all</code></a>."),
},
useStarlarkThread = true)
CommandLineArgsApi addJoined(
@@ -447,6 +463,7 @@
Boolean omitIfEmpty,
Boolean uniquify,
Boolean expandDirectories,
+ Boolean allowClosure,
StarlarkThread thread)
throws EvalException;
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/starlark/ArgsParamFileTest.java b/src/test/java/com/google/devtools/build/lib/analysis/starlark/ArgsParamFileTest.java
index 8074f98..258c052 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/starlark/ArgsParamFileTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/starlark/ArgsParamFileTest.java
@@ -63,6 +63,7 @@
false,
false,
false,
+ false,
thread);
args.addArgument("--nod", Starlark.UNBOUND, /* format= */ Starlark.NONE, thread);
args.addArgument("pos1", Starlark.UNBOUND, /* format= */ Starlark.NONE, thread);
@@ -76,6 +77,7 @@
false,
false,
Starlark.NONE,
+ false,
thread);
args.addArgument("--e", "'f'", /* format= */ Starlark.NONE, thread);
args.addArgument("pos2", Starlark.UNBOUND, /* format= */ Starlark.NONE, thread);
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/starlark/FlagPerLineTest.java b/src/test/java/com/google/devtools/build/lib/analysis/starlark/FlagPerLineTest.java
index 59fb5a7..eb5bc91 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/starlark/FlagPerLineTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/starlark/FlagPerLineTest.java
@@ -76,6 +76,7 @@
/* uniquify= */ false,
/* expandDirectories= */ false,
/* terminateWith= */ Starlark.NONE,
+ /* allowClosure= */ false,
thread);
// Absl would reject this line, but it's what we generate.
expectLines("--foo bar baz");
@@ -93,6 +94,7 @@
/* uniquify= */ false,
/* expandDirectories= */ false,
/* terminateWith= */ Starlark.NONE,
+ /* allowClosure= */ false,
thread);
// Absl interprets this as a single value "bar baz" for the flag "--foo",
// which is probably not what was intended.
@@ -111,6 +113,7 @@
/* omitIfEmpty= */ true, // the default
/* uniquify= */ false,
/* expandDirectories= */ false,
+ /* allowClosure= */ false,
thread);
// Absl would reject this line, but it's what we generate.
expectLines("--foo,bar,baz");
@@ -128,6 +131,7 @@
/* omitIfEmpty= */ true,
/* uniquify= */ false,
/* expandDirectories= */ false,
+ /* allowClosure= */ false,
thread);
expectLines("--foo=bar,baz,woof");
}
@@ -145,6 +149,7 @@
/* uniquify= */ false,
/* expandDirectories= */ false,
/* terminateWith= */ Starlark.NONE,
+ /* allowClosure= */ false,
thread);
args.addArgument("--foo1", "bar", /* format= */ Starlark.NONE, thread);
args.addArgument("--foo2", "bar", /* format= */ Starlark.NONE, thread);
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java
index 95f7350..ecc2ae3 100644
--- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java
+++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java
@@ -2448,6 +2448,31 @@
}
@Test
+ public void testArgsMapEachFunctionAllowClosure() throws Exception {
+ // lambda
+ scratch.file(
+ "test/rules.bzl",
+ getSimpleUnderTestDefinition(
+ "def local_fn(x): return 'local:%s' % x",
+ "args = ctx.actions.args()",
+ "args.add_all(['a', 'b'], allow_closure=True, map_each=lambda x: 'lambda:%s' % x)",
+ "args.add_joined(['c', 'd'], join_with=';', allow_closure=True, map_each=local_fn)",
+ "args.set_param_file_format('multiline')",
+ "ctx.actions.write(output=out, content=args)"),
+ testingRuleDefinition);
+ scratch.file("test/BUILD", simpleBuildDefinition);
+ StarlarkRuleContext ruleContext = createRuleContext("//test:testing");
+ setRuleContext(ruleContext);
+ ev.update("file", ev.eval("ruleContext.attr.dep.files.to_list()[0]"));
+ ev.update("action", ev.eval("ruleContext.attr.dep[Actions].by_file[file]"));
+
+ Object contentUnchecked = ev.eval("action.content");
+ assertThat(contentUnchecked).isInstanceOf(String.class);
+ // Args content ends the file with a newline
+ assertThat(ev.eval("action.content")).isEqualTo("lambda:a\nlambda:b\nlocal:c;local:d\n");
+ }
+
+ @Test
public void testTemplateExpansionActionInterface() throws Exception {
scratch.file(
"test/rules.bzl",
