Allow string_list flags to be set via repeated flag uses

For all parts of Bazel other than option parsing, string build setting
flags with `allow_multiple = True` should behave just like `string_list`
settings, even though they are fundamentally of a different type. This
requires a lot of special casing all over the code base and also causes
confusing user-facing behavior when transitioning on such settings.

This commit deprecates the `allow_multiple` named parameter of
`config.string` and as a replacement introduces a new named parameter
`repeatable` on `config.string_list`. Settings with the latter set to True
behave exactly like `string` settings with `allow_multiple = True` with
respect to command-line option parsing and exactly like ordinary
`string_list` flags in all other situations.

Fixes #13817

Closes #14911.

PiperOrigin-RevId: 462146752
Change-Id: I91ddc4328a1b2244f4303fcda7b4228b182a5d56
diff --git a/src/test/java/com/google/devtools/build/lib/rules/config/ConfigSettingTest.java b/src/test/java/com/google/devtools/build/lib/rules/config/ConfigSettingTest.java
index 31d7d4e..aa425fb 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/config/ConfigSettingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/config/ConfigSettingTest.java
@@ -1775,6 +1775,51 @@
   }
 
   @Test
+  public void buildsettings_repeatableWorks() throws Exception {
+    scratch.file(
+        "test/build_settings.bzl",
+        "def _impl(ctx):",
+        "  return []",
+        "string_list_flag = rule(",
+        "  implementation = _impl,",
+        "  build_setting = config.string_list(flag = True, repeatable = True),",
+        ")");
+    scratch.file(
+        "test/BUILD",
+        "load('//test:build_settings.bzl', 'string_list_flag')",
+        "config_setting(",
+        "    name = 'match',",
+        "    flag_values = {",
+        "        ':cheese': 'pepperjack',",
+        "    },",
+        ")",
+        "string_list_flag(name = 'cheese', build_setting_default = ['gouda'])");
+
+    useConfiguration(ImmutableMap.of("//test:cheese", ImmutableList.of("pepperjack", "brie")));
+    assertThat(getConfigMatchingProvider("//test:match").matches()).isTrue();
+  }
+
+  @Test
+  public void buildsettings_repeatableWithoutFlagErrors() throws Exception {
+    scratch.file(
+        "test/build_settings.bzl",
+        "def _impl(ctx):",
+        "  return []",
+        "string_list_setting = rule(",
+        "  implementation = _impl,",
+        "  build_setting = config.string_list(repeatable = True),",
+        ")");
+    scratch.file(
+        "test/BUILD",
+        "load('//test:build_settings.bzl', 'string_list_setting')",
+        "string_list_setting(name = 'cheese', build_setting_default = ['gouda'])");
+
+    reporter.removeHandler(failFastHandler);
+    getConfiguredTarget("//test:cheese");
+    assertContainsEvent("'repeatable' can only be set for a setting with 'flag = True'");
+  }
+
+  @Test
   public void notBuildSettingOrFeatureFlag() throws Exception {
     scratch.file(
         "test/rules.bzl",
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkOptionsParsingTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkOptionsParsingTest.java
index 0e93717..6579eec 100644
--- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkOptionsParsingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkOptionsParsingTest.java
@@ -478,4 +478,27 @@
     assertThat((List<String>) result.getStarlarkOptions().get("//test:cats"))
         .containsExactly("calico", "bengal");
   }
+
+  @Test
+  @SuppressWarnings("unchecked")
+  public void testRepeatedStringListFlag() throws Exception {
+    scratch.file(
+        "test/build_setting.bzl",
+        "def _build_setting_impl(ctx):",
+        "  return []",
+        "repeated_flag = rule(",
+        "  implementation = _build_setting_impl,",
+        "  build_setting = config.string_list(flag=True, repeatable=True)",
+        ")");
+    scratch.file(
+        "test/BUILD",
+        "load('//test:build_setting.bzl', 'repeated_flag')",
+        "repeated_flag(name = 'cats', build_setting_default = ['tabby'])");
+
+    OptionsParsingResult result = parseStarlarkOptions("--//test:cats=calico --//test:cats=bengal");
+
+    assertThat(result.getStarlarkOptions().keySet()).containsExactly("//test:cats");
+    assertThat((List<String>) result.getStarlarkOptions().get("//test:cats"))
+        .containsExactly("calico", "bengal");
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java
index 943d435..6a6b554 100644
--- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java
+++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java
@@ -3024,6 +3024,66 @@
         .containsExactly("some-other-value", "some-other-other-value");
   }
 
+  @SuppressWarnings("unchecked")
+  @Test
+  public void testBuildSettingValue_isRepeatedSetting() throws Exception {
+    scratch.file(
+        "test/build_setting.bzl",
+        "BuildSettingInfo = provider(fields = ['name', 'value'])",
+        "def _impl(ctx):",
+        "  return [BuildSettingInfo(name = ctx.attr.name, value = ctx.build_setting_value)]",
+        "",
+        "string_list_flag = rule(",
+        "  implementation = _impl,",
+        "  build_setting = config.string_list(flag = True, repeatable = True),",
+        ")");
+    scratch.file(
+        "test/BUILD",
+        "load('//test:build_setting.bzl', 'string_list_flag')",
+        "string_list_flag(name = 'string_list_flag', build_setting_default = ['some-value'])");
+
+    // from default
+    ConfiguredTarget buildSetting = getConfiguredTarget("//test:string_list_flag");
+    Provider.Key key =
+        new StarlarkProvider.Key(
+            Label.create(buildSetting.getLabel().getPackageIdentifier(), "build_setting.bzl"),
+            "BuildSettingInfo");
+    StructImpl buildSettingInfo = (StructImpl) buildSetting.get(key);
+
+    assertThat(buildSettingInfo.getValue("value")).isInstanceOf(List.class);
+    assertThat((List<String>) buildSettingInfo.getValue("value")).containsExactly("some-value");
+
+    // Set multiple times
+    useConfiguration(
+        ImmutableMap.of(
+            "//test:string_list_flag",
+            ImmutableList.of("some-other-value", "some-other-other-value")));
+    buildSetting = getConfiguredTarget("//test:string_list_flag");
+    key =
+        new StarlarkProvider.Key(
+            Label.create(buildSetting.getLabel().getPackageIdentifier(), "build_setting.bzl"),
+            "BuildSettingInfo");
+    buildSettingInfo = (StructImpl) buildSetting.get(key);
+
+    assertThat(buildSettingInfo.getValue("value")).isInstanceOf(List.class);
+    assertThat((List<String>) buildSettingInfo.getValue("value"))
+        .containsExactly("some-other-value", "some-other-other-value");
+
+    // No splitting on comma.
+    useConfiguration(
+        ImmutableMap.of("//test:string_list_flag", ImmutableList.of("a,b,c", "a", "b,c")));
+    buildSetting = getConfiguredTarget("//test:string_list_flag");
+    key =
+        new StarlarkProvider.Key(
+            Label.create(buildSetting.getLabel().getPackageIdentifier(), "build_setting.bzl"),
+            "BuildSettingInfo");
+    buildSettingInfo = (StructImpl) buildSetting.get(key);
+
+    assertThat(buildSettingInfo.getValue("value")).isInstanceOf(List.class);
+    assertThat((List<String>) buildSettingInfo.getValue("value"))
+        .containsExactly("a,b,c", "a", "b,c");
+  }
+
   @Test
   public void testBuildSettingValue_nonBuildSettingRule() throws Exception {
     scratch.file(