diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF047_for.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF047_for.py new file mode 100644 index 0000000000000..727efe4d15bdd --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF047_for.py @@ -0,0 +1,44 @@ +for _ in range(0): + loop_body_is_not_checked() + break +else: + pass + + +for this in comment: + belongs_to() # `for` +else: + ... + + +for this in second_comment: + belongs() # to +# `else` +else: + pass + + +for _and in so: + does() +# this +else: + pass + + +for of in course(): + this() +else: + ... # too + + +for of in course(): + this() +else: + ... + # too + +for of in course(): + this() +else: + ... +# this comment does not belong to the else diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF047_if.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF047_if.py new file mode 100644 index 0000000000000..6dcb76e9be3a4 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF047_if.py @@ -0,0 +1,60 @@ +if False: + condition_is_not_evaluated() +else: + pass + + +if this_comment(): + belongs_to() # `if` +else: + ... + + +if elif_is(): + treated() +elif the_same(): + as_if() +else: + pass + + +if this_second_comment(): + belongs() # to + # `else` +else: + pass + + +if this_second_comment(): + belongs() # to +# `else` +else: + pass + +if of_course(): + this() +else: + ... # too + +if of_course(): + this() +else: + ... + # comment + +if of_course(): + this() +else: + ... +# this comment doesn't belong to the if + +if of_course: this() +else: ... + +if of_course: + this() # comment +else: ... + +if of_course: + this() # comment +else: ... # trailing \ No newline at end of file diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF047_try.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF047_try.py new file mode 100644 index 0000000000000..bfa3f35710280 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF047_try.py @@ -0,0 +1,71 @@ +try: + raise try_body_is_not_checked() +except: + pass +else: + pass + + +try: + this() +except comment: + belongs() +except: + to() # `except` +else: + ... + + +try: + this() +except (second, comment): + belongs() # to +# `else` +else: + pass + + +try: + and_so() +except: + does() +# this +else: + ... + + +try: + of_course() +except: + this() +else: + ... # too + +try: + of_course() +except: + this() +else: + ... + # This comment belongs to else +finally: + pass + +try: + of_course() +except: + this() +else: + ... +# This comment belongs to finally +finally: + pass + + +try: + of_course() +except: + this() +else: + ... +# This comment belongs to the statement coming after the else diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF047_while.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF047_while.py new file mode 100644 index 0000000000000..2599f7b8a3cb3 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF047_while.py @@ -0,0 +1,44 @@ +while True: + loop_body_is_not_checked() + break +else: + pass + + +while this_comment: + belongs_to() # `for` +else: + ... + + +while this_second_comment: + belongs() # to +# `else` +else: + pass + + +while and_so: + does() +# this +else: + ... + + +while of_course(): + this() +else: + ... # too + +while of_course(): + this() +else: + ... + # this comment belongs to the else + +while of_course(): + this() +else: + ... +# this comment belongs to the statement coming after the else + diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index b9b55757178e9..aca2ce8dbe73f 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1244,6 +1244,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::IfKeyInDictDel) { ruff::rules::if_key_in_dict_del(checker, if_); } + if checker.enabled(Rule::NeedlessElse) { + ruff::rules::needless_else(checker, if_.into()); + } } Stmt::Assert( assert_stmt @ ast::StmtAssert { @@ -1350,6 +1353,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::AsyncBusyWait) { flake8_async::rules::async_busy_wait(checker, while_stmt); } + if checker.enabled(Rule::NeedlessElse) { + ruff::rules::needless_else(checker, while_stmt.into()); + } } Stmt::For( for_stmt @ ast::StmtFor { @@ -1436,14 +1442,19 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { refurb::rules::for_loop_set_mutations(checker, for_stmt); } } + if checker.enabled(Rule::NeedlessElse) { + ruff::rules::needless_else(checker, for_stmt.into()); + } } - Stmt::Try(ast::StmtTry { - body, - handlers, - orelse, - finalbody, - .. - }) => { + Stmt::Try( + try_stmt @ ast::StmtTry { + body, + handlers, + orelse, + finalbody, + .. + }, + ) => { if checker.enabled(Rule::TooManyNestedBlocks) { pylint::rules::too_many_nested_blocks(checker, stmt); } @@ -1510,6 +1521,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::ErrorInsteadOfException) { tryceratops::rules::error_instead_of_exception(checker, handlers); } + if checker.enabled(Rule::NeedlessElse) { + ruff::rules::needless_else(checker, try_stmt.into()); + } } Stmt::Assign(assign @ ast::StmtAssign { targets, value, .. }) => { if checker.enabled(Rule::SelfOrClsAssignment) { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index d1ab7466b2411..d87674b553bc1 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -990,6 +990,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Ruff, "041") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryNestedLiteral), (Ruff, "043") => (RuleGroup::Preview, rules::ruff::rules::PytestRaisesAmbiguousPattern), (Ruff, "046") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryCastToInt), + (Ruff, "047") => (RuleGroup::Preview, rules::ruff::rules::NeedlessElse), (Ruff, "048") => (RuleGroup::Preview, rules::ruff::rules::MapIntVersionParsing), (Ruff, "051") => (RuleGroup::Preview, rules::ruff::rules::IfKeyInDictDel), (Ruff, "052") => (RuleGroup::Preview, rules::ruff::rules::UsedDummyVariable), diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index 4c7e0a9621892..55d4f5e5c0cb2 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -421,6 +421,10 @@ mod tests { #[test_case(Rule::UnnecessaryCastToInt, Path::new("RUF046.py"))] #[test_case(Rule::PytestRaisesAmbiguousPattern, Path::new("RUF043.py"))] #[test_case(Rule::UnnecessaryRound, Path::new("RUF057.py"))] + #[test_case(Rule::NeedlessElse, Path::new("RUF047_if.py"))] + #[test_case(Rule::NeedlessElse, Path::new("RUF047_for.py"))] + #[test_case(Rule::NeedlessElse, Path::new("RUF047_while.py"))] + #[test_case(Rule::NeedlessElse, Path::new("RUF047_try.py"))] fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!( "preview__{}_{}", diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index 327d805fd9cc3..2c97b5dfadc5e 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -20,6 +20,7 @@ pub(crate) use missing_fstring_syntax::*; pub(crate) use mutable_class_default::*; pub(crate) use mutable_dataclass_default::*; pub(crate) use mutable_fromkeys_value::*; +pub(crate) use needless_else::*; pub(crate) use never_union::*; pub(crate) use none_not_at_end_of_union::*; pub(crate) use parenthesize_chained_operators::*; @@ -72,6 +73,7 @@ mod missing_fstring_syntax; mod mutable_class_default; mod mutable_dataclass_default; mod mutable_fromkeys_value; +mod needless_else; mod never_union; mod none_not_at_end_of_union; mod parenthesize_chained_operators; diff --git a/crates/ruff_linter/src/rules/ruff/rules/needless_else.rs b/crates/ruff_linter/src/rules/ruff/rules/needless_else.rs new file mode 100644 index 0000000000000..e3415afc929d7 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/rules/needless_else.rs @@ -0,0 +1,191 @@ +use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; +use ruff_macros::{derive_message_formats, ViolationMetadata}; +use ruff_python_ast::{Stmt, StmtExpr, StmtFor, StmtIf, StmtTry, StmtWhile}; +use ruff_python_parser::{TokenKind, Tokens}; +use ruff_source_file::LineRanges; +use ruff_text_size::{Ranged, TextRange}; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for `else` clauses that only contains `pass` and `...` statements. +/// +/// ## Why is this bad? +/// Such an else clause does nothing and can be removed. +/// +/// ## Example +/// ```python +/// if foo: +/// bar() +/// else: +/// pass +/// ``` +/// +/// Use instead: +/// ```python +/// if foo: +/// bar() +/// ``` +#[derive(ViolationMetadata)] +pub(crate) struct NeedlessElse; + +impl AlwaysFixableViolation for NeedlessElse { + #[derive_message_formats] + fn message(&self) -> String { + "Empty `else` clause".to_string() + } + + fn fix_title(&self) -> String { + "Remove `else` clause".to_string() + } +} + +/// RUF047 +pub(crate) fn needless_else(checker: &mut Checker, stmt: AnyNodeWithOrElse) { + let source = checker.source(); + + let else_body = stmt.else_body(); + + if !body_is_useless(else_body) { + return; + } + + let Some(else_range) = stmt.else_range(checker.tokens()) else { + return; + }; + + let before_else = stmt.body_before_else(); + let Some(preceding_stmt) = before_else.last() else { + return; + }; + + let before_else_full_end = source.full_line_end(preceding_stmt.end()); + let else_full_end = source.full_line_end(else_range.end()); + + // Preserve else blocks that contain a comment. + if checker + .comment_ranges() + .intersects(TextRange::new(before_else_full_end, else_full_end)) + { + return; + } + + let remove_range = TextRange::new(before_else_full_end, else_full_end); + + let edit = Edit::range_deletion(remove_range); + let fix = Fix::safe_edit(edit); + + let diagnostic = Diagnostic::new(NeedlessElse, else_range); + + checker.diagnostics.push(diagnostic.with_fix(fix)); +} + +/// Whether `body` contains only one `pass` or `...` statement. +fn body_is_useless(body: &[Stmt]) -> bool { + match body { + [Stmt::Pass(_)] => true, + [Stmt::Expr(StmtExpr { value, .. })] => value.is_ellipsis_literal_expr(), + _ => false, + } +} + +#[derive(Copy, Clone, Debug)] +pub(crate) enum AnyNodeWithOrElse<'a> { + While(&'a StmtWhile), + For(&'a StmtFor), + Try(&'a StmtTry), + If(&'a StmtIf), +} + +impl<'a> AnyNodeWithOrElse<'a> { + /// Returns the range from the `else` keyword to the last statement + /// in it's block. + fn else_range(self, tokens: &Tokens) -> Option { + match self { + Self::For(_) | Self::While(_) | Self::Try(_) => { + let before_else = self.body_before_else(); + + let else_body = self.else_body(); + let end = else_body.last()?.end(); + + let start = tokens + .in_range(TextRange::new(before_else.last()?.end(), end)) + .iter() + .find(|token| token.kind() == TokenKind::Else)? + .start(); + + Some(TextRange::new(start, end)) + } + + Self::If(StmtIf { + elif_else_clauses, .. + }) => elif_else_clauses + .last() + .filter(|clause| clause.test.is_none()) + .map(Ranged::range), + } + } + + /// Returns the suite before the else block. + fn body_before_else(self) -> &'a [Stmt] { + match self { + Self::Try(StmtTry { body, handlers, .. }) => handlers + .last() + .and_then(|handler| handler.as_except_handler()) + .map(|handler| &handler.body) + .unwrap_or(body), + Self::While(StmtWhile { body, .. }) | Self::For(StmtFor { body, .. }) => body, + Self::If(StmtIf { + body, + elif_else_clauses, + .. + }) => elif_else_clauses + .iter() + .rev() + .find(|clause| clause.test.is_some()) + .map(|clause| &*clause.body) + .unwrap_or(body), + } + } + + // Returns the `else` suite. Defaults to an empty suite if the statement + // has no `else` block. + fn else_body(self) -> &'a [Stmt] { + match self { + Self::While(StmtWhile { orelse, .. }) + | Self::For(StmtFor { orelse, .. }) + | Self::Try(StmtTry { orelse, .. }) => orelse, + Self::If(StmtIf { + elif_else_clauses, .. + }) => elif_else_clauses + .last() + .filter(|clause| clause.test.is_none()) + .map(|clause| &*clause.body) + .unwrap_or_default(), + } + } +} + +impl<'a> From<&'a StmtFor> for AnyNodeWithOrElse<'a> { + fn from(value: &'a StmtFor) -> Self { + Self::For(value) + } +} + +impl<'a> From<&'a StmtWhile> for AnyNodeWithOrElse<'a> { + fn from(value: &'a StmtWhile) -> Self { + Self::While(value) + } +} + +impl<'a> From<&'a StmtIf> for AnyNodeWithOrElse<'a> { + fn from(value: &'a StmtIf) -> Self { + Self::If(value) + } +} + +impl<'a> From<&'a StmtTry> for AnyNodeWithOrElse<'a> { + fn from(value: &'a StmtTry) -> Self { + Self::Try(value) + } +} diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF047_RUF047_for.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF047_RUF047_for.py.snap new file mode 100644 index 0000000000000..da5aca1aee3f6 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF047_RUF047_for.py.snap @@ -0,0 +1,64 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +snapshot_kind: text +--- +RUF047_for.py:4:1: RUF047 [*] Empty `else` clause + | +2 | loop_body_is_not_checked() +3 | break +4 | / else: +5 | | pass + | |________^ RUF047 + | + = help: Remove `else` clause + +ℹ Safe fix +1 1 | for _ in range(0): +2 2 | loop_body_is_not_checked() +3 3 | break +4 |-else: +5 |- pass +6 4 | +7 5 | +8 6 | for this in comment: + +RUF047_for.py:10:1: RUF047 [*] Empty `else` clause + | + 8 | for this in comment: + 9 | belongs_to() # `for` +10 | / else: +11 | | ... + | |_______^ RUF047 + | + = help: Remove `else` clause + +ℹ Safe fix +7 7 | +8 8 | for this in comment: +9 9 | belongs_to() # `for` +10 |-else: +11 |- ... +12 10 | +13 11 | +14 12 | for this in second_comment: + +RUF047_for.py:36:1: RUF047 [*] Empty `else` clause + | +34 | for of in course(): +35 | this() +36 | / else: +37 | | ... + | |_______^ RUF047 +38 | # too + | + = help: Remove `else` clause + +ℹ Safe fix +33 33 | +34 34 | for of in course(): +35 35 | this() +36 |-else: +37 |- ... +38 36 | # too +39 37 | +40 38 | for of in course(): diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF047_RUF047_if.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF047_RUF047_if.py.snap new file mode 100644 index 0000000000000..41ef4f97b2586 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF047_RUF047_if.py.snap @@ -0,0 +1,122 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +snapshot_kind: text +--- +RUF047_if.py:3:1: RUF047 [*] Empty `else` clause + | +1 | if False: +2 | condition_is_not_evaluated() +3 | / else: +4 | | pass + | |________^ RUF047 + | + = help: Remove `else` clause + +ℹ Safe fix +1 1 | if False: +2 2 | condition_is_not_evaluated() +3 |-else: +4 |- pass +5 3 | +6 4 | +7 5 | if this_comment(): + +RUF047_if.py:9:1: RUF047 [*] Empty `else` clause + | + 7 | if this_comment(): + 8 | belongs_to() # `if` + 9 | / else: +10 | | ... + | |_______^ RUF047 + | + = help: Remove `else` clause + +ℹ Safe fix +6 6 | +7 7 | if this_comment(): +8 8 | belongs_to() # `if` +9 |-else: +10 |- ... +11 9 | +12 10 | +13 11 | if elif_is(): + +RUF047_if.py:17:1: RUF047 [*] Empty `else` clause + | +15 | elif the_same(): +16 | as_if() +17 | / else: +18 | | pass + | |________^ RUF047 + | + = help: Remove `else` clause + +ℹ Safe fix +14 14 | treated() +15 15 | elif the_same(): +16 16 | as_if() +17 |-else: +18 |- pass +19 17 | +20 18 | +21 19 | if this_second_comment(): + +RUF047_if.py:41:1: RUF047 [*] Empty `else` clause + | +39 | if of_course(): +40 | this() +41 | / else: +42 | | ... + | |_______^ RUF047 +43 | # comment + | + = help: Remove `else` clause + +ℹ Safe fix +38 38 | +39 39 | if of_course(): +40 40 | this() +41 |-else: +42 |- ... +43 41 | # comment +44 42 | +45 43 | if of_course(): + +RUF047_if.py:52:1: RUF047 [*] Empty `else` clause + | +51 | if of_course: this() +52 | else: ... + | ^^^^^^^^^ RUF047 +53 | +54 | if of_course: + | + = help: Remove `else` clause + +ℹ Safe fix +49 49 | # this comment doesn't belong to the if +50 50 | +51 51 | if of_course: this() +52 |-else: ... +53 52 | +54 53 | if of_course: +55 54 | this() # comment + +RUF047_if.py:56:1: RUF047 [*] Empty `else` clause + | +54 | if of_course: +55 | this() # comment +56 | else: ... + | ^^^^^^^^^ RUF047 +57 | +58 | if of_course: + | + = help: Remove `else` clause + +ℹ Safe fix +53 53 | +54 54 | if of_course: +55 55 | this() # comment +56 |-else: ... +57 56 | +58 57 | if of_course: +59 58 | this() # comment diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF047_RUF047_try.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF047_RUF047_try.py.snap new file mode 100644 index 0000000000000..bf32be69eb4ac --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF047_RUF047_try.py.snap @@ -0,0 +1,65 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +snapshot_kind: text +--- +RUF047_try.py:5:1: RUF047 [*] Empty `else` clause + | +3 | except: +4 | pass +5 | / else: +6 | | pass + | |________^ RUF047 + | + = help: Remove `else` clause + +ℹ Safe fix +2 2 | raise try_body_is_not_checked() +3 3 | except: +4 4 | pass +5 |-else: +6 |- pass +7 5 | +8 6 | +9 7 | try: + +RUF047_try.py:15:1: RUF047 [*] Empty `else` clause + | +13 | except: +14 | to() # `except` +15 | / else: +16 | | ... + | |_______^ RUF047 + | + = help: Remove `else` clause + +ℹ Safe fix +12 12 | belongs() +13 13 | except: +14 14 | to() # `except` +15 |-else: +16 |- ... +17 15 | +18 16 | +19 17 | try: + +RUF047_try.py:48:1: RUF047 [*] Empty `else` clause + | +46 | except: +47 | this() +48 | / else: +49 | | ... + | |_______^ RUF047 +50 | # This comment belongs to else +51 | finally: + | + = help: Remove `else` clause + +ℹ Safe fix +45 45 | of_course() +46 46 | except: +47 47 | this() +48 |-else: +49 |- ... +50 48 | # This comment belongs to else +51 49 | finally: +52 50 | pass diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF047_RUF047_while.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF047_RUF047_while.py.snap new file mode 100644 index 0000000000000..5c29e8e4d13e8 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF047_RUF047_while.py.snap @@ -0,0 +1,64 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +snapshot_kind: text +--- +RUF047_while.py:4:1: RUF047 [*] Empty `else` clause + | +2 | loop_body_is_not_checked() +3 | break +4 | / else: +5 | | pass + | |________^ RUF047 + | + = help: Remove `else` clause + +ℹ Safe fix +1 1 | while True: +2 2 | loop_body_is_not_checked() +3 3 | break +4 |-else: +5 |- pass +6 4 | +7 5 | +8 6 | while this_comment: + +RUF047_while.py:10:1: RUF047 [*] Empty `else` clause + | + 8 | while this_comment: + 9 | belongs_to() # `for` +10 | / else: +11 | | ... + | |_______^ RUF047 + | + = help: Remove `else` clause + +ℹ Safe fix +7 7 | +8 8 | while this_comment: +9 9 | belongs_to() # `for` +10 |-else: +11 |- ... +12 10 | +13 11 | +14 12 | while this_second_comment: + +RUF047_while.py:35:1: RUF047 [*] Empty `else` clause + | +33 | while of_course(): +34 | this() +35 | / else: +36 | | ... + | |_______^ RUF047 +37 | # this comment belongs to the else + | + = help: Remove `else` clause + +ℹ Safe fix +32 32 | +33 33 | while of_course(): +34 34 | this() +35 |-else: +36 |- ... +37 35 | # this comment belongs to the else +38 36 | +39 37 | while of_course(): diff --git a/ruff.schema.json b/ruff.schema.json index 6cccea335d3c2..4e25fec7c192f 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3869,6 +3869,7 @@ "RUF041", "RUF043", "RUF046", + "RUF047", "RUF048", "RUF05", "RUF051",