diff --git a/resources/test/fixtures/flake8_simplify/SIM102.py b/resources/test/fixtures/flake8_simplify/SIM102.py index d28cabfb0d..79c5e9a359 100644 --- a/resources/test/fixtures/flake8_simplify/SIM102.py +++ b/resources/test/fixtures/flake8_simplify/SIM102.py @@ -3,6 +3,12 @@ if a: if b: c +# SIM102 +if a: + if b: + if c: + d + # SIM102 if a: pass diff --git a/src/checkers/ast.rs b/src/checkers/ast.rs index 47c779e74a..221fe367c8 100644 --- a/src/checkers/ast.rs +++ b/src/checkers/ast.rs @@ -1359,7 +1359,13 @@ where flake8_type_checking::rules::empty_type_checking_block(self, test, body); } if self.settings.rules.enabled(&Rule::NestedIfStatements) { - flake8_simplify::rules::nested_if_statements(self, stmt); + flake8_simplify::rules::nested_if_statements( + self, + stmt, + test, + body, + self.current_stmt_parent().map(Into::into), + ); } if self .settings diff --git a/src/rules/flake8_simplify/rules/ast_if.rs b/src/rules/flake8_simplify/rules/ast_if.rs index 495164591a..dc21410a52 100644 --- a/src/rules/flake8_simplify/rules/ast_if.rs +++ b/src/rules/flake8_simplify/rules/ast_if.rs @@ -3,8 +3,8 @@ use rustpython_ast::{Cmpop, Constant, Expr, ExprContext, ExprKind, Stmt, StmtKin use crate::ast::comparable::ComparableExpr; use crate::ast::helpers::{ - contains_call_path, contains_effect, create_expr, create_stmt, has_comments_in, unparse_expr, - unparse_stmt, + contains_call_path, contains_effect, create_expr, create_stmt, first_colon_range, + has_comments_in, unparse_expr, unparse_stmt, }; use crate::ast::types::Range; use crate::checkers::ast::Checker; @@ -37,36 +37,64 @@ fn is_main_check(expr: &Expr) -> bool { false } +/// Find the last nested if statement and return the test expression and the +/// first statement. +/// +/// ```python +/// if xxx: +/// if yyy: +/// # ^^^ returns this expression +/// z = 1 +/// # ^^^^^ and this statement +/// ... +/// ``` +fn find_last_nested_if(body: &[Stmt]) -> Option<(&Expr, &Stmt)> { + let [Stmt { node: StmtKind::If { test, body: inner_body, orelse }, ..}] = body else { return None }; + if !(orelse.is_empty() && body.len() == 1) { + return None; + } + find_last_nested_if(inner_body).or_else(|| { + Some(( + test, + inner_body.last().expect("Expected body to be non-empty"), + )) + }) +} + /// SIM102 -pub fn nested_if_statements(checker: &mut Checker, stmt: &Stmt) { - let StmtKind::If { test, body, orelse } = &stmt.node else { - return; - }; - - // if a: <--- - // if b: <--- - // c - let is_nested_if = { - if orelse.is_empty() && body.len() == 1 { - if let StmtKind::If { orelse, .. } = &body[0].node { - orelse.is_empty() - } else { - false - } - } else { - false - } - }; - - if !is_nested_if { - return; - }; - +pub fn nested_if_statements( + checker: &mut Checker, + stmt: &Stmt, + test: &Expr, + body: &[Stmt], + parent: Option<&Stmt>, +) { if is_main_check(test) { return; } - let mut diagnostic = Diagnostic::new(violations::NestedIfStatements, Range::from_located(stmt)); + if let Some(parent) = parent { + if let StmtKind::If { body, orelse, .. } = &parent.node { + if orelse.is_empty() && body.len() == 1 { + return; + } + } + } + + let Some((test, first_stmt)) = find_last_nested_if(body) else { + return; + }; + let colon = first_colon_range( + Range::new(test.end_location.unwrap(), first_stmt.location), + checker.locator, + ); + let mut diagnostic = Diagnostic::new( + violations::NestedIfStatements, + colon.map_or_else( + || Range::from_located(stmt), + |colon| Range::new(stmt.location, colon.end_location), + ), + ); if checker.patch(&Rule::NestedIfStatements) { // The fixer preserves comments in the nested body, but removes comments between // the outer and inner if statements. diff --git a/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM102_SIM102.py.snap b/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM102_SIM102.py.snap index debf360ea7..bb64580020 100644 --- a/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM102_SIM102.py.snap +++ b/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM102_SIM102.py.snap @@ -8,7 +8,7 @@ expression: diagnostics row: 2 column: 0 end_location: - row: 4 + row: 3 column: 9 fix: content: "if a and b:\n c\n" @@ -22,29 +22,36 @@ expression: diagnostics - kind: NestedIfStatements: ~ location: - row: 9 + row: 7 column: 0 end_location: - row: 11 - column: 9 + row: 9 + column: 13 fix: - content: "elif b and c:\n d\n" + content: "if a and b:\n if c:\n d\n" location: - row: 9 + row: 7 column: 0 end_location: - row: 12 + row: 11 column: 0 parent: ~ - kind: NestedIfStatements: ~ location: - row: 14 + row: 15 column: 0 end_location: - row: 17 + row: 16 column: 9 - fix: ~ + fix: + content: "elif b and c:\n d\n" + location: + row: 15 + column: 0 + end_location: + row: 18 + column: 0 parent: ~ - kind: NestedIfStatements: ~ @@ -52,83 +59,93 @@ expression: diagnostics row: 20 column: 0 end_location: - row: 23 + row: 22 + column: 9 + fix: ~ + parent: ~ +- kind: + NestedIfStatements: ~ + location: + row: 26 + column: 0 + end_location: + row: 27 column: 9 fix: content: "if a and b:\n # Fixable due to placement of this comment.\n c\n" location: - row: 20 + row: 26 column: 0 end_location: - row: 24 + row: 30 column: 0 parent: ~ - kind: NestedIfStatements: ~ location: - row: 45 + row: 51 column: 4 end_location: - row: 57 - column: 23 + row: 52 + column: 16 fix: content: " if True and True:\n \"\"\"this\nis valid\"\"\"\n\n \"\"\"the indentation on\n this line is significant\"\"\"\n\n \"this is\" \\\n\"allowed too\"\n\n (\"so is\"\n\"this for some reason\")\n" location: - row: 45 + row: 51 column: 0 end_location: - row: 58 + row: 64 column: 0 parent: ~ - kind: NestedIfStatements: ~ location: - row: 61 + row: 67 column: 0 end_location: - row: 73 - column: 23 + row: 68 + column: 12 fix: content: "if True and True:\n \"\"\"this\nis valid\"\"\"\n\n \"\"\"the indentation on\n this line is significant\"\"\"\n\n \"this is\" \\\n\"allowed too\"\n\n (\"so is\"\n\"this for some reason\")\n" location: - row: 61 + row: 67 column: 0 end_location: - row: 74 + row: 80 column: 0 parent: ~ - kind: NestedIfStatements: ~ location: - row: 77 + row: 83 column: 4 end_location: - row: 81 - column: 32 + row: 86 + column: 10 fix: content: " if node.module and (node.module == \"multiprocessing\" or node.module.startswith(\n \"multiprocessing.\"\n )):\n print(\"Bad module!\")\n" location: - row: 77 + row: 83 column: 0 end_location: - row: 82 + row: 88 column: 0 parent: ~ - kind: NestedIfStatements: ~ location: - row: 84 + row: 90 column: 0 end_location: - row: 88 - column: 28 + row: 93 + column: 6 fix: content: "if node.module and (node.module == \"multiprocessing\" or node.module.startswith(\n \"multiprocessing.\"\n)):\n print(\"Bad module!\")\n" location: - row: 84 + row: 90 column: 0 end_location: - row: 89 + row: 95 column: 0 parent: ~