Skip to content

Commit

Permalink
Avoid panic when fixing inlined else blocks
Browse files Browse the repository at this point in the history
  • Loading branch information
@charliermarsh
charliermarsh committed Jan 27, 2024
1 parent 157d5ba commit b533ae8
Show file tree
Hide file tree
Showing 4 changed files with 138 additions and 18 deletions.
14 changes: 14 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_return/RET505.py
View file
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
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
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
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


0 comments on commit b533ae8

Please sign in to comment.