Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Apr 23, 2025
Merged

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8a6c711
feat: add redundant boolean comparison
knavdeep152002 Mar 31, 2025
7c7a250
refactor: redundant boolean comparison and its respective check
knavdeep152002 Apr 2, 2025
649eff8
Update snapshots for E712 rule changes"
knavdeep152002 Apr 2, 2025
5c30c97
feat: check redundant_boolean_comparison for left expression and move…
knavdeep152002 Apr 3, 2025
03d6275
fix: typo on comment
knavdeep152002 Apr 3, 2025
1118724
fix: wrap in parenthesis for right side boolean comparator case
knavdeep152002 Apr 7, 2025
63258a6
chore: run cargo fmt
knavdeep152002 Apr 8, 2025
f45c98b
Update crates/ruff_linter/src/rules/pycodestyle/rules/literal_compari…
knavdeep152002 Apr 15, 2025
2cd6971
refactor: redundant comparison
knavdeep152002 Apr 15, 2025
246c858
refactor: move helper fn to main file instead of pub fn
knavdeep152002 Apr 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 76 additions & 11 deletions crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
use ruff_python_ast::parenthesize::parenthesized_range;
use rustc_hash::FxHashMap;

use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, ViolationMetadata};
use ruff_python_ast::helpers;
use ruff_python_ast::helpers::generate_comparison;
use ruff_python_ast::helpers::{self, generate_comparison, is_redundant_boolean_comparison};
use ruff_python_ast::{self as ast, CmpOp, Expr};
use ruff_text_size::Ranged;

Expand Down Expand Up @@ -170,6 +170,33 @@ impl AlwaysFixableViolation for TrueFalseComparison {
}
}

fn generate_redundant_comparison(
compare: &ast::ExprCompare,
comment_ranges: &ruff_python_trivia::CommentRanges,
source: &str,
comparator: &Expr,
kind: bool,
needs_wrap: bool,
) -> String {
let comparator_range =
parenthesized_range(comparator.into(), compare.into(), comment_ranges, source)
.unwrap_or(comparator.range());

let comparator_str = &source[comparator_range];

let result = if kind {
comparator_str.to_string()
} else {
format!("not {comparator_str}")
};

if needs_wrap {
format!("({result})")
} else {
result
}
}

