diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/useless_else_on_loop.py b/crates/ruff_linter/resources/test/fixtures/pylint/useless_else_on_loop.py index 5d2245e69ac7f..1eaf4fb82fb11 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/useless_else_on_loop.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/useless_else_on_loop.py @@ -135,3 +135,14 @@ def test_break_in_match(): else: return True return False + + +def test_retain_comment(): + """Retain the comment within the `else` block""" + for j in range(10): + pass + else: + # [useless-else-on-loop] + print("fat chance") + for j in range(10): + break diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 9182956716f34..394cee66bbd88 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -12,12 +12,13 @@ mod tests { use rustc_hash::FxHashSet; use test_case::test_case; - use crate::assert_messages; use crate::registry::Rule; use crate::rules::pylint; + use crate::settings::types::PreviewMode; use crate::settings::types::PythonVersion; use crate::settings::LinterSettings; use crate::test::test_path; + use crate::{assert_messages, settings}; #[test_case(Rule::AndOrTernary, Path::new("and_or_ternary.py"))] #[test_case(Rule::AssertOnStringLiteral, Path::new("assert_on_string_literal.py"))] @@ -190,6 +191,24 @@ mod tests { Ok(()) } + #[test_case(Rule::UselessElseOnLoop, Path::new("useless_else_on_loop.py"))] + fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> { + let snapshot = format!( + "preview__{}_{}", + rule_code.noqa_code(), + path.to_string_lossy() + ); + let diagnostics = test_path( + Path::new("pylint").join(path).as_path(), + &settings::LinterSettings { + preview: PreviewMode::Enabled, + ..settings::LinterSettings::for_rule(rule_code) + }, + )?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } + #[test] fn repeated_isinstance_calls() -> Result<()> { let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/pylint/rules/useless_else_on_loop.rs b/crates/ruff_linter/src/rules/pylint/rules/useless_else_on_loop.rs index ff354860c5cc3..24c9b9769b08b 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/useless_else_on_loop.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/useless_else_on_loop.rs @@ -1,10 +1,16 @@ -use ruff_python_ast::{self as ast, ExceptHandler, MatchCase, Stmt}; +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::identifier; +use ruff_python_ast::{self as ast, ExceptHandler, MatchCase, 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` clauses on loops without a `break` statement. @@ -42,15 +48,50 @@ use crate::checkers::ast::Checker; pub struct UselessElseOnLoop; impl Violation for UselessElseOnLoop { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; #[derive_message_formats] fn message(&self) -> String { format!( - "`else` clause on loop without a `break` statement; remove the `else` and de-indent all the \ - code inside it" + "`else` clause on loop without a `break` statement; remove the `else` and dedent its contents" ) } + + fn fix_title(&self) -> Option { + Some("Remove `else`".to_string()) + } } +/// PLW0120 +pub(crate) fn useless_else_on_loop( + checker: &mut Checker, + stmt: &Stmt, + body: &[Stmt], + orelse: &[Stmt], +) { + if orelse.is_empty() || loop_exits_early(body) { + return; + } + + let else_range = identifier::else_(stmt, checker.locator().contents()).expect("else clause"); + + let mut diagnostic = Diagnostic::new(UselessElseOnLoop, else_range); + + if checker.settings.preview.is_enabled() { + diagnostic.try_set_fix(|| { + remove_else( + stmt, + orelse, + else_range, + checker.locator(), + checker.stylist(), + ) + }); + } + + checker.diagnostics.push(diagnostic); +} + +/// Returns `true` if the given body contains a `break` statement. fn loop_exits_early(body: &[Stmt]) -> bool { body.iter().any(|stmt| match stmt { Stmt::If(ast::StmtIf { @@ -91,17 +132,50 @@ fn loop_exits_early(body: &[Stmt]) -> bool { }) } -/// PLW0120 -pub(crate) fn useless_else_on_loop( - checker: &mut Checker, +/// Generate a [`Fix`] to remove the `else` clause from the given statement. +fn remove_else( stmt: &Stmt, - body: &[Stmt], orelse: &[Stmt], -) { - if !orelse.is_empty() && !loop_exits_early(body) { - checker.diagnostics.push(Diagnostic::new( - UselessElseOnLoop, - identifier::else_(stmt, checker.locator().contents()).unwrap(), - )); + else_range: TextRange, + locator: &Locator, + stylist: &Stylist, +) -> Result { + let Some(start) = orelse.first() else { + return Err(anyhow::anyhow!("Empty `else` clause")); + }; + let Some(end) = orelse.last() else { + return Err(anyhow::anyhow!("Empty `else` clause")); + }; + + let start_indentation = indentation(locator, start); + if start_indentation.is_none() { + // Inline `else` block (e.g., `else: x = 1`). + Ok(Fix::safe_edit(Edit::deletion( + else_range.start(), + start.start(), + ))) + } else { + // Identify the indentation of the loop itself (e.g., the `while` or `for`). + let Some(desired_indentation) = indentation(locator, stmt) else { + return Err(anyhow::anyhow!("Compound statement cannot be inlined")); + }; + + // Dedent the content from the end of the `else` to the end of the loop. + let indented = adjust_indentation( + TextRange::new( + locator.full_line_end(else_range.start()), + locator.full_line_end(end.end()), + ), + desired_indentation, + locator, + stylist, + )?; + + // Replace the content from the start of the `else` to the end of the loop. + Ok(Fix::safe_edit(Edit::replacement( + indented, + locator.line_start(else_range.start()), + locator.full_line_end(end.end()), + ))) } } diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0120_useless_else_on_loop.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0120_useless_else_on_loop.py.snap index bed971a55b072..856658731629f 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0120_useless_else_on_loop.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0120_useless_else_on_loop.py.snap @@ -1,7 +1,7 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -useless_else_on_loop.py:9:5: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and de-indent all the code inside it +useless_else_on_loop.py:9:5: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and dedent its contents | 7 | if i % 2: 8 | return i @@ -10,8 +10,9 @@ useless_else_on_loop.py:9:5: PLW0120 `else` clause on loop without a `break` sta 10 | print("math is broken") 11 | return None | + = help: Remove `else` -useless_else_on_loop.py:18:5: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and de-indent all the code inside it +useless_else_on_loop.py:18:5: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and dedent its contents | 16 | while True: 17 | return 1 @@ -20,8 +21,9 @@ useless_else_on_loop.py:18:5: PLW0120 `else` clause on loop without a `break` st 19 | print("math is broken") 20 | return None | + = help: Remove `else` -useless_else_on_loop.py:30:1: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and de-indent all the code inside it +useless_else_on_loop.py:30:1: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and dedent its contents | 28 | break 29 | @@ -29,8 +31,9 @@ useless_else_on_loop.py:30:1: PLW0120 `else` clause on loop without a `break` st | ^^^^ PLW0120 31 | print("or else!") | + = help: Remove `else` -useless_else_on_loop.py:37:1: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and de-indent all the code inside it +useless_else_on_loop.py:37:1: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and dedent its contents | 35 | while False: 36 | break @@ -38,8 +41,9 @@ useless_else_on_loop.py:37:1: PLW0120 `else` clause on loop without a `break` st | ^^^^ PLW0120 38 | print("or else!") | + = help: Remove `else` -useless_else_on_loop.py:42:1: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and de-indent all the code inside it +useless_else_on_loop.py:42:1: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and dedent its contents | 40 | for j in range(10): 41 | pass @@ -48,8 +52,9 @@ useless_else_on_loop.py:42:1: PLW0120 `else` clause on loop without a `break` st 43 | print("fat chance") 44 | for j in range(10): | + = help: Remove `else` -useless_else_on_loop.py:88:5: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and de-indent all the code inside it +useless_else_on_loop.py:88:5: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and dedent its contents | 86 | else: 87 | print("all right") @@ -58,8 +63,9 @@ useless_else_on_loop.py:88:5: PLW0120 `else` clause on loop without a `break` st 89 | return True 90 | return False | + = help: Remove `else` -useless_else_on_loop.py:98:9: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and de-indent all the code inside it +useless_else_on_loop.py:98:9: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and dedent its contents | 96 | for _ in range(3): 97 | pass @@ -68,5 +74,17 @@ useless_else_on_loop.py:98:9: PLW0120 `else` clause on loop without a `break` st 99 | if 1 < 2: # pylint: disable=comparison-of-constants 100 | break | + = help: Remove `else` + +useless_else_on_loop.py:144:5: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and dedent its contents + | +142 | for j in range(10): +143 | pass +144 | else: + | ^^^^ PLW0120 +145 | # [useless-else-on-loop] +146 | print("fat chance") + | + = help: Remove `else` diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__preview__PLW0120_useless_else_on_loop.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__preview__PLW0120_useless_else_on_loop.py.snap new file mode 100644 index 0000000000000..ea72439be9eb0 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__preview__PLW0120_useless_else_on_loop.py.snap @@ -0,0 +1,187 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +useless_else_on_loop.py:9:5: PLW0120 [*] `else` clause on loop without a `break` statement; remove the `else` and dedent its contents + | + 7 | if i % 2: + 8 | return i + 9 | else: # [useless-else-on-loop] + | ^^^^ PLW0120 +10 | print("math is broken") +11 | return None + | + = help: Remove `else` + +ℹ Safe fix +6 6 | for i in range(10): +7 7 | if i % 2: +8 8 | return i +9 |- else: # [useless-else-on-loop] +10 |- print("math is broken") + 9 |+ print("math is broken") +11 10 | return None +12 11 | +13 12 | + +useless_else_on_loop.py:18:5: PLW0120 [*] `else` clause on loop without a `break` statement; remove the `else` and dedent its contents + | +16 | while True: +17 | return 1 +18 | else: # [useless-else-on-loop] + | ^^^^ PLW0120 +19 | print("math is broken") +20 | return None + | + = help: Remove `else` + +ℹ Safe fix +15 15 | """else + return is not acceptable.""" +16 16 | while True: +17 17 | return 1 +18 |- else: # [useless-else-on-loop] +19 |- print("math is broken") + 18 |+ print("math is broken") +20 19 | return None +21 20 | +22 21 | + +useless_else_on_loop.py:30:1: PLW0120 [*] `else` clause on loop without a `break` statement; remove the `else` and dedent its contents + | +28 | break +29 | +30 | else: # [useless-else-on-loop] + | ^^^^ PLW0120 +31 | print("or else!") + | + = help: Remove `else` + +ℹ Safe fix +27 27 | for _ in range(10): +28 28 | break +29 29 | +30 |-else: # [useless-else-on-loop] +31 |- print("or else!") + 30 |+print("or else!") +32 31 | +33 32 | +34 33 | while True: + +useless_else_on_loop.py:37:1: PLW0120 [*] `else` clause on loop without a `break` statement; remove the `else` and dedent its contents + | +35 | while False: +36 | break +37 | else: # [useless-else-on-loop] + | ^^^^ PLW0120 +38 | print("or else!") + | + = help: Remove `else` + +ℹ Safe fix +34 34 | while True: +35 35 | while False: +36 36 | break +37 |-else: # [useless-else-on-loop] +38 |- print("or else!") + 37 |+print("or else!") +39 38 | +40 39 | for j in range(10): +41 40 | pass + +useless_else_on_loop.py:42:1: PLW0120 [*] `else` clause on loop without a `break` statement; remove the `else` and dedent its contents + | +40 | for j in range(10): +41 | pass +42 | else: # [useless-else-on-loop] + | ^^^^ PLW0120 +43 | print("fat chance") +44 | for j in range(10): + | + = help: Remove `else` + +ℹ Safe fix +39 39 | +40 40 | for j in range(10): +41 41 | pass +42 |-else: # [useless-else-on-loop] +43 |- print("fat chance") +44 |- for j in range(10): +45 |- break + 42 |+print("fat chance") + 43 |+for j in range(10): + 44 |+ break +46 45 | +47 46 | +48 47 | def test_return_for2(): + +useless_else_on_loop.py:88:5: PLW0120 [*] `else` clause on loop without a `break` statement; remove the `else` and dedent its contents + | +86 | else: +87 | print("all right") +88 | else: # [useless-else-on-loop] + | ^^^^ PLW0120 +89 | return True +90 | return False + | + = help: Remove `else` + +ℹ Safe fix +85 85 | break +86 86 | else: +87 87 | print("all right") +88 |- else: # [useless-else-on-loop] +89 |- return True + 88 |+ return True +90 89 | return False +91 90 | +92 91 | + +useless_else_on_loop.py:98:9: PLW0120 [*] `else` clause on loop without a `break` statement; remove the `else` and dedent its contents + | + 96 | for _ in range(3): + 97 | pass + 98 | else: + | ^^^^ PLW0120 + 99 | if 1 < 2: # pylint: disable=comparison-of-constants +100 | break + | + = help: Remove `else` + +ℹ Safe fix +95 95 | for _ in range(10): +96 96 | for _ in range(3): +97 97 | pass +98 |- else: +99 |- if 1 < 2: # pylint: disable=comparison-of-constants +100 |- break + 98 |+ if 1 < 2: # pylint: disable=comparison-of-constants + 99 |+ break +101 100 | else: +102 101 | return True +103 102 | return False + +useless_else_on_loop.py:144:5: PLW0120 [*] `else` clause on loop without a `break` statement; remove the `else` and dedent its contents + | +142 | for j in range(10): +143 | pass +144 | else: + | ^^^^ PLW0120 +145 | # [useless-else-on-loop] +146 | print("fat chance") + | + = help: Remove `else` + +ℹ Safe fix +141 141 | """Retain the comment within the `else` block""" +142 142 | for j in range(10): +143 143 | pass +144 |- else: +145 |- # [useless-else-on-loop] +146 |- print("fat chance") +147 |- for j in range(10): +148 |- break + 144 |+ # [useless-else-on-loop] + 145 |+ print("fat chance") + 146 |+ for j in range(10): + 147 |+ break + + diff --git a/crates/ruff_linter/src/rules/pyupgrade/mod.rs b/crates/ruff_linter/src/rules/pyupgrade/mod.rs index 194bcef73fa3f..10fae94973062 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/mod.rs +++ b/crates/ruff_linter/src/rules/pyupgrade/mod.rs @@ -1,5 +1,5 @@ //! Rules from [pyupgrade](https://pypi.org/project/pyupgrade/). -mod fixes; +pub(crate) mod fixes; mod helpers; pub(crate) mod rules; pub mod settings;