From dbfd7d3a463e6d5ed6aef04a61848f17e8cdeac0 Mon Sep 17 00:00:00 2001 From: Steve C Date: Sat, 20 Jan 2024 17:25:12 -0500 Subject: [PATCH 1/6] [`pylint`] - add fix for `PLR5501` --- .../src/checkers/ast/analyze/statement.rs | 12 +-- crates/ruff_linter/src/rules/pylint/mod.rs | 1 + .../rules/pylint/rules/collapsible_else_if.rs | 82 +++++++++++++++---- 3 files changed, 70 insertions(+), 25 deletions(-) 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..fa5f3596ea4dc 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,9 +1,14 @@ -use ruff_python_ast::{ElifElseClause, Stmt}; +use ast::whitespace::indentation; +use ruff_python_ast::{self as ast, ElifElseClause, Stmt}; +use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer}; use ruff_text_size::{Ranged, TextRange}; -use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; +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 +45,74 @@ 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("Use `elif` instead of `else` then `if`, to reduce indentation".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; + }; + + let Some( + else_clause @ ElifElseClause { + body, + test: None, + range, + }, + ) = elif_else_clauses.last() else { - return None; + return; }; - if let [first @ Stmt::If(_)] = body.as_slice() { - return Some(Diagnostic::new( - CollapsibleElseIf, - TextRange::new(range.start(), first.start()), - )); + let [first @ Stmt::If(ast::StmtIf { .. })] = body.as_slice() else { + return; + }; + + let mut diagnostic = Diagnostic::new( + CollapsibleElseIf, + TextRange::new(range.start(), first.start()), + ); + + if checker.settings.preview.is_enabled() { + let inner_if_line_start = checker.locator().line_start(first.start()); + + let desired_indentation = indentation(checker.locator(), else_clause).unwrap_or(""); + + let indented = adjust_indentation( + TextRange::new(inner_if_line_start, first.end()), + desired_indentation, + checker.locator(), + checker.stylist(), + ) + .unwrap(); + + let fixed_indented = format!("el{}", indented.strip_prefix(desired_indentation).unwrap()); + + let else_colon = + SimpleTokenizer::starts_at(else_clause.start(), checker.locator().contents()) + .find(|token| token.kind() == SimpleTokenKind::Colon) + .unwrap(); + + diagnostic.set_fix(Fix::applicable_edits( + Edit::deletion(inner_if_line_start, first.end()), + [Edit::range_replacement( + fixed_indented, + TextRange::new(else_clause.start(), else_colon.end()), + )], + Applicability::Safe, + )) } - None + + checker.diagnostics.push(diagnostic); } From c99bfb5bd258ea55ec99dbbbdbc379e90205b619 Mon Sep 17 00:00:00 2001 From: Steve C Date: Sat, 20 Jan 2024 17:43:21 -0500 Subject: [PATCH 2/6] tweaks + snapshots --- .../fixtures/pylint/collapsible_else_if.py | 10 ++ .../rules/pylint/rules/collapsible_else_if.rs | 8 +- ...tests__PLR5501_collapsible_else_if.py.snap | 30 +++-- ...eview__PLR5501_collapsible_else_if.py.snap | 115 ++++++++++++++++++ 4 files changed, 153 insertions(+), 10 deletions(-) create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__preview__PLR5501_collapsible_else_if.py.snap 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..6850079bfc5cd 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,16 @@ def not_ok1(): pass +def not_ok_with_comments(): + if 1: + pass + else: # else 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: 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 fa5f3596ea4dc..96ab96535c7aa 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 @@ -86,11 +86,13 @@ pub(crate) fn collapsible_else_if(checker: &mut Checker, stmt: &Stmt) { if checker.settings.preview.is_enabled() { let inner_if_line_start = checker.locator().line_start(first.start()); + let inner_if_line_end = checker.locator().line_end(first.end()); + let inner_if_full_line_end = checker.locator().full_line_end(first.end()); let desired_indentation = indentation(checker.locator(), else_clause).unwrap_or(""); let indented = adjust_indentation( - TextRange::new(inner_if_line_start, first.end()), + TextRange::new(inner_if_line_start, inner_if_line_end), desired_indentation, checker.locator(), checker.stylist(), @@ -105,13 +107,13 @@ pub(crate) fn collapsible_else_if(checker: &mut Checker, stmt: &Stmt) { .unwrap(); diagnostic.set_fix(Fix::applicable_edits( - Edit::deletion(inner_if_line_start, first.end()), + Edit::deletion(inner_if_line_start, inner_if_full_line_end), [Edit::range_replacement( fixed_indented, TextRange::new(else_clause.start(), else_colon.end()), )], Applicability::Safe, - )) + )); } checker.diagnostics.push(diagnostic); 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..afad628038aef 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: Use `elif` instead of `else` then `if`, to reduce indentation collapsible_else_if.py:45:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation | @@ -23,17 +24,32 @@ collapsible_else_if.py:45:5: PLR5501 Use `elif` instead of `else` then `if`, to 47 | pass 48 | else: | + = help: Use `elif` instead of `else` then `if`, to reduce indentation -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: # else comment | _____^ -59 | | if True: +56 | | if 2: | |________^ PLR5501 -60 | print(3) -61 | else: +57 | pass +58 | else: | + = help: Use `elif` instead of `else` then `if`, to reduce indentation + +collapsible_else_if.py:68:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation + | +66 | elif True: +67 | print(2) +68 | else: + | _____^ +69 | | if True: + | |________^ PLR5501 +70 | print(3) +71 | else: + | + = help: Use `elif` instead of `else` then `if`, to reduce indentation 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..35e5b27759c8d --- /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,115 @@ +--- +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: Use `elif` instead of `else` then `if`, to reduce indentation + +ℹ 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: Use `elif` instead of `else` then `if`, to reduce indentation + +ℹ 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_ok_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: # else comment + | _____^ +56 | | if 2: + | |________^ PLR5501 +57 | pass +58 | else: + | + = help: Use `elif` instead of `else` then `if`, to reduce indentation + +ℹ Safe fix +52 52 | def not_ok_with_comments(): +53 53 | if 1: +54 54 | pass +55 |- else: # else comment +56 |- if 2: +57 |- pass +58 |- else: +59 |- pass # final pass comment + 55 |+ elif 2: + 56 |+ pass + 57 |+ else: + 58 |+ pass # final pass comment # else comment +60 59 | +61 60 | +62 61 | # Regression test for https://github.com/apache/airflow/blob/f1e1cdcc3b2826e68ba133f350300b5065bbca33/airflow/models/dag.py#L1737 + +collapsible_else_if.py:68:5: PLR5501 [*] Use `elif` instead of `else` then `if`, to reduce indentation + | +66 | elif True: +67 | print(2) +68 | else: + | _____^ +69 | | if True: + | |________^ PLR5501 +70 | print(3) +71 | else: + | + = help: Use `elif` instead of `else` then `if`, to reduce indentation + +ℹ Safe fix +65 65 | print(1) +66 66 | elif True: +67 67 | print(2) + 68 |+ elif True: + 69 |+ print(3) +68 70 | else: +69 |- if True: +70 |- print(3) +71 |- else: +72 |- print(4) + 71 |+ print(4) +73 72 | + + From f96365eaa89242381b57db98d18b1ccf35cb7cf5 Mon Sep 17 00:00:00 2001 From: Steve C Date: Sun, 21 Jan 2024 13:23:23 -0500 Subject: [PATCH 3/6] don't retain comments on the `else` line! --- .../rules/pylint/rules/collapsible_else_if.rs | 19 +++++++------------ ...eview__PLR5501_collapsible_else_if.py.snap | 2 +- 2 files changed, 8 insertions(+), 13 deletions(-) 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 96ab96535c7aa..2b993b61dbebb 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,6 +1,5 @@ use ast::whitespace::indentation; use ruff_python_ast::{self as ast, ElifElseClause, Stmt}; -use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer}; use ruff_text_size::{Ranged, TextRange}; use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, Violation}; @@ -87,10 +86,11 @@ pub(crate) fn collapsible_else_if(checker: &mut Checker, stmt: &Stmt) { if checker.settings.preview.is_enabled() { let inner_if_line_start = checker.locator().line_start(first.start()); let inner_if_line_end = checker.locator().line_end(first.end()); - let inner_if_full_line_end = checker.locator().full_line_end(first.end()); + // Identify the indentation of the loop itself (e.g., the `while` or `for`). let desired_indentation = indentation(checker.locator(), else_clause).unwrap_or(""); + // 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), desired_indentation, @@ -99,19 +99,14 @@ pub(crate) fn collapsible_else_if(checker: &mut Checker, stmt: &Stmt) { ) .unwrap(); + // Unindent the first line (which is the `if` and add `el` to the start) let fixed_indented = format!("el{}", indented.strip_prefix(desired_indentation).unwrap()); - let else_colon = - SimpleTokenizer::starts_at(else_clause.start(), checker.locator().contents()) - .find(|token| token.kind() == SimpleTokenKind::Colon) - .unwrap(); - - diagnostic.set_fix(Fix::applicable_edits( - Edit::deletion(inner_if_line_start, inner_if_full_line_end), - [Edit::range_replacement( + diagnostic.set_fix(Fix::applicable_edit( + Edit::range_replacement( fixed_indented, - TextRange::new(else_clause.start(), else_colon.end()), - )], + TextRange::new(else_clause.start(), inner_if_line_end), + ), Applicability::Safe, )); } 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 index 35e5b27759c8d..c3fa401c86141 100644 --- 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 @@ -80,7 +80,7 @@ collapsible_else_if.py:55:5: PLR5501 [*] Use `elif` instead of `else` then `if`, 55 |+ elif 2: 56 |+ pass 57 |+ else: - 58 |+ pass # final pass comment # else comment + 58 |+ pass # final pass comment 60 59 | 61 60 | 62 61 | # Regression test for https://github.com/apache/airflow/blob/f1e1cdcc3b2826e68ba133f350300b5065bbca33/airflow/models/dag.py#L1737 From fda5394a39830cfb78b412b561179be47bb68b10 Mon Sep 17 00:00:00 2001 From: Steve C Date: Sun, 21 Jan 2024 13:30:45 -0500 Subject: [PATCH 4/6] add inner comment --- .../fixtures/pylint/collapsible_else_if.py | 3 +- ...tests__PLR5501_collapsible_else_if.py.snap | 23 +++---- ...eview__PLR5501_collapsible_else_if.py.snap | 65 ++++++++++--------- 3 files changed, 47 insertions(+), 44 deletions(-) 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 6850079bfc5cd..80e405a4a260f 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 @@ -52,7 +52,8 @@ def not_ok1(): def not_ok_with_comments(): if 1: pass - else: # else comment + else: + # inner comment if 2: pass else: 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 afad628038aef..64592f60b8402 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 @@ -30,25 +30,26 @@ collapsible_else_if.py:55:5: PLR5501 Use `elif` instead of `else` then `if`, to | 53 | if 1: 54 | pass -55 | else: # else comment +55 | else: | _____^ -56 | | if 2: +56 | | # inner comment +57 | | if 2: | |________^ PLR5501 -57 | pass -58 | else: +58 | pass +59 | else: | = help: Use `elif` instead of `else` then `if`, to reduce indentation -collapsible_else_if.py:68:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation +collapsible_else_if.py:69:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation | -66 | elif True: -67 | print(2) -68 | else: +67 | elif True: +68 | print(2) +69 | else: | _____^ -69 | | if True: +70 | | if True: | |________^ PLR5501 -70 | print(3) -71 | else: +71 | print(3) +72 | else: | = help: Use `elif` instead of `else` then `if`, to reduce indentation 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 index c3fa401c86141..7a4ef9f3d4cb5 100644 --- 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 @@ -59,12 +59,13 @@ collapsible_else_if.py:55:5: PLR5501 [*] Use `elif` instead of `else` then `if`, | 53 | if 1: 54 | pass -55 | else: # else comment +55 | else: | _____^ -56 | | if 2: +56 | | # inner comment +57 | | if 2: | |________^ PLR5501 -57 | pass -58 | else: +58 | pass +59 | else: | = help: Use `elif` instead of `else` then `if`, to reduce indentation @@ -72,44 +73,44 @@ collapsible_else_if.py:55:5: PLR5501 [*] Use `elif` instead of `else` then `if`, 52 52 | def not_ok_with_comments(): 53 53 | if 1: 54 54 | pass -55 |- else: # else comment -56 |- if 2: -57 |- pass -58 |- else: -59 |- pass # final pass comment 55 |+ elif 2: 56 |+ pass - 57 |+ else: +55 57 | else: +56 |- # inner comment +57 |- if 2: +58 |- pass +59 |- else: +60 |- pass # final pass comment 58 |+ pass # final pass comment -60 59 | -61 60 | -62 61 | # Regression test for https://github.com/apache/airflow/blob/f1e1cdcc3b2826e68ba133f350300b5065bbca33/airflow/models/dag.py#L1737 +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:68:5: PLR5501 [*] Use `elif` instead of `else` then `if`, to reduce indentation +collapsible_else_if.py:69:5: PLR5501 [*] Use `elif` instead of `else` then `if`, to reduce indentation | -66 | elif True: -67 | print(2) -68 | else: +67 | elif True: +68 | print(2) +69 | else: | _____^ -69 | | if True: +70 | | if True: | |________^ PLR5501 -70 | print(3) -71 | else: +71 | print(3) +72 | else: | = help: Use `elif` instead of `else` then `if`, to reduce indentation ℹ Safe fix -65 65 | print(1) -66 66 | elif True: -67 67 | print(2) - 68 |+ elif True: - 69 |+ print(3) -68 70 | else: -69 |- if True: -70 |- print(3) -71 |- else: -72 |- print(4) - 71 |+ print(4) -73 72 | +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 | From 1e42501153062e965a592f888c3a0d3e702858c0 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 21 Jan 2024 19:34:11 -0500 Subject: [PATCH 5/6] Add more tests --- .../fixtures/pylint/collapsible_else_if.py | 27 +++++- .../rules/pylint/rules/collapsible_else_if.rs | 64 ++++++++------ ...tests__PLR5501_collapsible_else_if.py.snap | 38 +++++++++ ...eview__PLR5501_collapsible_else_if.py.snap | 83 ++++++++++++++++++- 4 files changed, 182 insertions(+), 30 deletions(-) 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 80e405a4a260f..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,7 +49,7 @@ def not_ok1(): pass -def not_ok_with_comments(): +def not_ok1_with_comments(): if 1: pass else: @@ -72,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/rules/pylint/rules/collapsible_else_if.rs b/crates/ruff_linter/src/rules/pylint/rules/collapsible_else_if.rs index 2b993b61dbebb..8b2d86a769ab1 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 @@ -2,8 +2,11 @@ use ast::whitespace::indentation; use ruff_python_ast::{self as ast, ElifElseClause, Stmt}; use ruff_text_size::{Ranged, TextRange}; +use anyhow::Result; use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_codegen::Stylist; +use ruff_source_file::Locator; use crate::checkers::ast::Checker; use crate::rules::pyupgrade::fixes::adjust_indentation; @@ -66,9 +69,7 @@ pub(crate) fn collapsible_else_if(checker: &mut Checker, stmt: &Stmt) { let Some( else_clause @ ElifElseClause { - body, - test: None, - range, + body, test: None, .. }, ) = elif_else_clauses.last() else { @@ -80,36 +81,45 @@ pub(crate) fn collapsible_else_if(checker: &mut Checker, stmt: &Stmt) { let mut diagnostic = Diagnostic::new( CollapsibleElseIf, - TextRange::new(range.start(), first.start()), + TextRange::new(else_clause.start(), first.start()), ); if checker.settings.preview.is_enabled() { - let inner_if_line_start = checker.locator().line_start(first.start()); - let inner_if_line_end = checker.locator().line_end(first.end()); + diagnostic.try_set_fix(|| do_fix(first, else_clause, checker.locator(), checker.stylist())); + } + + checker.diagnostics.push(diagnostic); +} - // Identify the indentation of the loop itself (e.g., the `while` or `for`). - let desired_indentation = indentation(checker.locator(), else_clause).unwrap_or(""); +fn do_fix( + 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()); - // 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), - desired_indentation, - checker.locator(), - checker.stylist(), - ) - .unwrap(); + // Identify the indentation of the loop itself (e.g., the `while` or `for`). + let desired_indentation = indentation(locator, else_clause).unwrap_or(""); - // Unindent the first line (which is the `if` and add `el` to the start) - let fixed_indented = format!("el{}", indented.strip_prefix(desired_indentation).unwrap()); + // 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), + desired_indentation, + locator, + stylist, + ) + .unwrap(); - diagnostic.set_fix(Fix::applicable_edit( - Edit::range_replacement( - fixed_indented, - TextRange::new(else_clause.start(), inner_if_line_end), - ), - Applicability::Safe, - )); - } + // Unindent the first line (which is the `if` and add `el` to the start) + let fixed_indented = format!("el{}", indented.strip_prefix(desired_indentation).unwrap()); - checker.diagnostics.push(diagnostic); + Ok(Fix::applicable_edit( + Edit::range_replacement( + fixed_indented, + TextRange::new(else_clause.start(), inner_if_line_end), + ), + Applicability::Safe, + )) } 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 64592f60b8402..a00a165f5f98e 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 @@ -53,4 +53,42 @@ collapsible_else_if.py:69:5: PLR5501 Use `elif` instead of `else` then `if`, to | = help: Use `elif` instead of `else` then `if`, to reduce indentation +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: Use `elif` instead of `else` then `if`, to reduce indentation + +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: Use `elif` instead of `else` then `if`, to reduce indentation + +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: Use `elif` instead of `else` then `if`, to reduce indentation + 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 index 7a4ef9f3d4cb5..3710346f75b5e 100644 --- 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 @@ -53,7 +53,7 @@ collapsible_else_if.py:45:5: PLR5501 [*] Use `elif` instead of `else` then `if`, 48 |+ pass 50 49 | 51 50 | -52 51 | def not_ok_with_comments(): +52 51 | def not_ok1_with_comments(): collapsible_else_if.py:55:5: PLR5501 [*] Use `elif` instead of `else` then `if`, to reduce indentation | @@ -70,7 +70,7 @@ collapsible_else_if.py:55:5: PLR5501 [*] Use `elif` instead of `else` then `if`, = help: Use `elif` instead of `else` then `if`, to reduce indentation ℹ Safe fix -52 52 | def not_ok_with_comments(): +52 52 | def not_ok1_with_comments(): 53 53 | if 1: 54 54 | pass 55 |+ elif 2: @@ -112,5 +112,84 @@ collapsible_else_if.py:69:5: PLR5501 [*] Use `elif` instead of `else` then `if`, 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: Use `elif` instead of `else` then `if`, to reduce indentation + +ℹ 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: Use `elif` instead of `else` then `if`, to reduce indentation + +ℹ 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: Use `elif` instead of `else` then `if`, to reduce indentation + +ℹ 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 From 34c274d6140c6357342e88a1c4e6183c5b45d7bf Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 21 Jan 2024 19:40:39 -0500 Subject: [PATCH 6/6] Separate method --- .../rules/pylint/rules/collapsible_else_if.rs | 52 +++++++++++-------- ...tests__PLR5501_collapsible_else_if.py.snap | 14 ++--- ...eview__PLR5501_collapsible_else_if.py.snap | 14 ++--- 3 files changed, 43 insertions(+), 37 deletions(-) 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 8b2d86a769ab1..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,12 +1,12 @@ -use ast::whitespace::indentation; -use ruff_python_ast::{self as ast, ElifElseClause, Stmt}; -use ruff_text_size::{Ranged, TextRange}; - use anyhow::Result; -use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, 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; @@ -48,13 +48,14 @@ 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("Use `elif` instead of `else` then `if`, to reduce indentation".to_string()) + Some("Convert to `elif`".to_string()) } } @@ -85,13 +86,16 @@ pub(crate) fn collapsible_else_if(checker: &mut Checker, stmt: &Stmt) { ); if checker.settings.preview.is_enabled() { - diagnostic.try_set_fix(|| do_fix(first, else_clause, checker.locator(), checker.stylist())); + diagnostic.try_set_fix(|| { + convert_to_elif(first, else_clause, checker.locator(), checker.stylist()) + }); } checker.diagnostics.push(diagnostic); } -fn do_fix( +/// Generate [`Fix`] to convert an `else` block to an `elif` block. +fn convert_to_elif( first: &Stmt, else_clause: &ElifElseClause, locator: &Locator, @@ -101,25 +105,27 @@ fn do_fix( let inner_if_line_end = locator.line_end(first.end()); // Identify the indentation of the loop itself (e.g., the `while` or `for`). - let desired_indentation = indentation(locator, else_clause).unwrap_or(""); + 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), - desired_indentation, + indentation, locator, stylist, - ) - .unwrap(); - - // Unindent the first line (which is the `if` and add `el` to the start) - let fixed_indented = format!("el{}", indented.strip_prefix(desired_indentation).unwrap()); - - Ok(Fix::applicable_edit( - Edit::range_replacement( - fixed_indented, - TextRange::new(else_clause.start(), inner_if_line_end), - ), - Applicability::Safe, - )) + )?; + + // 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 a00a165f5f98e..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,7 +11,7 @@ collapsible_else_if.py:37:5: PLR5501 Use `elif` instead of `else` then `if`, to | |________^ PLR5501 39 | pass | - = help: Use `elif` instead of `else` then `if`, to reduce indentation + = help: Convert to `elif` collapsible_else_if.py:45:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation | @@ -24,7 +24,7 @@ collapsible_else_if.py:45:5: PLR5501 Use `elif` instead of `else` then `if`, to 47 | pass 48 | else: | - = help: Use `elif` instead of `else` then `if`, to reduce indentation + = help: Convert to `elif` collapsible_else_if.py:55:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation | @@ -38,7 +38,7 @@ collapsible_else_if.py:55:5: PLR5501 Use `elif` instead of `else` then `if`, to 58 | pass 59 | else: | - = help: Use `elif` instead of `else` then `if`, to reduce indentation + = help: Convert to `elif` collapsible_else_if.py:69:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation | @@ -51,7 +51,7 @@ collapsible_else_if.py:69:5: PLR5501 Use `elif` instead of `else` then `if`, to 71 | print(3) 72 | else: | - = help: Use `elif` instead of `else` then `if`, to reduce indentation + = help: Convert to `elif` collapsible_else_if.py:79:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation | @@ -63,7 +63,7 @@ collapsible_else_if.py:79:5: PLR5501 Use `elif` instead of `else` then `if`, to | |________^ PLR5501 81 | else: pass | - = help: Use `elif` instead of `else` then `if`, to reduce indentation + = help: Convert to `elif` collapsible_else_if.py:87:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation | @@ -76,7 +76,7 @@ collapsible_else_if.py:87:5: PLR5501 Use `elif` instead of `else` then `if`, to 89 | else: 90 | pass | - = help: Use `elif` instead of `else` then `if`, to reduce indentation + = help: Convert to `elif` collapsible_else_if.py:96:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation | @@ -89,6 +89,6 @@ collapsible_else_if.py:96:5: PLR5501 Use `elif` instead of `else` then `if`, to 98 | pass 99 | else: pass | - = help: Use `elif` instead of `else` then `if`, to reduce indentation + = 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 index 3710346f75b5e..4d13e11126c9a 100644 --- 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 @@ -11,7 +11,7 @@ collapsible_else_if.py:37:5: PLR5501 [*] Use `elif` instead of `else` then `if`, | |________^ PLR5501 39 | pass | - = help: Use `elif` instead of `else` then `if`, to reduce indentation + = help: Convert to `elif` ℹ Safe fix 34 34 | def not_ok0(): @@ -37,7 +37,7 @@ collapsible_else_if.py:45:5: PLR5501 [*] Use `elif` instead of `else` then `if`, 47 | pass 48 | else: | - = help: Use `elif` instead of `else` then `if`, to reduce indentation + = help: Convert to `elif` ℹ Safe fix 42 42 | def not_ok1(): @@ -67,7 +67,7 @@ collapsible_else_if.py:55:5: PLR5501 [*] Use `elif` instead of `else` then `if`, 58 | pass 59 | else: | - = help: Use `elif` instead of `else` then `if`, to reduce indentation + = help: Convert to `elif` ℹ Safe fix 52 52 | def not_ok1_with_comments(): @@ -97,7 +97,7 @@ collapsible_else_if.py:69:5: PLR5501 [*] Use `elif` instead of `else` then `if`, 71 | print(3) 72 | else: | - = help: Use `elif` instead of `else` then `if`, to reduce indentation + = help: Convert to `elif` ℹ Safe fix 66 66 | print(1) @@ -125,7 +125,7 @@ collapsible_else_if.py:79:5: PLR5501 [*] Use `elif` instead of `else` then `if`, | |________^ PLR5501 81 | else: pass | - = help: Use `elif` instead of `else` then `if`, to reduce indentation + = help: Convert to `elif` ℹ Safe fix 76 76 | def not_ok3(): @@ -151,7 +151,7 @@ collapsible_else_if.py:87:5: PLR5501 [*] Use `elif` instead of `else` then `if`, 89 | else: 90 | pass | - = help: Use `elif` instead of `else` then `if`, to reduce indentation + = help: Convert to `elif` ℹ Safe fix 84 84 | def not_ok4(): @@ -178,7 +178,7 @@ collapsible_else_if.py:96:5: PLR5501 [*] Use `elif` instead of `else` then `if`, 98 | pass 99 | else: pass | - = help: Use `elif` instead of `else` then `if`, to reduce indentation + = help: Convert to `elif` ℹ Safe fix 93 93 | def not_ok5():