Skip to content

Commit ca0ae0a

Browse files
vincevannoortMichaReiser
vincevannoort
and
MichaReiser
authored
[pylint] Implement boolean-chained-comparison (R1716) (#13435)
Co-authored-by: Micha Reiser <[email protected]>
1 parent be1d5e3 commit ca0ae0a

File tree

8 files changed

+513
-0
lines changed

8 files changed

+513
-0
lines changed

‎crates/ruff_linter/resources/test/fixtures/pylint/boolean_chained_comparison.py

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
# ------------------
2+
# less than examples
3+
# ------------------
4+
5+
a = int(input())
6+
b = int(input())
7+
c = int(input())
8+
if a < b and b < c: # [boolean-chained-comparison]
9+
pass
10+
11+
a = int(input())
12+
b = int(input())
13+
c = int(input())
14+
if a < b and b <= c: # [boolean-chained-comparison]
15+
pass
16+
17+
a = int(input())
18+
b = int(input())
19+
c = int(input())
20+
if a <= b and b < c: # [boolean-chained-comparison]
21+
pass
22+
23+
a = int(input())
24+
b = int(input())
25+
c = int(input())
26+
if a <= b and b <= c: # [boolean-chained-comparison]
27+
pass
28+
29+
# ---------------------
30+
# greater than examples
31+
# ---------------------
32+
33+
a = int(input())
34+
b = int(input())
35+
c = int(input())
36+
if a > b and b > c: # [boolean-chained-comparison]
37+
pass
38+
39+
a = int(input())
40+
b = int(input())
41+
c = int(input())
42+
if a >= b and b > c: # [boolean-chained-comparison]
43+
pass
44+
45+
a = int(input())
46+
b = int(input())
47+
c = int(input())
48+
if a > b and b >= c: # [boolean-chained-comparison]
49+
pass
50+
51+
a = int(input())
52+
b = int(input())
53+
c = int(input())
54+
if a >= b and b >= c: # [boolean-chained-comparison]
55+
pass
56+
57+
# -----------------------
58+
# multiple fixes examples
59+
# -----------------------
60+
61+
a = int(input())
62+
b = int(input())
63+
c = int(input())
64+
d = int(input())
65+
if a < b and b < c and c < d: # [boolean-chained-comparison]
66+
pass
67+
68+
a = int(input())
69+
b = int(input())
70+
c = int(input())
71+
d = int(input())
72+
e = int(input())
73+
if a < b and b < c and c < d and d < e: # [boolean-chained-comparison]
74+
pass
75+
76+
# ------------
77+
# bad examples
78+
# ------------
79+
80+
a = int(input())
81+
b = int(input())
82+
c = int(input())
83+
if a > b or b > c:
84+
pass
85+
86+
a = int(input())
87+
b = int(input())
88+
c = int(input())
89+
if a > b and b in (1, 2):
90+
pass
91+
92+
a = int(input())
93+
b = int(input())
94+
c = int(input())
95+
if a < b and b > c:
96+
pass
97+
98+
a = int(input())
99+
b = int(input())
100+
c = int(input())
101+
if a < b and b >= c:
102+
pass
103+
104+
a = int(input())
105+
b = int(input())
106+
c = int(input())
107+
if a <= b and b > c:
108+
pass
109+
110+
a = int(input())
111+
b = int(input())
112+
c = int(input())
113+
if a <= b and b >= c:
114+
pass
115+
116+
a = int(input())
117+
b = int(input())
118+
c = int(input())
119+
if a > b and b < c:
120+
pass

‎crates/ruff_linter/src/checkers/ast/analyze/expression.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1537,6 +1537,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
15371537
}
15381538
}
15391539
Expr::BoolOp(bool_op) => {
1540+
if checker.enabled(Rule::BooleanChainedComparison) {
1541+
pylint::rules::boolean_chained_comparison(checker, bool_op);
1542+
}
15401543
if checker.enabled(Rule::MultipleStartsEndsWith) {
15411544
flake8_pie::rules::multiple_starts_ends_with(checker, expr);
15421545
}

‎crates/ruff_linter/src/codes.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -257,6 +257,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
257257
(Pylint, "R1714") => (RuleGroup::Stable, rules::pylint::rules::RepeatedEqualityComparison),
258258
(Pylint, "R1722") => (RuleGroup::Stable, rules::pylint::rules::SysExitAlias),
259259
(Pylint, "R1730") => (RuleGroup::Stable, rules::pylint::rules::IfStmtMinMax),
260+
(Pylint, "R1716") => (RuleGroup::Preview, rules::pylint::rules::BooleanChainedComparison),
260261
(Pylint, "R1733") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryDictIndexLookup),
261262
(Pylint, "R1736") => (RuleGroup::Stable, rules::pylint::rules::UnnecessaryListIndexLookup),
262263
(Pylint, "R2004") => (RuleGroup::Stable, rules::pylint::rules::MagicValueComparison),

‎crates/ruff_linter/src/rules/pylint/mod.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,10 @@ mod tests {
3636
#[test_case(Rule::BadStringFormatType, Path::new("bad_string_format_type.py"))]
3737
#[test_case(Rule::BidirectionalUnicode, Path::new("bidirectional_unicode.py"))]
3838
#[test_case(Rule::BinaryOpException, Path::new("binary_op_exception.py"))]
39+
#[test_case(
40+
Rule::BooleanChainedComparison,
41+
Path::new("boolean_chained_comparison.py")
42+
)]
3943
#[test_case(Rule::CollapsibleElseIf, Path::new("collapsible_else_if.py"))]
4044
#[test_case(Rule::CompareToEmptyString, Path::new("compare_to_empty_string.py"))]
4145
#[test_case(Rule::ComparisonOfConstant, Path::new("comparison_of_constant.py"))]

