diff --git a/README.md b/README.md index 6569d225bb..925e6f490c 100644 --- a/README.md +++ b/README.md @@ -1259,7 +1259,7 @@ For more, see [flake8-simplify](https://pypi.org/project/flake8-simplify/) on Py | SIM110 | convert-loop-to-any | Use `{any}` instead of `for` loop | 🛠 | | SIM111 | convert-loop-to-all | Use `{all}` instead of `for` loop | 🛠 | | SIM112 | use-capital-environment-variables | Use capitalized environment variable `{expected}` instead of `{original}` | 🛠 | -| SIM114 | [if-with-same-arms](https://github.com/charliermarsh/ruff/blob/main/docs/rules/if-with-same-arms.md) | Combine `if` statements using `or` | | +| SIM114 | [if-with-same-arms](https://github.com/charliermarsh/ruff/blob/main/docs/rules/if-with-same-arms.md) | Combine `if` branches using logical `or` operator | | | SIM115 | open-file-with-context-handler | Use context handler for opening files | | | SIM117 | multiple-with-statements | Use a single `with` statement with multiple contexts instead of nested `with` statements | 🛠 | | SIM118 | key-in-dict | Use `{key} in {dict}` instead of `{key} in {dict}.keys()` | 🛠 | diff --git a/crates/ruff/resources/test/fixtures/flake8_simplify/SIM114.py b/crates/ruff/resources/test/fixtures/flake8_simplify/SIM114.py index 27a9dc0c41..2a4f155468 100644 --- a/crates/ruff/resources/test/fixtures/flake8_simplify/SIM114.py +++ b/crates/ruff/resources/test/fixtures/flake8_simplify/SIM114.py @@ -55,16 +55,13 @@ if ( elif 1 == 2: pass -failures = errors = skipped = disabled = 0 if result.eofs == "O": pass elif result.eofs == "S": skipped = 1 elif result.eofs == "F": - failures = 1 -elif result.eofs == "E": errors = 1 -else: +elif result.eofs == "E": errors = 1 @@ -91,3 +88,9 @@ elif b: z = 2 elif c: z = 1 + +# False negative (or arguably a different rule) +if result.eofs == "F": + errors = 1 +else: + errors = 1 diff --git a/crates/ruff/src/checkers/ast.rs b/crates/ruff/src/checkers/ast.rs index 13295219b6..7b532c7fd8 100644 --- a/crates/ruff/src/checkers/ast.rs +++ b/crates/ruff/src/checkers/ast.rs @@ -1513,7 +1513,11 @@ where ); } if self.settings.rules.enabled(&Rule::IfWithSameArms) { - flake8_simplify::rules::if_with_same_arms(self, body, orelse); + flake8_simplify::rules::if_with_same_arms( + self, + stmt, + self.current_stmt_parent().map(std::convert::Into::into), + ); } if self.settings.rules.enabled(&Rule::NeedlessBool) { flake8_simplify::rules::return_bool_condition_directly(self, stmt); diff --git a/crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs b/crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs index 31e277b8ad..9f682c3c65 100644 --- a/crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs +++ b/crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs @@ -134,7 +134,7 @@ define_violation!( impl Violation for IfWithSameArms { #[derive_message_formats] fn message(&self) -> String { - format!("Combine `if` statements using `or`") + format!("Combine `if` branches using logical `or` operator") } } @@ -497,57 +497,73 @@ pub fn use_ternary_operator(checker: &mut Checker, stmt: &Stmt, parent: Option<& checker.diagnostics.push(diagnostic); } -fn get_if_body_pairs(orelse: &[Stmt], result: &mut Vec>) { - if orelse.is_empty() { - return; - } - let mut current_vec: Vec = vec![]; - for if_line in orelse { - if let StmtKind::If { - body: orelse_body, - orelse: orelse_orelse, - .. - } = &if_line.node - { - if !current_vec.is_empty() { - result.push(current_vec.clone()); - current_vec = vec![]; - } - if !orelse_body.is_empty() { - result.push(orelse_body.clone()); - } - get_if_body_pairs(orelse_orelse, result); - } else { - current_vec.push(if_line.clone()); +fn get_if_body_pairs<'a>( + test: &'a Expr, + body: &'a [Stmt], + orelse: &'a [Stmt], +) -> Vec<(&'a Expr, &'a [Stmt])> { + let mut pairs = vec![(test, body)]; + let mut orelse = orelse; + loop { + if orelse.len() != 1 { + break; } + let StmtKind::If { test, body, orelse: orelse_orelse, .. } = &orelse[0].node else { + break; + }; + pairs.push((test, body)); + orelse = orelse_orelse; } - if !current_vec.is_empty() { - result.push(current_vec.clone()); - } + pairs } /// SIM114 -pub fn if_with_same_arms(checker: &mut Checker, body: &[Stmt], orelse: &[Stmt]) { - if orelse.is_empty() { +pub fn if_with_same_arms(checker: &mut Checker, stmt: &Stmt, parent: Option<&Stmt>) { + let StmtKind::If { test, body, orelse } = &stmt.node else { return; + }; + + // It's part of a bigger if-elif block: + // https://github.com/MartinThoma/flake8-simplify/issues/115 + if let Some(StmtKind::If { + orelse: parent_orelse, + .. + }) = parent.map(|parent| &parent.node) + { + if parent_orelse.len() == 1 && stmt == &parent_orelse[0] { + // TODO(charlie): These two cases have the same AST: + // + // if True: + // pass + // elif a: + // b = 1 + // else: + // b = 2 + // + // if True: + // pass + // else: + // if a: + // b = 1 + // else: + // b = 2 + // + // We want to flag the latter, but not the former. Right now, we flag neither. + return; + } } - // It's not all combinations because of this: - // https://github.com/MartinThoma/flake8-simplify/issues/70#issuecomment-924074984 - let mut final_stmts: Vec> = vec![body.to_vec()]; - get_if_body_pairs(orelse, &mut final_stmts); - let if_statements = final_stmts.len(); - if if_statements <= 1 { - return; - } - - for i in 0..(if_statements - 1) { - if compare_body(&final_stmts[i], &final_stmts[i + 1]) { - let first = &final_stmts[i].first().unwrap(); - let last = &final_stmts[i].last().unwrap(); + let if_body_pairs = get_if_body_pairs(test, body, orelse); + for i in 0..(if_body_pairs.len() - 1) { + let (test, body) = &if_body_pairs[i]; + let (.., next_body) = &if_body_pairs[i + 1]; + if compare_body(body, next_body) { checker.diagnostics.push(Diagnostic::new( IfWithSameArms, - Range::new(first.location, last.end_location.unwrap()), + Range::new( + if i == 0 { stmt.location } else { test.location }, + next_body.last().unwrap().end_location.unwrap(), + ), )); } } diff --git a/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM114_SIM114.py.snap b/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM114_SIM114.py.snap index 695f966d26..ee04790b30 100644 --- a/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM114_SIM114.py.snap +++ b/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM114_SIM114.py.snap @@ -5,30 +5,40 @@ expression: diagnostics - kind: IfWithSameArms: ~ location: - row: 3 - column: 4 + row: 2 + column: 0 end_location: - row: 3 + row: 5 column: 5 fix: ~ parent: ~ - kind: IfWithSameArms: ~ location: - row: 8 - column: 4 + row: 7 + column: 0 end_location: - row: 9 + row: 12 column: 22 fix: ~ parent: ~ - kind: IfWithSameArms: ~ location: - row: 15 - column: 4 + row: 14 + column: 0 end_location: - row: 17 + row: 21 + column: 26 + fix: ~ + parent: ~ +- kind: + IfWithSameArms: ~ + location: + row: 23 + column: 0 + end_location: + row: 36 column: 26 fix: ~ parent: ~ @@ -45,70 +55,30 @@ expression: diagnostics - kind: IfWithSameArms: ~ location: - row: 25 - column: 8 + row: 31 + column: 4 end_location: - row: 26 + row: 36 column: 26 fix: ~ parent: ~ - kind: IfWithSameArms: ~ location: - row: 32 - column: 8 + row: 38 + column: 0 end_location: - row: 33 - column: 26 - fix: ~ - parent: ~ -- kind: - IfWithSameArms: ~ - location: - row: 54 - column: 4 - end_location: - row: 54 + row: 56 column: 8 fix: ~ parent: ~ - kind: IfWithSameArms: ~ location: - row: 66 - column: 4 + row: 62 + column: 5 end_location: - row: 66 - column: 14 - fix: ~ - parent: ~ -- kind: - IfWithSameArms: ~ - location: - row: 66 - column: 4 - end_location: - row: 66 - column: 14 - fix: ~ - parent: ~ -- kind: - IfWithSameArms: ~ - location: - row: 66 - column: 4 - end_location: - row: 66 - column: 14 - fix: ~ - parent: ~ -- kind: - IfWithSameArms: ~ - location: - row: 66 - column: 4 - end_location: - row: 66 + row: 65 column: 14 fix: ~ parent: ~