Skip to content

Commit baf97c0

Browse files
ShreeM01fmeum
ShreeM01
and
fmeum
authored
Allow TemplateDict#map_each callback to return a list of strings (bazelbuild#17306)
Returning a list of strings is needed to get correct `uniquify` semantics when a value may expand to multiple strings. Work towards bazelbuild#1920 Closes bazelbuild#17008. PiperOrigin-RevId: 495868960 Change-Id: I1f0ec560fd783846e916a07bb524dd2179f7c535 Co-authored-by: Fabian Meumertzheim <[email protected]>
1 parent 5a7eca2 commit baf97c0

File tree

3 files changed

+134
-8
lines changed

3 files changed

+134
-8
lines changed

‎src/main/java/com/google/devtools/build/lib/analysis/starlark/TemplateDict.java‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import java.util.List;
2929
import net.starlark.java.eval.EvalException;
3030
import net.starlark.java.eval.Mutability;
31+
import net.starlark.java.eval.Sequence;
3132
import net.starlark.java.eval.Starlark;
3233
import net.starlark.java.eval.StarlarkCallable;
3334
import net.starlark.java.eval.StarlarkFunction;
@@ -121,11 +122,22 @@ public String getValue() throws EvalException {
121122
/*kwargs=*/ ImmutableMap.of());
122123
if (ret instanceof String) {
123124
parts.add((String) ret);
125+
} else if (ret instanceof Sequence) {
126+
for (Object v : ((Sequence) ret)) {
127+
if (!(v instanceof String)) {
128+
throw Starlark.errorf(
129+
"Function provided to map_each must return string, None, or list of strings,"
130+
+ " but returned list containing element '%s' of type %s for key '%s' and"
131+
+ " value: %s",
132+
v, Starlark.type(v), getKey(), val);
133+
}
134+
parts.add((String) v);
135+
}
124136
} else if (ret != Starlark.NONE) {
125137
throw Starlark.errorf(
126-
"Function provided to map_each must return a String or None, but returned type "
127-
+ "%s for key '%s' and value: %s",
128-
Starlark.type(ret), getKey(), Starlark.repr(val));
138+
"Function provided to map_each must return string, None, or list of strings, but "
139+
+ "returned type %s for key '%s' and value: %s",
140+
Starlark.type(ret), getKey(), val);
129141
}
130142
} catch (InterruptedException e) {
131143
// Report the error to the user, but the stack trace is not of use to them

‎src/main/java/com/google/devtools/build/lib/starlarkbuildapi/TemplateDictApi.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -70,9 +70,9 @@ public interface TemplateDictApi extends StarlarkValue {
7070
named = true,
7171
positional = false,
7272
doc =
73-
"A Starlark function accepting a single argument and returning either a String or "
74-
+ "<code>None</code>. This function is applied to each item of the depset "
75-
+ "specified in the <code>values</code> parameter"),
73+
"A Starlark function accepting a single argument and returning either a string, "
74+
+ "<code>None</code>, or a list of strings. This function is applied to each "
75+
+ "item of the depset specified in the <code>values</code> parameter"),
7676
@Param(
7777
name = "uniquify",
7878
named = true,

‎src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java‎

Lines changed: 116 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3740,6 +3740,68 @@ public void testTemplateExpansionComputedSubstitutionWithUniquify() throws Excep
37403740
assertThat(substitutionsUnchecked).isEqualTo(ImmutableMap.of("exts", "txt%%log%%exe%%sh"));
37413741
}
37423742

