Skip to content

Commit

Permalink
Bzlmod: Starlarkify default attr values for TypeCheckedTags
Browse files Browse the repository at this point in the history 
(#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
  • Loading branch information
@Wyverald
Wyverald authored and Copybara-Service committed Jan 11, 2022
1 parent 15a602a commit 6200202
Show file tree
Hide file tree
Showing 3 changed files with 140 additions and 13 deletions.
16 changes: 8 additions & 8 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/TypeCheckedTag.java
View file
Expand Up @@ -81,14 +81,18 @@ public static TypeCheckedTag create(
attrValues[attrIndex] = Attribute.valueToStarlark(nativeValue);
}

// Check that all mandatory attributes have been specified.
// Check that all mandatory attributes have been specified, and fill in default values.
for (int i = 0; i < attrValues.length; i++) {
if (tagClass.getAttributes().get(i).isMandatory() && attrValues[i] == null) {
Attribute attr = tagClass.getAttributes().get(i);
if (attr.isMandatory() && attrValues[i] == null) {
throw ExternalDepsException.withMessage(
Code.BAD_MODULE,
"in tag at %s, mandatory attribute %s isn't being specified",
tag.getLocation(),
tagClass.getAttributes().get(i).getPublicName());
attr.getPublicName());
}
if (attrValues[i] == null) {
attrValues[i] = Attribute.valueToStarlark(attr.getDefaultValueUnchecked());
}
}
return new TypeCheckedTag(tagClass, attrValues);
Expand All @@ -106,11 +110,7 @@ public Object getValue(String name) throws EvalException {
if (attrIndex == null) {
return null;
}
Object value = attrValues[attrIndex];
if (value != null) {
return value;
}
return tagClass.getAttributes().get(attrIndex).getDefaultValueUnchecked();
return attrValues[attrIndex];
}

@Override
Expand Down
78 changes: 78 additions & 0 deletions src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java
View file
Expand Up @@ -720,6 +720,84 @@ public void labels_constructedInModuleExtension() throws Exception {
.isEqualTo("requirements: get up at 6am. go to bed at 11pm.");
}

/** Tests that a complex-typed attribute (here, string_list_dict) behaves well on a tag. */
@Test
public void complexTypedAttribute() throws Exception {
scratch.file(
workspaceRoot.getRelative("MODULE.bazel").getPathString(),
"bazel_dep(name='data_repo', version='1.0')",
"ext = use_extension('//:defs.bzl', 'ext')",
"ext.tag(data={'foo':['val1','val2'],'bar':['val3','val4']})",
"use_repo(ext, 'foo', 'bar')");
scratch.file(
workspaceRoot.getRelative("defs.bzl").getPathString(),
"load('@data_repo//:defs.bzl','data_repo')",
"tag = tag_class(attrs = {'data':attr.string_list_dict()})",
"def _ext_impl(ctx):",
" for mod in ctx.modules:",
" for tag in mod.tags.tag:",
" for key in tag.data:",
" data_repo(name=key,data=','.join(tag.data[key]))",
"ext = module_extension(implementation=_ext_impl, tag_classes={'tag':tag})");
scratch.file(workspaceRoot.getRelative("BUILD").getPathString());
scratch.file(
workspaceRoot.getRelative("data.bzl").getPathString(),
"load('@foo//:data.bzl', foo_data='data')",
"load('@bar//:data.bzl', bar_data='data')",
"data = 'foo:'+foo_data+' bar:'+bar_data");

SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseAbsoluteUnchecked("//:data.bzl"));
EvaluationResult<BzlLoadValue> result =
evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
if (result.hasError()) {
throw result.getError().getException();
}
assertThat(result.get(skyKey).getModule().getGlobal("data"))
.isEqualTo("foo:val1,val2 bar:val3,val4");
}

/**
* Tests that a complex-typed attribute (here, string_list_dict) behaves well when it has a
* default value and is omitted in a tag.
*/
@Test
public void complexTypedAttribute_default() throws Exception {
scratch.file(
workspaceRoot.getRelative("MODULE.bazel").getPathString(),
"bazel_dep(name='data_repo', version='1.0')",
"ext = use_extension('//:defs.bzl', 'ext')",
"ext.tag()",
"use_repo(ext, 'foo', 'bar')");
scratch.file(
workspaceRoot.getRelative("defs.bzl").getPathString(),
"load('@data_repo//:defs.bzl','data_repo')",
"tag = tag_class(attrs = {",
" 'data': attr.string_list_dict(",
" default = {'foo':['val1','val2'],'bar':['val3','val4']},",
")})",
"def _ext_impl(ctx):",
" for mod in ctx.modules:",
" for tag in mod.tags.tag:",
" for key in tag.data:",
" data_repo(name=key,data=','.join(tag.data[key]))",
"ext = module_extension(implementation=_ext_impl, tag_classes={'tag':tag})");
scratch.file(workspaceRoot.getRelative("BUILD").getPathString());
scratch.file(
workspaceRoot.getRelative("data.bzl").getPathString(),
"load('@foo//:data.bzl', foo_data='data')",
"load('@bar//:data.bzl', bar_data='data')",
"data = 'foo:'+foo_data+' bar:'+bar_data");

SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseAbsoluteUnchecked("//:data.bzl"));
EvaluationResult<BzlLoadValue> result =
evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
if (result.hasError()) {
throw result.getError().getException();
}
assertThat(result.get(skyKey).getModule().getGlobal("data"))
.isEqualTo("foo:val1,val2 bar:val3,val4");
}

