Add rule to enforce parentheses in `a or b and c` #9440

AlexWaygood · 2024-01-08T15:22:45Z

Summary

This implements the rule proposed in #8721, as RUF021. and always binds more tightly than or when 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

github-actions · 2024-01-08T15:41:58Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+93 -0 violations, +0 -0 fixes in 11 projects; 32 projects unchanged)

DisnakeDev/disnake (+5 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ disnake/ext/commands/converter.py:1168:12: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ disnake/interactions/base.py:1880:21: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ disnake/interactions/base.py:1905:21: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ disnake/interactions/base.py:1923:18: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ disnake/interactions/base.py:213:17: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear

RasaHQ/rasa (+6 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ rasa/cli/utils.py:74:27: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ rasa/core/processor.py:587:17: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ rasa/shared/core/training_data/structures.py:619:17: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ rasa/shared/core/training_data/structures.py:621:20: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ rasa/utils/train_utils.py:534:9: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ rasa/utils/train_utils.py:536:12: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear

apache/airflow (+15 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview --select ALL

+ airflow/metrics/validators.py:217:16: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ airflow/models/dag.py:3222:17: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ airflow/models/dagrun.py:1070:20: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ airflow/operators/python.py:369:16: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ airflow/providers/cncf/kubernetes/operators/pod.py:389:16: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ airflow/providers/oracle/hooks/oracle.py:298:38: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ airflow/serialization/serde.py:220:8: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ airflow/www/utils.py:827:20: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ airflow/www/utils.py:840:20: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ airflow/www/validators.py:50:32: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
... 5 additional changes omitted for project

bokeh/bokeh (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview --select ALL

+ src/bokeh/core/property/dataspec.py:334:35: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ src/bokeh/plotting/_graph.py:74:8: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear

ibis-project/ibis (+4 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ ibis/expr/analysis.py:339:20: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ ibis/expr/datatypes/cast.py:122:9: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ ibis/expr/datatypes/cast.py:124:12: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ ibis/expr/datatypes/cast.py:127:12: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear

milvus-io/pymilvus (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ pymilvus/client/check.py:238:26: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear

pandas-dev/pandas (+33 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ asv_bench/benchmarks/groupby.py:514:13: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ pandas/_testing/asserters.py:754:13: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ pandas/_testing/asserters.py:756:16: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ pandas/_testing/asserters.py:911:13: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ pandas/_testing/asserters.py:913:16: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ pandas/core/arrays/arrow/array.py:1407:20: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ pandas/core/computation/eval.py:340:16: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ pandas/core/computation/expr.py:510:13: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ pandas/core/computation/pytables.py:410:13: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ pandas/core/computation/pytables.py:412:16: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ pandas/core/dtypes/dtypes.py:1061:13: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ pandas/core/dtypes/dtypes.py:1714:21: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ pandas/core/frame.py:4035:17: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ pandas/core/frame.py:6894:16: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ pandas/core/indexes/range.py:1042:34: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ pandas/core/internals/construction.py:493:24: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ pandas/core/resample.py:2126:13: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
... 16 additional changes omitted for project

python-poetry/poetry (+3 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ src/poetry/console/io/inputs/run_argv_input.py:48:38: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ src/poetry/utils/dependency_specification.py:161:16: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ src/poetry/utils/env/env_manager.py:235:33: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear

rotki/rotki (+17 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ rotkehlchen/accounting/export/csv.py:155:13: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ rotkehlchen/accounting/export/csv.py:156:13: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ rotkehlchen/api/rest.py:1076:17: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ rotkehlchen/api/rest.py:1077:17: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ rotkehlchen/api/v1/schemas.py:3223:13: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ rotkehlchen/api/v1/schemas.py:3224:13: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ rotkehlchen/assets/utils.py:261:17: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ rotkehlchen/chain/ethereum/modules/uniswap/v2/common.py:134:17: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ rotkehlchen/chain/ethereum/modules/yearn/vaults.py:153:9: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ rotkehlchen/chain/evm/decoding/metamask/decoder.py:75:17: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
... 7 additional changes omitted for project

scikit-build/scikit-build-core (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ tests/test_settings_overrides.py:401:45: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear

zulip/zulip (+6 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview --select ALL

+ tools/lib/provision.py:163:4: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ tools/lib/provision.py:163:51: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ tools/lib/template_parser.py:677:52: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ zerver/lib/cache.py:656:12: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ zerver/models/users.py:1049:9: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
+ zerver/models/users.py:1051:12: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
RUF021	93	93	0	0	0

AlexWaygood · 2024-01-08T15:44:20Z

ruff-ecosystem results

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 ruff ecosystem report at the moment). That's much more than any of the ones in the report here. Maybe AST munging in Python is particularly likely to end up with long comparison chains? 🤷

AlexWaygood · 2024-01-08T15:59:36Z

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 ruff format nor Black would strip away the new parentheses.)

diff --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)
         ):

AlexWaygood · 2024-01-08T16:14:29Z

I skimmed through most of the ecosystem results; I can't see any false positives.

crates/ruff_linter/src/rules/ruff/rules/parenthesize_logical_operators.rs

BurntSushi · 2024-01-08T17:40:24Z

Looking through the ecosystem results, I see a few instances of the x and y or z idiom flagged. You also commonly see it used as x and y or None. I wonder if we want to flag those? AIUI, it's a somewhat common tactic for a ternary operator in Python, although it has some gotchas.

AlexWaygood · 2024-01-08T18:02:24Z

Looking through the ecosystem results, I see a few instances of the x and y or z idiom flagged. You also commonly see it used as x and y or None. I wonder if we want to flag those? AIUI, it's a somewhat common tactic for a ternary operator in Python, although it has some gotchas.

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 x and y or z is less surprising than for x or y and z, so we could exclude it for now. But I lean towards leaving it in -- codebases who find it too opinionated can presumably just not opt into this rule? WDYT?

codspeed-hq · 2024-01-08T18:03:13Z

CodSpeed Performance Report

Merging #9440 will not alter performance

_{Comparing AlexWaygood:force-parens-lint (ab36c83) with main (f419af4)}

Summary

✅ 30 untouched benchmarks

crates/ruff_linter/src/rules/ruff/rules/parenthesize_logical_operators.rs

charliermarsh

We could probably add an (unsafe) fix for this, if you're interested, by inserting parentheses around the expression.

AlexWaygood · 2024-01-08T18:06:11Z

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?

BurntSushi · 2024-01-08T18:10:20Z

Looking through the ecosystem results, I see a few instances of the x and y or z idiom flagged. You also commonly see it used as x and y or None. I wonder if we want to flag those? AIUI, it's a somewhat common tactic for a ternary operator in Python, although it has some gotchas.

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 x and y or z is less surprising than for x or y and z, so we could exclude it for now. But I lean towards leaving it in -- codebases who find it too opinionated can presumably just not opt into this rule? WDYT?

I don't think I have a strong opinion here honestly.

AlexWaygood · 2024-01-08T18:33:48Z

Merging #9440 will degrade performances by 5.36%

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 BoolOp nodes in Python in general, and we have to visit all of them if this rule is enabled?

charliermarsh · 2024-01-08T18:41:48Z

@AlexWaygood - Can you try rebasing or merging in main? We upgraded the Rust version, so my guess is that it's just comparing against the "wrong" baseline.

crates/ruff_linter/src/rules/ruff/rules/parenthesize_logical_operators.rs

charliermarsh · 2024-01-08T18:52:25Z

Yes, definitely! Would you prefer me to do that as part of this PR, or as a followup?

Separate PR is probably a better practice, since it'll enable this one to get merged sooner, but it's marginal :)

JelleZijlstra · 2024-01-08T18:53:35Z

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 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.

AlexWaygood · 2024-01-08T18:56:25Z

@AlexWaygood - Can you try rebasing or merging in main? We upgraded the Rust version, so my guess is that it's just comparing against the "wrong" baseline.

Looks like that was the issue, thanks 😅 By the way, do you have a preference between rebases or merge commits?

Separate PR is probably a better practice, since it'll enable this one to get merged sooner, but it's marginal :)

Expect a followup PR tomorrow :D

charliermarsh · 2024-01-08T18:57:22Z

@AlexWaygood - Within a PR, it's dealer's choice. But we always squash-and-merge (it's the only option enabled) when merging into main, such that every PR corresponds to a single commit. I personally tend to rebase my PRs.

AlexWaygood · 2024-01-08T23:44:49Z

Fixed the crash I accidentally introduced in 87ddff6. The ecosystem check is looking sane again now.

charliermarsh · 2024-01-09T01:28:10Z

Very nice!

## Summary This adds an autofix for the newly added RUF021 (see #9440).

konstin · 2024-01-11T11:42:24Z

Re x and y or z, we have a preview rule, and-or-ternary (PLR1706), linting and fixing that case.

AlexWaygood added 2 commits January 8, 2024 15:17

Add rule to enforce parentheses in a or b and c

6ce42ca

grammar

e473e82

BurntSushi approved these changes Jan 8, 2024

View reviewed changes

crates/ruff_linter/src/rules/ruff/rules/parenthesize_logical_operators.rs Show resolved Hide resolved

crates/ruff_linter/src/rules/ruff/rules/parenthesize_logical_operators.rs Show resolved Hide resolved

markdown

d4395af