From 8a6c7116a2f7494702ac3f4426aa60d038199469 Mon Sep 17 00:00:00 2001 From: knavdeep152002 Date: Mon, 31 Mar 2025 17:10:09 +0530 Subject: [PATCH 01/10] feat: add redundant boolean comparison --- crates/ruff_python_ast/src/helpers.rs | 29 +++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index 4d207ee547bcd..e0bd91b22cde0 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -1355,6 +1355,24 @@ fn is_empty_f_string(expr: &ast::ExprFString) -> bool { }) } +fn is_redundant_boolean_comparison(op: CmpOp, comparator: &Expr) -> (bool, bool) { + match op { + CmpOp::Is => { + if let Expr::BooleanLiteral(ast::ExprBooleanLiteral { value, .. }) = comparator { + return (true, *value); + } + (false, false) + } + CmpOp::IsNot => { + if let Expr::BooleanLiteral(ast::ExprBooleanLiteral { value, .. }) = comparator { + return (true, !*value); + } + (false, false) + } + _ => (false, false), + } +} + pub fn generate_comparison( left: &Expr, ops: &[CmpOp], @@ -1373,6 +1391,17 @@ pub fn generate_comparison( .unwrap_or(left.range())], ); + if ops.len() == 1 { + let (op, comparator) = (ops[0], &comparators[0]); + if let (true, kind) = is_redundant_boolean_comparison(op, comparator) { + return if kind { + contents + } else { + format!("not {contents}") + }; + } + } + for (op, comparator) in ops.iter().zip(comparators) { // Add the operator. contents.push_str(match op { From 7c7a25088c77d37fbf751d4fc703739d9822ac89 Mon Sep 17 00:00:00 2001 From: knavdeep152002 Date: Wed, 2 Apr 2025 23:05:53 +0530 Subject: [PATCH 02/10] refactor: redundant boolean comparison and its respective check --- crates/ruff_python_ast/src/helpers.rs | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index e0bd91b22cde0..2813b2a27f48a 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -1355,21 +1355,12 @@ fn is_empty_f_string(expr: &ast::ExprFString) -> bool { }) } -fn is_redundant_boolean_comparison(op: CmpOp, comparator: &Expr) -> (bool, bool) { +fn is_redundant_boolean_comparison(op: CmpOp, comparator: &Expr) -> Option { + let value = comparator.as_boolean_literal_expr()?.value; match op { - CmpOp::Is => { - if let Expr::BooleanLiteral(ast::ExprBooleanLiteral { value, .. }) = comparator { - return (true, *value); - } - (false, false) - } - CmpOp::IsNot => { - if let Expr::BooleanLiteral(ast::ExprBooleanLiteral { value, .. }) = comparator { - return (true, !*value); - } - (false, false) - } - _ => (false, false), + CmpOp::Is => Some(value), + CmpOp::IsNot => Some(!value), + _ => None, } } @@ -1391,9 +1382,8 @@ pub fn generate_comparison( .unwrap_or(left.range())], ); - if ops.len() == 1 { - let (op, comparator) = (ops[0], &comparators[0]); - if let (true, kind) = is_redundant_boolean_comparison(op, comparator) { + if let ([op], [comparator]) = (ops, comparators) { + if let Some(kind) = is_redundant_boolean_comparison(*op, comparator) { return if kind { contents } else { From 649eff849d216b158bbe255735ce412566133707 Mon Sep 17 00:00:00 2001 From: knavdeep152002 Date: Thu, 3 Apr 2025 00:25:47 +0530 Subject: [PATCH 03/10] Update snapshots for E712 rule changes" --- ...r__rules__pycodestyle__tests__E712_E712.py.snap | 14 +++++++------- ...les__pycodestyle__tests__constant_literals.snap | 5 ++--- 2 files changed, 9 insertions(+), 10 deletions(-) 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 02f2c26951687..0876fe33341f6 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 @@ -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: @@ -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: @@ -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: @@ -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 @@ -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 @@ -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 @@ -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 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 04831e37b3bd6..115a12047ec21 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 @@ -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 | @@ -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 = [] @@ -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 = [] From 5c30c971f95c69ca2c53311acb9a2558d0ca689e Mon Sep 17 00:00:00 2001 From: knavdeep152002 Date: Fri, 4 Apr 2025 01:44:33 +0530 Subject: [PATCH 04/10] feat: check redundant_boolean_comparison for left expression and move this to literal_comparisons from generic helpers crate --- .../pycodestyle/rules/literal_comparisons.rs | 78 ++++++++++++++++--- ...les__pycodestyle__tests__E712_E712.py.snap | 8 +- ...pycodestyle__tests__constant_literals.snap | 4 +- crates/ruff_python_ast/src/helpers.rs | 16 +--- 4 files changed, 76 insertions(+), 30 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 265d0c7046a6c..a028571aaeaf9 100644 --- a/crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs +++ b/crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs @@ -1,10 +1,11 @@ +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::{self as ast, CmpOp, Expr}; +use ruff_python_ast::helpers::{self, is_redundant_boolean_comparison}; +use ruff_python_ast::{self as ast, CmpOp, Expr, ExprRef}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -323,7 +324,6 @@ 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() @@ -331,14 +331,70 @@ pub(crate) fn literal_comparisons(checker: &Checker, compare: &ast::ExprCompare) .map(|(idx, op)| bad_ops.get(&idx).unwrap_or(op)) .copied() .collect::>(); - 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 comparator_range = parenthesized_range( + comparator.into(), + compare.into(), + comment_ranges, + source, + ) + .unwrap_or(comparator.range()); + let comparator_str = &source[comparator_range]; + + let content = if kind { + comparator_str.to_string() + } else { + format!("not {comparator_str}") + }; + + // NOTE: Adding paranthesis if left is boolean and enclosed in parentheses + if compare.left.range().start() == compare.range().start() { + content + } else { + format!("({content})") + } + } else if let Some(kind) = is_redundant_boolean_comparison(*op, comparator) { + let left_range = parenthesized_range( + ExprRef::from(compare.left.as_ref()), + compare.into(), + comment_ranges, + source, + ) + .unwrap_or(compare.left.range()); + let left_str = &source[left_range]; + + if kind { + left_str.to_string() + } else { + format!("not {left_str}") + } + } 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(), 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 0876fe33341f6..76d290a4b5cea 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 @@ -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: @@ -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: @@ -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: 23 23 | pass 24 24 | 25 25 | if res == True != False: @@ -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: 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 115a12047ec21..7adb6732e7bf3 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 @@ -122,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 @@ -143,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 diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index 2813b2a27f48a..f89ba4cf8b925 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -1355,11 +1355,11 @@ fn is_empty_f_string(expr: &ast::ExprFString) -> bool { }) } -fn is_redundant_boolean_comparison(op: CmpOp, comparator: &Expr) -> Option { +pub fn is_redundant_boolean_comparison(op: CmpOp, comparator: &Expr) -> Option { let value = comparator.as_boolean_literal_expr()?.value; match op { - CmpOp::Is => Some(value), - CmpOp::IsNot => Some(!value), + CmpOp::Is | CmpOp::Eq => Some(value), + CmpOp::IsNot | CmpOp::NotEq => Some(!value), _ => None, } } @@ -1382,16 +1382,6 @@ pub fn generate_comparison( .unwrap_or(left.range())], ); - if let ([op], [comparator]) = (ops, comparators) { - if let Some(kind) = is_redundant_boolean_comparison(*op, comparator) { - return if kind { - contents - } else { - format!("not {contents}") - }; - } - } - for (op, comparator) in ops.iter().zip(comparators) { // Add the operator. contents.push_str(match op { From 03d6275b723241a0f826289bb03899c1e447d0f0 Mon Sep 17 00:00:00 2001 From: knavdeep152002 Date: Fri, 4 Apr 2025 02:00:12 +0530 Subject: [PATCH 05/10] fix: typo on comment --- .../src/rules/pycodestyle/rules/literal_comparisons.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 a028571aaeaf9..14c081a7e087b 100644 --- a/crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs +++ b/crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs @@ -353,7 +353,7 @@ pub(crate) fn literal_comparisons(checker: &Checker, compare: &ast::ExprCompare) format!("not {comparator_str}") }; - // NOTE: Adding paranthesis if left is boolean and enclosed in parentheses + // NOTE: Adding parenthesis if left is boolean and enclosed in parentheses if compare.left.range().start() == compare.range().start() { content } else { From 11187245643e28bb1e61874b03aad9c58a85ef18 Mon Sep 17 00:00:00 2001 From: knavdeep152002 Date: Mon, 7 Apr 2025 20:48:14 +0530 Subject: [PATCH 06/10] fix: wrap in parenthesis for right side boolean comparator case --- .../pycodestyle/rules/literal_comparisons.rs | 50 +++++++++++-------- 1 file changed, 29 insertions(+), 21 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 14c081a7e087b..eec9bebdfac77 100644 --- a/crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs +++ b/crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs @@ -3,8 +3,7 @@ use rustc_hash::FxHashMap; use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, ViolationMetadata}; -use ruff_python_ast::helpers::generate_comparison; -use ruff_python_ast::helpers::{self, is_redundant_boolean_comparison}; +use ruff_python_ast::helpers::{self, is_redundant_boolean_comparison, generate_comparison}; use ruff_python_ast::{self as ast, CmpOp, Expr, ExprRef}; use ruff_text_size::Ranged; @@ -171,6 +170,24 @@ impl AlwaysFixableViolation for TrueFalseComparison { } } + +fn build_conditional_string(source_slice: &str, kind: bool) -> String { + if kind { + source_slice.to_string() + } else { + format!("not {source_slice}") + } +} + +// NOTE: Adding parenthesis if comparator is boolean and enclosed in parentheses +fn maybe_wrap(content: String, needs_wrap: bool) -> String { + if needs_wrap { + format!("({content})") + } else { + content + } +} + /// E711, E712 pub(crate) fn literal_comparisons(checker: &Checker, compare: &ast::ExprCompare) { // Mapping from (bad operator index) to (replacement operator). As we iterate @@ -345,20 +362,12 @@ pub(crate) fn literal_comparisons(checker: &Checker, compare: &ast::ExprCompare) source, ) .unwrap_or(comparator.range()); + let comparator_str = &source[comparator_range]; - - let content = if kind { - comparator_str.to_string() - } else { - format!("not {comparator_str}") - }; - - // NOTE: Adding parenthesis if left is boolean and enclosed in parentheses - if compare.left.range().start() == compare.range().start() { - content - } else { - format!("({content})") - } + let result = build_conditional_string(comparator_str, kind); + + let needs_wrap = compare.left.range().start() != compare.range().start(); + maybe_wrap(result, needs_wrap) } else if let Some(kind) = is_redundant_boolean_comparison(*op, comparator) { let left_range = parenthesized_range( ExprRef::from(compare.left.as_ref()), @@ -367,13 +376,12 @@ pub(crate) fn literal_comparisons(checker: &Checker, compare: &ast::ExprCompare) source, ) .unwrap_or(compare.left.range()); + let left_str = &source[left_range]; - - if kind { - left_str.to_string() - } else { - format!("not {left_str}") - } + let result = build_conditional_string(left_str, kind); + + let needs_wrap = comparator.range().end() != compare.range().end(); + maybe_wrap(result, needs_wrap) } else { generate_comparison( &compare.left, From 63258a62c0ae81a2015e1b2b88c3a467a1d26e31 Mon Sep 17 00:00:00 2001 From: knavdeep152002 Date: Tue, 8 Apr 2025 11:38:00 +0530 Subject: [PATCH 07/10] chore: run cargo fmt --- .../rules/pycodestyle/rules/literal_comparisons.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 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 eec9bebdfac77..e564d18ca8c7d 100644 --- a/crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs +++ b/crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs @@ -3,7 +3,7 @@ use rustc_hash::FxHashMap; use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, ViolationMetadata}; -use ruff_python_ast::helpers::{self, is_redundant_boolean_comparison, generate_comparison}; +use ruff_python_ast::helpers::{self, generate_comparison, is_redundant_boolean_comparison}; use ruff_python_ast::{self as ast, CmpOp, Expr, ExprRef}; use ruff_text_size::Ranged; @@ -170,7 +170,6 @@ impl AlwaysFixableViolation for TrueFalseComparison { } } - fn build_conditional_string(source_slice: &str, kind: bool) -> String { if kind { source_slice.to_string() @@ -362,10 +361,10 @@ pub(crate) fn literal_comparisons(checker: &Checker, compare: &ast::ExprCompare) source, ) .unwrap_or(comparator.range()); - + let comparator_str = &source[comparator_range]; let result = build_conditional_string(comparator_str, kind); - + let needs_wrap = compare.left.range().start() != compare.range().start(); maybe_wrap(result, needs_wrap) } else if let Some(kind) = is_redundant_boolean_comparison(*op, comparator) { @@ -376,10 +375,10 @@ pub(crate) fn literal_comparisons(checker: &Checker, compare: &ast::ExprCompare) source, ) .unwrap_or(compare.left.range()); - + let left_str = &source[left_range]; let result = build_conditional_string(left_str, kind); - + let needs_wrap = comparator.range().end() != compare.range().end(); maybe_wrap(result, needs_wrap) } else { From f45c98ba7ddf8e9c1358afb6d0e7fc9be02c7615 Mon Sep 17 00:00:00 2001 From: Navdeep K <56593523+knavdeep152002@users.noreply.github.com> Date: Tue, 15 Apr 2025 12:59:31 +0530 Subject: [PATCH 08/10] Update crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com> --- .../pycodestyle/rules/literal_comparisons.rs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 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 e564d18ca8c7d..913aaccb614f0 100644 --- a/crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs +++ b/crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs @@ -368,19 +368,15 @@ pub(crate) fn literal_comparisons(checker: &Checker, compare: &ast::ExprCompare) let needs_wrap = compare.left.range().start() != compare.range().start(); maybe_wrap(result, needs_wrap) } else if let Some(kind) = is_redundant_boolean_comparison(*op, comparator) { - let left_range = parenthesized_range( - ExprRef::from(compare.left.as_ref()), - compare.into(), + let needs_wrap = comparator.range().end() != compare.range().end(); + generate_redundant_comparison( + compare, comment_ranges, source, + &compare.left, + kind, + needs_wrap, ) - .unwrap_or(compare.left.range()); - - let left_str = &source[left_range]; - let result = build_conditional_string(left_str, kind); - - let needs_wrap = comparator.range().end() != compare.range().end(); - maybe_wrap(result, needs_wrap) } else { generate_comparison( &compare.left, From 2cd697113ffa0e73c6a84db65564ff2e005bb5f5 Mon Sep 17 00:00:00 2001 From: knavdeep152002 Date: Tue, 15 Apr 2025 13:44:31 +0530 Subject: [PATCH 09/10] refactor: redundant comparison --- .../pycodestyle/rules/literal_comparisons.rs | 48 +++++++++++-------- 1 file changed, 27 insertions(+), 21 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 913aaccb614f0..ee484da52f476 100644 --- a/crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs +++ b/crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs @@ -4,7 +4,7 @@ use rustc_hash::FxHashMap; use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_ast::helpers::{self, generate_comparison, is_redundant_boolean_comparison}; -use ruff_python_ast::{self as ast, CmpOp, Expr, ExprRef}; +use ruff_python_ast::{self as ast, CmpOp, Expr}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -170,20 +170,30 @@ impl AlwaysFixableViolation for TrueFalseComparison { } } -fn build_conditional_string(source_slice: &str, kind: bool) -> String { - if kind { - source_slice.to_string() +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 {source_slice}") - } -} + format!("not {comparator_str}") + }; -// NOTE: Adding parenthesis if comparator is boolean and enclosed in parentheses -fn maybe_wrap(content: String, needs_wrap: bool) -> String { if needs_wrap { - format!("({content})") + format!("({result})") } else { - content + result } } @@ -354,19 +364,15 @@ pub(crate) fn literal_comparisons(checker: &Checker, compare: &ast::ExprCompare) let content = match (&*compare.ops, &*compare.comparators) { ([op], [comparator]) => { if let Some(kind) = is_redundant_boolean_comparison(*op, &compare.left) { - let comparator_range = parenthesized_range( - comparator.into(), - compare.into(), + let needs_wrap = compare.left.range().start() != compare.range().start(); + generate_redundant_comparison( + compare, comment_ranges, source, + comparator, + kind, + needs_wrap, ) - .unwrap_or(comparator.range()); - - let comparator_str = &source[comparator_range]; - let result = build_conditional_string(comparator_str, kind); - - let needs_wrap = compare.left.range().start() != compare.range().start(); - maybe_wrap(result, 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( From 246c858ab7d5c99d918aecc6621fb1222a1bf0ca Mon Sep 17 00:00:00 2001 From: knavdeep152002 Date: Sat, 19 Apr 2025 00:20:33 +0530 Subject: [PATCH 10/10] refactor: move helper fn to main file instead of pub fn --- .../rules/pycodestyle/rules/literal_comparisons.rs | 11 ++++++++++- crates/ruff_python_ast/src/helpers.rs | 9 --------- 2 files changed, 10 insertions(+), 10 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 ee484da52f476..cffcde998fddf 100644 --- a/crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs +++ b/crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs @@ -3,7 +3,7 @@ use rustc_hash::FxHashMap; use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, ViolationMetadata}; -use ruff_python_ast::helpers::{self, generate_comparison, is_redundant_boolean_comparison}; +use ruff_python_ast::helpers::{self, generate_comparison}; use ruff_python_ast::{self as ast, CmpOp, Expr}; use ruff_text_size::Ranged; @@ -170,6 +170,15 @@ impl AlwaysFixableViolation for TrueFalseComparison { } } +fn is_redundant_boolean_comparison(op: CmpOp, comparator: &Expr) -> Option { + let value = comparator.as_boolean_literal_expr()?.value; + match op { + CmpOp::Is | CmpOp::Eq => Some(value), + CmpOp::IsNot | CmpOp::NotEq => Some(!value), + _ => None, + } +} + fn generate_redundant_comparison( compare: &ast::ExprCompare, comment_ranges: &ruff_python_trivia::CommentRanges, diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index f89ba4cf8b925..4d207ee547bcd 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -1355,15 +1355,6 @@ fn is_empty_f_string(expr: &ast::ExprFString) -> bool { }) } -pub fn is_redundant_boolean_comparison(op: CmpOp, comparator: &Expr) -> Option { - 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],