diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF047_for.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF047_for.py index c7365b75a2c148..727efe4d15bdd5 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF047_for.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF047_for.py @@ -16,14 +16,12 @@ # `else` else: pass - ... for _and in so: does() # this else: - ... pass @@ -31,3 +29,16 @@ 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 index 57f20621a99d3a..6dcb76e9be3a4c 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF047_if.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF047_if.py @@ -20,21 +20,41 @@ if this_second_comment(): belongs() # to -# `else` + # `else` else: pass - ... -if and_so(): - does() -# this +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 index 75ecd66dcaf2bb..bfa3f357102809 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF047_try.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF047_try.py @@ -23,7 +23,6 @@ # `else` else: pass - ... try: @@ -33,7 +32,6 @@ # this else: ... - pass try: @@ -42,3 +40,32 @@ 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 index a564c04dbea465..2599f7b8a3cb31 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF047_while.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF047_while.py @@ -16,7 +16,6 @@ # `else` else: pass - ... while and_so: @@ -24,10 +23,22 @@ # this else: ... - pass 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 88db8907448d1a..f266bf106bb17e 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1239,7 +1239,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { ruff::rules::if_key_in_dict_del(checker, if_); } if checker.enabled(Rule::NeedlessElse) { - ruff::rules::needless_else(checker, stmt); + ruff::rules::needless_else(checker, if_.into()); } } Stmt::Assert( @@ -1348,7 +1348,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { flake8_async::rules::async_busy_wait(checker, while_stmt); } if checker.enabled(Rule::NeedlessElse) { - ruff::rules::needless_else(checker, stmt); + ruff::rules::needless_else(checker, while_stmt.into()); } } Stmt::For( @@ -1437,16 +1437,18 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { } } if checker.enabled(Rule::NeedlessElse) { - ruff::rules::needless_else(checker, stmt); + 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); } @@ -1514,7 +1516,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { tryceratops::rules::error_instead_of_exception(checker, handlers); } if checker.enabled(Rule::NeedlessElse) { - ruff::rules::needless_else(checker, stmt); + ruff::rules::needless_else(checker, try_stmt.into()); } } Stmt::Assign(assign @ ast::StmtAssign { targets, value, .. }) => { 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 386b7d7aa3fde4..19ebcf1dfebf66 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 @@ -73,8 +73,7 @@ pub(crate) fn useless_else_on_loop( return; } - let else_range = - identifier::else_loop(stmt, checker.locator().contents()).expect("else clause"); + let else_range = identifier::else_(stmt, checker.locator().contents()).expect("else clause"); let mut diagnostic = Diagnostic::new(UselessElseOnLoop, else_range); diagnostic.try_set_fix(|| { diff --git a/crates/ruff_linter/src/rules/ruff/rules/needless_else.rs b/crates/ruff_linter/src/rules/ruff/rules/needless_else.rs index 45dd4428a1fbce..2d4753484ab138 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/needless_else.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/needless_else.rs @@ -1,7 +1,7 @@ use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, ViolationMetadata}; -use ruff_python_ast::identifier::{else_loop, else_try}; -use ruff_python_ast::{ExceptHandler, Expr, Stmt, StmtExpr, StmtFor, StmtWhile}; +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}; @@ -41,123 +41,151 @@ impl AlwaysFixableViolation for NeedlessElse { } /// RUF047 -pub(crate) fn needless_else(checker: &mut Checker, stmt: &Stmt) { - let comment_ranges = checker.comment_ranges(); +pub(crate) fn needless_else(checker: &mut Checker, stmt: AnyNodeWithOrElse) { let source = checker.source(); - let (body, diagnostic_range, remove_range, comment_range) = match stmt { - Stmt::For(StmtFor { body, orelse, .. }) | Stmt::While(StmtWhile { body, orelse, .. }) => { - let (previous, else_body) = (&body, &orelse[..]); - - let Some(keyword_range) = else_loop(stmt, source) else { - return; - }; - let Some(last_stmt) = else_body.last() else { - return; - }; - let diagnostic_range = TextRange::new(keyword_range.start(), last_stmt.end()); - - let Some((remove_range, comment_range)) = - remove_and_comment_range(source, previous, else_body) - else { - return; - }; - - (else_body, diagnostic_range, remove_range, comment_range) - } - - Stmt::Try(try_stmt) => { - let else_body = &try_stmt.orelse[..]; - - let Some(keyword_range) = else_try(stmt, source) else { - return; - }; - let Some(last_stmt) = else_body.last() else { - return; - }; - let report_range = TextRange::new(keyword_range.start(), last_stmt.end()); - - let mut previous = &try_stmt.body[..]; - - if let [.., ExceptHandler::ExceptHandler(last_handler)] = &try_stmt.handlers[..] { - previous = &last_handler.body[..]; - }; - - let Some((remove_range, comment_range)) = - remove_and_comment_range(source, previous, else_body) - else { - return; - }; - - (else_body, report_range, remove_range, comment_range) - } - - Stmt::If(if_stmt) => { - let (previous, else_clause) = match &if_stmt.elif_else_clauses[..] { - [.., elif, possibly_else] if possibly_else.test.is_none() => { - (&elif.body, possibly_else) - } - [possibly_else] if possibly_else.test.is_none() => (&if_stmt.body, possibly_else), - _ => return, - }; - - let body = &else_clause.body[..]; - - let Some((remove_range, comment_range)) = - remove_and_comment_range(source, previous, body) - else { - return; - }; + let else_body = stmt.else_body(); - (body, else_clause.range, remove_range, comment_range) - } + if !body_is_useless(else_body) { + return; + } - _ => return, + let Some(else_range) = stmt.else_range(checker.tokens()) else { + return; }; - if !body_is_empty(body) { + 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()); - if comment_ranges.has_comments(&comment_range, source) { + // 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, diagnostic_range); + let diagnostic = Diagnostic::new(NeedlessElse, else_range); checker.diagnostics.push(diagnostic.with_fix(fix)); } -fn remove_and_comment_range( - source: &str, - previous_body: &[Stmt], - else_body: &[Stmt], -) -> Option<(TextRange, TextRange)> { - let preceding_stmt = previous_body.last()?; - let else_last_stmt = else_body.last()?; +/// Whether `body` contains only `pass` or `...` statements. +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), + } + } - let previous_block_end = source.line_end(preceding_stmt.end()); - let previous_block_end_plus_line_break = source.full_line_end(preceding_stmt.end()); - let else_block_end = source.line_end(else_last_stmt.end()); + // 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(), + } + } +} - let remove_range = TextRange::new(previous_block_end, else_block_end); - let comment_range = TextRange::new(previous_block_end_plus_line_break, else_block_end); +impl<'a> From<&'a StmtFor> for AnyNodeWithOrElse<'a> { + fn from(value: &'a StmtFor) -> Self { + Self::For(value) + } +} - Some((remove_range, comment_range)) +impl<'a> From<&'a StmtWhile> for AnyNodeWithOrElse<'a> { + fn from(value: &'a StmtWhile) -> Self { + Self::While(value) + } } -/// Whether `body` contains only `pass` or `...` statements. -fn body_is_empty(body: &[Stmt]) -> bool { - if body.is_empty() { - return false; +impl<'a> From<&'a StmtIf> for AnyNodeWithOrElse<'a> { + fn from(value: &'a StmtIf) -> Self { + Self::If(value) } +} - body.iter().all(|stmt| match stmt { - Stmt::Pass(..) => true, - Stmt::Expr(StmtExpr { value, .. }) => matches!(value.as_ref(), Expr::EllipsisLiteral(..)), - _ => false, - }) +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 index 68bc714e9e5036..36e2ea1060d88d 100644 --- 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 @@ -41,3 +41,24 @@ RUF047_for.py:10:1: RUF047 [*] Empty `else` clause 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 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 index cba158203e68ea..6bb02deacf2e18 100644 --- 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 @@ -60,3 +60,63 @@ RUF047_if.py:17:1: RUF047 [*] Empty `else` clause 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 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 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 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 index b8c76dfa9dd953..a7a48f138ba0b6 100644 --- 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 @@ -41,3 +41,25 @@ RUF047_try.py:15:1: RUF047 [*] Empty `else` clause 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 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 index 8774f2820eb5cb..97e8bed7eafa07 100644 --- 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 @@ -41,3 +41,24 @@ RUF047_while.py:10:1: RUF047 [*] Empty `else` clause 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 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/crates/ruff_python_ast/src/identifier.rs b/crates/ruff_python_ast/src/identifier.rs index f5bd4c95f9958e..566b9c79251be1 100644 --- a/crates/ruff_python_ast/src/identifier.rs +++ b/crates/ruff_python_ast/src/identifier.rs @@ -111,7 +111,7 @@ pub fn except(handler: &ExceptHandler, source: &str) -> TextRange { } /// Return the [`TextRange`] of the `else` token in a `For` or `While` statement. -pub fn else_loop(stmt: &Stmt, source: &str) -> Option { +pub fn else_(stmt: &Stmt, source: &str) -> Option { let (Stmt::For(ast::StmtFor { body, orelse, .. }) | Stmt::While(ast::StmtWhile { body, orelse, .. })) = stmt else { @@ -129,30 +129,6 @@ pub fn else_loop(stmt: &Stmt, source: &str) -> Option { .next() } -/// Return the [`TextRange`] of the `else` token in a `Try` statement. -pub fn else_try(stmt: &Stmt, source: &str) -> Option { - let Stmt::Try(stmt_try) = stmt else { - return None; - }; - - if stmt_try.orelse.is_empty() { - return None; - } - - let handlers = &stmt_try.handlers; - let mut body = &stmt_try.body[..]; - - if let [.., ExceptHandler::ExceptHandler(last_handler)] = &handlers[..] { - body = &last_handler.body[..]; - }; - - IdentifierTokenizer::starts_at( - body.last().expect("Expected body to be non-empty").end(), - source, - ) - .next() -} - /// Return `true` if the given character starts a valid Python identifier. /// /// Python identifiers must start with an alphabetic character or an underscore. diff --git a/crates/ruff_python_ast_integration_tests/tests/identifier.rs b/crates/ruff_python_ast_integration_tests/tests/identifier.rs index 2238482cbb1567..324390b8454c95 100644 --- a/crates/ruff_python_ast_integration_tests/tests/identifier.rs +++ b/crates/ruff_python_ast_integration_tests/tests/identifier.rs @@ -1,44 +1,23 @@ use ruff_python_ast::identifier; use ruff_python_parser::{parse_module, ParseError}; -use ruff_text_size::TextRange; - -macro_rules! test { - ($func:expr, $contents:expr, $expected:expr, $start:expr, $end:expr) => {{ - let contents = $contents.trim(); - - let stmts = parse_module(contents)?.into_suite(); - let stmt = stmts.first().unwrap(); - let range = $func(stmt, contents).unwrap(); - - assert_eq!(&contents[range], $expected); - assert_eq!(range, TextRange::new($start.into(), $end.into())); - - Ok(()) - }}; -} +use ruff_text_size::{TextRange, TextSize}; #[test] -fn extract_else_range_loop() -> Result<(), ParseError> { +fn extract_else_range() -> Result<(), ParseError> { let contents = r" for x in y: pass else: pass -"; - - test!(identifier::else_loop, contents, "else", 21, 25) -} - -#[test] -fn extract_else_range_try() -> Result<(), ParseError> { - let contents = r" -try: - pass -except: - pass -else: - pass -"; - - test!(identifier::else_try, contents, "else", 31, 35) +" + .trim(); + let stmts = parse_module(contents)?.into_suite(); + let stmt = stmts.first().unwrap(); + let range = identifier::else_(stmt, contents).unwrap(); + assert_eq!(&contents[range], "else"); + assert_eq!( + range, + TextRange::new(TextSize::from(21), TextSize::from(25)) + ); + Ok(()) }