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.

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

Fix SIM102 to handle indented elif #6072

Merged
merged 1 commit into from
Jul 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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