config.string with allow_multiple=True should use list for build_setting_default

[Yannic]

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.

# 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.

[gregestren]

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.

[fmeum]

@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.

[fmeum]

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.

[gregestren]

Thanks. I'll take a look. Also cc @aranguyen FYI who's been looking recently at flag parsing logic in general.

[aranguyen]

@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.

[fmeum]

@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.