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 6e4ec1e
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -118,3 +118,9 @@
c = int(input())
if a > b and b < c:
pass


# Unfixable due to parenthesis
(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,5 +1,5 @@
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_text_size::{Ranged, TextRange};
Expand Down Expand Up @@ -36,14 +36,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 +61,8 @@ pub(crate) fn boolean_chained_comparison(checker: &mut Checker, expr_bool_op: &E
return;
}

let locator = checker.locator();

// retrieve all compare statements from expression
let compare_expressions = expr_bool_op
.values
Expand All @@ -83,20 +87,30 @@ 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 replace_range =
TextRange::new(left_compare_right.start(), right_compare_left.end());

// It's unsafe to apply the fix if the range contains a parenthesis
let fix = if locator.slice(&replace_range).contains(')') {
None
} else {
let edit =
Edit::range_replacement(left_compare_right.id().to_string(), replace_range);
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()),
);

Some(
Diagnostic::new(
BooleanChainedComparison {
variable: left_compare_right.id().clone(),
},
TextRange::new(left_compare.start(), right_compare.end()),
)
.with_fix(Fix::safe_edit(edit)),
)
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,32 @@ 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 parenthesis
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:3: PLR1716 Contains chained boolean comparison that can be simplified
|
123 | # Unfixable due to parenthesis
124 | (a < b) and b < c
125 | ((a < b) and b < c)
| ^^^^^^^^^^^^^^^^ PLR1716
126 | (a < b) and (b < c)
|
= help: Use a single compare expression

boolean_chained_comparison.py:126:2: 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
|
= help: Use a single compare expression

0 comments on commit 6e4ec1e

Please sign in to comment.