From 71e257890ecfedcd24e9fa6192987be9033fffeb Mon Sep 17 00:00:00 2001 From: "bazel.build machine account" Date: Wed, 11 Sep 2024 18:14:48 -0400 Subject: [PATCH] [7.4.0] Convert values to Starlark values when concating string_list_dict type (#23603) Convert values to Starlark values when concating string_list_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, when Java's List isn't a Starlark value. Fixes #23065 My first PR to bazel :star_struck: Would love you to review the PR. Checked that the test didn't pass before the change and did after it. Closes #23424. PiperOrigin-RevId: 673463108 Change-Id: Ib2cdce7d94243f4e3bda23203a196fee4e766189 Commit https://github.com/bazelbuild/bazel/commit/f1f8d5829a987854d49913058087765db68fc92c --------- Co-authored-by: Lior Gorelik Co-authored-by: Alexandre Rostovtsev --- .../devtools/build/lib/packages/Type.java | 6 ++-- .../analysis/ConfigurableAttributesTest.java | 30 ++++++++++++++++++- .../devtools/build/lib/packages/TypeTest.java | 29 ++++++++++++++++++ 3 files changed, 61 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/packages/Type.java b/src/main/java/com/google/devtools/build/lib/packages/Type.java index 697376891c9964..11429c9a0b481b 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Type.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Type.java @@ -626,11 +626,11 @@ public Object copyAndLiftStarlarkValue( @Override public Map concat(Iterable> iterable) { - Dict.Builder output = new Dict.Builder<>(); + ImmutableMap.Builder builder = ImmutableMap.builder(); for (Map map : iterable) { - output.putAll(map); + builder.putAll(map); } - return output.buildImmutable(); + return builder.buildKeepingLast(); } @Override diff --git a/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java b/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java index ef6be77deebae6..905c5fdd19032b 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java @@ -35,6 +35,7 @@ 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; @@ -164,6 +165,12 @@ public Object getDefault(AttributeMap rule) { .add(attr("deps", LABEL_LIST).allowedFileTypes()) .useToolchainResolution(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", Type.STRING_LIST_DICT)); + @Override protected ConfiguredRuleClassProvider createRuleClassProvider() { ConfiguredRuleClassProvider.Builder builder = @@ -173,7 +180,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); @@ -1927,4 +1935,24 @@ public void selectWithLabelKeysInMacro() throws Exception { /*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", Type.STRING_LIST_DICT)) + .containsExactly("a", Arrays.asList("a.out"), "b", Arrays.asList("b.out")); + } } diff --git a/src/test/java/com/google/devtools/build/lib/packages/TypeTest.java b/src/test/java/com/google/devtools/build/lib/packages/TypeTest.java index 7f98e1c8c3f2ef..2be36545385fae 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/TypeTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/TypeTest.java @@ -371,6 +371,35 @@ public void testStringListDict() throws Exception { assertThat(collectLabels(Type.STRING_LIST_DICT, converted)).isEmpty(); } + @Test + public void testStringListDict_concat() throws Exception { + assertThat(Type.STRING_LIST_DICT.concat(ImmutableList.of())).isEmpty(); + + ImmutableMap> expected = + ImmutableMap.of( + "foo", Arrays.asList("foo", "bar"), + "wiz", Arrays.asList("bang")); + assertThat(Type.STRING_LIST_DICT.concat(ImmutableList.of(expected))).isEqualTo(expected); + + ImmutableMap> map1 = + ImmutableMap.of( + "foo", Arrays.asList("a", "b"), + "bar", Arrays.asList("c", "d")); + ImmutableMap> map2 = + ImmutableMap.of( + "bar", Arrays.asList("x", "y"), + "baz", Arrays.asList("z")); + + ImmutableMap> expectedAfterConcat = + ImmutableMap.of( + "foo", Arrays.asList("a", "b"), + "bar", Arrays.asList("x", "y"), + "baz", Arrays.asList("z")); + + assertThat(Type.STRING_LIST_DICT.concat(ImmutableList.of(map1, map2))) + .isEqualTo(expectedAfterConcat); + } + @Test public void testStringListDictBadFirstElement() throws Exception { Object input =