@Test
public void generatedReposHaveCorrectMappings() throws Exception {
scratch.file(
Expand Down
59 changes: 54 additions & 5 deletions src/test/java/com/google/devtools/build/lib/bazel/bzlmod/TypeCheckedTagTest.java
View file
Expand Up @@ -23,15 +23,22 @@
import static com.google.devtools.build.lib.packages.Attribute.attr;
import static org.junit.Assert.assertThrows;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.packages.Attribute.AllowedValueSet;
import com.google.devtools.build.lib.packages.BuildType;
import com.google.devtools.build.lib.packages.BuildType.LabelConversionContext;
import com.google.devtools.build.lib.packages.Type;
import com.google.devtools.build.lib.util.FileTypeSet;
import java.util.HashMap;
import net.starlark.java.eval.Dict;
import net.starlark.java.eval.Mutability;
import net.starlark.java.eval.Starlark;
import net.starlark.java.eval.StarlarkInt;
import net.starlark.java.eval.StarlarkList;
import net.starlark.java.eval.StarlarkSemantics;
import net.starlark.java.eval.Structure;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand All @@ -40,6 +47,15 @@
@RunWith(JUnit4.class)
public class TypeCheckedTagTest {

private static Object getattr(Structure structure, String fieldName) throws Exception {
return Starlark.getattr(
Mutability.IMMUTABLE,
StarlarkSemantics.DEFAULT,
structure,
fieldName,
/*defaultValue=*/ null);
}

@Test
public void basic() throws Exception {
TypeCheckedTag typeCheckedTag =
Expand All @@ -48,7 +64,7 @@ public void basic() throws Exception {
buildTag("tag_name").addAttr("foo", StarlarkInt.of(3)).build(),
/*labelConversionContext=*/ null);
assertThat(typeCheckedTag.getFieldNames()).containsExactly("foo");
assertThat(typeCheckedTag.getValue("foo")).isEqualTo(StarlarkInt.of(3));
assertThat(getattr(typeCheckedTag, "foo")).isEqualTo(StarlarkInt.of(3));
}

@Test
Expand All @@ -66,14 +82,47 @@ public void label() throws Exception {
createRepositoryMapping(createModuleKey("test", "1.0"), "repo", "other_repo"),
new HashMap<>()));
assertThat(typeCheckedTag.getFieldNames()).containsExactly("foo");
assertThat(typeCheckedTag.getValue("foo"))
assertThat(getattr(typeCheckedTag, "foo"))
.isEqualTo(
StarlarkList.immutableOf(
Label.parseAbsoluteUnchecked("@myrepo//mypkg:thing1"),
Label.parseAbsoluteUnchecked("@myrepo//pkg:thing2"),
Label.parseAbsoluteUnchecked("@other_repo//pkg:thing3")));
}

@Test
public void label_withoutDefaultValue() throws Exception {
TypeCheckedTag typeCheckedTag =
TypeCheckedTag.create(
createTagClass(
attr("foo", BuildType.LABEL).allowedFileTypes(FileTypeSet.ANY_FILE).build()),
buildTag("tag_name").build(),
new LabelConversionContext(
Label.parseAbsoluteUnchecked("@myrepo//mypkg:defs.bzl"),
createRepositoryMapping(createModuleKey("test", "1.0"), "repo", "other_repo"),
new HashMap<>()));
assertThat(typeCheckedTag.getFieldNames()).containsExactly("foo");
assertThat(getattr(typeCheckedTag, "foo")).isEqualTo(Starlark.NONE);
}

@Test
public void stringListDict_default() throws Exception {
TypeCheckedTag typeCheckedTag =
TypeCheckedTag.create(
createTagClass(
attr("foo", Type.STRING_LIST_DICT)
.value(ImmutableMap.of("key", ImmutableList.of("value1", "value2")))
.build()),
buildTag("tag_name").build(),
null);
assertThat(typeCheckedTag.getFieldNames()).containsExactly("foo");
assertThat(getattr(typeCheckedTag, "foo"))
.isEqualTo(
Dict.builder()
.put("key", StarlarkList.immutableOf("value1", "value2"))
.buildImmutable());
}

@Test
public void multipleAttributesAndDefaults() throws Exception {
TypeCheckedTag typeCheckedTag =
Expand All @@ -88,9 +137,9 @@ public void multipleAttributesAndDefaults() throws Exception {
.build(),
/*labelConversionContext=*/ null);
assertThat(typeCheckedTag.getFieldNames()).containsExactly("foo", "bar", "quux");
assertThat(typeCheckedTag.getValue("foo")).isEqualTo("fooValue");
assertThat(typeCheckedTag.getValue("bar")).isEqualTo(StarlarkInt.of(3));
assertThat(typeCheckedTag.getValue("quux"))
assertThat(getattr(typeCheckedTag, "foo")).isEqualTo("fooValue");
assertThat(getattr(typeCheckedTag, "bar")).isEqualTo(StarlarkInt.of(3));
assertThat(getattr(typeCheckedTag, "quux"))
.isEqualTo(StarlarkList.immutableOf("quuxValue1", "quuxValue2"));
}

Expand Down

0 comments on commit 6200202

Please sign in to comment.