Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Starlark: Disallow conversion of select to bool #14506

Open
criemen opened this issue Jan 3, 2022 · 5 comments
Open

Starlark: Disallow conversion of select to bool #14506

criemen opened this issue Jan 3, 2022 · 5 comments
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Loading-API BUILD file and macro processing: labels, package(), visibility, glob type: feature request

Comments

@criemen
Copy link
Contributor

criemen commented Jan 3, 2022

Description of the problem / feature request:

The return type of select converts silently in truthy contexts to True.
This can lead to the following code being legal, but not doing what the programmer intended to do:

def is_linux():
    return select({
        "@platforms//os:linux": True,
        "//conditions:default": False
    })

def my_macro()
    if is_linux():
        # do one thing
    else:
        # do another thing

This can lead to behavior that bazel novices would think of as unexpected (the if always evaluates to the true branch).
Instead, bazel should emit a warning or an error whenever the return value of select gets converted to a bool.
I cannot imagine any valid use case where the return of a select should be converted to a truth value (it's always True anyways), so taking this behavior away, while a breaking change, would be an improvement for bazel.

Feature requests: what underlying problem are you trying to solve with this feature?

Improve the user experience for a more confusing part of starlark (selects), by providing a clear error on a beginner mistake.

Have you found anything relevant by searching the web?

The documentation calls this starlark behavior out, but I believe that in this case, starlark should diverge from the pythonic way that everything silently converts to bool without fail.
The reasoning for that is mainly that understanding select and the bazel/starlark execution semantics is difficult enough as-is, and this change would remove a pitfall.

@gregestren
Copy link
Contributor

I appreciate your point.

I believe core Starlark devs are very reluctant to deviate from Pythonic standards. select() in particular really messes with those standards. But the confusion you're expressing is legitimate.

@brandjon brandjon added P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) and removed untriaged labels Feb 10, 2022
@brandjon
Copy link
Member

We can't do this currently in the Starlark language because all types must be truth-able. Filed bazelbuild/starlark#211 for the language implications.

I believe core Starlark devs are very reluctant to deviate from Pythonic standards.

We don't necessarily need to support it just because Python does. For example, we deviate to be more restrictive than Python in other situations, e.g. implicit string concatenation ('a' 'b' == 'ab'). Although that's a static error, not a dynamic one. I'm more worried about breaking generic code that wants to truth check things without fear of breaking.

I think the best approach is to just carefully document this trap, and consider this problem alongside the more general issues we have with navigating the API of select() in macros.

@gregestren
Copy link
Contributor

Just re-emphasizing (@criemen already noted) that we document the current behavior at https://bazel.build/docs/configurable-attributes#faq-boolean-select.

@brandjon brandjon added team-Loading-API BUILD file and macro processing: labels, package(), visibility, glob and removed team-Build-Language labels Nov 4, 2022
Copy link

github-actions bot commented Jan 9, 2024

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-bazeler". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Jan 9, 2024
@brandjon brandjon added P2 We'll consider working on this in future. (Assignee optional) and removed P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) labels Mar 27, 2024
@brandjon
Copy link
Member

Update in late Q1 2024: We've decided that we should ban implicit conversion of select() values to boolean. Explicit conversions via the bool() operator would likely still be allowed.

I'm not sure if there's any corner cases that might surprise us as a consequence of having an non-automatically-boolable type, but it seems like it shouldn't be a big concern.

The motivation for this is mainly to allow the upcoming symbolic macros feature to (#11992) to automatically promote raw values to select() values without inadvertently causing Starlark code to have surprising changes in behavior that do not fail-fast.

Probably targeting Q2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Loading-API BUILD file and macro processing: labels, package(), visibility, glob type: feature request
Projects
None yet
Development

No branches or pull requests

4 participants