diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/confusing_consecutive_elif.py b/crates/ruff_linter/resources/test/fixtures/pylint/confusing_consecutive_elif.py new file mode 100644 index 0000000000000..63a8f04fb289e --- /dev/null +++ b/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) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index d8147fe2db1f1..78544f95c6e83 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/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_); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 5cf0293d1292e..626bcc7c5556f 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/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), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index e9599b963aa70..c1332ee6652bb 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/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( diff --git a/crates/ruff_linter/src/rules/pylint/rules/confusing_consecutive_elif.rs b/crates/ruff_linter/src/rules/pylint/rules/confusing_consecutive_elif.rs new file mode 100644 index 0000000000000..2cbfdac548b06 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/confusing_consecutive_elif.rs @@ -0,0 +1,147 @@ +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. +/// Adding an explicit else, or 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 +/// # Option 1: add explicit else +/// 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: # [confusing-consecutive-elif] +/// machine.enable(new_conf.value) +/// +/// +/// # Option 2: extract function +/// 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!("Consecutive elif with differing indentation level, consider creating a function to separate the inner elif") + } +} + +/// 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) +} diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 9963339844a84..00e85e2387115 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/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; diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR5601_confusing_consecutive_elif.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR5601_confusing_consecutive_elif.py.snap new file mode 100644 index 0000000000000..57491135ea353 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR5601_confusing_consecutive_elif.py.snap @@ -0,0 +1,34 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +confusing_consecutive_elif.py:14:5: PLR5601 Consecutive elif with differing indentation level, consider creating a function to separate the inner elif + | +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 Consecutive elif with differing indentation level, consider creating a function to separate the inner elif + | +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 Consecutive elif with differing indentation level, consider creating a function to separate the inner elif + | +87 | if new_new_conf: +88 | machine.disable() +89 | elif new_conf: # [confusing-consecutive-elif] + | _____^ +90 | | machine.enable(new_conf.value) + | |______________________________________^ PLR5601 + | + + diff --git a/ruff.schema.json b/ruff.schema.json index ae81665c41985..c28fd947e91f7 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3294,6 +3294,9 @@ "PLR55", "PLR550", "PLR5501", + "PLR56", + "PLR560", + "PLR5601", "PLR6", "PLR62", "PLR620",