/// E711, E712
pub(crate) fn literal_comparisons(checker: &Checker, compare: &ast::ExprCompare) {
// Mapping from (bad operator index) to (replacement operator). As we iterate
Expand Down Expand Up @@ -323,22 +350,60 @@ pub(crate) fn literal_comparisons(checker: &Checker, compare: &ast::ExprCompare)
// TODO(charlie): Respect `noqa` directives. If one of the operators has a
// `noqa`, but another doesn't, both will be removed here.
if !bad_ops.is_empty() {
// Replace the entire comparison expression.
let ops = compare
.ops
.iter()
.enumerate()
.map(|(idx, op)| bad_ops.get(&idx).unwrap_or(op))
.copied()
.collect::<Vec<_>>();
let content = generate_comparison(
&compare.left,
&ops,
&compare.comparators,
compare.into(),
checker.comment_ranges(),
checker.source(),
);

let comment_ranges = checker.comment_ranges();
let source = checker.source();

let content = match (&*compare.ops, &*compare.comparators) {
([op], [comparator]) => {
if let Some(kind) = is_redundant_boolean_comparison(*op, &compare.left) {
let needs_wrap = compare.left.range().start() != compare.range().start();
generate_redundant_comparison(
compare,
comment_ranges,
source,
comparator,
kind,
needs_wrap,
)
} else if let Some(kind) = is_redundant_boolean_comparison(*op, comparator) {
let needs_wrap = comparator.range().end() != compare.range().end();
generate_redundant_comparison(
compare,
comment_ranges,
source,
&compare.left,
kind,
needs_wrap,
)
} else {
generate_comparison(
&compare.left,
&ops,
&compare.comparators,
compare.into(),
comment_ranges,
source,
)
}
}
_ => generate_comparison(
&compare.left,
&ops,
&compare.comparators,
compare.into(),
comment_ranges,
source,
),
};

for diagnostic in &mut diagnostics {
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
content.to_string(),
Expand Down
22 changes: 11 additions & 11 deletions ...src/rules/pycodestyle/snapshots/ruff_linter__rules__pycodestyle__tests__E712_E712.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ E712.py:2:4: E712 [*] Avoid equality comparisons to `True`; use `if res:` for tr
ℹ Unsafe fix
1 1 | #: E712
2 |-if res == True:
2 |+if res is True:
2 |+if res:
3 3 | pass
4 4 | #: E712
5 5 | if res != False:
Expand All @@ -35,7 +35,7 @@ E712.py:5:4: E712 [*] Avoid inequality comparisons to `False`; use `if res:` for
3 3 | pass
4 4 | #: E712
5 |-if res != False:
5 |+if res is not False:
5 |+if res:
6 6 | pass
7 7 | #: E712
8 8 | if True != res:
Expand All @@ -56,7 +56,7 @@ E712.py:8:4: E712 [*] Avoid inequality comparisons to `True`; use `if not res:`
6 6 | pass
7 7 | #: E712
8 |-if True != res:
8 |+if True is not res:
8 |+if not res:
9 9 | pass
10 10 | #: E712
11 11 | if False == res:
Expand All @@ -77,7 +77,7 @@ E712.py:11:4: E712 [*] Avoid equality comparisons to `False`; use `if not res:`
9 9 | pass
10 10 | #: E712
11 |-if False == res:
11 |+if False is res:
11 |+if not res:
12 12 | pass
13 13 | #: E712
14 14 | if res[1] == True:
Expand All @@ -98,7 +98,7 @@ E712.py:14:4: E712 [*] Avoid equality comparisons to `True`; use `if res[1]:` fo
12 12 | pass
13 13 | #: E712
14 |-if res[1] == True:
14 |+if res[1] is True:
14 |+if res[1]:
15 15 | pass
16 16 | #: E712
17 17 | if res[1] != False:
Expand All @@ -119,7 +119,7 @@ E712.py:17:4: E712 [*] Avoid inequality comparisons to `False`; use `if res[1]:`
15 15 | pass
16 16 | #: E712
17 |-if res[1] != False:
17 |+if res[1] is not False:
17 |+if res[1]:
18 18 | pass
19 19 | #: E712
20 20 | var = 1 if cond == True else -1 if cond == False else cond
Expand All @@ -140,7 +140,7 @@ E712.py:20:12: E712 [*] Avoid equality comparisons to `True`; use `if cond:` for
18 18 | pass
19 19 | #: E712
20 |-var = 1 if cond == True else -1 if cond == False else cond
20 |+var = 1 if cond is True else -1 if cond == False else cond
20 |+var = 1 if cond else -1 if cond == False else cond
21 21 | #: E712
22 22 | if (True) == TrueElement or x == TrueElement:
23 23 | pass
Expand All @@ -161,7 +161,7 @@ E712.py:20:36: E712 [*] Avoid equality comparisons to `False`; use `if not cond:
18 18 | pass
19 19 | #: E712
20 |-var = 1 if cond == True else -1 if cond == False else cond
20 |+var = 1 if cond == True else -1 if cond is False else cond
20 |+var = 1 if cond == True else -1 if not cond else cond
21 21 | #: E712
22 22 | if (True) == TrueElement or x == TrueElement:
23 23 | pass
Expand All @@ -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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

23 23 | pass
24 24 |
25 25 | if res == True != False:
Expand Down Expand Up @@ -241,7 +241,7 @@ E712.py:28:3: E712 [*] Avoid equality comparisons to `True`; use `if TrueElement
26 26 | pass
27 27 |
28 |-if(True) == TrueElement or x == TrueElement:
28 |+if(True) is TrueElement or x == TrueElement:
28 |+if(TrueElement) or x == TrueElement:
29 29 | pass
30 30 |
31 31 | if (yield i) == True:
Expand All @@ -261,7 +261,7 @@ E712.py:31:4: E712 [*] Avoid equality comparisons to `True`; use `if yield i:` f
29 29 | pass
30 30 |
31 |-if (yield i) == True:
31 |+if (yield i) is True:
31 |+if (yield i):
32 32 | print("even")
33 33 |
34 34 | #: Okay
9 changes: 4 additions & 5 deletions ...ules/pycodestyle/snapshots/ruff_linter__rules__pycodestyle__tests__constant_literals.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
---
source: crates/ruff_linter/src/rules/pycodestyle/mod.rs
snapshot_kind: text
---
constant_literals.py:4:4: F632 [*] Use `==` to compare constant literals
|
Expand Down Expand Up @@ -123,7 +122,7 @@ constant_literals.py:14:4: E712 [*] Avoid equality comparisons to `False`; use `
12 12 | if False is "abc": # F632 (fix, but leaves behind unfixable E712)
13 13 | pass
14 |-if False == None: # E711, E712 (fix)
14 |+if False is None: # E711, E712 (fix)
14 |+if not None: # E711, E712 (fix)
15 15 | pass
16 16 | if None == False: # E711, E712 (fix)
17 17 | pass
Expand All @@ -144,7 +143,7 @@ constant_literals.py:14:13: E711 [*] Comparison to `None` should be `cond is Non
12 12 | if False is "abc": # F632 (fix, but leaves behind unfixable E712)
13 13 | pass
14 |-if False == None: # E711, E712 (fix)
14 |+if False is None: # E711, E712 (fix)
14 |+if not None: # E711, E712 (fix)
15 15 | pass
16 16 | if None == False: # E711, E712 (fix)
17 17 | pass
Expand All @@ -164,7 +163,7 @@ constant_literals.py:16:4: E711 [*] Comparison to `None` should be `cond is None
14 14 | if False == None: # E711, E712 (fix)
15 15 | pass
16 |-if None == False: # E711, E712 (fix)
16 |+if None is False: # E711, E712 (fix)
16 |+if not None: # E711, E712 (fix)
17 17 | pass
18 18 |
19 19 | named_var = []
Expand All @@ -184,7 +183,7 @@ constant_literals.py:16:4: E712 [*] Avoid equality comparisons to `False`; use `
14 14 | if False == None: # E711, E712 (fix)
15 15 | pass
16 |-if None == False: # E711, E712 (fix)
16 |+if None is False: # E711, E712 (fix)
16 |+if not None: # E711, E712 (fix)
17 17 | pass
18 18 |
19 19 | named_var = []
Expand Down
9 changes: 9 additions & 0 deletions crates/ruff_python_ast/src/helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1355,6 +1355,15 @@ fn is_empty_f_string(expr: &ast::ExprFString) -> bool {
})
}

pub fn is_redundant_boolean_comparison(op: CmpOp, comparator: &Expr) -> Option<bool> {
let value = comparator.as_boolean_literal_expr()?.value;
match op {
CmpOp::Is | CmpOp::Eq => Some(value),
CmpOp::IsNot | CmpOp::NotEq => Some(!value),
_ => None,
}
}

pub fn generate_comparison(
left: &Expr,
ops: &[CmpOp],
Expand Down
Loading