mirror of https://github.com/astral-sh/ruff
[`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<c` which gives a syntax error. It could in theory have been fixed in a separate PR, but seems to be on theme here. ---------- - Closes #13524 - (1), (2), and (3) are implemented in separate commits for ease of review and modification. - Technically a user can trigger an error in ruff (by reaching max iterations) if they have a humongous boolean chained comparison with differing parentheses levels.
This commit is contained in:
Dylan
GitHub
parent
b56b3c813c
commit
9d641fa714
|
|
@ -119,10 +119,30 @@ c = int(input())
|
|||
if a > 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<b) and b<c and ((c<d))
|
||||
|
||||
# should error and fix
|
||||
a<b<c and c<d
|
||||
|
||||
# more involved examples (all should error and fix)
|
||||
a < ( # sneaky comment
|
||||
b
|
||||
# more comments
|
||||
) and b < c
|
||||
|
||||
(
|
||||
a
|
||||
<b
|
||||
# hmmm...
|
||||
<c
|
||||
and ((c<d))
|
||||
)
|
||||
|
||||
a < (b) and (((b)) < c)
|
||||
|
|
|
|||
|
|
@ -1,8 +1,9 @@
|
|||
use itertools::Itertools;
|
||||
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
|
||||
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
|
||||
use ruff_macros::{derive_message_formats, ViolationMetadata};
|
||||
use ruff_python_ast::{
|
||||
parenthesize::parenthesized_range, BoolOp, CmpOp, Expr, ExprBoolOp, ExprCompare,
|
||||
parenthesize::{parentheses_iterator, parenthesized_range},
|
||||
BoolOp, CmpOp, Expr, ExprBoolOp, ExprCompare,
|
||||
};
|
||||
use ruff_text_size::{Ranged, TextRange};
|
||||
|
||||
|
|
@ -36,16 +37,14 @@ use crate::checkers::ast::Checker;
|
|||
#[derive(ViolationMetadata)]
|
||||
pub(crate) struct BooleanChainedComparison;
|
||||
|
||||
impl Violation for BooleanChainedComparison {
|
||||
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
|
||||
|
||||
impl AlwaysFixableViolation for BooleanChainedComparison {
|
||||
#[derive_message_formats]
|
||||
fn message(&self) -> String {
|
||||
"Contains chained boolean comparison that can be simplified".to_string()
|
||||
}
|
||||
|
||||
fn fix_title(&self) -> Option<String> {
|
||||
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))<c`, we need to handle the
|
||||
// parentheses when specifying the fix range.
|
||||
let left_compare_right_range = parenthesized_range(
|
||||
left_compare_right.into(),
|
||||
left_compare.into(),
|
||||
comment_ranges,
|
||||
locator.contents(),
|
||||
)
|
||||
.unwrap_or(left_compare_right.range());
|
||||
let right_compare_left_range = parenthesized_range(
|
||||
right_compare_left.into(),
|
||||
right_compare.into(),
|
||||
comment_ranges,
|
||||
locator.contents(),
|
||||
)
|
||||
.unwrap_or(right_compare_left.range());
|
||||
let edit = Edit::range_replacement(
|
||||
locator.slice(left_compare_right_range).to_string(),
|
||||
TextRange::new(
|
||||
left_compare_right_range.start(),
|
||||
right_compare_left_range.end(),
|
||||
),
|
||||
);
|
||||
|
||||
// Balance left and right parentheses
|
||||
let fix = match left_paren_count.cmp(&right_paren_count) {
|
||||
std::cmp::Ordering::Less => {
|
||||
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)
|
||||
)
|
||||
})
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<b) and b<c and ((c<d))
|
||||
|
||||
boolean_chained_comparison.py:127: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)
|
||||
125 | ((a < b) and b < c)
|
||||
126 | (a < b) and (b < c)
|
||||
127 | (((a < b))) and (b < c)
|
||||
| ^^^^^^^^^^^^^^^^^^^ PLR1716
|
||||
128 |
|
||||
129 | (a<b) and b<c and ((c<d))
|
||||
|
|
||||
= help: Use a single compare expression
|
||||
|
||||
ℹ Safe fix
|
||||
124 124 | a < b and (b < c)
|
||||
125 125 | ((a < b) and b < c)
|
||||
126 126 | (a < b) and (b < c)
|
||||
127 |-(((a < b))) and (b < c)
|
||||
127 |+(((a < b < c)))
|
||||
128 128 |
|
||||
129 129 | (a<b) and b<c and ((c<d))
|
||||
130 130 |
|
||||
|
||||
boolean_chained_comparison.py:129:2: PLR1716 [*] Contains chained boolean comparison that can be simplified
|
||||
|
|
||||
127 | (((a < b))) and (b < c)
|
||||
128 |
|
||||
129 | (a<b) and b<c and ((c<d))
|
||||
| ^^^^^^^^^^^^ PLR1716
|
||||
130 |
|
||||
131 | # should error and fix
|
||||
|
|
||||
= help: Use a single compare expression
|
||||
|
||||
ℹ Safe fix
|
||||
126 126 | (a < b) and (b < c)
|
||||
127 127 | (((a < b))) and (b < c)
|
||||
128 128 |
|
||||
129 |-(a<b) and b<c and ((c<d))
|
||||
129 |+(a<b<c) and ((c<d))
|
||||
130 130 |
|
||||
131 131 | # should error and fix
|
||||
132 132 | a<b<c and c<d
|
||||
|
||||
boolean_chained_comparison.py:129:11: PLR1716 [*] Contains chained boolean comparison that can be simplified
|
||||
|
|
||||
127 | (((a < b))) and (b < c)
|
||||
128 |
|
||||
129 | (a<b) and b<c and ((c<d))
|
||||
| ^^^^^^^^^^^^^ PLR1716
|
||||
130 |
|
||||
131 | # should error and fix
|
||||
|
|
||||
= help: Use a single compare expression
|
||||
|
||||
ℹ Safe fix
|
||||
126 126 | (a < b) and (b < c)
|
||||
127 127 | (((a < b))) and (b < c)
|
||||
128 128 |
|
||||
129 |-(a<b) and b<c and ((c<d))
|
||||
129 |+(a<b) and ((b<c<d))
|
||||
130 130 |
|
||||
131 131 | # should error and fix
|
||||
132 132 | a<b<c and c<d
|
||||
|
||||
boolean_chained_comparison.py:132:1: PLR1716 [*] Contains chained boolean comparison that can be simplified
|
||||
|
|
||||
131 | # should error and fix
|
||||
132 | a<b<c and c<d
|
||||
| ^^^^^^^^^^^^^ PLR1716
|
||||
133 |
|
||||
134 | # more involved examples (all should error and fix)
|
||||
|
|
||||
= help: Use a single compare expression
|
||||
|
||||
ℹ Safe fix
|
||||
129 129 | (a<b) and b<c and ((c<d))
|
||||
130 130 |
|
||||
131 131 | # should error and fix
|
||||
132 |-a<b<c and c<d
|
||||
132 |+a<b<c<d
|
||||
133 133 |
|
||||
134 134 | # more involved examples (all should error and fix)
|
||||
135 135 | a < ( # sneaky comment
|
||||
|
||||
boolean_chained_comparison.py:135:1: PLR1716 [*] Contains chained boolean comparison that can be simplified
|
||||
|
|
||||
134 | # more involved examples (all should error and fix)
|
||||
135 | / a < ( # sneaky comment
|
||||
136 | | b
|
||||
137 | | # more comments
|
||||
138 | | ) and b < c
|
||||
| |___________^ PLR1716
|
||||
139 |
|
||||
140 | (
|
||||
|
|
||||
= help: Use a single compare expression
|
||||
|
||||
ℹ Safe fix
|
||||
135 135 | a < ( # sneaky comment
|
||||
136 136 | b
|
||||
137 137 | # more comments
|
||||
138 |-) and b < c
|
||||
138 |+) < c
|
||||
139 139 |
|
||||
140 140 | (
|
||||
141 141 | a
|
||||
|
||||
boolean_chained_comparison.py:141:5: PLR1716 [*] Contains chained boolean comparison that can be simplified
|
||||
|
|
||||
140 | (
|
||||
141 | a
|
||||
| _____^
|
||||
142 | | <b
|
||||
143 | | # hmmm...
|
||||
144 | | <c
|
||||
145 | | and ((c<d))
|
||||
| |_____________^ PLR1716
|
||||
146 | )
|
||||
|
|
||||
= help: Use a single compare expression
|
||||
|
||||
ℹ Safe fix
|
||||
138 138 | ) and b < c
|
||||
139 139 |
|
||||
140 140 | (
|
||||
141 |- a
|
||||
141 |+ ((a
|
||||
142 142 | <b
|
||||
143 143 | # hmmm...
|
||||
144 |- <c
|
||||
145 |- and ((c<d))
|
||||
144 |+ <c<d))
|
||||
146 145 | )
|
||||
147 146 |
|
||||
148 147 | a < (b) and (((b)) < c)
|
||||
|
||||
boolean_chained_comparison.py:148:1: PLR1716 [*] Contains chained boolean comparison that can be simplified
|
||||
|
|
||||
146 | )
|
||||
147 |
|
||||
148 | a < (b) and (((b)) < c)
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^ PLR1716
|
||||
|
|
||||
= help: Use a single compare expression
|
||||
|
||||
ℹ Safe fix
|
||||
145 145 | and ((c<d))
|
||||
146 146 | )
|
||||
147 147 |
|
||||
148 |-a < (b) and (((b)) < c)
|
||||
148 |+(a < (b) < c)
|
||||
|
|
|
|||
Loading…
Reference in New Issue