mirror of
https://github.com/astral-sh/ruff.git
synced 2025-09-26 11:59:35 +00:00
De-duplicate SIM102 (#2050)
The idea is the same as #1867. Avoids emitting `SIM102` twice for the following code: ```python if a: if b: if c: d ``` ``` resources/test/fixtures/flake8_simplify/SIM102.py:1:1: SIM102 Use a single `if` statement instead of nested `if` statements resources/test/fixtures/flake8_simplify/SIM102.py:2:5: SIM102 Use a single `if` statement instead of nested `if` statements ```
This commit is contained in:
parent
8e558a3458
commit
eb1b5e5454
4 changed files with 118 additions and 61 deletions
|
@ -3,6 +3,12 @@ if a:
|
|||
if b:
|
||||
c
|
||||
|
||||
# SIM102
|
||||
if a:
|
||||
if b:
|
||||
if c:
|
||||
d
|
||||
|
||||
# SIM102
|
||||
if a:
|
||||
pass
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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.
|
||||
|
|
|
@ -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: ~
|
||||
|
||||
|
|
Loading…
Add table
Add a link
Reference in a new issue