Skip to content

Commit

Permalink
Do not offer an invalid fix for PLR1716 where the comparison contains…
Browse files Browse the repository at this point in the history
… parenthesis
  • Loading branch information
zanieb committed Sep 26, 2024
1 parent 7c83af4 commit f3e80e3
Show file tree
Hide file tree
Showing 3 changed files with 111 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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<String> {
Some("Use a single compare expression".to_string())
}
}

Expand All @@ -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
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit f3e80e3

Please sign in to comment.