From 9d641fa71470cce68a05590efb318a4dd0439898 Mon Sep 17 00:00:00 2001 From: Dylan <53534755+dylwil3@users.noreply.github.com> Date: Sun, 8 Dec 2024 22:58:45 -0600 Subject: [PATCH] [`pylint`] Include parentheses and multiple comparators in check for `boolean-chained-comparison (PLR1716)` (#14781) This PR introduces three changes to the diagnostic and fix behavior (still under preview) for [boolean-chained-comparison (PLR1716)](https://docs.astral.sh/ruff/rules/boolean-chained-comparison/#boolean-chained-comparison-plr1716). 1. We now offer a _fix_ in the case of parenthesized expressions like `(a < b) and b < c`. The fix will merge the chains of comparisons and then balance parentheses by _adding_ parentheses to one side of the expression. 2. We now trigger a diagnostic (and fix) in the case where some comparisons have multiple comparators like `a < b < c and c < d`. 3. When adjacent comparators are parenthesized, we prefer the left parenthesization and apply the replacement to the whole parenthesized range. So, for example, `a < (b) and ((b)) < c` becomes `a < (b) < c`. While these seem like somewhat disconnected changes, they are actually related. If we only offered (1), then we would see the following fix behavior: ```diff - (a < b) and b < c and ((c < d)) + (a < b < c) and ((c < d)) ``` This is because the fix which add parentheses to the first pair of comparisons overlaps with the fix that removes the `and` between the second two comparisons. So the latter fix is deferred. However, the latter fix does not get a second chance because, upon the next lint iteration, there is no violation of `PLR1716`. Upon adopting (2), however, both fixes occur by the time ruff completes several iterations and we get: ```diff - (a < b) and b < c and ((c < d)) + ((a < b < c < d)) ``` Finally, (3) fixes a previously unobserved bug wherein the autofix for `a < (b) and b < c` used to result in `a<(b b and b < c: pass - -# Unfixable due to parentheses. +# fixes will balance 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) + +(a String { "Contains chained boolean comparison that can be simplified".to_string() } - fn fix_title(&self) -> Option { - Some("Use a single compare expression".to_string()) + fn fix_title(&self) -> String { + "Use a single compare expression".to_string() } } @@ -64,11 +63,11 @@ pub(crate) fn boolean_chained_comparison(checker: &mut Checker, expr_bool_op: &E let locator = checker.locator(); let comment_ranges = checker.comment_ranges(); - // retrieve all compare statements from expression + // retrieve all compare expressions from boolean expression let compare_expressions = expr_bool_op .values .iter() - .map(|stmt| stmt.as_compare_expr().unwrap()); + .map(|expr| expr.as_compare_expr().unwrap()); let diagnostics = compare_expressions .tuple_windows() @@ -76,7 +75,7 @@ pub(crate) fn boolean_chained_comparison(checker: &mut Checker, expr_bool_op: &E are_compare_expr_simplifiable(left_compare, right_compare) }) .filter_map(|(left_compare, right_compare)| { - let Expr::Name(left_compare_right) = left_compare.comparators.first()? else { + let Expr::Name(left_compare_right) = left_compare.comparators.last()? else { return None; }; @@ -88,33 +87,65 @@ pub(crate) fn boolean_chained_comparison(checker: &mut Checker, expr_bool_op: &E return None; } - let left_has_paren = parenthesized_range( + let left_paren_count = parentheses_iterator( left_compare.into(), - expr_bool_op.into(), + Some(expr_bool_op.into()), comment_ranges, locator.contents(), ) - .is_some(); + .count(); - let right_has_paren = parenthesized_range( + let right_paren_count = parentheses_iterator( right_compare.into(), - expr_bool_op.into(), + Some(expr_bool_op.into()), comment_ranges, locator.contents(), ) - .is_some(); + .count(); - // 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)) + // Create the edit that removes the comparison operator + + // In `a<(b) and ((b)) { + let balance_parens_edit = Edit::insertion( + "(".repeat(right_paren_count - left_paren_count), + left_compare.start(), + ); + Fix::safe_edits(edit, [balance_parens_edit]) + } + std::cmp::Ordering::Equal => Fix::safe_edit(edit), + std::cmp::Ordering::Greater => { + let balance_parens_edit = Edit::insertion( + ")".repeat(left_paren_count - right_paren_count), + right_compare.end(), + ); + Fix::safe_edits(edit, [balance_parens_edit]) + } }; let mut diagnostic = Diagnostic::new( @@ -122,9 +153,7 @@ pub(crate) fn boolean_chained_comparison(checker: &mut Checker, expr_bool_op: &E TextRange::new(left_compare.start(), right_compare.end()), ); - if let Some(fix) = fix { - diagnostic.set_fix(fix); - } + diagnostic.set_fix(fix); Some(diagnostic) }); @@ -134,17 +163,15 @@ pub(crate) fn boolean_chained_comparison(checker: &mut Checker, expr_bool_op: &E /// Checks whether two compare expressions are simplifiable fn are_compare_expr_simplifiable(left: &ExprCompare, right: &ExprCompare) -> bool { - let [left_operator] = &*left.ops else { - return false; - }; - - let [right_operator] = &*right.ops else { - return false; - }; - - matches!( - (left_operator, right_operator), - (CmpOp::Lt | CmpOp::LtE, CmpOp::Lt | CmpOp::LtE) - | (CmpOp::Gt | CmpOp::GtE, CmpOp::Gt | CmpOp::GtE) - ) + left.ops + .iter() + .chain(right.ops.iter()) + .tuple_windows::<(_, _)>() + .all(|(left_operator, right_operator)| { + matches!( + (left_operator, right_operator), + (CmpOp::Lt | CmpOp::LtE, CmpOp::Lt | CmpOp::LtE) + | (CmpOp::Gt | CmpOp::GtE, CmpOp::Gt | CmpOp::GtE) + ) + }) } 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 1628f99501..f18b102153 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 @@ -1,6 +1,5 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs -snapshot_kind: text --- boolean_chained_comparison.py:8:4: PLR1716 [*] Contains chained boolean comparison that can be simplified | @@ -262,53 +261,235 @@ boolean_chained_comparison.py:73:24: PLR1716 [*] Contains chained boolean compar 75 75 | 76 76 | # ------------ -boolean_chained_comparison.py:124:2: PLR1716 Contains chained boolean comparison that can be simplified +boolean_chained_comparison.py:123:2: PLR1716 [*] Contains chained boolean comparison that can be simplified | -123 | # Unfixable due to parentheses. -124 | (a < b) and b < c +122 | # fixes will balance parentheses +123 | (a < b) and b < c | ^^^^^^^^^^^^^^^^ PLR1716 -125 | a < b and (b < c) -126 | ((a < b) and b < c) +124 | a < b and (b < c) +125 | ((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 +ℹ Safe fix +120 120 | pass +121 121 | +122 122 | # fixes will balance parentheses +123 |-(a < b) and b < c + 123 |+(a < b < c) +124 124 | a < b and (b < c) +125 125 | ((a < b) and b < c) +126 126 | (a < b) and (b < c) + +boolean_chained_comparison.py:124: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) +122 | # fixes will balance parentheses +123 | (a < b) and b < c +124 | a < b and (b < c) | ^^^^^^^^^^^^^^^^ PLR1716 -126 | ((a < b) and b < c) -127 | (a < b) and (b < c) +125 | ((a < b) and b < c) +126 | (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 +ℹ Safe fix +121 121 | +122 122 | # fixes will balance parentheses +123 123 | (a < b) and b < c +124 |-a < b and (b < c) + 124 |+(a < b < c) +125 125 | ((a < b) and b < c) +126 126 | (a < b) and (b < c) +127 127 | (((a < b))) and (b < c) + +boolean_chained_comparison.py:125: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) +123 | (a < b) and b < c +124 | a < b and (b < c) +125 | ((a < b) and b < c) | ^^^^^^^^^^^^^^^^ PLR1716 -127 | (a < b) and (b < c) -128 | (((a < b))) and (b < c) +126 | (a < b) and (b < c) +127 | (((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 +ℹ Safe fix +122 122 | # fixes will balance parentheses +123 123 | (a < b) and b < c +124 124 | a < b and (b < c) +125 |-((a < b) and b < c) + 125 |+((a < b < c)) +126 126 | (a < b) and (b < c) +127 127 | (((a < b))) and (b < c) +128 128 | + +boolean_chained_comparison.py:126: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) +124 | a < b and (b < c) +125 | ((a < b) and b < c) +126 | (a < b) and (b < c) | ^^^^^^^^^^^^^^^^^ PLR1716 -128 | (((a < b))) and (b < c) +127 | (((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 +ℹ Safe fix +123 123 | (a < b) and b < c +124 124 | a < b and (b < c) +125 125 | ((a < b) and b < c) +126 |-(a < b) and (b < c) + 126 |+(a < b < c) +127 127 | (((a < b))) and (b < c) +128 128 | +129 129 | (a