mirror of https://github.com/astral-sh/ruff
Fix `SIM102` to handle indented `elif` (#6072)
<!--
Thank you for contributing to Ruff! To help us out with reviewing,
please consider the following:
- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title?
- Does this pull request include references to any relevant issues?
-->
## Summary
<!-- What's the purpose of the change? What does it do, and why? -->
The `SIM102` auto-fix fails if `elif` is indented like this:
## Example
```python
def f():
# SIM102
if a:
pass
elif b:
if c:
d
```
```
> cargo run -p ruff_cli -- check --select SIM102 --fix a.py
...
error: Failed to fix nested if: Failed to extract statement from source
a.py:5:5: SIM102 Use a single `if` statement instead of nested `if` statements
Found 1 error.
```
## Test Plan
<!-- How was it tested? -->
New test
This commit is contained in:
parent
16e1737d1b
commit
77396c6f92
|
|
@ -156,3 +156,12 @@ if False:
|
|||
if True:
|
||||
if a:
|
||||
pass
|
||||
|
||||
|
||||
# SIM102
|
||||
def f():
|
||||
if a:
|
||||
pass
|
||||
elif b:
|
||||
if c:
|
||||
d
|
||||
|
|
|
|||
|
|
@ -312,7 +312,8 @@ fn find_last_nested_if(body: &[Stmt]) -> Option<(&Expr, &Stmt)> {
|
|||
})
|
||||
}
|
||||
|
||||
fn nested_if_body(stmt_if: &StmtIf) -> Option<(&[Stmt], TextRange)> {
|
||||
/// Returns the body, the range of the `if` or `elif` and whether the range is for an `if` or `elif`
|
||||
fn nested_if_body(stmt_if: &StmtIf) -> Option<(&[Stmt], TextRange, bool)> {
|
||||
let StmtIf {
|
||||
test,
|
||||
body,
|
||||
|
|
@ -322,15 +323,15 @@ fn nested_if_body(stmt_if: &StmtIf) -> Option<(&[Stmt], TextRange)> {
|
|||
|
||||
// It must be the last condition, otherwise there could be another `elif` or `else` that only
|
||||
// depends on the outer of the two conditions
|
||||
let (test, body, range) = if let Some(clause) = elif_else_clauses.last() {
|
||||
let (test, body, range, is_elif) = if let Some(clause) = elif_else_clauses.last() {
|
||||
if let Some(test) = &clause.test {
|
||||
(test, &clause.body, clause.range())
|
||||
(test, &clause.body, clause.range(), true)
|
||||
} else {
|
||||
// The last condition is an `else` (different rule)
|
||||
return None;
|
||||
}
|
||||
} else {
|
||||
(test.as_ref(), body, stmt_if.range())
|
||||
(test.as_ref(), body, stmt_if.range(), false)
|
||||
};
|
||||
|
||||
// The nested if must be the only child, otherwise there is at least one more statement that
|
||||
|
|
@ -355,12 +356,12 @@ fn nested_if_body(stmt_if: &StmtIf) -> Option<(&[Stmt], TextRange)> {
|
|||
return None;
|
||||
}
|
||||
|
||||
Some((body, range))
|
||||
Some((body, range, is_elif))
|
||||
}
|
||||
|
||||
/// SIM102
|
||||
pub(crate) fn nested_if_statements(checker: &mut Checker, stmt_if: &StmtIf, parent: Option<&Stmt>) {
|
||||
let Some((body, range)) = nested_if_body(stmt_if) else {
|
||||
let Some((body, range, is_elif)) = nested_if_body(stmt_if) else {
|
||||
return;
|
||||
};
|
||||
|
||||
|
|
@ -376,7 +377,7 @@ pub(crate) fn nested_if_statements(checker: &mut Checker, stmt_if: &StmtIf, pare
|
|||
|
||||
// Check if the parent is already emitting a larger diagnostic including this if statement
|
||||
if let Some(Stmt::If(stmt_if)) = parent {
|
||||
if let Some((body, _range)) = nested_if_body(stmt_if) {
|
||||
if let Some((body, _range, _is_elif)) = nested_if_body(stmt_if) {
|
||||
// In addition to repeating the `nested_if_body` and `find_last_nested_if` check, we
|
||||
// also need to be the first child in the parent
|
||||
if matches!(&body[0], Stmt::If(inner) if inner == stmt_if)
|
||||
|
|
@ -400,7 +401,12 @@ pub(crate) fn nested_if_statements(checker: &mut Checker, stmt_if: &StmtIf, pare
|
|||
.comment_ranges()
|
||||
.intersects(TextRange::new(range.start(), nested_if.start()))
|
||||
{
|
||||
match fix_if::fix_nested_if_statements(checker.locator(), checker.stylist(), range) {
|
||||
match fix_if::fix_nested_if_statements(
|
||||
checker.locator(),
|
||||
checker.stylist(),
|
||||
range,
|
||||
is_elif,
|
||||
) {
|
||||
Ok(edit) => {
|
||||
if edit
|
||||
.content()
|
||||
|
|
|
|||
|
|
@ -34,6 +34,7 @@ pub(crate) fn fix_nested_if_statements(
|
|||
locator: &Locator,
|
||||
stylist: &Stylist,
|
||||
range: TextRange,
|
||||
is_elif: bool,
|
||||
) -> Result<Edit> {
|
||||
// Infer the indentation of the outer block.
|
||||
let Some(outer_indent) = whitespace::indentation(locator, &range) else {
|
||||
|
|
@ -45,7 +46,6 @@ pub(crate) fn fix_nested_if_statements(
|
|||
|
||||
// If this is an `elif`, we have to remove the `elif` keyword for now. (We'll
|
||||
// restore the `el` later on.)
|
||||
let is_elif = contents.starts_with("elif");
|
||||
let module_text = if is_elif {
|
||||
Cow::Owned(contents.replacen("elif", "if", 1))
|
||||
} else {
|
||||
|
|
|
|||
|
|
@ -332,4 +332,26 @@ SIM102.py:132:5: SIM102 [*] Use a single `if` statement instead of nested `if` s
|
|||
136 135 | print("bar")
|
||||
137 136 |
|
||||
|
||||
SIM102.py:165:5: SIM102 [*] Use a single `if` statement instead of nested `if` statements
|
||||
|
|
||||
163 | if a:
|
||||
164 | pass
|
||||
165 | elif b:
|
||||
| _____^
|
||||
166 | | if c:
|
||||
| |_____________^ SIM102
|
||||
167 | d
|
||||
|
|
||||
= help: Combine `if` statements using `and`
|
||||
|
||||
ℹ Suggested fix
|
||||
162 162 | def f():
|
||||
163 163 | if a:
|
||||
164 164 | pass
|
||||
165 |- elif b:
|
||||
166 |- if c:
|
||||
167 |- d
|
||||
165 |+ elif b and c:
|
||||
166 |+ d
|
||||
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue