Allow string_list flags to be set via repeated flag uses (bazelbuild#…

…15943)

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 bazelbuild#13817

Closes bazelbuild#14911.

PiperOrigin-RevId: 462146752
Change-Id: I91ddc4328a1b2244f4303fcda7b4228b182a5d56

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>

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

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 + ">");

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) {

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(

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();
   }
 

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(

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");
+  }
 }

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(