Skip to content

Commit a233aaa

Browse files
WyveraldWyverald
committed
Bzlmod: Starlarkify default attr values for TypeCheckedTags
(#13316) Attribute values are stored normally as "native" values (ie. Java types), and we Starlarkify them before exposing them to Starlark. We were, however, not doing it for attribute default values, which means that the Java `null` was not being converted to Starlark `None`, and the Java `ImmutableMap` was not being converted to Starlark `Dict`, etc. Fixes #14528. PiperOrigin-RevId: 421046160
1 parent 48a0fc5 commit a233aaa

3 files changed

Lines changed: 140 additions & 13 deletions

File tree

‎src/main/java/com/google/devtools/build/lib/bazel/bzlmod/TypeCheckedTag.java‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -81,14 +81,18 @@ public static TypeCheckedTag create(
8181
attrValues[attrIndex] = Attribute.valueToStarlark(nativeValue);
8282
}
8383

84-
// Check that all mandatory attributes have been specified.
84+
// Check that all mandatory attributes have been specified, and fill in default values.
8585
for (int i = 0; i < attrValues.length; i++) {
86-
if (tagClass.getAttributes().get(i).isMandatory() && attrValues[i] == null) {
86+
Attribute attr = tagClass.getAttributes().get(i);
87+
if (attr.isMandatory() && attrValues[i] == null) {
8788
throw ExternalDepsException.withMessage(
8889
Code.BAD_MODULE,
8990
"in tag at %s, mandatory attribute %s isn't being specified",
9091
tag.getLocation(),
91-
tagClass.getAttributes().get(i).getPublicName());
92+
attr.getPublicName());
93+
}
94+
if (attrValues[i] == null) {
95+
attrValues[i] = Attribute.valueToStarlark(attr.getDefaultValueUnchecked());
9296
}
9397
}
9498
return new TypeCheckedTag(tagClass, attrValues);
@@ -106,11 +110,7 @@ public Object getValue(String name) throws EvalException {
106110
if (attrIndex == null) {
107111
return null;
108112
}
109-
Object value = attrValues[attrIndex];
110-
if (value != null) {
111-
return value;
112-
}
113-
return tagClass.getAttributes().get(attrIndex).getDefaultValueUnchecked();
113+
return attrValues[attrIndex];
114114
}
115115

116116
@Override

‎src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -725,6 +725,84 @@ public void labels_constructedInModuleExtension() throws Exception {
725725
.isEqualTo("requirements: get up at 6am. go to bed at 11pm.");
726726
}
727727

728+
/** Tests that a complex-typed attribute (here, string_list_dict) behaves well on a tag. */
729+
@Test
730+
public void complexTypedAttribute() throws Exception {
731+
scratch.file(
732+
workspaceRoot.getRelative("MODULE.bazel").getPathString(),
733+
"bazel_dep(name='data_repo', version='1.0')",
734+
"ext = use_extension('//:defs.bzl', 'ext')",
735+
"ext.tag(data={'foo':['val1','val2'],'bar':['val3','val4']})",
736+
"use_repo(ext, 'foo', 'bar')");
737+
scratch.file(
738+
workspaceRoot.getRelative("defs.bzl").getPathString(),
739+
"load('@data_repo//:defs.bzl','data_repo')",
740+
"tag = tag_class(attrs = {'data':attr.string_list_dict()})",
741+
"def _ext_impl(ctx):",
742+
" for mod in ctx.modules:",
743+
" for tag in mod.tags.tag:",
744+
" for key in tag.data:",
745+
" data_repo(name=key,data=','.join(tag.data[key]))",
746+
"ext = module_extension(implementation=_ext_impl, tag_classes={'tag':tag})");
747+
scratch.file(workspaceRoot.getRelative("BUILD").getPathString());
748+
scratch.file(
749+
workspaceRoot.getRelative("data.bzl").getPathString(),
750+
"load('@foo//:data.bzl', foo_data='data')",
751+
"load('@bar//:data.bzl', bar_data='data')",
752+
"data = 'foo:'+foo_data+' bar:'+bar_data");
753+
754+
SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseAbsoluteUnchecked("//:data.bzl"));
755+
EvaluationResult<BzlLoadValue> result =
756+
evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
757+
if (result.hasError()) {
758+
throw result.getError().getException();
759+
}
760+
assertThat(result.get(skyKey).getModule().getGlobal("data"))
761+
.isEqualTo("foo:val1,val2 bar:val3,val4");
762+
}
763+
764+
/**
765+
* Tests that a complex-typed attribute (here, string_list_dict) behaves well when it has a
766+
* default value and is omitted in a tag.
767+
*/
768+
@Test
769+
public void complexTypedAttribute_default() throws Exception {
770+
scratch.file(
771+
workspaceRoot.getRelative("MODULE.bazel").getPathString(),
772+
"bazel_dep(name='data_repo', version='1.0')",
773+
"ext = use_extension('//:defs.bzl', 'ext')",
774+
"ext.tag()",
775+
"use_repo(ext, 'foo', 'bar')");
776+
scratch.file(
777+
workspaceRoot.getRelative("defs.bzl").getPathString(),
778+
"load('@data_repo//:defs.bzl','data_repo')",
779+
"tag = tag_class(attrs = {",
780+
" 'data': attr.string_list_dict(",
781+
" default = {'foo':['val1','val2'],'bar':['val3','val4']},",
782+
")})",
783+
"def _ext_impl(ctx):",
784+
" for mod in ctx.modules:",
785+
" for tag in mod.tags.tag:",
786+
" for key in tag.data:",
787+
" data_repo(name=key,data=','.join(tag.data[key]))",
788+
"ext = module_extension(implementation=_ext_impl, tag_classes={'tag':tag})");
789+
scratch.file(workspaceRoot.getRelative("BUILD").getPathString());
790+
scratch.file(
791+
workspaceRoot.getRelative("data.bzl").getPathString(),
792+
"load('@foo//:data.bzl', foo_data='data')",
793+
"load('@bar//:data.bzl', bar_data='data')",
794+
"data = 'foo:'+foo_data+' bar:'+bar_data");
795+
796+
SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseAbsoluteUnchecked("//:data.bzl"));
797+
EvaluationResult<BzlLoadValue> result =
798+
evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
799+
if (result.hasError()) {
800+
throw result.getError().getException();
801+
}
802+
assertThat(result.get(skyKey).getModule().getGlobal("data"))
803+
.isEqualTo("foo:val1,val2 bar:val3,val4");
804+
}
805+
728806
@Test
729807
public void generatedReposHaveCorrectMappings() throws Exception {
730808
scratch.file(

‎src/test/java/com/google/devtools/build/lib/bazel/bzlmod/TypeCheckedTagTest.java‎

Lines changed: 54 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,15 +23,22 @@
2323
import static com.google.devtools.build.lib.packages.Attribute.attr;
2424
import static org.junit.Assert.assertThrows;
2525

26+
import com.google.common.collect.ImmutableList;
27+
import com.google.common.collect.ImmutableMap;
2628
import com.google.devtools.build.lib.cmdline.Label;
2729
import com.google.devtools.build.lib.packages.Attribute.AllowedValueSet;
2830
import com.google.devtools.build.lib.packages.BuildType;
2931
import com.google.devtools.build.lib.packages.BuildType.LabelConversionContext;
3032
import com.google.devtools.build.lib.packages.Type;
3133
import com.google.devtools.build.lib.util.FileTypeSet;
3234
import java.util.HashMap;
35+
import net.starlark.java.eval.Dict;
36+
import net.starlark.java.eval.Mutability;
37+
import net.starlark.java.eval.Starlark;
3338
import net.starlark.java.eval.StarlarkInt;
3439
import net.starlark.java.eval.StarlarkList;
40+
import net.starlark.java.eval.StarlarkSemantics;
41+
import net.starlark.java.eval.Structure;
3542
import org.junit.Test;
3643
import org.junit.runner.RunWith;
3744
import org.junit.runners.JUnit4;
@@ -40,6 +47,15 @@
4047
@RunWith(JUnit4.class)
4148
public class TypeCheckedTagTest {
4249

50+
private static Object getattr(Structure structure, String fieldName) throws Exception {
51+
return Starlark.getattr(
52+
Mutability.IMMUTABLE,
53+
StarlarkSemantics.DEFAULT,
54+
structure,
55+
fieldName,
56+
/*defaultValue=*/ null);
57+
}
58+
4359
@Test
4460
public void basic() throws Exception {
4561
TypeCheckedTag typeCheckedTag =
@@ -48,7 +64,7 @@ public void basic() throws Exception {
4864
buildTag("tag_name").addAttr("foo", StarlarkInt.of(3)).build(),
4965
/*labelConversionContext=*/ null);
5066
assertThat(typeCheckedTag.getFieldNames()).containsExactly("foo");
51-
assertThat(typeCheckedTag.getValue("foo")).isEqualTo(StarlarkInt.of(3));
67+
assertThat(getattr(typeCheckedTag, "foo")).isEqualTo(StarlarkInt.of(3));
5268
}
5369

5470
@Test
@@ -66,14 +82,47 @@ public void label() throws Exception {
6682
createRepositoryMapping(createModuleKey("test", "1.0"), "repo", "other_repo"),
6783
new HashMap<>()));
6884
assertThat(typeCheckedTag.getFieldNames()).containsExactly("foo");
69-
assertThat(typeCheckedTag.getValue("foo"))
85+
assertThat(getattr(typeCheckedTag, "foo"))
7086
.isEqualTo(
7187
StarlarkList.immutableOf(
7288
Label.parseAbsoluteUnchecked("@myrepo//mypkg:thing1"),
7389
Label.parseAbsoluteUnchecked("@myrepo//pkg:thing2"),
7490
Label.parseAbsoluteUnchecked("@other_repo//pkg:thing3")));
7591
}
7692

93+
@Test
94+
public void label_withoutDefaultValue() throws Exception {
95+
TypeCheckedTag typeCheckedTag =
96+
TypeCheckedTag.create(
97+
createTagClass(
98+
attr("foo", BuildType.LABEL).allowedFileTypes(FileTypeSet.ANY_FILE).build()),
99+
buildTag("tag_name").build(),
100+
new LabelConversionContext(
101+
Label.parseAbsoluteUnchecked("@myrepo//mypkg:defs.bzl"),
102+
createRepositoryMapping(createModuleKey("test", "1.0"), "repo", "other_repo"),
103+
new HashMap<>()));
104+
assertThat(typeCheckedTag.getFieldNames()).containsExactly("foo");
105+
assertThat(getattr(typeCheckedTag, "foo")).isEqualTo(Starlark.NONE);
106+
}
107+
108+
@Test
109+
public void stringListDict_default() throws Exception {
110+
TypeCheckedTag typeCheckedTag =
111+
TypeCheckedTag.create(
112+
createTagClass(
113+
attr("foo", Type.STRING_LIST_DICT)
114+
.value(ImmutableMap.of("key", ImmutableList.of("value1", "value2")))
115+
.build()),
116+
buildTag("tag_name").build(),
117+
null);
118+
assertThat(typeCheckedTag.getFieldNames()).containsExactly("foo");
119+
assertThat(getattr(typeCheckedTag, "foo"))
120+
.isEqualTo(
121+
Dict.builder()
122+
.put("key", StarlarkList.immutableOf("value1", "value2"))
123+
.buildImmutable());
124+
}
125+
77126
@Test
78127
public void multipleAttributesAndDefaults() throws Exception {
79128
TypeCheckedTag typeCheckedTag =
@@ -88,9 +137,9 @@ public void multipleAttributesAndDefaults() throws Exception {
88137
.build(),
89138
/*labelConversionContext=*/ null);
90139
assertThat(typeCheckedTag.getFieldNames()).containsExactly("foo", "bar", "quux");
91-
assertThat(typeCheckedTag.getValue("foo")).isEqualTo("fooValue");
92-
assertThat(typeCheckedTag.getValue("bar")).isEqualTo(StarlarkInt.of(3));
93-
assertThat(typeCheckedTag.getValue("quux"))
140+
assertThat(getattr(typeCheckedTag, "foo")).isEqualTo("fooValue");
141+
assertThat(getattr(typeCheckedTag, "bar")).isEqualTo(StarlarkInt.of(3));
142+
assertThat(getattr(typeCheckedTag, "quux"))
94143
.isEqualTo(StarlarkList.immutableOf("quuxValue1", "quuxValue2"));
95144
}
96145

0 commit comments

Comments
 (0)