Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid panic when fixing inlined else blocks #9657

Merged
merged 1 commit into from
Jan 27, 2024
Merged

Avoid panic when fixing inlined else blocks #9657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 14 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_return/RET505.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
79 changes: 63 additions & 16 deletions crates/ruff_linter/src/rules/flake8_return/rules/function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand Down Expand Up @@ -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<Fix> {
/// Generate a [`Fix`] to remove an `else` or `elif` clause.
fn remove_else(
elif_else: &ElifElseClause,
locator: &Locator,
indexer: &Indexer,
stylist: &Stylist,
) -> Result<Fix> {
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) =
Expand All @@ -779,13 +814,25 @@ fn remove_else(checker: &mut Checker, elif_else: &ElifElseClause) -> Result<Fix>
};

// 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
Expand All @@ -795,8 +842,8 @@ fn remove_else(checker: &mut Checker, elif_else: &ElifElseClause) -> Result<Fix>
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
Expand All @@ -806,8 +853,8 @@ fn remove_else(checker: &mut Checker, elif_else: &ElifElseClause) -> Result<Fix>
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(
Expand Down
19 changes: 19 additions & 0 deletions ...s/flake8_return/snapshots/ruff_linter__rules__flake8_return__tests__RET505_RET505.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`


44 changes: 42 additions & 2 deletions ...return/snapshots/ruff_linter__rules__flake8_return__tests__preview__RET505_RET505.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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