[flake8-simplify] Preserve operand order in SIM109 fix

crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM109.py

@@ -11,9 +11,19 @@
     d
 
 # SIM109
+# Known limitation (see #18945): the unmatched operand falls *between* the
+# two merged comparisons, so it's moved after the merged `in` comparison
+# instead of staying in its original position.
 if a == b or None or a == c:
     d
 
+# SIM109
+# Regression test for #18945: the unmatched operand precedes both merged
+# comparisons, so it must stay first in the fix instead of being moved
+# after the merged `in` comparison.
+if None or a == b or a == c:
+    d
+
 # OK
 if a in (b, c):
     d

crates/ruff_linter/src/rules/flake8_simplify/rules/ast_bool_op.rs

@@ -91,6 +91,15 @@ impl Violation for DuplicateIsinstanceCall {
 ///     ...
 /// ```
 ///
+/// ## Fix safety
+///
+/// This fix is always unsafe. It may change the value of the expression if any of the
+/// comparators have side effects, and, for expressions that mix equality comparisons with
+/// other operands, it can change the order in which operands are evaluated. Ruff preserves
+/// the original order when an operand falls before or after the group of merged comparisons,
+/// but if an unmatched operand falls *between* two merged comparisons (e.g. `foo == x or bar
+/// or foo == y`), it's moved after the merged `in` comparison.
+///
 /// ## References
 /// - [Python documentation: Membership test operations](https://docs.python.org/3/reference/expressions.html#membership-test-operations)
 #[derive(ViolationMetadata)]
@@ -516,6 +525,16 @@ pub(crate) fn compare_with_tuple(checker: &Checker, expr: &Expr) {
             continue;
         }
 
+        // Anchor the replacement expression to the position of the earliest matched
+        // comparator, so that it sorts correctly against any unmatched operands below.
+        let Some(node_range) = comparators
+            .iter()
+            .map(Ranged::range)
+            .min_by_key(Ranged::start)
+        else {
+            continue;
+        };
+
         // Create a `x in (a, b)` expression.
         let node = ast::ExprTuple {
             elts: comparators.into_iter().cloned().collect(),
@@ -533,7 +552,7 @@ pub(crate) fn compare_with_tuple(checker: &Checker, expr: &Expr) {
         let node2 = ast::ExprCompare {
             ops: [CmpOp::In].into(),
             operands: Box::from([node1.into(), node.into()]),
-            range: TextRange::default(),
+            range: node_range,
             node_index: ruff_python_ast::AtomicNodeIndex::NONE,
         };
         let in_expr = node2.into();
@@ -552,10 +571,14 @@ pub(crate) fn compare_with_tuple(checker: &Checker, expr: &Expr) {
         let in_expr = if unmatched.is_empty() {
             in_expr
         } else {
-            // Wrap in a `x in (a, b) or ...` boolean operation.
+            // Wrap in a `x in (a, b) or ...` boolean operation, preserving the original
+            // left-to-right order of the replacement and any unmatched operands.
             let node = ast::ExprBoolOp {
                 op: BoolOp::Or,
-                values: iter::once(in_expr).chain(unmatched).collect(),
+                values: iter::once(in_expr)
+                    .chain(unmatched)
+                    .sorted_by_key(Ranged::start)
+                    .collect(),
                 range: TextRange::default(),
                 node_index: ruff_python_ast::AtomicNodeIndex::NONE,
             };

crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__compare-with-tuple_SIM109.py.snap

@@ -53,18 +53,37 @@ help: Replace with `a in (b, c)`
 note: This is an unsafe fix and may change runtime behavior
 
 SIM109 [*] Use `a in (b, c)` instead of multiple equality comparisons
-  --> SIM109.py:14:4
+  --> SIM109.py:17:4
    |
-13 | # SIM109
-14 | if a == b or None or a == c:
+15 | # two merged comparisons, so it's moved after the merged `in` comparison
+16 | # instead of staying in its original position.
+17 | if a == b or None or a == c:
    |    ^^^^^^^^^^^^^^^^^^^^^^^^
-15 |     d
+18 |     d
    |
 help: Replace with `a in (b, c)`
    |
-13 | # SIM109
+16 | # instead of staying in its original position.
    - if a == b or None or a == c:
-14 + if a in (b, c) or None:
-15 |     d
+17 + if a in (b, c) or None:
+18 |     d
+   |
+note: This is an unsafe fix and may change runtime behavior
+
+SIM109 [*] Use `a in (b, c)` instead of multiple equality comparisons
+  --> SIM109.py:24:4
+   |
+22 | # comparisons, so it must stay first in the fix instead of being moved
+23 | # after the merged `in` comparison.
+24 | if None or a == b or a == c:
+   |    ^^^^^^^^^^^^^^^^^^^^^^^^
+25 |     d
+   |
+help: Replace with `a in (b, c)`
+   |
+23 | # after the merged `in` comparison.
+   - if None or a == b or a == c:
+24 + if None or a in (b, c):
+25 |     d
    |
 note: This is an unsafe fix and may change runtime behavior