Add rule PLR5601

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

@@ -0,0 +1,140 @@
+def triggered_if_if_block_ends_with_elif(machine, old_conf, new_conf):
+    """Example code that will trigger the message
+
+    Given an if-elif construct
+    When the body of the if ends with an elif
+    Then the message confusing-consecutive-elif must be triggered.
+    """
+    if old_conf:
+        if not new_conf:
+            machine.disable()
+        elif old_conf.value != new_conf.value:
+            machine.disable()
+            machine.enable(new_conf.value)
+    elif new_conf:  # [confusing-consecutive-elif]
+        machine.enable(new_conf.value)
+
+
+def not_triggered_if_indented_block_ends_with_else(machine, old_conf, new_conf):
+    """Example code must not trigger the message, because the inner block ends with else.
+
+    Given an if-elif construct
+    When the body of the if ends with an else
+    Then no message shall be triggered.
+    """
+    if old_conf:
+        if not new_conf:
+            machine.disable()
+        elif old_conf.value != new_conf.value:
+            machine.disable()
+            machine.enable(new_conf.value)
+        else:
+            pass
+    elif new_conf:
+        machine.enable(new_conf.value)
+
+
+def not_triggered_if_indentend_block_ends_with_call(machine, old_conf, new_conf):
+    """
+    Example code must not trigger the message,
+
+    Given an if-elif construct
+    When the body of the if ends with a function call
+    Then no message shall be triggered.
+
+    Note: There is nothing special about the body ending with a function call.
+    This is just taken as a representative value for the equivalence class of
+    "every node class unrelated to if/elif/else".
+    """
+    if old_conf:
+        if not new_conf:
+            machine.disable()
+        elif old_conf.value != new_conf.value:
+            machine.disable()
+            machine.enable(new_conf.value)
+        print("Processed old configuration...")
+    elif new_conf:
+        machine.enable(new_conf.value)
+
+
+def triggered_if_elif_block_ends_with_elif(machine, old_conf, new_conf, new_new_conf):
+    """Example code that will trigger the message
+
+    Given an if-elif-elif construct
+    When the body of the first elif ends with an elif
+    Then the message confusing-consecutive-elif must be triggered.
+    """
+    if old_conf:
+        machine.disable()
+    elif not new_conf:
+        if new_new_conf:
+            machine.disable()
+        elif old_conf.value != new_conf.value:
+            machine.disable()
+            machine.enable(new_conf.value)
+    elif new_conf:  # [confusing-consecutive-elif]
+        machine.enable(new_conf.value)
+
+
+def triggered_if_block_ends_with_if(machine, old_conf, new_conf, new_new_conf):
+    """Example code that will trigger the message
+
+    Given an if-elif construct
+    When the body of the if ends with an if
+    Then the message confusing-consecutive-elif must be triggered.
+    """
+    if old_conf:
+        if new_new_conf:
+            machine.disable()
+    elif new_conf:  # [confusing-consecutive-elif]
+        machine.enable(new_conf.value)
+
+
+def not_triggered_if_indented_block_ends_with_ifexp(machine, old_conf, new_conf):
+    """
+    Example code must not trigger the message,
+
+    Given an if-elif construct
+    When the body of the if ends with an if expression
+    Then no message shall be triggered.
+    """
+    if old_conf:
+        if not new_conf:
+            machine.disable()
+        print("Processed old configuration...")
+    elif new_conf:
+        machine.enable(new_conf.value)
+
+
+def not_triggered_if_outer_block_does_not_have_elif(machine, old_conf, new_conf):
+    """Example code must not trigger the message
+
+    Given an if construct without an elif
+    When the body of the if ends with an if
+    Then no message shall be triggered.
+    """
+    if old_conf:
+        if not new_conf:
+            machine.disable()
+        elif old_conf.value != new_conf.value:
+            machine.disable()
+            machine.enable(new_conf.value)
+    else:
+        pass
+
+
+def not_triggered_if_outer_block_continues_with_if(machine, old_conf, new_conf, new_new_conf):
+    """Example code that will trigger the message
+
+    Given an if construct which continues with a new if construct
+    When the body of the first if ends with an if expression
+    Then no message shall be triggered.
+    """
+    if old_conf:
+        if new_new_conf:
+            machine.disable()
+        elif old_conf.value != new_conf.value:
+            machine.disable()
+            machine.enable(new_conf.value)
+    if new_conf:
+        machine.enable(new_conf.value)

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

