Allow literal default for allow_multiple build setting in transitions

[fmeum]

Instead of requiring the new value for a build setting with
allow_multiple = True specified by a transition to always be a list, we
make an exception for the literal default value for two reasons:

1. It ensures that specifying a setting with allows multiple as an input
   but not as an output of a transition does not run into the exception
   below.
2. It provides a clear way to reset the setting to its (literal) default.

Fixes #14894

RELNOTES: Fixed a bug that prevented build settings with
allow_multiple = True from appearing only as inputs but not as
outputs of a transition.

[Wyverald]

@gregestren @lberki could we get this looked at? This potentially fixes a 5.0 regression and it would great to have it in 5.1.

[gregestren]

@Wyverald there are a couple of PRs and issues (at least 4) related to allow_multiple. I'm trying to wrap my head around them. It's on my agenda but that's why I haven't responded yet.

I'll comment on them by today but I'm not ready as of this message to advocate any specific step forward.

[gregestren]

To be clear:

• config.string with allow_multiple=True should use list for build_setting_default #13817
• Build settings defined with allow_multiple = True cannot pass their default values to transition inputs without also being outputs #14894
• Allow literal default for allow_multiple build setting in transitions #14908 (this)
• Allow string_list flags to be set via repeated flag uses #14911

[gregestren]

Generally speaking, how much of this PR remains relevant after #14911?

[gregestren]

It really took me some time to realize changedSettingToRule from line 310 is now a misleading name. As I understand, the reality is:

1. changedSettingToRule was originally written to encapsulate settings the transition changed
2. build_setting inputs are implicitly changed by transition code as an implementation detail, from non-existent to their explicit default values, to guarantee the transition can read them
3. Trim input build settings after transition #13971 causes the original logic, which historically only checked user-changed settings, to also include these implicit changes. For the purpose of guaranteeing the default that the transition added are then removed.

I'm worried about these overloading of concepts, even if it keeps the code concise.

What if we broke out the logic some: revert https://github.com/bazelbuild/bazel/pull/13971/files, then do a separate distinct call to getRelevantStarlarkSettingsFromTransition(Settings.INPUTS) which only replicates the part of validate that removes an option if it's at its default value?

I realize that takes more refactoring, likely breaking out a helper method for validate(). But I'd really like to keep the code as clear as possible in transition logic, since it's super-complicated as it is.

At the very least I'd like the naming and commenting (including method Javadoc) to remain accurate. I worry it could be more work to clearly make the commenting accurate than do the refactoring I'm suggesting.

[fmeum]

That's a very good point - input build settings should be cleared unconditionally rather than undergo validation. I created #14972 for this refactoring and a better fix for #14894.

[gregestren]

2. here feels a bit API-shaky to me. Basically, a transition has to set a setting to a certain type unless it's set to a certain special value in which case it's exempt. I get the intended use case described here. But I can also see transitions that, say, read their values from attributes, which happen to set values to defaults - or not - by coincidence. i.e. whoever's setting the values has no insight into this and may be confused why some arbitrary string works while another fails.

To the point, do you think #14911 obsoletes all this complexity?

[fmeum]

I agree it looks very hacky and is another reason to consider #14972 the better kind of fix. That said, I don't think this accidental resetting would be very realistic: The "magic" literal default value is the only valid one of its type (string rather than list of string) due to type validation and any attribute would only ever yield one type.

To the point, do you think #14911 obsoletes all this complexity?

The new representation of repeated string flags would no longer require any special handling for transitions, so we could eventually delete the entire allowsMultiple branch. But given that finalizing the design of #14911 will take some more time, either this PR or #14972 are probably needed to deal with the regression for 5.1.0.

[gregestren]

Agreed on your last point - I'm happy to fast track a temporary fix for the regression. I just wanted to verify the long-term expected state.

[fmeum]

Superseded by #14972.