[pylint] Implement boolean-chained-comparison (R1716)

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

@@ -0,0 +1,121 @@
+# ------------------
+# less than examples
+# ------------------
+
+a = int(input())
+b = int(input())
+c = int(input())
+if a < b and b < c:  # [boolean-chained-comparison]
+    pass
+
+a = int(input())
+b = int(input())
+c = int(input())
+if a < b and b <= c:  # [boolean-chained-comparison]
+    pass
+
+a = int(input())
+b = int(input())
+c = int(input())
+if a <= b and b < c:  # [boolean-chained-comparison]
+    pass
+
+a = int(input())
+b = int(input())
+c = int(input())
+if a <= b and b <= c:  # [boolean-chained-comparison]
+    pass
+
+# ---------------------
+# greater than examples
+# ---------------------
+
+a = int(input())
+b = int(input())
+c = int(input())
+if a > b and b > c:  # [boolean-chained-comparison]
+    pass
+
+a = int(input())
+b = int(input())
+c = int(input())
+if a >= b and b > c:  # [boolean-chained-comparison]
+    pass
+
+a = int(input())
+b = int(input())
+c = int(input())
+if a > b and b >= c:  # [boolean-chained-comparison]
+    pass
+
+a = int(input())
+b = int(input())
+c = int(input())
+if a >= b and b >= c:  # [boolean-chained-comparison]
+    pass
+
+# -----------------------
+# multiple fixes examples
+# -----------------------
+
+a = int(input())
+b = int(input())
+c = int(input())
+d = int(input())
+if a < b and b < c and c < d:  # [boolean-chained-comparison]
+    pass
+
+a = int(input())
+b = int(input())
+c = int(input())
+d = int(input())
+e = int(input())
+if a < b and b < c and c < d and d < e:  # [boolean-chained-comparison]
+    pass
+
+# ------------
+# bad examples
+# ------------
+
+a = int(input())
+b = int(input())
+c = int(input())
+if a > b or b > c: 
+    pass
+
+a = int(input())
+b = int(input())
+c = int(input())
+if a > b or b in (1, 2): 
+    pass
+
+a = int(input())
+b = int(input())
+c = int(input())
+if a < b and b > c: 
+    pass
+
+a = int(input())
+b = int(input())
+c = int(input())
+if a < b and b >= c: 
+    pass
+
+a = int(input())
+b = int(input())
+c = int(input())
+if a <= b and b > c: 
+    pass
+
+a = int(input())
+b = int(input())
+c = int(input())
+if a <= b and b >= c: 
+    pass
+
+# TODO: check if this example should be fixable or not
+a = int(input())
+b = int(input())
+c = int(input())
+if a > b and b < c: 
+    pass

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

@@ -1537,6 +1537,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
             }
         }
         Expr::BoolOp(bool_op) => {
+            if checker.enabled(Rule::BooleanChainedComparison) {
+                pylint::rules::boolean_chained_comparison(checker, bool_op);
+            }
             if checker.enabled(Rule::MultipleStartsEndsWith) {
                 flake8_pie::rules::multiple_starts_ends_with(checker, expr);
             }

crates/ruff_linter/src/codes.rs

@@ -257,6 +257,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Pylint, "R1714") => (RuleGroup::Stable, rules::pylint::rules::RepeatedEqualityComparison),
         (Pylint, "R1722") => (RuleGroup::Stable, rules::pylint::rules::SysExitAlias),
         (Pylint, "R1730") => (RuleGroup::Stable, rules::pylint::rules::IfStmtMinMax),
+        (Pylint, "R1716") => (RuleGroup::Stable, rules::pylint::rules::BooleanChainedComparison),
         (Pylint, "R1733") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryDictIndexLookup),
         (Pylint, "R1736") => (RuleGroup::Stable, rules::pylint::rules::UnnecessaryListIndexLookup),
         (Pylint, "R2004") => (RuleGroup::Stable, rules::pylint::rules::MagicValueComparison),

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

@@ -36,6 +36,10 @@ mod tests {
     #[test_case(Rule::BadStringFormatType, Path::new("bad_string_format_type.py"))]
     #[test_case(Rule::BidirectionalUnicode, Path::new("bidirectional_unicode.py"))]
     #[test_case(Rule::BinaryOpException, Path::new("binary_op_exception.py"))]
+    #[test_case(
+        Rule::BooleanChainedComparison,
+        Path::new("boolean_chained_comparison.py")
+    )]
     #[test_case(Rule::CollapsibleElseIf, Path::new("collapsible_else_if.py"))]
     #[test_case(Rule::CompareToEmptyString, Path::new("compare_to_empty_string.py"))]
     #[test_case(Rule::ComparisonOfConstant, Path::new("comparison_of_constant.py"))]

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

@@ -0,0 +1,145 @@
+use itertools::Itertools;
+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::TextRange;
+
+use crate::checkers::ast::Checker;
+
+/// ## What it does
+/// Check for chained boolean operations that can be simplified.
+///
+/// ## Why is this bad?
+/// Refactoring the code will improve readability for these cases.
+///
+/// ## Example
+///
+/// ```python
+/// a = int(input())
+/// b = int(input())
+/// c = int(input())
+/// if a < b and b < c:
+///     pass
+/// ```
+///
+/// Use instead:
+///
+/// ```python
+/// a = int(input())
+/// b = int(input())
+/// c = int(input())
+/// if a < b < c:
+///     pass
+/// ```
+#[violation]
+pub struct BooleanChainedComparison {
+    variable: Name,
+    range: TextRange,
+    replace_range: TextRange,
+}
+
+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) -> Option<String> {
+        Some("Simplify chained boolean comparisons".to_string())
+    }
+}
+
+/// PLR1716
+pub(crate) fn boolean_chained_comparison(checker: &mut Checker, expr_bool_op: &ExprBoolOp) {
+    // early out for boolean expression without multiple compare values
+    if expr_bool_op.values.len() == 1 {
+        return;
+    }
+
+    // early out for non `and` boolean operations
+    if expr_bool_op.op != BoolOp::And {
+        return;
+    }
+
+    // early exit when not all expressions are compare expressions
+    // TODO: check if this can even happen?
+    if !expr_bool_op.values.iter().all(Expr::is_compare_expr) {
+        return;
+    }
+
+    // retrieve all compare statements from expression
+    let compare_exprs: Vec<&ExprCompare> = expr_bool_op
+        .values
+        .iter()
+        .map(|stmt| stmt.as_compare_expr().unwrap())
+        .collect();
+
+    let results: Vec<Result<(), BooleanChainedComparison>> = compare_exprs
+        .iter()
+        .tuple_windows::<(&&ExprCompare, &&ExprCompare)>()
+        .filter(|(left_compare, right_compare)| {
+            are_compare_expr_simplifiable(left_compare, right_compare)
+        })
+        .map(|(left_compare, right_compare)| {
+            let Expr::Name(left_compare_right) = left_compare.comparators.first().unwrap() else {
+                return Ok(());
+            };
+
+            let Expr::Name(ref right_compare_left) = right_compare.left.as_ref() else {
+                return Ok(());
+            };
+
+            if left_compare_right.id() != right_compare_left.id() {
+                return Ok(());
+            }
+
+            Err(BooleanChainedComparison {
+                variable: left_compare_right.id().clone(),
+                range: TextRange::new(left_compare.range.start(), right_compare.range.end()),
+                replace_range: TextRange::new(
+                    left_compare_right.range.start(),
+                    right_compare_left.range.end(),
+                ),
+            })
+        })
+        .collect();
+
+    let results: Vec<BooleanChainedComparison> = results
+        .into_iter()
+        .filter(Result::is_err)
+        .map(|result| result.err().unwrap())
+        .collect();
+
+    checker.diagnostics.extend(
+        results
+            .into_iter()
+            .map(|result| {
+                let variable = result.variable.clone();
+                let range = result.range;
+                let replace_range = result.replace_range;
+                let mut diagnostic = Diagnostic::new(result, range);
+                diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
+                    variable.to_string(),
+                    replace_range,
+                )));
+                diagnostic
+            })
+            .collect::<Vec<Diagnostic>>(),
+    );
+}
+
+/// Checks whether two compare expressions are simplifiable
+fn are_compare_expr_simplifiable(left: &ExprCompare, right: &ExprCompare) -> bool {
+    // only allow simplifying simple compare operations
+    if left.ops.len() != 1 || right.ops.len() != 1 {
+        return false;
+    }
+
+    matches!(
+        (left.ops.first().unwrap(), right.ops.first().unwrap()),
+        (CmpOp::Lt | CmpOp::LtE, CmpOp::Lt | CmpOp::LtE)
+            | (CmpOp::Gt | CmpOp::GtE, CmpOp::Gt | CmpOp::GtE)
+    )
+}

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

@@ -9,6 +9,7 @@ pub(crate) use bad_string_format_character::BadStringFormatCharacter;
 pub(crate) use bad_string_format_type::*;
 pub(crate) use bidirectional_unicode::*;
 pub(crate) use binary_op_exception::*;
+pub(crate) use boolean_chained_comparison::*;
 pub(crate) use collapsible_else_if::*;
 pub(crate) use compare_to_empty_string::*;
 pub(crate) use comparison_of_constant::*;
@@ -112,6 +113,7 @@ pub(crate) mod bad_string_format_character;
 mod bad_string_format_type;
 mod bidirectional_unicode;
 mod binary_op_exception;
+mod boolean_chained_comparison;
 mod collapsible_else_if;
 mod compare_to_empty_string;
 mod comparison_of_constant;