3743+
@Test
3744+
public void testTemplateExpansionComputedSubstitutionWithUniquifyAndListMapEach()
3745+
throws Exception {
3746+
setBuildLanguageOptions("--experimental_lazy_template_expansion");
3747+
scratch.file(
3748+
"test/rules.bzl",
3749+
"def _artifact_to_extension(file):",
3750+
" if file.extension == 'sh':",
3751+
" return [file.extension]",
3752+
" return [file.extension, '.' + file.extension]",
3753+
"",
3754+
"def _undertest_impl(ctx):",
3755+
" template_dict = ctx.actions.template_dict()",
3756+
" template_dict.add_joined('exts', depset(ctx.files.srcs),",
3757+
" map_each = _artifact_to_extension,",
3758+
" uniquify = True,",
3759+
" join_with = '%%',",
3760+
" )",
3761+
" ctx.actions.expand_template(output=ctx.outputs.out,",
3762+
" template=ctx.file.template,",
3763+
" computed_substitutions=template_dict,",
3764+
" )",
3765+
"undertest_rule = rule(",
3766+
" implementation = _undertest_impl,",
3767+
" outputs = {'out': '%{name}.txt'},",
3768+
" attrs = {'template': attr.label(allow_single_file=True),",
3769+
" 'srcs':attr.label_list(allow_files=True)",
3770+
" },",
3771+
" _skylark_testable = True,",
3772+
")",
3773+
testingRuleDefinition);
3774+
scratch.file("test/template.txt", "exts", "exts");
3775+
scratch.file(
3776+
"test/BUILD",
3777+
"load(':rules.bzl', 'undertest_rule', 'testing_rule')",
3778+
"undertest_rule(",
3779+
" name = 'undertest',",
3780+
" template = ':template.txt',",
3781+
" srcs = ['foo.txt', 'bar.log', 'baz.txt', 'bak.exe', 'far.sh', 'boo.sh'],",
3782+
")",
3783+
"testing_rule(",
3784+
" name = 'testing',",
3785+
" dep = ':undertest',",
3786+
")");
3787+
StarlarkRuleContext ruleContext = createRuleContext("//test:testing");
3788+
setRuleContext(ruleContext);
3789+
ev.update("file", ev.eval("ruleContext.attr.dep.files.to_list()[0]"));
3790+
ev.update("action", ev.eval("ruleContext.attr.dep[Actions].by_file[file]"));
3791+
3792+
assertThat(ev.eval("type(action)")).isEqualTo("Action");
3793+
3794+
Object contentUnchecked = ev.eval("action.content");
3795+
assertThat(contentUnchecked).isInstanceOf(String.class);
3796+
assertThat(contentUnchecked)
3797+
.isEqualTo("txt%%.txt%%log%%.log%%exe%%.exe%%sh\ntxt%%.txt%%log%%.log%%exe%%.exe%%sh\n");
3798+
3799+
Object substitutionsUnchecked = ev.eval("action.substitutions");
3800+
assertThat(substitutionsUnchecked).isInstanceOf(Dict.class);
3801+
assertThat(substitutionsUnchecked)
3802+
.isEqualTo(ImmutableMap.of("exts", "txt%%.txt%%log%%.log%%exe%%.exe%%sh"));
3803+
}
3804+
37433805
@Test
37443806
public void testTemplateExpansionComputedSubstitutionDuplicateKeys() throws Exception {
37453807
setBuildLanguageOptions("--experimental_lazy_template_expansion");
@@ -3912,8 +3974,60 @@ public void testTemplateExpansionComputedSubstitutionMapEachBadReturnType() thro
39123974
assertThat(evalException)
39133975
.hasMessageThat()
39143976
.isEqualTo(
3915-
"Function provided to map_each must return a String or None, but returned "
3916-
+ "type Label for key '%files%' and value: <source file test/template.txt>");
3977+
"Function provided to map_each must return string, None, or list of strings, "
3978+
+ "but returned type Label for key '%files%' and value: "
3979+
+ "File:[/workspace[source]]test/template.txt");
3980+
}
3981+
3982+
@Test
3983+
public void testTemplateExpansionComputedSubstitutionMapEachBadListReturnType() throws Exception {
3984+
setBuildLanguageOptions("--experimental_lazy_template_expansion");
3985+
scratch.file(
3986+
"test/rules.bzl",
3987+
"def file_to_owner_label(file):",
3988+
" return [file.owner]",
3989+
"",
3990+
"def _undertest_impl(ctx):",
3991+
" template_dict = ctx.actions.template_dict()",
3992+
" template_dict.add_joined('%files%', depset(ctx.files.template),",
3993+
" map_each = file_to_owner_label,",
3994+
" join_with = '')",
3995+
" ctx.actions.expand_template(output=ctx.outputs.out,",
3996+
" template=ctx.file.template,",
3997+
" computed_substitutions=template_dict,",
3998+
" )",
3999+
"undertest_rule = rule(",
4000+
" implementation = _undertest_impl,",
4001+
" outputs = {'out': '%{name}.txt'},",
4002+
" attrs = {'template': attr.label(allow_single_file=True),},",
4003+
" _skylark_testable = True,",
4004+
")",
4005+
testingRuleDefinition);
4006+
scratch.file("test/template.txt");
4007+
scratch.file(
4008+
"test/BUILD",
4009+
"load(':rules.bzl', 'undertest_rule', 'testing_rule')",
4010+
"undertest_rule(",
4011+
" name = 'undertest',",
4012+
" template = ':template.txt',",
4013+
")",
4014+
"testing_rule(",
4015+
" name = 'testing',",
4016+
" dep = ':undertest',",
4017+
")");
4018+
StarlarkRuleContext ruleContext = createRuleContext("//test:testing");
4019+
setRuleContext(ruleContext);
4020+
ev.update("file", ev.eval("ruleContext.attr.dep.files.to_list()[0]"));
4021+
ev.update("action", ev.eval("ruleContext.attr.dep[Actions].by_file[file]"));
4022+
4023+
EvalException evalException =
4024+
assertThrows(EvalException.class, () -> ev.eval("action.content"));
4025+
assertThat(evalException)
4026+
.hasMessageThat()
4027+
.isEqualTo(
4028+
"Function provided to map_each must return string, None, or list of strings, "
4029+
+ "but returned list containing element '//test:template.txt' of type Label for "
4030+
+ "key '%files%' and value: File:[/workspace[source]]test/template.txt");
39174031
}
39184032

39194033
@Test

0 commit comments

Comments
 (0)