diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/boolean_chained_comparison.py b/crates/ruff_linter/resources/test/fixtures/pylint/boolean_chained_comparison.py index 90c87fe3d446f5..954285487de597 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/boolean_chained_comparison.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/boolean_chained_comparison.py @@ -118,3 +118,11 @@ c = int(input()) if a > b and b < c: pass + + +# Unfixable due to parentheses. +(a < b) and b < c +a < b and (b < c) +((a < b) and b < c) +(a < b) and (b < c) +(((a < b))) and (b < c) diff --git a/crates/ruff_linter/src/rules/pylint/rules/boolean_chained_comparison.rs b/crates/ruff_linter/src/rules/pylint/rules/boolean_chained_comparison.rs index da8e8ef132b7b8..b980f2f56061e7 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/boolean_chained_comparison.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/boolean_chained_comparison.rs @@ -1,7 +1,10 @@ use itertools::Itertools; -use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; +use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::{name::Name, BoolOp, CmpOp, Expr, ExprBoolOp, ExprCompare}; +use ruff_python_ast::{ + name::Name, parenthesize::parenthesized_range, BoolOp, CmpOp, Expr, ExprBoolOp, + ExprCompare, +}; use ruff_text_size::{Ranged, TextRange}; use crate::checkers::ast::Checker; @@ -36,14 +39,16 @@ pub struct BooleanChainedComparison { variable: Name, } -impl AlwaysFixableViolation for BooleanChainedComparison { +impl Violation for BooleanChainedComparison { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + #[derive_message_formats] fn message(&self) -> String { format!("Contains chained boolean comparison that can be simplified") } - fn fix_title(&self) -> String { - "Use a single compare expression".to_string() + fn fix_title(&self) -> Option { + Some("Use a single compare expression".to_string()) } } @@ -59,6 +64,9 @@ pub(crate) fn boolean_chained_comparison(checker: &mut Checker, expr_bool_op: &E return; } + let locator = checker.locator(); + let comment_ranges = checker.comment_ranges(); + // retrieve all compare statements from expression let compare_expressions = expr_bool_op .values @@ -83,20 +91,47 @@ pub(crate) fn boolean_chained_comparison(checker: &mut Checker, expr_bool_op: &E return None; } - let edit = Edit::range_replacement( - left_compare_right.id().to_string(), - TextRange::new(left_compare_right.start(), right_compare_left.end()), - ); + let left_has_paren = parenthesized_range( + left_compare.into(), + expr_bool_op.into(), + comment_ranges, + locator.contents(), + ) + .is_some(); - Some( - Diagnostic::new( - BooleanChainedComparison { - variable: left_compare_right.id().clone(), - }, - TextRange::new(left_compare.start(), right_compare.end()), - ) - .with_fix(Fix::safe_edit(edit)), + let right_has_paren = parenthesized_range( + right_compare.into(), + expr_bool_op.into(), + comment_ranges, + locator.contents(), ) + .is_some(); + + // Do not offer a fix if there are any parentheses + // TODO: We can support a fix here, we just need to be careful to balance the + // parentheses which requires a more sophisticated edit + let fix = if left_has_paren || right_has_paren { + None + } else { + let edit = Edit::range_replacement( + left_compare_right.id().to_string(), + TextRange::new(left_compare_right.start(), right_compare_left.end()), + ); + Some(Fix::safe_edit(edit)) + }; + + let mut diagnostic = Diagnostic::new( + BooleanChainedComparison { + variable: left_compare_right.id().clone(), + }, + TextRange::new(left_compare.start(), right_compare.end()), + ); + + if let Some(fix) = fix { + diagnostic.set_fix(fix); + } + + Some(diagnostic) }); checker.diagnostics.extend(diagnostics); diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1716_boolean_chained_comparison.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1716_boolean_chained_comparison.py.snap index cf45c0ae9c4ebe..46f98755a10ebd 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1716_boolean_chained_comparison.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1716_boolean_chained_comparison.py.snap @@ -260,3 +260,54 @@ boolean_chained_comparison.py:73:24: PLR1716 [*] Contains chained boolean compar 74 74 | pass 75 75 | 76 76 | # ------------ + +boolean_chained_comparison.py:124:2: PLR1716 Contains chained boolean comparison that can be simplified + | +123 | # Unfixable due to parentheses. +124 | (a < b) and b < c + | ^^^^^^^^^^^^^^^^ PLR1716 +125 | a < b and (b < c) +126 | ((a < b) and b < c) + | + = help: Use a single compare expression + +boolean_chained_comparison.py:125:1: PLR1716 Contains chained boolean comparison that can be simplified + | +123 | # Unfixable due to parentheses. +124 | (a < b) and b < c +125 | a < b and (b < c) + | ^^^^^^^^^^^^^^^^ PLR1716 +126 | ((a < b) and b < c) +127 | (a < b) and (b < c) + | + = help: Use a single compare expression + +boolean_chained_comparison.py:126:3: PLR1716 Contains chained boolean comparison that can be simplified + | +124 | (a < b) and b < c +125 | a < b and (b < c) +126 | ((a < b) and b < c) + | ^^^^^^^^^^^^^^^^ PLR1716 +127 | (a < b) and (b < c) +128 | (((a < b))) and (b < c) + | + = help: Use a single compare expression + +boolean_chained_comparison.py:127:2: PLR1716 Contains chained boolean comparison that can be simplified + | +125 | a < b and (b < c) +126 | ((a < b) and b < c) +127 | (a < b) and (b < c) + | ^^^^^^^^^^^^^^^^^ PLR1716 +128 | (((a < b))) and (b < c) + | + = help: Use a single compare expression + +boolean_chained_comparison.py:128:4: PLR1716 Contains chained boolean comparison that can be simplified + | +126 | ((a < b) and b < c) +127 | (a < b) and (b < c) +128 | (((a < b))) and (b < c) + | ^^^^^^^^^^^^^^^^^^^ PLR1716 + | + = help: Use a single compare expression