Fix SIM222 and SIM223 false positives and auto-fix by JonathanPlasse · Pull Request #4063 · astral-sh/ruff

JonathanPlasse · 2023-04-22T14:30:24Z

Close SIM222 False positive #4058

Special case truthy expressions like side-effect expressions.
If a truthy expression is followed by the short-circuiting boolean, remove the short-circuiting boolean too as it is unnecessary.
The violation message should be changed in this case to something like "remove everything after the truth expression".

assert ("foo" and False) == "foo"
assert ("foo" or True) == "foo"

I use Truthiness from flake8-bandit, but flake8-pytest also has is_falsy_constant().
Should we make extract Truthiness from flake8-bandit, add the missing cases from flake8-pytest, and use it for flake8-bandit, flake8-pytest, and flake8-simplify?
I would be open making another PR for this.

github-actions · 2023-04-22T14:51:15Z

PR Check Results

Ecosystem

ℹ️ ecosystem check detected changes. (+0, -1, 0 error(s))

airflow (+0, -1)

- airflow/providers/apache/kafka/operators/consume.py:100:29: SIM222 [*] Use `True` instead of `... or True`

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.00     15.5±0.04ms     2.6 MB/sec    1.00     15.4±0.02ms     2.6 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      3.9±0.01ms     4.3 MB/sec    1.00      3.9±0.01ms     4.3 MB/sec
linter/all-rules/numpy/globals.py          1.03    431.4±8.32µs     6.8 MB/sec    1.00    420.7±1.34µs     7.0 MB/sec
linter/all-rules/pydantic/types.py         1.01      6.6±0.01ms     3.9 MB/sec    1.00      6.6±0.01ms     3.9 MB/sec
linter/default-rules/large/dataset.py      1.02      8.2±0.02ms     4.9 MB/sec    1.00      8.1±0.01ms     5.0 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01   1791.0±3.17µs     9.3 MB/sec    1.00   1775.1±4.08µs     9.4 MB/sec
linter/default-rules/numpy/globals.py      1.00    184.6±0.93µs    16.0 MB/sec    1.00    185.5±1.35µs    15.9 MB/sec
linter/default-rules/pydantic/types.py     1.01      3.7±0.01ms     6.8 MB/sec    1.00      3.7±0.01ms     6.9 MB/sec
parser/large/dataset.py                    1.00      6.4±0.01ms     6.4 MB/sec    1.01      6.5±0.00ms     6.3 MB/sec
parser/numpy/ctypeslib.py                  1.00   1257.8±1.34µs    13.2 MB/sec    1.01   1272.6±1.50µs    13.1 MB/sec
parser/numpy/globals.py                    1.00    126.4±0.35µs    23.4 MB/sec    1.01    127.5±0.30µs    23.1 MB/sec
parser/pydantic/types.py                   1.00      2.8±0.00ms     9.3 MB/sec    1.01      2.8±0.00ms     9.2 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.01     17.6±0.33ms     2.3 MB/sec    1.00     17.4±0.29ms     2.3 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.4±0.07ms     3.8 MB/sec    1.02      4.5±0.08ms     3.7 MB/sec
linter/all-rules/numpy/globals.py          1.00   546.1±11.96µs     5.4 MB/sec    1.00   548.7±10.70µs     5.4 MB/sec
linter/all-rules/pydantic/types.py         1.00      7.3±0.10ms     3.5 MB/sec    1.01      7.4±0.11ms     3.4 MB/sec
linter/default-rules/large/dataset.py      1.00      8.8±0.06ms     4.6 MB/sec    1.03      9.1±0.14ms     4.5 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1925.1±18.13µs     8.6 MB/sec    1.03  1976.8±26.42µs     8.4 MB/sec
linter/default-rules/numpy/globals.py      1.00    216.5±3.16µs    13.6 MB/sec    1.03   223.7±13.01µs    13.2 MB/sec
linter/default-rules/pydantic/types.py     1.00      4.0±0.06ms     6.4 MB/sec    1.02      4.1±0.06ms     6.2 MB/sec
parser/large/dataset.py                    1.00      7.0±0.07ms     5.8 MB/sec    1.01      7.0±0.04ms     5.8 MB/sec
parser/numpy/ctypeslib.py                  1.00  1326.4±21.77µs    12.6 MB/sec    1.01  1335.3±15.68µs    12.5 MB/sec
parser/numpy/globals.py                    1.00    131.9±2.01µs    22.4 MB/sec    1.01    133.7±2.12µs    22.1 MB/sec
parser/pydantic/types.py                   1.00      3.0±0.04ms     8.4 MB/sec    1.00      3.0±0.03ms     8.4 MB/sec

charliermarsh · 2023-04-22T22:15:18Z

Yeah, I'd vote to extract that concept into ruff_python_semantic and use it in more places!

crates/ruff/src/rules/flake8_simplify/rules/ast_bool_op.rs

crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs

...c/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM222_SIM222.py.snap

...c/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM223_SIM223.py.snap

crates/ruff/src/rules/flake8_simplify/rules/ast_bool_op.rs

JonathanPlasse · 2023-04-23T16:32:40Z

I added in_test to Context.
It is considered to be in a test context when you are inside an if or an assert test, or inside the builtin bool.
Boolean operations inside other expressions than boolean operations are not in a test context.

For example:

# Inside test `a` is simplified

bool(a or [1] or True or [2])

assert a or [1] or True or [2]

if (a or [1] or True or [2]) and (a or [1] or True or [2]):
    pass

# Outside test `a` is not simplified

a or [1] or True or [2]

if (a or [1] or True or [2]) == (a or [1]):
    pass

if f(a or [1] or True or [2]):
    pass

JonathanPlasse · 2023-04-23T17:10:34Z

There is a false positive that has been fixed in airflow.

self.max_messages = max_messages or True

JonathanPlasse · 2023-04-23T19:21:29Z

Should I split this PR?

charliermarsh · 2023-04-23T19:55:20Z

I think it would be helpful. What do you think we can carve out into separate PRs?

JonathanPlasse · 2023-04-23T20:54:29Z

Here is the list of possible PRs

I will rebase this PR on main after the first two are merged.

JonathanPlasse · 2023-04-24T06:13:22Z

The rebase is done.
It is ready for review.

JonathanPlasse · 2023-04-24T06:20:35Z

I will document the rules as we diverge a lot from the behavior of flake8-simplify.

JonathanPlasse · 2023-04-24T10:42:40Z

I documented the rules and detect when ... or True, True or ..., or ... or True or ... is used and adapted the violation message.

JonathanPlasse · 2023-04-25T07:17:13Z

You for your edits, it is much cleaner and clearer now.

MichaReiser approved these changes Apr 22, 2023

View reviewed changes

crates/ruff/src/rules/flake8_simplify/rules/ast_bool_op.rs Outdated Show resolved Hide resolved

charliermarsh reviewed Apr 22, 2023

View reviewed changes

crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs Outdated Show resolved Hide resolved

charliermarsh reviewed Apr 23, 2023

View reviewed changes

...c/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM222_SIM222.py.snap Outdated Show resolved Hide resolved

charliermarsh reviewed Apr 23, 2023

View reviewed changes

...c/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM223_SIM223.py.snap Outdated Show resolved Hide resolved

JonathanPlasse commented Apr 23, 2023

View reviewed changes

crates/ruff/src/rules/flake8_simplify/rules/ast_bool_op.rs Outdated Show resolved Hide resolved

JonathanPlasse changed the title ~~Fix SIM222 SIM223 false positive with truthy value~~ Fix SIM222 and SIM223 false positives and auto-fix Apr 23, 2023

Fix SIM222 and SIM223 false positives and auto-fix

fd084bf

JonathanPlasse added 2 commits April 24, 2023 12:08

Add documentation and improve violation messages

ff2f978

Fix replaced expression form in violation message

c6b1909

JonathanPlasse and others added 7 commits April 24, 2023 14:15

Fix rules message

fa1ad6a

Move fixture with truthiness out of boolean test

8136cbf

Replace by boolean in boolean test

920f0c9

gs

1f4e947

Tweak messages

570e030

Move some responsibilities

4d6bbea

Update some fixtures

425f5b2

charliermarsh enabled auto-merge (squash) April 25, 2023 04:39

charliermarsh merged commit 1f3b0fd into astral-sh:main Apr 25, 2023

charliermarsh added the bug Something isn't working label Apr 25, 2023

JonathanPlasse deleted the fix-sim222-sim223-false-positive-with-truthy-expressions branch April 25, 2023 07:05

Tenzer mentioned this pull request Apr 25, 2023

ruff 0.0.263 Homebrew/homebrew-core#129297

Merged

Conversation

JonathanPlasse commented Apr 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Check Results

Ecosystem

Benchmark

Linux

Windows

Uh oh!

charliermarsh commented Apr 22, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JonathanPlasse commented Apr 23, 2023

Uh oh!

JonathanPlasse commented Apr 23, 2023

Uh oh!

JonathanPlasse commented Apr 23, 2023

Uh oh!

charliermarsh commented Apr 23, 2023

Uh oh!

JonathanPlasse commented Apr 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JonathanPlasse commented Apr 24, 2023

Uh oh!

JonathanPlasse commented Apr 24, 2023

Uh oh!

JonathanPlasse commented Apr 24, 2023

Uh oh!

JonathanPlasse commented Apr 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JonathanPlasse commented Apr 22, 2023 •

edited

Loading

github-actions bot commented Apr 22, 2023 •

edited

Loading

JonathanPlasse commented Apr 23, 2023 •

edited

Loading