From 72c9f7e4c9928ad2714e8ca68c1defe2e778d4f3 Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Fri, 8 Mar 2024 01:20:56 +0000 Subject: [PATCH] Include actual conditions in E712 diagnostics (#10254) ## Summary Changes the generic recommendation to replace ```python if foo == True: ... ``` with `if cond:` to `if foo:`. Still uses a generic message for compound comparisons as a specific message starts to become confusing. For example, ```python if foo == True != False: ... ``` produces two recommendations, one of which would recommend `if True:`, which is confusing. Resolves [recommendation in a previous PR](https://github.com/astral-sh/ruff/pull/8613/files#r1514915070). ## Test Plan `cargo nextest run` --- .../pycodestyle/rules/literal_comparisons.rs | 96 +++++++++++++++---- ...les__pycodestyle__tests__E712_E712.py.snap | 76 +++++++-------- ...pycodestyle__tests__constant_literals.snap | 14 ++- 3 files changed, 120 insertions(+), 66 deletions(-) diff --git a/crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs b/crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs index ea2760b7cbfa6..68a9ba3d7f4e7 100644 --- a/crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs +++ b/crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs @@ -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,52 @@ 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, +} 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 Some(cond) = cond.as_ref().and_then(|cond| cond.full_display()) else { + 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 +192,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() == 1 { + 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() == 1 { + 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 +263,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() == 1 { + 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() == 1 { + 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); } diff --git a/crates/ruff_linter/src/rules/pycodestyle/snapshots/ruff_linter__rules__pycodestyle__tests__E712_E712.py.snap b/crates/ruff_linter/src/rules/pycodestyle/snapshots/ruff_linter__rules__pycodestyle__tests__E712_E712.py.snap index 7aba8288f2888..0c98438051b37 100644 --- a/crates/ruff_linter/src/rules/pycodestyle/snapshots/ruff_linter__rules__pycodestyle__tests__E712_E712.py.snap +++ b/crates/ruff_linter/src/rules/pycodestyle/snapshots/ruff_linter__rules__pycodestyle__tests__E712_E712.py.snap @@ -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 - - diff --git a/crates/ruff_linter/src/rules/pycodestyle/snapshots/ruff_linter__rules__pycodestyle__tests__constant_literals.snap b/crates/ruff_linter/src/rules/pycodestyle/snapshots/ruff_linter__rules__pycodestyle__tests__constant_literals.snap index 311ae7ecfd60d..23cbd094618d0 100644 --- a/crates/ruff_linter/src/rules/pycodestyle/snapshots/ruff_linter__rules__pycodestyle__tests__constant_literals.snap +++ b/crates/ruff_linter/src/rules/pycodestyle/snapshots/ruff_linter__rules__pycodestyle__tests__constant_literals.snap @@ -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 = [] - -