‎crates/ruff_linter/src/rules/pylint/rules/boolean_chained_comparison.rs

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
use itertools::Itertools;
2+
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
3+
use ruff_macros::{derive_message_formats, violation};
4+
use ruff_python_ast::{name::Name, BoolOp, CmpOp, Expr, ExprBoolOp, ExprCompare};
5+
use ruff_text_size::{Ranged, TextRange};
6+
7+
use crate::checkers::ast::Checker;
8+
9+
/// ## What it does
10+
/// Check for chained boolean operations that can be simplified.
11+
///
12+
/// ## Why is this bad?
13+
/// Refactoring the code will improve readability for these cases.
14+
///
15+
/// ## Example
16+
///
17+
/// ```python
18+
/// a = int(input())
19+
/// b = int(input())
20+
/// c = int(input())
21+
/// if a < b and b < c:
22+
/// pass
23+
/// ```
24+
///
25+
/// Use instead:
26+
///
27+
/// ```python
28+
/// a = int(input())
29+
/// b = int(input())
30+
/// c = int(input())
31+
/// if a < b < c:
32+
/// pass
33+
/// ```
34+
#[violation]
35+
pub struct BooleanChainedComparison {
36+
variable: Name,
37+
}
38+
39+
impl AlwaysFixableViolation for BooleanChainedComparison {
40+
#[derive_message_formats]
41+
fn message(&self) -> String {
42+
format!("Contains chained boolean comparison that can be simplified")
43+
}
44+
45+
fn fix_title(&self) -> String {
46+
"Use a single compare expression".to_string()
47+
}
48+
}
49+
50+
/// PLR1716
51+
pub(crate) fn boolean_chained_comparison(checker: &mut Checker, expr_bool_op: &ExprBoolOp) {
52+
// early exit for non `and` boolean operations
53+
if expr_bool_op.op != BoolOp::And {
54+
return;
55+
}
56+
57+
// early exit when not all expressions are compare expressions
58+
if !expr_bool_op.values.iter().all(Expr::is_compare_expr) {
59+
return;
60+
}
61+
62+
// retrieve all compare statements from expression
63+
let compare_expressions = expr_bool_op
64+
.values
65+
.iter()
66+
.map(|stmt| stmt.as_compare_expr().unwrap());
67+
68+
let diagnostics = compare_expressions
69+
.tuple_windows()
70+
.filter(|(left_compare, right_compare)| {
71+
are_compare_expr_simplifiable(left_compare, right_compare)
72+
})
73+
.filter_map(|(left_compare, right_compare)| {
74+
let Expr::Name(left_compare_right) = left_compare.comparators.first()? else {
75+
return None;
76+
};
77+
78+
let Expr::Name(right_compare_left) = &*right_compare.left else {
79+
return None;
80+
};
81+
82+
if left_compare_right.id() != right_compare_left.id() {
83+
return None;
84+
}
85+
86+
let edit = Edit::range_replacement(
87+
left_compare_right.id().to_string(),
88+
TextRange::new(left_compare_right.start(), right_compare_left.end()),
89+
);
90+
91+
Some(
92+
Diagnostic::new(
93+
BooleanChainedComparison {
94+
variable: left_compare_right.id().clone(),
95+
},
96+
TextRange::new(left_compare.start(), right_compare.end()),
97+
)
98+
.with_fix(Fix::safe_edit(edit)),
99+
)
100+
});
101+
102+
checker.diagnostics.extend(diagnostics);
103+
}
104+
105+
/// Checks whether two compare expressions are simplifiable
106+
fn are_compare_expr_simplifiable(left: &ExprCompare, right: &ExprCompare) -> bool {
107+
let [left_operator] = &*left.ops else {
108+
return false;
109+
};
110+
111+
let [right_operator] = &*right.ops else {
112+
return false;
113+
};
114+
115+
matches!(
116+
(left_operator, right_operator),
117+
(CmpOp::Lt | CmpOp::LtE, CmpOp::Lt | CmpOp::LtE)
118+
| (CmpOp::Gt | CmpOp::GtE, CmpOp::Gt | CmpOp::GtE)
119+
)
120+
}

‎crates/ruff_linter/src/rules/pylint/rules/mod.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ pub(crate) use bad_string_format_character::BadStringFormatCharacter;
99
pub(crate) use bad_string_format_type::*;
1010
pub(crate) use bidirectional_unicode::*;
1111
pub(crate) use binary_op_exception::*;
12+
pub(crate) use boolean_chained_comparison::*;
1213
pub(crate) use collapsible_else_if::*;
1314
pub(crate) use compare_to_empty_string::*;
1415
pub(crate) use comparison_of_constant::*;
@@ -112,6 +113,7 @@ pub(crate) mod bad_string_format_character;
112113
mod bad_string_format_type;
113114
mod bidirectional_unicode;
114115
mod binary_op_exception;
116+
mod boolean_chained_comparison;
115117
mod collapsible_else_if;
116118
mod compare_to_empty_string;
117119
mod comparison_of_constant;

0 commit comments

Comments
 (0)