Repository: bazelbuild/bazel
Issue: config.string with allow_multiple=True should use list for build_setting_default
Labels: P2
type: support / not a bug (process)
team-Configurability

I'm trying to migrate a native flag (`--protocopt`) to a Starlark flag. For this, we need a flag that a) can be supplied multiple times (like with `allow_multiple=True` - `config.string_list` doesn't allow this) and b) has an empty default.

Today, it's impossible to get an empty list or a list with multiple elements as `ctx.build_setting_value` when using `config.string` with `allow_multiple=True`.

```starlark
# foo.bzl

def _impl(ctx):
    pass

foo = rule(
    implementation = _impl,
    build_setting = config.string(flag = True, allow_multiple = True),
)

# BUILD
load(":foo.bzl", "foo")

foo(name="foo", build_setting_default="")  # <-- `ctx.build_setting_value` becomes `[""]`.
```

There are ways to work around this (i.e. setting `build_setting_default` to a nonsensical value and checking for `ctx.build_setting_value` with one element that is the specified default value), but that seems unnecessarily complex.

From a technical perspective, changing the allowed type for `build_setting_default` when `allow_multiple=True` looks pretty straight forward to me.

`allow_multiple` was added in https://cs.opensource.google/bazel/bazel/+/a13f590b69bcbcaa10b1a49bfd9a4607dfbd8f47 after cutting Bazel 4.0.0, so its usage in the wild should be low.


Issue comments:
Interesting. Are you comfortable if it still uses the same syntax `build_setting_default=""`? And that semantically means an empty list?

I get your point, and agree this is worth addressing. I'd want to be cautious to make sure whatever solution doesn't create some unknown other consistency in some other API piece with different expectations.
@gregestren What would you think of allowing both `list of <type>` and `<type>` as default values, with the latter continuing to be converted to a singleton list for backwards compatibility? Since there is already a `string_list` type, having the `BuildSetting` return a type that dynamically depends on the value of its `allowMultiple` member may actually not be that much work.

Edit: I now see that the problem is not that `BuildSetting` can't return a different type, it's that `build_setting_default` has to go through the usual `attr`, which does not allow for this kind of fallback behavior.
I ultimately came to the conclusion that the source of issues is that repeatable string flags should be represented as `string_list` flags with a different CLI argument passing style rather than `string` flags that conditionally provide a list value. I created #14911 to create a replacement for the current `allow_multiple` flag that requires significantly less special casing internally and should also be more transparent to users.
Thanks. I'll take a look. Also cc @aranguyen FYI who's been looking recently at flag parsing logic in general.
@fmeum  thanks for sending out the proposal. I don't know how to make comments on the proposal you linked so I'm dropping them here instead.
1) Have you considered what the overriding/accumulating behavior would look like if users do the following `bazel build --flag=["a", "b"] --flag=["c", "d"]`? Would the final evaluation be `--flag=["c", "d"]` or would it be `--flag=["a", "b", "c", "d"]`? 
2) If the answer to the previous question is `--flag=["c", "d"]`. Would it break for the case of inheritance (test inheriting from build for example)? Some users might want to pass in additional flags for testing. 

@aranguyen My proposal doesn't allow for this syntax, so it (luckily) doesn't have to specify this interaction. My proposal only affects how repeated flags are represented in Starlark, not how they are specified on the command-line: You still either specify a `string_list` as `--flag=first,second` (where later occurrences of `--flag` override all previous occurrences) or as `--flag=first --first=second`, depending on the flag style. The starlark value would be `["first", "second"]` in both cases.

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkConfig.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkConfig.java
index 96584b184e7bd1..d583cb09514828 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkConfig.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkConfig.java
@@ -22,6 +22,7 @@
 import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
 import com.google.devtools.build.lib.packages.BuildSetting;
 import com.google.devtools.build.lib.starlarkbuildapi.StarlarkConfigApi;
+import net.starlark.java.eval.EvalException;
 import net.starlark.java.eval.Printer;
 import net.starlark.java.eval.Starlark;
 
