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

C413 fix for reversed(sorted(..., reverse=...)) needs parentheses when key is a boolop to avoid changing semantics #10335

Closed
jakkdl opened this issue Mar 11, 2024 · 1 comment · Fixed by #10346
Assignees
Labels
bug Something isn't working fixes Related to suggested fixes for violations help wanted Contributions especially welcome

Comments

@jakkdl
Copy link

jakkdl commented Mar 11, 2024

reversed(sorted([1, 2, 3], reverse=False or True)) # reverses twice -> 1, 2, 3
# ruff C413 autofixes the above line to the below
sorted([1, 2, 3], reverse=not False or True) # reverses once -> 3, 2, 1
# if a paren were to be added, the problem is resolved
sorted([1, 2, 3], reverse=not (False or True)) # doesn't reverse -> 1, 2, 3

Due to https://docs.python.org/3/reference/expressions.html#operator-precedence
This affects boolean operators and if-expr's. Higher-precedented operators won't need parens added, walruses need to be wrapped in parens in the original statement, and lambda's are invalid in this context.

command invocation & version

$ ruff check --select=C413 --fix --unsafe-fixes --isolated - <<< 'reversed(sorted([1, 2, 3], reverse=True if True else False))' 
sorted([1, 2, 3], reverse=not True if True else False)
Found 1 error (1 fixed, 0 remaining).
$ ruff --version
ruff 0.3.2
@AlexWaygood AlexWaygood added fixes Related to suggested fixes for violations bug Something isn't working labels Mar 11, 2024
@zanieb zanieb added the help wanted Contributions especially welcome label Mar 11, 2024
@zanieb
Copy link
Member

zanieb commented Mar 11, 2024

Thanks for the report! Your assessment looks right to me and this is notably not the reason this fix is marked as unsafe.

@charliermarsh charliermarsh self-assigned this Mar 11, 2024
charliermarsh added a commit that referenced this issue Mar 11, 2024
## Summary

When negating an expression like `a or b`, we need to wrap it in
parentheses, e.g., `not (a or b)` instead of `not a or b`, due to
operator precedence.

Closes #10335.

## Test Plan

`cargo test`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixes Related to suggested fixes for violations help wanted Contributions especially welcome
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants