[`pycodestyle`] Auto-fix redundant boolean comparison (`E712`) #17090

knavdeep152002 · 2025-03-31T12:04:28Z

This pull request fixes #17014

changes this

from __future__ import annotations

flag1 = True
flag2 = True

if flag1 == True or flag2 == True:
    pass

if flag1 == False and flag2 == False:
    pass

flag3 = True
if flag1 == flag3 and (flag2 == False or flag3 == True):  # Should become: if flag1==flag3 and (not flag2 or flag3)
    pass

if flag1 == True and (flag2 == False or not flag3 == True):  # Should become: if flag1 and (not flag2 or not flag3)
    pass

if flag1 != True and (flag2 != False or not flag3 == True):  # Should become: if not flag1 and (flag2 or not flag3)
    pass


flag = True
while flag == True:  # Should become: while flag
    flag = False

flag = True
x = 5
if flag == True and x > 0:  # Should become: if flag and x > 0
    print("ok")

flag = True
result = "yes" if flag == True else "no"  # Should become: result = "yes" if flag else "no"

x = flag == True < 5

x = (flag == True) == False < 5

to this

from __future__ import annotations

flag1 = True
flag2 = True

if flag1 or flag2:
    pass

if not flag1 and not flag2:
    pass

flag3 = True
if flag1 == flag3 and (not flag2 or flag3):  # Should become: if flag1 == flag3 and (not flag2 or flag3)
    pass

if flag1 and (not flag2 or not flag3):  # Should become: if flag1 and (not flag2 or not flag3)
    pass

if not flag1 and (flag2 or not flag3):  # Should become: if not flag1 and (flag2 or not flag3)
    pass


flag = True
while flag:  # Should become: while flag
    flag = False

flag = True
x = 5
if flag and x > 0:  # Should become: if flag and x > 0
    print("ok")

flag = True
result = "yes" if flag else "no"  # Should become: result = "yes" if flag else "no"

x = flag is True < 5

x = (flag) is False < 5

MichaReiser · 2025-03-31T12:16:46Z

For review, see the comment in #17014 (comment)

ntBre

Thanks for working on this! I added some code-style suggestions in-line.

To your question on the issue, I think it does make sense to restrict this to the single-comparison case, especially to start. I think that's what you have here.

More broadly, I'm a bit worried about making these changes in this helpers file, which is used by multiple rules. The original issue was just about E712. If you can get CI passing here, we can see some results from our ecosystem check and see if any other rules are affected. It could still be an improvement to multiple rules, which would be good, but it does mean it's a larger change.

crates/ruff_python_ast/src/helpers.rs

knavdeep152002 · 2025-04-02T17:57:44Z

Hey, Thanks for the code-style suggestions I have refactored the code accordingly.

On running the tests it has only failed on the E712 rule-related snapshots ( 2 failed ). So should I move the logic to the rule level ( rules/literal_comparison.rs) or keep it in helpers as it may be helpful for other rules.

ntBre · 2025-04-02T18:21:16Z

So should I move the logic to the rule level ( rules/literal_comparison.rs) or keep it in helpers as it may be helpful for other rules.

I'm still not sure. Can you fix the test failures? If the tests run successfully, we will get a helpful comment like this on this PR showing changes in ruff output on a set of ecosystem packages. That will help us to decide if we need to restrict the changes to E712 or if we can change the helper, which might be more convenient or useful.

If there are many changes, or especially if any of them look incorrect, that would strongly suggest limiting the change to E712.

knavdeep152002 · 2025-04-02T18:32:59Z

The below is an example of the failed testcase. Previous snapshot was if res == True was changed to if res is True, Now its if res instead.

Snapshot file: crates/ruff_linter/src/rules/pycodestyle/snapshots/ruff_linter__rules__pycodestyle__tests__E712_E712.py.snap
Snapshot: E712_E712.py
Source: crates/ruff_linter/src/rules/pycodestyle/mod.rs:74
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
    9     9 │
   10    10 │ ℹ Unsafe fix
   11    11 │ 1 1 | #: E712
   12    12 │ 2   |-if res == True:
   13       │-  2 |+if res is True:
         13 │+  2 |+if res:
   14    14 │ 3 3 |     pass
   15    15 │ 4 4 | #: E712
   16    16 │ 5 5 | if res != False:
   17    17 │

I am not sure about snapshots. Should I edit this snapshot and replace it with what is expected? Whats the general approach with snapshots?

ntBre · 2025-04-02T18:44:08Z

You should use cargo-insta to update the snapshots. See the Prerequisites section of the ruff CONTRIBUTING.md file.

github-actions · 2025-04-02T22:02:03Z

`ruff-ecosystem` results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+7 -7 violations, +0 -0 fixes in 1 projects; 54 projects unchanged)

apache/superset (+7 -7 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --no-preview --select ALL

+ tests/integration_tests/charts/api_tests.py:350:13: PERF401 Use `list.extend` to create a transformed list
- tests/integration_tests/charts/api_tests.py:350:13: PERF401 Use a list comprehension to create a transformed list
+ tests/integration_tests/charts/api_tests.py:464:13: PERF401 Use `list.extend` to create a transformed list
- tests/integration_tests/charts/api_tests.py:464:13: PERF401 Use a list comprehension to create a transformed list
+ tests/integration_tests/charts/api_tests.py:515:13: PERF401 Use `list.extend` to create a transformed list
- tests/integration_tests/charts/api_tests.py:515:13: PERF401 Use a list comprehension to create a transformed list
+ tests/integration_tests/dashboards/api_tests.py:1280:13: PERF401 Use `list.extend` to create a transformed list
- tests/integration_tests/dashboards/api_tests.py:1280:13: PERF401 Use a list comprehension to create a transformed list
+ tests/integration_tests/dashboards/api_tests.py:1307:13: PERF401 Use `list.extend` to create a transformed list
- tests/integration_tests/dashboards/api_tests.py:1307:13: PERF401 Use a list comprehension to create a transformed list
+ tests/integration_tests/dashboards/api_tests.py:1441:13: PERF401 Use `list.extend` to create a transformed list
- tests/integration_tests/dashboards/api_tests.py:1441:13: PERF401 Use a list comprehension to create a transformed list
+ tests/integration_tests/dashboards/api_tests.py:1506:13: PERF401 Use `list.extend` to create a transformed list
- tests/integration_tests/dashboards/api_tests.py:1506:13: PERF401 Use a list comprehension to create a transformed list

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
PERF401	14	7	7	0	0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+7 -7 violations, +0 -0 fixes in 1 projects; 54 projects unchanged)

apache/superset (+7 -7 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ tests/integration_tests/charts/api_tests.py:350:13: PERF401 Use `list.extend` to create a transformed list
- tests/integration_tests/charts/api_tests.py:350:13: PERF401 Use a list comprehension to create a transformed list
+ tests/integration_tests/charts/api_tests.py:464:13: PERF401 Use `list.extend` to create a transformed list
- tests/integration_tests/charts/api_tests.py:464:13: PERF401 Use a list comprehension to create a transformed list
+ tests/integration_tests/charts/api_tests.py:515:13: PERF401 Use `list.extend` to create a transformed list
- tests/integration_tests/charts/api_tests.py:515:13: PERF401 Use a list comprehension to create a transformed list
+ tests/integration_tests/dashboards/api_tests.py:1280:13: PERF401 Use `list.extend` to create a transformed list
- tests/integration_tests/dashboards/api_tests.py:1280:13: PERF401 Use a list comprehension to create a transformed list
+ tests/integration_tests/dashboards/api_tests.py:1307:13: PERF401 Use `list.extend` to create a transformed list
- tests/integration_tests/dashboards/api_tests.py:1307:13: PERF401 Use a list comprehension to create a transformed list
+ tests/integration_tests/dashboards/api_tests.py:1441:13: PERF401 Use `list.extend` to create a transformed list
- tests/integration_tests/dashboards/api_tests.py:1441:13: PERF401 Use a list comprehension to create a transformed list
+ tests/integration_tests/dashboards/api_tests.py:1506:13: PERF401 Use `list.extend` to create a transformed list
- tests/integration_tests/dashboards/api_tests.py:1506:13: PERF401 Use a list comprehension to create a transformed list

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
PERF401	14	7	7	0	0

… this to literal_comparisons from generic helpers crate

knavdeep152002 · 2025-04-03T20:17:09Z

Hey, I was looking into the E712.py and there were a few edge cases which I missed (the left is a bool and the right is a comparator ).
So I decided to move this redundancy check to the parent block in literal_comparision.rs and have added the redundancy check for left as well as right.

There was this edge case

if(True) == TrueElement or x == TrueElement:
    pass

If this was generally checked for redundancy and converted it would have been

ifTrueElement or x == TrueElement:
    pass

which is syntactically wrong so I added a special case for this where the compare range and left range don't start on the same index. I enclose the comparator in parentheses. So it becomes

if(TrueElement) or x == TrueElement:
    pass

I tried looking at the ast-parsed tree to find its parentheses or not, but I couldn't find any apart from the range numbers so I have used them to check whether to add parentheses to the comparator or not

Can you review the changes and let me know if I have to make any other changes?
Thank you.

ntBre · 2025-04-04T20:26:31Z

I think you can actually rely on yoda-conditions (SIM300) to handle these edge cases (playground link) and avoid the added complexity in this PR. Fixing the simple case like your first two commits would be plenty for this PR, in my opinion!

Edit: to be a bit more explicit, SIM300 will recommend reversing the condition so that your initial changes catch the issue, without having to handle the redundancy on the left-hand side separately.

NavdeepK15 · 2025-04-04T20:30:28Z

So should i revert to my previous commit?

ntBre · 2025-04-04T20:31:48Z

So should i revert to my previous commit?

Yes, that is what I was thinking. Does that sound reasonable to you?

knavdeep152002 · 2025-04-05T11:27:44Z

Yeah, if ruff --fix does SIM300 fix first and then if it does the E712 fix then it is completely fine.

i tried running this command

cargo run -p ruff check --fix --unsafe-fixes test.py

the below was fixed as

if (True) == TrueElement or x == TrueElement:
    pass

if (True) is TrueElement or x == TrueElement:
    pass

So it seems like E712 is being fixed first and then 'yoda-condition' isn't being raised anymore. So I thought to include the redundancy comparison on the left expression as well.

Currently, in the expression.rs The literal comparison is being done first and then the Yoda detection. We can change it as below (but I am not sure why SIM300 fix was placed at the end before. Was this intentional?).

    if checker.enabled(Rule::YodaConditions) {
        flake8_simplify::rules::yoda_conditions(checker, expr, left, ops, comparators);
    }
    if checker.any_enabled(&[Rule::NoneComparison, Rule::TrueFalseComparison]) {
        pycodestyle::rules::literal_comparisons(checker, compare);
    }

Also the test case on #[test_case(Rule::TrueFalseComparison, Path::new("E712.py"))]
would still be the older one (since its individual test on literal comparison alone) will that be ok?

   54    54 │ 8   |-if True != res:
   55       │-  8 |+if not res:
         55 │+  8 |+if True is not res:

ntBre · 2025-04-07T14:52:49Z

Thanks for looking into this! I asked internally, and it sounds like my suggestion was actually wrong, and your handling of both sides is the right way to go because we want to avoid these kinds of ordering dependencies between rules.

I'll give this another review to see if we can simplify some of the parentheses handling, but this is looking good.

I also realized that the parentheses might be an issue on the right side of the comparison. Could you add a test case like this (if there's not one already):

if x is (True)or False: ...

I didn't realize you could do something like that until your example here, so thank you for catching that!

knavdeep152002 · 2025-04-07T15:21:13Z

Hey, that was a case I hadn’t accounted for earlier. I’ve made the necessary changes along with some code cleanup.

I believe this should now handle all relevant cases. To ensure validity, we can either enclose the expression in parentheses or add an extra space. I couldn’t identify any other edge cases beyond the need for parentheses, so I’ve opted to wrap the expression in them for now.

ntBre

This looks good. I only had one refactoring suggestion until I noticed a question about parentheses too.

crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs

ntBre · 2025-04-10T13:19:32Z

...er/src/rules/pycodestyle/snapshots/ruff_linter__rules__pycodestyle__tests__E712_E712.py.snap

@@ -181,7 +181,7 @@ E712.py:22:4: E712 [*] Avoid equality comparisons to `True`; use `if TrueElement
 20 20 | var = 1 if cond == True else -1 if cond == False else cond
 21 21 | #: E712
 22    |-if (True) == TrueElement or x == TrueElement:
-   22 |+if (True) is TrueElement or x == TrueElement:
+   22 |+if (TrueElement) or x == TrueElement:


Oh, is the parenthesis detection wrong here? I don't think we need these parens.

According to the logic, the comparison of the start indexes is on the (true) == TrueElement, and the comparator is true. So, there is a mismatch, so a bracket was included.

I guess the point I was trying to make is that I would rather not have these parentheses because the expression is valid without them. I would have expected this output here:

if TrueElement or x == TrueElement: ...

The current logic may not be correct if it preserves unnecessary parentheses.

I tried a couple of approaches to get rid of the range comparisons in an earlier review and couldn't come up with something better, though, so this doesn't necessarily need to block this PR. It's still an improvement over the old fix.

…sons.rs Co-authored-by: Brent Westbrook <[email protected]>

crates/ruff_python_ast/src/helpers.rs

ntBre

Thanks again!

…tructor * origin/main: [red-knot] Trust module-level undeclared symbols in stubs (#17577) [`airflow`] Apply auto fixes to cases where the names have changed in Airflow 3 (`AIR301`) (#17355) [`pycodestyle`] Auto-fix redundant boolean comparison (`E712`) (#17090) [red-knot] Detect semantic syntax errors (#17463) Fix stale diagnostics in Ruff playground (#17583) [red-knot] Early return from `project.is_file_open` for vendored files (#17580)

…var-instance * dcreager/generic-constructor: Revert FunctionLiteral type [red-knot] Trust module-level undeclared symbols in stubs (#17577) [`airflow`] Apply auto fixes to cases where the names have changed in Airflow 3 (`AIR301`) (#17355) [`pycodestyle`] Auto-fix redundant boolean comparison (`E712`) (#17090) Clean this up a bit clippy [red-knot] Detect semantic syntax errors (#17463) Fix stale diagnostics in Ruff playground (#17583) [red-knot] Early return from `project.is_file_open` for vendored files (#17580)

feat: add redundant boolean comparison

8a6c711

MichaReiser added the fixes Related to suggested fixes for violations label Mar 31, 2025

MichaReiser requested a review from ntBre April 2, 2025 07:50

ntBre requested changes Apr 2, 2025

View reviewed changes

crates/ruff_python_ast/src/helpers.rs Outdated Show resolved Hide resolved

crates/ruff_python_ast/src/helpers.rs Outdated Show resolved Hide resolved

refactor: redundant boolean comparison and its respective check

7c7a250

knavdeep152002 requested a review from ntBre April 2, 2025 18:15

Update snapshots for E712 rule changes"

649eff8

feat: check redundant_boolean_comparison for left expression and move…

5c30c97

… this to literal_comparisons from generic helpers crate

fix: typo on comment

03d6275

ntBre self-assigned this Apr 3, 2025

fix: wrap in parenthesis for right side boolean comparator case

1118724

chore: run cargo fmt

63258a6

ntBre reviewed Apr 10, 2025

View reviewed changes

knavdeep152002 and others added 2 commits April 15, 2025 12:59

Update crates/ruff_linter/src/rules/pycodestyle/rules/literal_compari…

f45c98b

…sons.rs Co-authored-by: Brent Westbrook <[email protected]>

refactor: redundant comparison

2cd6971

ntBre reviewed Apr 18, 2025

View reviewed changes

crates/ruff_python_ast/src/helpers.rs Outdated Show resolved Hide resolved

refactor: move helper fn to main file instead of pub fn

246c858

ntBre changed the title ~~feat: add redundant boolean comparison~~ [pycodestyle] Auto-fix redundant boolean comparison (E712) Apr 23, 2025

ntBre approved these changes Apr 23, 2025

View reviewed changes

ntBre merged commit 5a719f2 into astral-sh:main Apr 23, 2025
22 checks passed

BrewTestBot mentioned this pull request Apr 24, 2025

ruff 0.11.7 Homebrew/homebrew-core#221359

Merged

knavdeep152002 deleted the feature/E712-autofix branch May 10, 2025 12:00

[pycodestyle] Auto-fix redundant boolean comparison (E712) #17090

[pycodestyle] Auto-fix redundant boolean comparison (E712) #17090

Uh oh!

Conversation

knavdeep152002 commented Mar 31, 2025 • edited by AlexWaygood Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaReiser commented Mar 31, 2025

Uh oh!

ntBre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

knavdeep152002 commented Apr 2, 2025

Uh oh!

ntBre commented Apr 2, 2025

Uh oh!

knavdeep152002 commented Apr 2, 2025

Uh oh!

ntBre commented Apr 2, 2025

Uh oh!

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

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

knavdeep152002 commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ntBre commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NavdeepK15 commented Apr 4, 2025

Uh oh!

ntBre commented Apr 4, 2025

Uh oh!

knavdeep152002 commented Apr 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ntBre commented Apr 7, 2025

Uh oh!

knavdeep152002 commented Apr 7, 2025

Uh oh!

ntBre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ntBre Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

knavdeep152002 Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

ntBre Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ntBre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

[`pycodestyle`] Auto-fix redundant boolean comparison (`E712`) #17090

[`pycodestyle`] Auto-fix redundant boolean comparison (`E712`) #17090

knavdeep152002 commented Mar 31, 2025 •

edited by AlexWaygood

Loading

github-actions bot commented Apr 2, 2025 •

edited

Loading

`ruff-ecosystem` results

knavdeep152002 commented Apr 3, 2025 •

edited

Loading

ntBre commented Apr 4, 2025 •

edited

Loading

knavdeep152002 commented Apr 5, 2025 •

edited

Loading