Skip to content
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.

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

Include actual conditions in E712 diagnostics #10254

Merged
merged 3 commits into from
Mar 8, 2024
Merged
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -9,6 +9,7 @@ use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
use crate::codes::Rule;
use crate::fix::snippet::SourceCodeSnippet;

#[derive(Debug, PartialEq, Eq, Copy, Clone)]
enum EqCmpOp {
@@ -102,39 +103,53 @@ impl AlwaysFixableViolation for NoneComparison {
///
/// [PEP 8]: https://peps.python.org/pep-0008/#programming-recommendations
#[violation]
pub struct TrueFalseComparison(bool, EqCmpOp);
pub struct TrueFalseComparison {
value: bool,
op: EqCmpOp,
cond: Option<SourceCodeSnippet>,
}

impl AlwaysFixableViolation for TrueFalseComparison {
#[derive_message_formats]
fn message(&self) -> String {
let TrueFalseComparison(value, op) = self;
let TrueFalseComparison { value, op, cond } = self;
let Some(cond) = cond else {
return "Avoid equality comparisons to `True` or `False`".to_string();
};
let cond = cond.truncated_display();
match (value, op) {
(true, EqCmpOp::Eq) => {
format!("Avoid equality comparisons to `True`; use `if cond:` for truth checks")
format!("Avoid equality comparisons to `True`; use `if {cond}:` for truth checks")
}
(true, EqCmpOp::NotEq) => {
format!(
"Avoid inequality comparisons to `True`; use `if not cond:` for false checks"
"Avoid inequality comparisons to `True`; use `if not {cond}:` for false checks"
)
}
(false, EqCmpOp::Eq) => {
format!(
"Avoid equality comparisons to `False`; use `if not cond:` for false checks"
"Avoid equality comparisons to `False`; use `if not {cond}:` for false checks"
)
}
(false, EqCmpOp::NotEq) => {
format!("Avoid inequality comparisons to `False`; use `if cond:` for truth checks")
format!(
"Avoid inequality comparisons to `False`; use `if {cond}:` for truth checks"
)
}
}
}

fn fix_title(&self) -> String {
let TrueFalseComparison(value, op) = self;
let TrueFalseComparison { value, op, cond } = self;
let cond = match cond {
Some(cond) if cond.full_display().is_some() => cond.full_display().unwrap().to_string(),
_ => return "Replace comparison".to_string(),
};
match (value, op) {
(true, EqCmpOp::Eq) => "Replace with `cond`".to_string(),
(true, EqCmpOp::NotEq) => "Replace with `not cond`".to_string(),
(false, EqCmpOp::Eq) => "Replace with `not cond`".to_string(),
(false, EqCmpOp::NotEq) => "Replace with `cond`".to_string(),
(true, EqCmpOp::Eq) => format!("Replace with `{cond}`"),
(true, EqCmpOp::NotEq) => format!("Replace with `not {cond}`"),
(false, EqCmpOp::Eq) => format!("Replace with `not {cond}`"),
(false, EqCmpOp::NotEq) => format!("Replace with `{cond}`"),
}
}
}
@@ -178,17 +193,35 @@ pub(crate) fn literal_comparisons(checker: &mut Checker, compare: &ast::ExprComp
if let Expr::BooleanLiteral(ast::ExprBooleanLiteral { value, .. }) = comparator {
match op {
EqCmpOp::Eq => {
let cond = if compare.ops.len() < 2 {
Some(SourceCodeSnippet::from_str(checker.locator().slice(next)))
} else {
None
};
let diagnostic = Diagnostic::new(
TrueFalseComparison(*value, op),
comparator.range(),
TrueFalseComparison {
value: *value,
op,
cond,
},
compare.range(),
);
bad_ops.insert(0, CmpOp::Is);
diagnostics.push(diagnostic);
}
EqCmpOp::NotEq => {
let cond = if compare.ops.len() < 2 {
Some(SourceCodeSnippet::from_str(checker.locator().slice(next)))
} else {
None
};
let diagnostic = Diagnostic::new(
TrueFalseComparison(*value, op),
comparator.range(),
TrueFalseComparison {
value: *value,
op,
cond,
},
compare.range(),
);
bad_ops.insert(0, CmpOp::IsNot);
diagnostics.push(diagnostic);
@@ -231,14 +264,40 @@ pub(crate) fn literal_comparisons(checker: &mut Checker, compare: &ast::ExprComp
if let Expr::BooleanLiteral(ast::ExprBooleanLiteral { value, .. }) = next {
match op {
EqCmpOp::Eq => {
let diagnostic =
Diagnostic::new(TrueFalseComparison(*value, op), next.range());
let cond = if compare.ops.len() < 2 {
Some(SourceCodeSnippet::from_str(
checker.locator().slice(comparator),
))
} else {
None
};
let diagnostic = Diagnostic::new(
TrueFalseComparison {
value: *value,
op,
cond,
},
compare.range(),
);
bad_ops.insert(index, CmpOp::Is);
diagnostics.push(diagnostic);
}
EqCmpOp::NotEq => {
let diagnostic =
Diagnostic::new(TrueFalseComparison(*value, op), next.range());
let cond = if compare.ops.len() < 2 {
Some(SourceCodeSnippet::from_str(
checker.locator().slice(comparator),
))
} else {
None
};
let diagnostic = Diagnostic::new(
TrueFalseComparison {
value: *value,
op,
cond,
},
compare.range(),
);
bad_ops.insert(index, CmpOp::IsNot);
diagnostics.push(diagnostic);
}
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
---
source: crates/ruff_linter/src/rules/pycodestyle/mod.rs
---
E712.py:2:11: E712 [*] Avoid equality comparisons to `True`; use `if cond:` for truth checks
E712.py:2:4: E712 [*] Avoid equality comparisons to `True`; use `if res:` for truth checks
|
1 | #: E712
2 | if res == True:
| ^^^^ E712
| ^^^^^^^^^^^ E712
3 | pass
4 | #: E712
|
= help: Replace with `cond`
= help: Replace with `res`

Unsafe fix
1 1 | #: E712
@@ -19,16 +19,16 @@ E712.py:2:11: E712 [*] Avoid equality comparisons to `True`; use `if cond:` for
4 4 | #: E712
5 5 | if res != False:

E712.py:5:11: E712 [*] Avoid inequality comparisons to `False`; use `if cond:` for truth checks
E712.py:5:4: E712 [*] Avoid inequality comparisons to `False`; use `if res:` for truth checks
|
3 | pass
4 | #: E712
5 | if res != False:
| ^^^^^ E712
| ^^^^^^^^^^^^ E712
6 | pass
7 | #: E712
|
= help: Replace with `cond`
= help: Replace with `res`

Unsafe fix
2 2 | if res == True:
@@ -40,16 +40,16 @@ E712.py:5:11: E712 [*] Avoid inequality comparisons to `False`; use `if cond:` f
7 7 | #: E712
8 8 | if True != res:

E712.py:8:4: E712 [*] Avoid inequality comparisons to `True`; use `if not cond:` for false checks
E712.py:8:4: E712 [*] Avoid inequality comparisons to `True`; use `if not res:` for false checks
|
6 | pass
7 | #: E712
8 | if True != res:
| ^^^^ E712
| ^^^^^^^^^^^ E712
9 | pass
10 | #: E712
|
= help: Replace with `not cond`
= help: Replace with `not res`

Unsafe fix
5 5 | if res != False:
@@ -61,16 +61,16 @@ E712.py:8:4: E712 [*] Avoid inequality comparisons to `True`; use `if not cond:`
10 10 | #: E712
11 11 | if False == res:

E712.py:11:4: E712 [*] Avoid equality comparisons to `False`; use `if not cond:` for false checks
E712.py:11:4: E712 [*] Avoid equality comparisons to `False`; use `if not res:` for false checks
|
9 | pass
10 | #: E712
11 | if False == res:
| ^^^^^ E712
| ^^^^^^^^^^^^ E712
12 | pass
13 | #: E712
|
= help: Replace with `not cond`
= help: Replace with `not res`

Unsafe fix
8 8 | if True != res:
@@ -82,16 +82,16 @@ E712.py:11:4: E712 [*] Avoid equality comparisons to `False`; use `if not cond:`
13 13 | #: E712
14 14 | if res[1] == True:

E712.py:14:14: E712 [*] Avoid equality comparisons to `True`; use `if cond:` for truth checks
E712.py:14:4: E712 [*] Avoid equality comparisons to `True`; use `if res[1]:` for truth checks
|
12 | pass
13 | #: E712
14 | if res[1] == True:
| ^^^^ E712
| ^^^^^^^^^^^^^^ E712
15 | pass
16 | #: E712
|
= help: Replace with `cond`
= help: Replace with `res[1]`

Unsafe fix
11 11 | if False == res:
@@ -103,16 +103,16 @@ E712.py:14:14: E712 [*] Avoid equality comparisons to `True`; use `if cond:` for
16 16 | #: E712
17 17 | if res[1] != False:

E712.py:17:14: E712 [*] Avoid inequality comparisons to `False`; use `if cond:` for truth checks
E712.py:17:4: E712 [*] Avoid inequality comparisons to `False`; use `if res[1]:` for truth checks
|
15 | pass
16 | #: E712
17 | if res[1] != False:
| ^^^^^ E712
| ^^^^^^^^^^^^^^^ E712
18 | pass
19 | #: E712
|
= help: Replace with `cond`
= help: Replace with `res[1]`

Unsafe fix
14 14 | if res[1] == True:
@@ -124,12 +124,12 @@ E712.py:17:14: E712 [*] Avoid inequality comparisons to `False`; use `if cond:`
19 19 | #: E712
20 20 | var = 1 if cond == True else -1 if cond == False else cond

E712.py:20:20: E712 [*] Avoid equality comparisons to `True`; use `if cond:` for truth checks
E712.py:20:12: E712 [*] Avoid equality comparisons to `True`; use `if cond:` for truth checks
|
18 | pass
19 | #: E712
20 | var = 1 if cond == True else -1 if cond == False else cond
| ^^^^ E712
| ^^^^^^^^^^^^ E712
21 | #: E712
22 | if (True) == TrueElement or x == TrueElement:
|
@@ -145,12 +145,12 @@ E712.py:20:20: E712 [*] Avoid equality comparisons to `True`; use `if cond:` for
22 22 | if (True) == TrueElement or x == TrueElement:
23 23 | pass

E712.py:20:44: E712 [*] Avoid equality comparisons to `False`; use `if not cond:` for false checks
E712.py:20:36: E712 [*] Avoid equality comparisons to `False`; use `if not cond:` for false checks
|
18 | pass
19 | #: E712
20 | var = 1 if cond == True else -1 if cond == False else cond
| ^^^^^ E712
| ^^^^^^^^^^^^^ E712
21 | #: E712
22 | if (True) == TrueElement or x == TrueElement:
|
@@ -166,15 +166,15 @@ E712.py:20:44: E712 [*] Avoid equality comparisons to `False`; use `if not cond:
22 22 | if (True) == TrueElement or x == TrueElement:
23 23 | pass

E712.py:22:5: E712 [*] Avoid equality comparisons to `True`; use `if cond:` for truth checks
E712.py:22:4: E712 [*] Avoid equality comparisons to `True`; use `if TrueElement:` for truth checks
|
20 | var = 1 if cond == True else -1 if cond == False else cond
21 | #: E712
22 | if (True) == TrueElement or x == TrueElement:
| ^^^^ E712
| ^^^^^^^^^^^^^^^^^^^^^ E712
23 | pass
|
= help: Replace with `cond`
= help: Replace with `TrueElement`

Unsafe fix
19 19 | #: E712
@@ -186,15 +186,15 @@ E712.py:22:5: E712 [*] Avoid equality comparisons to `True`; use `if cond:` for
24 24 |
25 25 | if res == True != False:

E712.py:25:11: E712 [*] Avoid equality comparisons to `True`; use `if cond:` for truth checks
E712.py:25:4: E712 [*] Avoid equality comparisons to `True` or `False`
|
23 | pass
24 |
25 | if res == True != False:
| ^^^^ E712
| ^^^^^^^^^^^^^^^^^^^^ E712
26 | pass
|
= help: Replace with `cond`
= help: Replace comparison

Unsafe fix
22 22 | if (True) == TrueElement or x == TrueElement:
@@ -206,15 +206,15 @@ E712.py:25:11: E712 [*] Avoid equality comparisons to `True`; use `if cond:` for
27 27 |
28 28 | if(True) == TrueElement or x == TrueElement:

E712.py:25:19: E712 [*] Avoid inequality comparisons to `False`; use `if cond:` for truth checks
E712.py:25:4: E712 [*] Avoid equality comparisons to `True` or `False`
|
23 | pass
24 |
25 | if res == True != False:
| ^^^^^ E712
| ^^^^^^^^^^^^^^^^^^^^ E712
26 | pass
|
= help: Replace with `cond`
= help: Replace comparison

Unsafe fix
22 22 | if (True) == TrueElement or x == TrueElement:
@@ -226,15 +226,15 @@ E712.py:25:19: E712 [*] Avoid inequality comparisons to `False`; use `if cond:`
27 27 |
28 28 | if(True) == TrueElement or x == TrueElement:

E712.py:28:4: E712 [*] Avoid equality comparisons to `True`; use `if cond:` for truth checks
E712.py:28:3: E712 [*] Avoid equality comparisons to `True`; use `if TrueElement:` for truth checks
|
26 | pass
27 |
28 | if(True) == TrueElement or x == TrueElement:
| ^^^^ E712
| ^^^^^^^^^^^^^^^^^^^^^ E712
29 | pass
|
= help: Replace with `cond`
= help: Replace with `TrueElement`

Unsafe fix
25 25 | if res == True != False:
@@ -246,15 +246,15 @@ E712.py:28:4: E712 [*] Avoid equality comparisons to `True`; use `if cond:` for
30 30 |
31 31 | if (yield i) == True:

E712.py:31:17: E712 [*] Avoid equality comparisons to `True`; use `if cond:` for truth checks
E712.py:31:4: E712 [*] Avoid equality comparisons to `True`; use `if yield i:` for truth checks
|
29 | pass
30 |
31 | if (yield i) == True:
| ^^^^ E712
| ^^^^^^^^^^^^^^^^^ E712
32 | print("even")
|
= help: Replace with `cond`
= help: Replace with `yield i`

Unsafe fix
28 28 | if(True) == TrueElement or x == TrueElement:
@@ -265,5 +265,3 @@ E712.py:31:17: E712 [*] Avoid equality comparisons to `True`; use `if cond:` for
32 32 | print("even")
33 33 |
34 34 | #: Okay


Original file line number Diff line number Diff line change
@@ -106,16 +106,16 @@ constant_literals.py:12:4: F632 [*] Use `==` to compare constant literals
14 14 | if False == None: # E711, E712 (fix)
15 15 | pass

constant_literals.py:14:4: E712 [*] Avoid equality comparisons to `False`; use `if not cond:` for false checks
constant_literals.py:14:4: E712 [*] Avoid equality comparisons to `False`; use `if not None:` for false checks
|
12 | if False is "abc": # F632 (fix, but leaves behind unfixable E712)
13 | pass
14 | if False == None: # E711, E712 (fix)
| ^^^^^ E712
| ^^^^^^^^^^^^^ E712
15 | pass
16 | if None == False: # E711, E712 (fix)
|
= help: Replace with `not cond`
= help: Replace with `not None`

Unsafe fix
11 11 | pass
@@ -168,15 +168,15 @@ constant_literals.py:16:4: E711 [*] Comparison to `None` should be `cond is None
18 18 |
19 19 | named_var = []

constant_literals.py:16:12: E712 [*] Avoid equality comparisons to `False`; use `if not cond:` for false checks
constant_literals.py:16:4: E712 [*] Avoid equality comparisons to `False`; use `if not None:` for false checks
|
14 | if False == None: # E711, E712 (fix)
15 | pass
16 | if None == False: # E711, E712 (fix)
| ^^^^^ E712
| ^^^^^^^^^^^^^ E712
17 | pass
|
= help: Replace with `not cond`
= help: Replace with `not None`

Unsafe fix
13 13 | pass
@@ -187,5 +187,3 @@ constant_literals.py:16:12: E712 [*] Avoid equality comparisons to `False`; use
17 17 | pass
18 18 |
19 19 | named_var = []