Skip to content

Commit

Permalink
Fix SIM102 to handle indented elif (#6072)
Browse files Browse the repository at this point in the history
<!--
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
  • Loading branch information
harupy authored Jul 26, 2023
1 parent 16e1737 commit 77396c6
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 9 deletions.
9 changes: 9 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_simplify/SIM102.py
Original file line number Diff line number Diff line change
Expand Up @@ -156,3 +156,12 @@
if True:
if a:
pass


# SIM102
def f():
if a:
pass
elif b:
if c:
d
22 changes: 14 additions & 8 deletions crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand All @@ -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;
};

Expand All @@ -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)
Expand All @@ -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()
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff/src/rules/flake8_simplify/rules/fix_if.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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


0 comments on commit 77396c6

Please sign in to comment.