@@ -1124,6 +1124,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
             if checker.enabled(Rule::CollapsibleElseIf) {
                 pylint::rules::collapsible_else_if(checker, stmt);
             }
+            if checker.enabled(Rule::ConfusingConsecutiveElif) {
+                pylint::rules::confusing_consecutive_elif(checker, if_);
+            }
             if checker.enabled(Rule::CheckAndRemoveFromSet) {
                 refurb::rules::check_and_remove_from_set(checker, if_);
             }

crates/ruff_linter/src/codes.rs

@@ -284,6 +284,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Pylint, "R2004") => (RuleGroup::Stable, rules::pylint::rules::MagicValueComparison),
         (Pylint, "R2044") => (RuleGroup::Preview, rules::pylint::rules::EmptyComment),
         (Pylint, "R5501") => (RuleGroup::Stable, rules::pylint::rules::CollapsibleElseIf),
+        (Pylint, "R5601") => (RuleGroup::Preview, rules::pylint::rules::ConfusingConsecutiveElif),
         (Pylint, "R6201") => (RuleGroup::Preview, rules::pylint::rules::LiteralMembership),
         #[allow(deprecated)]
         (Pylint, "R6301") => (RuleGroup::Nursery, rules::pylint::rules::NoSelfUse),

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

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

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

@@ -0,0 +1,132 @@
+use ruff_diagnostics::{Diagnostic, Violation};
+use ruff_macros::{derive_message_formats, violation};
+use ruff_python_ast::{self as ast, ElifElseClause, Stmt, StmtIf};
+use ruff_text_size::{Ranged, TextRange};
+
+use crate::checkers::ast::Checker;
+
+/// ## What it does
+/// Checks for an `elif` statement which follows right after an indented block which itself ends with if or elif."
+///
+/// ## Why is this bad?
+/// It may not be ovious if the elif statement was willingly or mistakenly unindented.
+/// Extracting the indented if statement into a separate function might avoid confusion and prevent errors.
+///
+/// ## Example
+/// ```python
+/// if old_conf:
+///     if not new_conf:
+///         machine.disable()
+///     elif old_conf.value != new_conf.value:
+///         machine.disable()
+///         machine.enable(new_conf.value)
+/// elif new_conf:  # [confusing-consecutive-elif]
+///     machine.enable(new_conf.value)
+/// ```
+///
+/// Use instead:
+/// ```python
+/// def extracted(old_conf, new_conf, machine):
+///     if not new_conf:
+///         machine.disable()
+///     elif old_conf.value != new_conf.value:
+///         machine.disable()
+///         machine.enable(new_conf.value)
+///
+/// 
+/// if old_conf:
+///     extracted(old_conf, new_conf, machine)
+/// elif new_conf:
+///     machine.enable(new_conf.value)
+/// ```
+///
+/// ## References
+/// - [Python documentation: `if` Statements](https://docs.python.org/3/tutorial/controlflow.html#if-statements)
+#[violation]
+pub struct ConfusingConsecutiveElif;
+
+impl Violation for ConfusingConsecutiveElif {
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        format!("Use `elif` instead of `else` then `if`, to reduce indentation")
+    }
+}
+
+/// PLR5601
+pub(crate) fn confusing_consecutive_elif(checker: &mut Checker, stmt_if: &StmtIf) {
+    let ast::StmtIf {
+        body,
+        elif_else_clauses,
+        ..
+    } = stmt_if;
+
+    // The last clause must be an elif
+    let Some(ElifElseClause { test: Some(_), .. }) = elif_else_clauses.last() else {
+        return;
+    };
+
+    // Take the second last elif, or if that does not exist, take the if
+    let orelse = match elif_else_clauses.len() {
+        0 => return,
+        1 => {
+            let Some(Stmt::If(ast::StmtIf {
+                elif_else_clauses: orelse,
+                ..
+            })) = body.last()
+            else {
+                return;
+            };
+            orelse
+        }
+        _ => {
+            let [.., ElifElseClause {
+                body: body_stmt,
+                test: Some(_),
+                ..
+            }, _] = elif_else_clauses.as_slice()
+            else {
+                return;
+            };
+            let Some(Stmt::If(ast::StmtIf {
+                elif_else_clauses: orelse,
+                ..
+            })) = body_stmt.last()
+            else {
+                return;
+            };
+            orelse
+        }
+    };
+    if !has_no_else_clause(orelse) {
+        return;
+    }
+
+    let diagnostic = Diagnostic::new(
+        ConfusingConsecutiveElif,
+        TextRange::new(elif_else_clauses.last().unwrap().start(), stmt_if.end()),
+    );
+
+    checker.diagnostics.push(diagnostic);
+}
+
+fn has_no_else_clause(orelse: &[ElifElseClause]) -> bool {
+    if orelse.is_empty() {
+        return true;
+    }
+    let Some(ElifElseClause {
+        body: body_stmt,
+        test: Some(_),
+        ..
+    }) = orelse.last()
+    else {
+        return false;
+    };
+    let Some(Stmt::If(ast::StmtIf {
+        elif_else_clauses: orelse,
+        ..
+    })) = body_stmt.last()
+    else {
+        return true;
+    };
+    has_no_else_clause(orelse)
+}

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

@@ -12,6 +12,7 @@ pub(crate) use collapsible_else_if::*;
 pub(crate) use compare_to_empty_string::*;
 pub(crate) use comparison_of_constant::*;
 pub(crate) use comparison_with_itself::*;
+pub(crate) use confusing_consecutive_elif::*;
 pub(crate) use continue_in_finally::*;
 pub(crate) use dict_iter_missing_items::*;
 pub(crate) use duplicate_bases::*;
@@ -98,6 +99,7 @@ mod collapsible_else_if;
 mod compare_to_empty_string;
 mod comparison_of_constant;
 mod comparison_with_itself;
+mod confusing_consecutive_elif;
 mod continue_in_finally;
 mod dict_iter_missing_items;
 mod duplicate_bases;

crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR5601_confusing_consecutive_elif.py.snap

@@ -0,0 +1,32 @@
+---
+source: crates/ruff_linter/src/rules/pylint/mod.rs
+---
+confusing_consecutive_elif.py:14:5: PLR5601 Use `elif` instead of `else` then `if`, to reduce indentation
+   |
+12 |               machine.disable()
+13 |               machine.enable(new_conf.value)
+14 |       elif new_conf:  # [confusing-consecutive-elif]
+   |  _____^
+15 | |         machine.enable(new_conf.value)
+   | |______________________________________^ PLR5601
+   |
+
+confusing_consecutive_elif.py:75:5: PLR5601 Use `elif` instead of `else` then `if`, to reduce indentation
+   |
+73 |               machine.disable()
+74 |               machine.enable(new_conf.value)
+75 |       elif new_conf:  # [confusing-consecutive-elif]
+   |  _____^
+76 | |         machine.enable(new_conf.value)
+   | |______________________________________^ PLR5601
+   |
+
+confusing_consecutive_elif.py:89:5: PLR5601 Use `elif` instead of `else` then `if`, to reduce indentation
+   |
+87 |           if new_new_conf:
+88 |               machine.disable()
+89 |       elif new_conf:  # [confusing-consecutive-elif]
+   |  _____^
+90 | |         machine.enable(new_conf.value)
+   | |______________________________________^ PLR5601
+   |