From 7329bf459cb87c11e4bd399c4b21db8f64214bcd Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 27 Jan 2024 06:15:33 -0800 Subject: [PATCH] Avoid panic when fixing inlined else blocks (#9657) Closes https://github.com/astral-sh/ruff/issues/9655. --- .../test/fixtures/flake8_return/RET505.py | 14 ++++ .../src/rules/flake8_return/rules/function.rs | 79 +++++++++++++++---- ...lake8_return__tests__RET505_RET505.py.snap | 19 +++++ ...urn__tests__preview__RET505_RET505.py.snap | 44 ++++++++++- 4 files changed, 138 insertions(+), 18 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_return/RET505.py b/crates/ruff_linter/resources/test/fixtures/flake8_return/RET505.py index a222c5055b683..ee5781e15f44c 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_return/RET505.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_return/RET505.py @@ -162,6 +162,20 @@ def bar7(): : # comment pass + +def bar8(): + if True: + return + else: pass + + +def bar9(): + if True: + return + else:\ + pass + + x = 0 if x == 1: diff --git a/crates/ruff_linter/src/rules/flake8_return/rules/function.rs b/crates/ruff_linter/src/rules/flake8_return/rules/function.rs index 36dc832dc6fb2..7e016ecba70c7 100644 --- a/crates/ruff_linter/src/rules/flake8_return/rules/function.rs +++ b/crates/ruff_linter/src/rules/flake8_return/rules/function.rs @@ -12,8 +12,11 @@ use ruff_python_ast::helpers::{is_const_false, is_const_true}; use ruff_python_ast::stmt_if::elif_else_range; use ruff_python_ast::visitor::Visitor; use ruff_python_ast::whitespace::indentation; +use ruff_python_codegen::Stylist; +use ruff_python_index::Indexer; use ruff_python_semantic::SemanticModel; use ruff_python_trivia::{is_python_whitespace, SimpleTokenKind, SimpleTokenizer}; +use ruff_source_file::Locator; use crate::checkers::ast::Checker; use crate::fix::edits; @@ -635,7 +638,14 @@ fn superfluous_else_node( ); if checker.enabled(diagnostic.kind.rule()) { if checker.settings.preview.is_enabled() { - diagnostic.try_set_fix(|| remove_else(checker, elif_else)); + diagnostic.try_set_fix(|| { + remove_else( + elif_else, + checker.locator(), + checker.indexer(), + checker.stylist(), + ) + }); } checker.diagnostics.push(diagnostic); } @@ -648,7 +658,14 @@ fn superfluous_else_node( ); if checker.enabled(diagnostic.kind.rule()) { if checker.settings.preview.is_enabled() { - diagnostic.try_set_fix(|| remove_else(checker, elif_else)); + diagnostic.try_set_fix(|| { + remove_else( + elif_else, + checker.locator(), + checker.indexer(), + checker.stylist(), + ) + }); } checker.diagnostics.push(diagnostic); } @@ -661,7 +678,14 @@ fn superfluous_else_node( ); if checker.enabled(diagnostic.kind.rule()) { if checker.settings.preview.is_enabled() { - diagnostic.try_set_fix(|| remove_else(checker, elif_else)); + diagnostic.try_set_fix(|| { + remove_else( + elif_else, + checker.locator(), + checker.indexer(), + checker.stylist(), + ) + }); } checker.diagnostics.push(diagnostic); } @@ -674,7 +698,14 @@ fn superfluous_else_node( ); if checker.enabled(diagnostic.kind.rule()) { if checker.settings.preview.is_enabled() { - diagnostic.try_set_fix(|| remove_else(checker, elif_else)); + diagnostic.try_set_fix(|| { + remove_else( + elif_else, + checker.locator(), + checker.indexer(), + checker.stylist(), + ) + }); } checker.diagnostics.push(diagnostic); } @@ -754,22 +785,26 @@ pub(crate) fn function(checker: &mut Checker, body: &[Stmt], returns: Option<&Ex } } -fn remove_else(checker: &mut Checker, elif_else: &ElifElseClause) -> Result { +/// Generate a [`Fix`] to remove an `else` or `elif` clause. +fn remove_else( + elif_else: &ElifElseClause, + locator: &Locator, + indexer: &Indexer, + stylist: &Stylist, +) -> Result { if elif_else.test.is_some() { - // it's an elif, so we can just make it an if - - // delete "el" from "elif" + // Ex) `elif` -> `if` Ok(Fix::safe_edit(Edit::deletion( elif_else.start(), elif_else.start() + TextSize::from(2), ))) } else { // the start of the line where the `else`` is - let else_line_start = checker.locator().line_start(elif_else.start()); + let else_line_start = locator.line_start(elif_else.start()); // making a tokenizer to find the Colon for the `else`, not always on the same line! let mut else_line_tokenizer = - SimpleTokenizer::starts_at(elif_else.start(), checker.locator().contents()); + SimpleTokenizer::starts_at(elif_else.start(), locator.contents()); // find the Colon for the `else` let Some(else_colon) = @@ -779,13 +814,25 @@ fn remove_else(checker: &mut Checker, elif_else: &ElifElseClause) -> Result }; // get the indentation of the `else`, since that is the indent level we want to end with - let Some(desired_indentation) = indentation(checker.locator(), elif_else) else { + let Some(desired_indentation) = indentation(locator, elif_else) else { return Err(anyhow::anyhow!("Compound statement cannot be inlined")); }; + // If the statement is on the same line as the `else`, just remove the `else: `. + // Ex) `else: return True` -> `return True` + let Some(first) = elif_else.body.first() else { + return Err(anyhow::anyhow!("`else` statement has no body")); + }; + if indexer.in_multi_statement_line(first, locator) { + return Ok(Fix::safe_edit(Edit::deletion( + elif_else.start(), + first.start(), + ))); + } + // we're deleting the `else`, and it's Colon, and the rest of the line(s) they're on, // so here we get the last position of the line the Colon is on - let else_colon_end = checker.locator().full_line_end(else_colon.end()); + let else_colon_end = locator.full_line_end(else_colon.end()); // if there is a comment on the same line as the Colon, let's keep it // and give it the proper indentation once we unindent it @@ -795,8 +842,8 @@ fn remove_else(checker: &mut Checker, elif_else: &ElifElseClause) -> Result if token.kind == SimpleTokenKind::Comment && token.start() < else_colon_end { return Some(format!( "{desired_indentation}{}{}", - checker.locator().slice(token), - checker.stylist().line_ending().as_str(), + locator.slice(token), + stylist.line_ending().as_str(), )); } None @@ -806,8 +853,8 @@ fn remove_else(checker: &mut Checker, elif_else: &ElifElseClause) -> Result let indented = adjust_indentation( TextRange::new(else_colon_end, elif_else.end()), desired_indentation, - checker.locator(), - checker.stylist(), + locator, + stylist, )?; Ok(Fix::safe_edit(Edit::replacement( diff --git a/crates/ruff_linter/src/rules/flake8_return/snapshots/ruff_linter__rules__flake8_return__tests__RET505_RET505.py.snap b/crates/ruff_linter/src/rules/flake8_return/snapshots/ruff_linter__rules__flake8_return__tests__RET505_RET505.py.snap index e3ecd603fa652..bead74c93b308 100644 --- a/crates/ruff_linter/src/rules/flake8_return/snapshots/ruff_linter__rules__flake8_return__tests__RET505_RET505.py.snap +++ b/crates/ruff_linter/src/rules/flake8_return/snapshots/ruff_linter__rules__flake8_return__tests__RET505_RET505.py.snap @@ -132,4 +132,23 @@ RET505.py:161:5: RET505 Unnecessary `else` after `return` statement | = help: Remove unnecessary `else` +RET505.py:169:5: RET505 Unnecessary `else` after `return` statement + | +167 | if True: +168 | return +169 | else: pass + | ^^^^ RET505 + | + = help: Remove unnecessary `else` + +RET505.py:175:5: RET505 Unnecessary `else` after `return` statement + | +173 | if True: +174 | return +175 | else:\ + | ^^^^ RET505 +176 | pass + | + = help: Remove unnecessary `else` + diff --git a/crates/ruff_linter/src/rules/flake8_return/snapshots/ruff_linter__rules__flake8_return__tests__preview__RET505_RET505.py.snap b/crates/ruff_linter/src/rules/flake8_return/snapshots/ruff_linter__rules__flake8_return__tests__preview__RET505_RET505.py.snap index 2291503080edc..6d495926e4077 100644 --- a/crates/ruff_linter/src/rules/flake8_return/snapshots/ruff_linter__rules__flake8_return__tests__preview__RET505_RET505.py.snap +++ b/crates/ruff_linter/src/rules/flake8_return/snapshots/ruff_linter__rules__flake8_return__tests__preview__RET505_RET505.py.snap @@ -276,7 +276,47 @@ RET505.py:161:5: RET505 [*] Unnecessary `else` after `return` statement 161 |+ # comment 162 |+ pass 164 163 | -165 164 | x = 0 -166 165 | +165 164 | +166 165 | def bar8(): + +RET505.py:169:5: RET505 [*] Unnecessary `else` after `return` statement + | +167 | if True: +168 | return +169 | else: pass + | ^^^^ RET505 + | + = help: Remove unnecessary `else` + +ℹ Safe fix +166 166 | def bar8(): +167 167 | if True: +168 168 | return +169 |- else: pass + 169 |+ pass +170 170 | +171 171 | +172 172 | def bar9(): + +RET505.py:175:5: RET505 [*] Unnecessary `else` after `return` statement + | +173 | if True: +174 | return +175 | else:\ + | ^^^^ RET505 +176 | pass + | + = help: Remove unnecessary `else` + +ℹ Safe fix +172 172 | def bar9(): +173 173 | if True: +174 174 | return +175 |- else:\ +176 |- pass + 175 |+ pass +177 176 | +178 177 | +179 178 | x = 0