Avoid duplicates in if-with-same-arms (#2827)

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()` | 🛠 |

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

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);

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<Vec<Stmt>>) {
-    if orelse.is_empty() {
-        return;
-    }
-    let mut current_vec: Vec<Stmt> = 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<Stmt>> = 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(),
+                ),
             ));
         }
     }

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: ~