Skip to content

Commit

Permalink
Use ImmutableMap when concating string_list_dict type instead of
Browse files Browse the repository at this point in the history
Dict type.

This is because Dict checks that all the values put in it are Starlark
valid values. But string_list_dict value is of type List<String>, when
Java's List isn't a Starlark value.

Also added some tests.

Fixes #23065
  • Loading branch information
lior10r committed Sep 8, 2024
1 parent 39481ad commit dfcf5a0
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -620,11 +620,11 @@ public Object copyAndLiftStarlarkValue(

@Override
public Map<KeyT, ValueT> concat(Iterable<Map<KeyT, ValueT>> iterable) {
Dict.Builder<KeyT, ValueT> output = new Dict.Builder<>();
ImmutableMap.Builder<KeyT, ValueT> builder = ImmutableMap.builder();
for (Map<KeyT, ValueT> map : iterable) {
output.putAll(map);
builder.putAll(map);
}
return output.buildImmutable();
return builder.buildKeepingLast();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static org.junit.Assert.assertThrows;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.analysis.util.BuildViewTestBase.AnalysisFailureRecorder;
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
import com.google.devtools.build.lib.analysis.util.DummyTestFragment;
Expand All @@ -31,10 +32,12 @@
import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.packages.RuleClass.ToolchainResolutionMode;
import com.google.devtools.build.lib.packages.Type;
import com.google.devtools.build.lib.packages.Types;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
import com.google.devtools.build.lib.util.FileTypeSet;
import java.io.IOException;
import java.util.Arrays;
import java.util.Collection;
import java.util.Set;
import org.junit.Before;
Expand Down Expand Up @@ -166,6 +169,13 @@ public Object getDefault(AttributeMap rule) {
.add(attr("deps", LABEL_LIST).allowedFileTypes())
.toolchainResolutionMode(ToolchainResolutionMode.DISABLED));


private static final MockRule RULE_WITH_STRING_LIST_DICT_ATTR =
() -> MockRule.define(
"rule_with_string_list_dict_attr",
attr("string_list_dict_attr", Types.STRING_LIST_DICT
));

@Override
protected ConfiguredRuleClassProvider createRuleClassProvider() {
ConfiguredRuleClassProvider.Builder builder =
Expand All @@ -175,7 +185,8 @@ protected ConfiguredRuleClassProvider createRuleClassProvider() {
.addRuleDefinition(RULE_WITH_BOOLEAN_ATTR)
.addRuleDefinition(RULE_WITH_ALLOWED_VALUES)
.addRuleDefinition(RULE_WITH_LABEL_DEFAULT)
.addRuleDefinition(RULE_WITH_NO_PLATFORM);
.addRuleDefinition(RULE_WITH_NO_PLATFORM)
.addRuleDefinition(RULE_WITH_STRING_LIST_DICT_ATTR);
TestRuleClassProvider.addStandardRules(builder);
// Allow use of --foo as a dummy flag
builder.addConfigurationFragment(DummyTestFragment.class);
Expand Down Expand Up @@ -2115,4 +2126,24 @@ def my_java_binary(name, deps = [], **kwargs):
/*expected:*/ ImmutableList.of("bin java/foo/libb.jar", "bin java/foo/libb2.jar"),
/*not expected:*/ ImmutableList.of("bin java/foo/liba.jar", "bin java/foo/liba2.jar"));
}

@Test
public void stringListDictTypeConcatConfigurable() throws Exception {
writeConfigRules();
scratch.file(
"foo/BUILD",
"""
rule_with_string_list_dict_attr(
name = 'rule',
string_list_dict_attr = {'a': ['a.out']} | select({
'//conditions:b': {'b': ['b.out']},
}))
""");

useConfiguration("--foo=b");
ConfiguredTargetAndData ctad = getConfiguredTargetAndData("//foo:rule");
AttributeMap attributes = getMapperFromConfiguredTargetAndTarget(ctad);
assertThat(attributes.get("string_list_dict_attr", Types.STRING_LIST_DICT))
.isEqualTo(ImmutableMap.of("a", Arrays.asList("a.out"), "b", Arrays.asList("b.out")));
}
}
31 changes: 31 additions & 0 deletions src/test/java/com/google/devtools/build/lib/packages/TypeTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,37 @@ public void testStringListDict() throws Exception {
assertThat(collectLabels(Types.STRING_LIST_DICT, converted)).isEmpty();
}

@Test
public void testStringListDict_concat() throws Exception {
assertThat(Types.STRING_LIST_DICT.concat(ImmutableList.of()))
.isEqualTo(ImmutableMap.of());

Map<String, List<String>> expected =
ImmutableMap.of(
"foo", Arrays.asList("foo", "bar"),
"wiz", Arrays.asList("bang"));
assertThat(Types.STRING_LIST_DICT.concat(ImmutableList.of(expected)))
.isEqualTo(expected);

Map<String, List<String>> map1 =
ImmutableMap.of(
"foo", Arrays.asList("a", "b"),
"bar", Arrays.asList("c", "d"));
Map<String, List<String>> map2 =
ImmutableMap.of(
"bar", Arrays.asList("x", "y"),
"baz", Arrays.asList("z"));

Map<String, List<String>> expected_after_concat =
ImmutableMap.of(
"foo", Arrays.asList("a", "b"),
"bar", Arrays.asList("x", "y"),
"baz", Arrays.asList("z"));

assertThat(Types.STRING_LIST_DICT.concat(ImmutableList.of(map1, map2)))
.isEqualTo(expected_after_concat);
}

@Test
public void testStringListDictBadFirstElement() throws Exception {
Object input =
Expand Down

0 comments on commit dfcf5a0

Please sign in to comment.