diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/collapsible_else_if.py b/crates/ruff_linter/resources/test/fixtures/pylint/collapsible_else_if.py index fb00f5d329a6e..7848b2c05725d 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/collapsible_else_if.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/collapsible_else_if.py @@ -49,6 +49,17 @@ def not_ok1(): pass +def not_ok1_with_comments(): + if 1: + pass + else: + # inner comment + if 2: + pass + else: + pass # final pass comment + + # Regression test for https://github.com/apache/airflow/blob/f1e1cdcc3b2826e68ba133f350300b5065bbca33/airflow/models/dag.py#L1737 def not_ok2(): if True: @@ -61,3 +72,28 @@ def not_ok2(): else: print(4) + +def not_ok3(): + if 1: + pass + else: + if 2: pass + else: pass + + +def not_ok4(): + if 1: + pass + else: + if 2: pass + else: + pass + + +def not_ok5(): + if 1: + pass + else: + if 2: + pass + else: pass diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index c31f147997663..67b8f209b3d8f 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1069,13 +1069,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { ruff::rules::sort_dunder_all_aug_assign(checker, aug_assign); } } - Stmt::If( - if_ @ ast::StmtIf { - test, - elif_else_clauses, - .. - }, - ) => { + Stmt::If(if_ @ ast::StmtIf { test, .. }) => { if checker.enabled(Rule::EmptyTypeCheckingBlock) { if typing::is_type_checking_block(if_, &checker.semantic) { flake8_type_checking::rules::empty_type_checking_block(checker, if_); @@ -1117,9 +1111,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { pyupgrade::rules::outdated_version_block(checker, if_); } if checker.enabled(Rule::CollapsibleElseIf) { - if let Some(diagnostic) = pylint::rules::collapsible_else_if(elif_else_clauses) { - checker.diagnostics.push(diagnostic); - } + pylint::rules::collapsible_else_if(checker, stmt); } if checker.enabled(Rule::CheckAndRemoveFromSet) { refurb::rules::check_and_remove_from_set(checker, if_); diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index bf5bce4a353e3..5786e857f96b9 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -193,6 +193,7 @@ mod tests { } #[test_case(Rule::UselessElseOnLoop, Path::new("useless_else_on_loop.py"))] + #[test_case(Rule::CollapsibleElseIf, Path::new("collapsible_else_if.py"))] fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!( "preview__{}_{}", diff --git a/crates/ruff_linter/src/rules/pylint/rules/collapsible_else_if.rs b/crates/ruff_linter/src/rules/pylint/rules/collapsible_else_if.rs index 3cee804326cef..675e24351bfbb 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/collapsible_else_if.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/collapsible_else_if.rs @@ -1,8 +1,15 @@ -use ruff_python_ast::{ElifElseClause, Stmt}; -use ruff_text_size::{Ranged, TextRange}; +use anyhow::Result; -use ruff_diagnostics::{Diagnostic, Violation}; +use ast::whitespace::indentation; +use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{self as ast, ElifElseClause, Stmt}; +use ruff_python_codegen::Stylist; +use ruff_source_file::Locator; +use ruff_text_size::{Ranged, TextRange}; + +use crate::checkers::ast::Checker; +use crate::rules::pyupgrade::fixes::adjust_indentation; /// ## What it does /// Checks for `else` blocks that consist of a single `if` statement. @@ -40,27 +47,85 @@ use ruff_macros::{derive_message_formats, violation}; pub struct CollapsibleElseIf; impl Violation for CollapsibleElseIf { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + #[derive_message_formats] fn message(&self) -> String { format!("Use `elif` instead of `else` then `if`, to reduce indentation") } + + fn fix_title(&self) -> Option { + Some("Convert to `elif`".to_string()) + } } /// PLR5501 -pub(crate) fn collapsible_else_if(elif_else_clauses: &[ElifElseClause]) -> Option { - let Some(ElifElseClause { - body, - test: None, - range, - }) = elif_else_clauses.last() +pub(crate) fn collapsible_else_if(checker: &mut Checker, stmt: &Stmt) { + let Stmt::If(ast::StmtIf { + elif_else_clauses, .. + }) = stmt else { - return None; + return; }; - if let [first @ Stmt::If(_)] = body.as_slice() { - return Some(Diagnostic::new( - CollapsibleElseIf, - TextRange::new(range.start(), first.start()), - )); + + let Some( + else_clause @ ElifElseClause { + body, test: None, .. + }, + ) = elif_else_clauses.last() + else { + return; + }; + let [first @ Stmt::If(ast::StmtIf { .. })] = body.as_slice() else { + return; + }; + + let mut diagnostic = Diagnostic::new( + CollapsibleElseIf, + TextRange::new(else_clause.start(), first.start()), + ); + + if checker.settings.preview.is_enabled() { + diagnostic.try_set_fix(|| { + convert_to_elif(first, else_clause, checker.locator(), checker.stylist()) + }); } - None + + checker.diagnostics.push(diagnostic); +} + +/// Generate [`Fix`] to convert an `else` block to an `elif` block. +fn convert_to_elif( + first: &Stmt, + else_clause: &ElifElseClause, + locator: &Locator, + stylist: &Stylist, +) -> Result { + let inner_if_line_start = locator.line_start(first.start()); + let inner_if_line_end = locator.line_end(first.end()); + + // Identify the indentation of the loop itself (e.g., the `while` or `for`). + let Some(indentation) = indentation(locator, else_clause) else { + return Err(anyhow::anyhow!("`else` is expected to be on its own line")); + }; + + // Dedent the content from the end of the `else` to the end of the `if`. + let indented = adjust_indentation( + TextRange::new(inner_if_line_start, inner_if_line_end), + indentation, + locator, + stylist, + )?; + + // Strip the indent from the first line of the `if` statement, and add `el` to the start. + let Some(unindented) = indented.strip_prefix(indentation) else { + return Err(anyhow::anyhow!("indented block to start with indentation")); + }; + let indented = format!("{indentation}el{unindented}"); + + Ok(Fix::safe_edit(Edit::replacement( + indented, + locator.line_start(else_clause.start()), + inner_if_line_end, + ))) } diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR5501_collapsible_else_if.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR5501_collapsible_else_if.py.snap index 0d369bcb0492f..b936ba56a2771 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR5501_collapsible_else_if.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR5501_collapsible_else_if.py.snap @@ -11,6 +11,7 @@ collapsible_else_if.py:37:5: PLR5501 Use `elif` instead of `else` then `if`, to | |________^ PLR5501 39 | pass | + = help: Convert to `elif` collapsible_else_if.py:45:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation | @@ -23,17 +24,71 @@ collapsible_else_if.py:45:5: PLR5501 Use `elif` instead of `else` then `if`, to 47 | pass 48 | else: | + = help: Convert to `elif` -collapsible_else_if.py:58:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation +collapsible_else_if.py:55:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation | -56 | elif True: -57 | print(2) -58 | else: +53 | if 1: +54 | pass +55 | else: | _____^ -59 | | if True: +56 | | # inner comment +57 | | if 2: | |________^ PLR5501 -60 | print(3) -61 | else: +58 | pass +59 | else: | + = help: Convert to `elif` + +collapsible_else_if.py:69:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation + | +67 | elif True: +68 | print(2) +69 | else: + | _____^ +70 | | if True: + | |________^ PLR5501 +71 | print(3) +72 | else: + | + = help: Convert to `elif` + +collapsible_else_if.py:79:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation + | +77 | if 1: +78 | pass +79 | else: + | _____^ +80 | | if 2: pass + | |________^ PLR5501 +81 | else: pass + | + = help: Convert to `elif` + +collapsible_else_if.py:87:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation + | +85 | if 1: +86 | pass +87 | else: + | _____^ +88 | | if 2: pass + | |________^ PLR5501 +89 | else: +90 | pass + | + = help: Convert to `elif` + +collapsible_else_if.py:96:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation + | +94 | if 1: +95 | pass +96 | else: + | _____^ +97 | | if 2: + | |________^ PLR5501 +98 | pass +99 | else: pass + | + = help: Convert to `elif` diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__preview__PLR5501_collapsible_else_if.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__preview__PLR5501_collapsible_else_if.py.snap new file mode 100644 index 0000000000000..4d13e11126c9a --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__preview__PLR5501_collapsible_else_if.py.snap @@ -0,0 +1,195 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +collapsible_else_if.py:37:5: PLR5501 [*] Use `elif` instead of `else` then `if`, to reduce indentation + | +35 | if 1: +36 | pass +37 | else: + | _____^ +38 | | if 2: + | |________^ PLR5501 +39 | pass + | + = help: Convert to `elif` + +ℹ Safe fix +34 34 | def not_ok0(): +35 35 | if 1: +36 36 | pass +37 |- else: +38 |- if 2: +39 |- pass + 37 |+ elif 2: + 38 |+ pass +40 39 | +41 40 | +42 41 | def not_ok1(): + +collapsible_else_if.py:45:5: PLR5501 [*] Use `elif` instead of `else` then `if`, to reduce indentation + | +43 | if 1: +44 | pass +45 | else: + | _____^ +46 | | if 2: + | |________^ PLR5501 +47 | pass +48 | else: + | + = help: Convert to `elif` + +ℹ Safe fix +42 42 | def not_ok1(): +43 43 | if 1: +44 44 | pass + 45 |+ elif 2: + 46 |+ pass +45 47 | else: +46 |- if 2: +47 |- pass +48 |- else: +49 |- pass + 48 |+ pass +50 49 | +51 50 | +52 51 | def not_ok1_with_comments(): + +collapsible_else_if.py:55:5: PLR5501 [*] Use `elif` instead of `else` then `if`, to reduce indentation + | +53 | if 1: +54 | pass +55 | else: + | _____^ +56 | | # inner comment +57 | | if 2: + | |________^ PLR5501 +58 | pass +59 | else: + | + = help: Convert to `elif` + +ℹ Safe fix +52 52 | def not_ok1_with_comments(): +53 53 | if 1: +54 54 | pass + 55 |+ elif 2: + 56 |+ pass +55 57 | else: +56 |- # inner comment +57 |- if 2: +58 |- pass +59 |- else: +60 |- pass # final pass comment + 58 |+ pass # final pass comment +61 59 | +62 60 | +63 61 | # Regression test for https://github.com/apache/airflow/blob/f1e1cdcc3b2826e68ba133f350300b5065bbca33/airflow/models/dag.py#L1737 + +collapsible_else_if.py:69:5: PLR5501 [*] Use `elif` instead of `else` then `if`, to reduce indentation + | +67 | elif True: +68 | print(2) +69 | else: + | _____^ +70 | | if True: + | |________^ PLR5501 +71 | print(3) +72 | else: + | + = help: Convert to `elif` + +ℹ Safe fix +66 66 | print(1) +67 67 | elif True: +68 68 | print(2) + 69 |+ elif True: + 70 |+ print(3) +69 71 | else: +70 |- if True: +71 |- print(3) +72 |- else: +73 |- print(4) + 72 |+ print(4) +74 73 | +75 74 | +76 75 | def not_ok3(): + +collapsible_else_if.py:79:5: PLR5501 [*] Use `elif` instead of `else` then `if`, to reduce indentation + | +77 | if 1: +78 | pass +79 | else: + | _____^ +80 | | if 2: pass + | |________^ PLR5501 +81 | else: pass + | + = help: Convert to `elif` + +ℹ Safe fix +76 76 | def not_ok3(): +77 77 | if 1: +78 78 | pass +79 |- else: +80 |- if 2: pass +81 |- else: pass + 79 |+ elif 2: pass + 80 |+ else: pass +82 81 | +83 82 | +84 83 | def not_ok4(): + +collapsible_else_if.py:87:5: PLR5501 [*] Use `elif` instead of `else` then `if`, to reduce indentation + | +85 | if 1: +86 | pass +87 | else: + | _____^ +88 | | if 2: pass + | |________^ PLR5501 +89 | else: +90 | pass + | + = help: Convert to `elif` + +ℹ Safe fix +84 84 | def not_ok4(): +85 85 | if 1: +86 86 | pass + 87 |+ elif 2: pass +87 88 | else: +88 |- if 2: pass +89 |- else: +90 |- pass + 89 |+ pass +91 90 | +92 91 | +93 92 | def not_ok5(): + +collapsible_else_if.py:96:5: PLR5501 [*] Use `elif` instead of `else` then `if`, to reduce indentation + | +94 | if 1: +95 | pass +96 | else: + | _____^ +97 | | if 2: + | |________^ PLR5501 +98 | pass +99 | else: pass + | + = help: Convert to `elif` + +ℹ Safe fix +93 93 | def not_ok5(): +94 94 | if 1: +95 95 | pass +96 |- else: +97 |- if 2: +98 |- pass +99 |- else: pass + 96 |+ elif 2: + 97 |+ pass + 98 |+ else: pass + +