Add rule to enforce parentheses in a or b and c#9440
Add rule to enforce parentheses in a or b and c#9440charliermarsh merged 6 commits intoastral-sh:mainfrom
a or b and c#9440Conversation
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| RUF021 | 93 | 93 | 0 | 0 | 0 |
I found 45 errors on the mypy codebase when running ruff manually with this branch, interestingly (I guess mypy isn't one of the repos in the |
|
I did a bit of manual testing with some of the mypy hits I found from running ruff manually with this branch. In particular, this span triggers six instances(!) of the new warning: https://github.com/python/mypy/blob/35f402c74205d373b81076ea82f507eb85161a25/mypy/messages.py#L2940-L2955 Applying this diff to mypy fixes all of them, while maintaining an identical Python AST. (And, neither Detailsdiff --git a/mypy/messages.py b/mypy/messages.py
index 450faf4c1..7dee269b1 100644
--- a/mypy/messages.py
+++ b/mypy/messages.py
@@ -2938,20 +2938,16 @@ def get_bad_protocol_flags(
bad_flags = []
for name, subflags, superflags in all_flags:
if (
- IS_CLASSVAR in subflags
- and IS_CLASSVAR not in superflags
- and IS_SETTABLE in superflags
- or IS_CLASSVAR in superflags
- and IS_CLASSVAR not in subflags
- or IS_SETTABLE in superflags
- and IS_SETTABLE not in subflags
- or IS_CLASS_OR_STATIC in superflags
- and IS_CLASS_OR_STATIC not in subflags
- or class_obj
- and IS_VAR in superflags
- and IS_CLASSVAR not in subflags
- or class_obj
- and IS_CLASSVAR in superflags
+ (
+ IS_CLASSVAR in subflags
+ and IS_CLASSVAR not in superflags
+ and IS_SETTABLE in superflags
+ )
+ or (IS_CLASSVAR in superflags and IS_CLASSVAR not in subflags)
+ or (IS_SETTABLE in superflags and IS_SETTABLE not in subflags)
+ or (IS_CLASS_OR_STATIC in superflags and IS_CLASS_OR_STATIC not in subflags)
+ or (class_obj and IS_VAR in superflags and IS_CLASSVAR not in subflags)
+ or (class_obj and IS_CLASSVAR in superflags)
): |
|
I skimmed through most of the ecosystem results; I can't see any false positives. |
|
Looking through the ecosystem results, I see a few instances of the |
Hmmm... I know it's reasonably common, especially in older codebases, but I'd honestly consider that something of an antipattern due to those gotchas. I'd definitely object to it if somebody tried submitting a CPython PR with it 😄 I suppose the precedence for |
CodSpeed Performance ReportMerging #9440 will not alter performanceComparing Summary
|
charliermarsh
left a comment
There was a problem hiding this comment.
We could probably add an (unsafe) fix for this, if you're interested, by inserting parentheses around the expression.
Yes, definitely! Would you prefer me to do that as part of this PR, or as a followup? |
I don't think I have a strong opinion here honestly. |
That seems suboptimal... Are there any things I'm doing here that are obviously bad, performance-wise? Is this just because there are loads of |
|
@AlexWaygood - Can you try rebasing or merging in |
Separate PR is probably a better practice, since it'll enable this one to get merged sooner, but it's marginal :) |
I think it's mostly a legacy pattern from before the if-else ternary was added to Python (which was Python 2.5). I'd be OK with having a linter flag this pattern. |
Looks like that was the issue, thanks 😅 By the way, do you have a preference between rebases or merge commits?
Expect a followup PR tomorrow :D |
|
@AlexWaygood - Within a PR, it's dealer's choice. But we always squash-and-merge (it's the only option enabled) when merging into |
|
Fixed the crash I accidentally introduced in 87ddff6. The ecosystem check is looking sane again now. |
|
Very nice! |
## Summary This adds an autofix for the newly added RUF021 (see #9440).
|
Re |
Fixes #8721
Summary
This implements the rule proposed in #8721, as RUF021.
andalways binds more tightly thanorwhen chaining the two together.(This should definitely be autofixable, but I'm leaving that to a followup PR for now.)
Test Plan
cargo test/cargo insta review