Allow multiple matching select branches if they resolve to the same value

[AlessandroPatti]

Select branches that map to the same value can unambiguosly be resolved even when multiple are true. This is currently not allowed and requires the use of constructs like bazel-skylib's selects.config_setting_group. With this change, assuming A and B are true, the following is allowed:

select({
     "//:A": "Value",
     "//:B": "Value",
})

[gregestren]

Interesting proposal.

What use cases inspired it?

I'm reluctant to loosen the current strict matching requirements without a good, thorough discussion (happy to have it here). My main conceptual worry is there's also meaningful signal knowing - decisively and unambiguously - which branch matches. For example, for select()s where the values may change over time (from regular refactoring) but the conditions are conceptual and static.

I appreciate that strict interpretation results in awkward workarounds a you describe. And we already have one instance of multiple matches when one match is a clear specialization of the other. Although even in that case a single branch is clearly selected by design, and without having to consider the values.

[keith]

FWIW I have hit this many times in complex select statements with envoy where you may want to do specific flags for a platform, and for a compilation mode. In general we have worked around them at this point but here are some complex examples https://github.com/envoyproxy/envoy/blob/4a03e625960267032577f6792eef41d715b87b40/bazel/envoy_binary.bzl#L56-L117

[AlessandroPatti]

@gregestren the main rationale is complexity. As Keith mentioned, conditions can easily get quite complex, especially when combining multiple together.
As an example, consider having a string_list_flag that can assume several values (e.g. v1, ..., v10). You can workaround the limitation by creating several config_setting_groups like v1-or-v2-or-v3, but you'd have to create all possible combinations. With this change you just need one config_setting per possible value (orders of magnitudes less targets).

[gregestren]

@AlessandroPatti can you trace your your example a bit more explicitly? Apologies for not parsing it more quickly. I want to make sure I fully understand the relationship you're describing between flag values, config_setting, select, and the values in select.

@keith would select.with_or or config_setting_group be insufficient for your cases?

[keith]

I wish I had a better example off the top of my head, since we had to work around most of them already. In general the situation has been we've had a bunch of feature flags in the envoy build, some of which support specific platforms only, so when selects have both platform specifics, and feature flag specifics, we ended up with this conflict.

Maybe this case could be refactored now though? https://github.com/envoyproxy/envoy/blob/b8c4d4bae0c48c2a97126e486651987ceab7b493/bazel/BUILD#L219-L282

The other thing I worry about is the combinatorial explosion of config_setting_groups if we need use that for disambiguation.

[AlessandroPatti]

@gregestren sorry I wasn't clear before, I'll try to expand on that here. I was suggesting a case with a string_list_flag like the following:

VALUES = ["v"+ str(i) for i in range(9)]
string_list_flag(
    name = "setting",
    build_setting_default = VALUES,
)
[
    config_setting(
        name = v,
        flag_values = {
            "//:setting": v,
        }
    )
    for v in VALUES
]

which can be set on the command like like --//:setting=v1,v2,v3, with multiple being enabled at the same time. Then this can be used to declare compatibility of some targets, e.g.

my_rule(
    ...
    target_compatible_with = select({
        "//:v1-or-v5-or-v6": [],
        "//conditions:defaults": ["@platforms//:incompatible"],
    }),
)

The complexity araises when creating the targets like //:v1-or-v5-or-v6. Since all combinations are possible, you need to do something like the following in the BUILD file

load("//:utils.bzl", "powerset")
[
    selects.config_setting_group(
        # Sort values to a deterministic name. Note that this mean that `v1-or-v3` is valid but `v3-or-v1` is not
        name = "-or-".join(sorted(group)),
        match_any = ["//:{}".format(v) for v in group],
    )
    for group in powerset(VALUES)
    # exclude empty group and unary groups which are already defined as `config_settings` above
    if len(group) > 1
]

The above requires implementing powerset and ends up creating 2^n targets since selects.config_setting_group is implemented as a chain of alias targets. Athough there is workaround, it'd be much simpler to just be able to write the above select as

select({
    "//:{}".format(v): [],
    for v in ["v1", "v5", "v6"],
})

[AlessandroPatti]

@gregestren any update on this?

[gregestren]

I discussed this with other devs last week. Let me follow up soon to your last comments - I'll aim for tomorrow or the day after. I appreciate your patience.

[gregestren]

Caught up.

1. Is being able to set multiple values at the same time important to your scenario? If it was just a string_flag that can only be set to --//:setting=v2, does that also cover your concern? Even in that case //:v1-or-v5-or-v6 has the same meaning as I'm reading it.

   I'm asking this to see if we can separate the concerns of how select() matches multi-value flags (which values match a single condition?) vs. multiple condition matching (which conditions match across the select()?)

2. All combinations are possible, but presumably all combinations won't be attached to rules. Why not just write ad hoc config_setting_groups for whatever combinations a rule needs? Or wrap that behind a macro for easier syntax?

[sgowroji]

Hello @AlessandroPatti , Can you please take a look on above comments and reply. Thanks!

[AlessandroPatti]

@sgowroji @gregestren Sorry, I lost track of this.

Is being able to set multiple values at the same time important to your scenario? If it was just a string_flag that can only be set to --//:setting=v2, does that also cover your concern? Even in that case //:v1-or-v5-or-v6 has the same meaning as I'm reading it.

Yes, multiple value could be set in the scenario I was describing, and indeed defining and using //:v1-or-v5-or-v6 is a valid alternative.

All combinations are possible, but presumably all combinations won't be attached to rules. Why not just write ad hoc config_setting_groups for whatever combinations a rule needs? Or wrap that behind a macro for easier syntax?

Yes, this is what I did in practice, but I found this to still be more complex than creating a simple select with the individual values (i.e. v1, v2 instead of v1-or-v2)

AFAIK, this is just a matter of convenience, using config_setting_group is possible and I can't think of a case that would not be covered by it. I found this approach easier to implement, but I agree it might create ambiguity on which branch gets selected. If this is a concern, please feel free to close this.

[gregestren]

@aiuto revived this request to me. I see I was the last one to drop the ball - apologies.

I'm open to this if people still want it. My main followup comments would be updating documentation. And one pedantic technical nitpick about which condition is chosen in the code and if it matters?

[AlessandroPatti]

My main followup comments would be updating documentation.

Done, I think. Please let me know if I missed any other place that needs update.

And one pedantic technical nitpick about which condition is chosen in the code and if it matters?

I think this is already deterministic: the SelectorList should maintain an ordered list of conditions and the matching ones are stored into a LinkedHashMap, which should as well maintain the order. I've change the variable type to also be a LinkedHashMap and left a small comment to make this as explicit as possible, but we should always select the first branch that matches. I think this doesn't make much of a difference in practice but it is good to resolve ambiguity.

[gregestren]

Just one small nit (for a wrinkle you're not responsible for).

Otherwise LGTM, and thanks for persevering through this.

[fmeum]

@bazel-io flag

[ShreeM01]

@bazel-io fork 6.2.0