@@ -40,12 +41,15 @@ public BuildSetting boolSetting(Boolean flag) {
 
   @Override
   public BuildSetting stringSetting(Boolean flag, Boolean allowMultiple) {
-    return BuildSetting.create(flag, STRING, allowMultiple);
+    return BuildSetting.create(flag, STRING, allowMultiple, false);
   }
 
   @Override
-  public BuildSetting stringListSetting(Boolean flag) {
-    return BuildSetting.create(flag, STRING_LIST);
+  public BuildSetting stringListSetting(Boolean flag, Boolean repeatable) throws EvalException {
+    if (repeatable && !flag) {
+      throw Starlark.errorf("'repeatable' can only be set for a setting with 'flag = True'");
+    }
+    return BuildSetting.create(flag, STRING_LIST, false, repeatable);
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/packages/BuildSetting.java b/src/main/java/com/google/devtools/build/lib/packages/BuildSetting.java
index c64e81f657e1d0..d9b01dcca86316 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/BuildSetting.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/BuildSetting.java
@@ -27,22 +27,25 @@ public class BuildSetting implements BuildSettingApi {
   private final boolean isFlag;
   private final Type<?> type;
   private final boolean allowMultiple;
+  private final boolean repeatable;
 
-  private BuildSetting(boolean isFlag, Type<?> type, boolean allowMultiple) {
+  private BuildSetting(boolean isFlag, Type<?> type, boolean allowMultiple, boolean repeatable) {
     this.isFlag = isFlag;
     this.type = type;
     this.allowMultiple = allowMultiple;
+    this.repeatable = repeatable;
   }
 
-  public static BuildSetting create(boolean isFlag, Type<?> type, boolean allowMultiple) {
-    return new BuildSetting(isFlag, type, allowMultiple);
+  public static BuildSetting create(
+      boolean isFlag, Type<?> type, boolean allowMultiple, boolean repeatable) {
+    return new BuildSetting(isFlag, type, allowMultiple, repeatable);
   }
 
   public static BuildSetting create(boolean isFlag, Type<?> type) {
     Preconditions.checkState(
         type.getLabelClass() != LabelClass.DEPENDENCY,
         "Build settings should not create a dependency with their default attribute");
-    return new BuildSetting(isFlag, type, /* allowMultiple= */ false);
+    return new BuildSetting(isFlag, type, /* allowMultiple= */ false, false);
   }
 
   public Type<?> getType() {
@@ -58,6 +61,10 @@ public boolean allowsMultiple() {
     return allowMultiple;
   }
 
+  public boolean isRepeatableFlag() {
+    return repeatable;
+  }
+
   @Override
   public void repr(Printer printer) {
     printer.append("<build_setting." + type + ">");
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java b/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java
index 1d9eb89d490305..0c60ee576e0541 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java
@@ -168,6 +168,9 @@ public void parse(ExtendedEventHandler eventHandler) throws OptionsParsingExcept
             String.format("Unrecognized option: %s=%s", loadedFlag, unparsedValue));
       }
       Type<?> type = buildSetting.getType();
+      if (buildSetting.isRepeatableFlag()) {
+        type = Preconditions.checkNotNull(type.getListElementType());
+      }
       Converter<?> converter = BUILD_SETTING_CONVERTERS.get(type);
       Object value;
       try {
@@ -179,7 +182,7 @@ public void parse(ExtendedEventHandler eventHandler) throws OptionsParsingExcept
                 loadedFlag, unparsedValue, unparsedValue, type),
             e);
       }
-      if (buildSetting.allowsMultiple()) {
+      if (buildSetting.allowsMultiple() || buildSetting.isRepeatableFlag()) {
         List<Object> newValue;
         if (buildSettingWithTargetAndValue.containsKey(loadedFlag)) {
           newValue =
@@ -371,7 +374,8 @@ public OptionsParser getNativeOptionsParserFortesting() {
   }
 
   public boolean checkIfParsedOptionAllowsMultiple(String option) {
-    return parsedBuildSettings.get(option).allowsMultiple();
+    BuildSetting setting = parsedBuildSettings.get(option);
+    return setting.allowsMultiple() || setting.isRepeatableFlag();
   }
 
   public Type<?> getParsedOptionType(String option) {
diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkConfigApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkConfigApi.java
index 1b5b9e79d2808b..bba02e72ea9b5e 100644
--- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkConfigApi.java
+++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkConfigApi.java
@@ -19,6 +19,7 @@
 import net.starlark.java.annot.ParamType;
 import net.starlark.java.annot.StarlarkBuiltin;
 import net.starlark.java.annot.StarlarkMethod;
+import net.starlark.java.eval.EvalException;
 import net.starlark.java.eval.NoneType;
 import net.starlark.java.eval.StarlarkValue;
 
@@ -90,11 +91,13 @@ public interface StarlarkConfigApi extends StarlarkValue {
             name = "allow_multiple",
             defaultValue = "False",
             doc =
-                "If set, this flag is allowed to be set multiple times on the command line. The"
-                    + " Value of the flag as accessed in transitions and build setting"
-                    + " implementation function will be a list of strings. Insertion order and"
-                    + " repeated values are both maintained. This list can be post-processed in the"
-                    + " build setting implementation function if different behavior is desired.",
+                "Deprecated, use a <code>string_list</code> setting with"
+                    + " <code>repeatable = True</code> instead. If set, this flag is allowed to be"
+                    + " set multiple times on the command line. The Value of the flag as accessed"
+                    + " in transitions and build setting implementation function will be a list of"
+                    + " strings. Insertion order and repeated values are both maintained. This list"
+                    + " can be post-processed in the build setting implementation function if"
+                    + " different behavior is desired.",
             named = true,
             positional = false)
       })
@@ -111,9 +114,20 @@ public interface StarlarkConfigApi extends StarlarkValue {
             defaultValue = "False",
             doc = FLAG_ARG_DOC,
             named = true,
+            positional = false),
+        @Param(
+            name = "repeatable",
+            defaultValue = "False",
+            doc =
+                "If set, instead of expecting a comma-separated value, this flag is allowed to be"
+                    + " set multiple times on the command line with each individual value treated"
+                    + " as a single string to add to the list value. Insertion order and repeated"
+                    + " values are both maintained. This list can be post-processed in the build"
+                    + " setting implementation function if different behavior is desired.",
+            named = true,
             positional = false)
       })
-  BuildSettingApi stringListSetting(Boolean flag);
+  BuildSettingApi stringListSetting(Boolean flag, Boolean repeatable) throws EvalException;
 
   /** The API for build setting descriptors. */
   @StarlarkBuiltin(
diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeConfigApi.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeConfigApi.java
index f40afe104280dd..0582beb4d71b73 100644
--- a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeConfigApi.java
+++ b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeConfigApi.java
@@ -38,7 +38,7 @@ public BuildSettingApi stringSetting(Boolean flag, Boolean allowMultiple) {
   }
 
   @Override
-  public BuildSettingApi stringListSetting(Boolean flag) {
+  public BuildSettingApi stringListSetting(Boolean flag, Boolean repeated) {
     return new FakeBuildSettingDescriptor();
   }
 


Test patch:
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 02813807ab0e05..d6f5782143e727 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
@@ -1534,6 +1534,51 @@ public void buildsettings_allowMultipleWorks() throws Exception {
     assertThat(getConfigMatchingProvider("//test:match").matches()).isTrue();
   }
 
+  @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(
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 0e93717352e940..6579eec0c1b3a5 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 @@ public void testAllowMultipleStringFlag() throws Exception {
     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 94964936fa1ae1..b87da48480fdc2 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
@@ -2960,6 +2960,66 @@ public void testBuildSettingValue_allowMultipleSetting() throws Exception {
         .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(