Fix issues with SIM101 (adjacent isinstance() calls)

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

@@ -31,6 +31,15 @@
 if isinstance(a, int) or isinstance(a.b, float):
     pass
 
+# OK
+if isinstance(a, int) or unrelated_condition or isinstance(a, float):
+    pass
+
+if x or isinstance(a, int) or isinstance(a, float):
+    pass
+
+if x or y or isinstance(a, int) or isinstance(a, float) or z:
+    pass
 
 def f():
     # OK

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

@@ -5,7 +5,6 @@ use itertools::Either::{Left, Right};
 use itertools::Itertools;
 use ruff_python_ast::{self as ast, Arguments, BoolOp, CmpOp, Expr, ExprContext, UnaryOp};
 use ruff_text_size::{Ranged, TextRange};
-use rustc_hash::FxHashMap;
 
 use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix, FixAvailability, Violation};
 use ruff_macros::{derive_message_formats, violation};
@@ -310,9 +309,10 @@ pub(crate) fn duplicate_isinstance_call(checker: &mut Checker, expr: &Expr) {
         return;
     };
 
-    // Locate duplicate `isinstance` calls, represented as a map from `ComparableExpr`
-    // to indices of the relevant `Expr` instances in `values`.
-    let mut duplicates: FxHashMap<ComparableExpr, Vec<usize>> = FxHashMap::default();
+    // Locate duplicate `isinstance` calls, represented as a vector of vectors
+    // of indices of the relevant `Expr` instances in `values`.
+    let mut duplicates: Vec<Vec<usize>> = Vec::new();
+    let mut last_target_option: Option<ComparableExpr> = None;
     for (index, call) in values.iter().enumerate() {
         // Verify that this is an `isinstance` call.
         let Expr::Call(ast::ExprCall {
@@ -326,34 +326,48 @@ pub(crate) fn duplicate_isinstance_call(checker: &mut Checker, expr: &Expr) {
             range: _,
         }) = &call
         else {
+            last_target_option = None;
             continue;
         };
         if args.len() != 2 {
+            last_target_option = None;
             continue;
         }
         if !keywords.is_empty() {
+            last_target_option = None;
             continue;
         }
         let Expr::Name(ast::ExprName { id: func_name, .. }) = func.as_ref() else {
+            last_target_option = None;
             continue;
         };
         if func_name != "isinstance" {
+            last_target_option = None;
             continue;
         }
         if !checker.semantic().is_builtin("isinstance") {
+            last_target_option = None;
             continue;
         }
 
         // Collect the target (e.g., `obj` in `isinstance(obj, int)`).
         let target = &args[0];
-        duplicates
-            .entry(target.into())
-            .or_insert_with(Vec::new)
-            .push(index);
+        if last_target_option
+            .as_ref()
+            .is_some_and(|last_target| *last_target == ComparableExpr::from(target))
+        {
+            duplicates
+                .last_mut()
+                .expect("last_target should have a corresponding entry")
+                .push(index);
+        } else {
+            last_target_option = Some(target.into());
+            duplicates.push(vec![index]);
+        }
     }
 
     // Generate a `Diagnostic` for each duplicate.
-    for indices in duplicates.values() {
+    for indices in duplicates {
         if indices.len() > 1 {
             // Grab the target used in each duplicate `isinstance` call (e.g., `obj` in
             // `isinstance(obj, int)`).
@@ -429,17 +443,14 @@ pub(crate) fn duplicate_isinstance_call(checker: &mut Checker, expr: &Expr) {
                     let call = node2.into();
 
                     // Generate the combined `BoolOp`.
+                    let [first, .., last] = indices.as_slice() else {
+                        unreachable!("Indices should have at least two elements")
+                    };
+                    let before = values.iter().take(*first).cloned();
+                    let after = values.iter().skip(last + 1).cloned();
                     let node = ast::ExprBoolOp {
                         op: BoolOp::Or,
-                        values: iter::once(call)
-                            .chain(
-                                values
-                                    .iter()
-                                    .enumerate()
-                                    .filter(|(index, _)| !indices.contains(index))
-                                    .map(|(_, elt)| elt.clone()),
-                            )
-                            .collect(),
+                        values: before.chain(iter::once(call)).chain(after).collect(),
                         range: TextRange::default(),
                     };
                     let bool_op = node.into();

crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM101_SIM101.py.snap

@@ -71,31 +71,11 @@ SIM101.py:10:4: SIM101 [*] Multiple `isinstance` calls for `a`, merge into a sin
 8  8  |     pass
 9  9  | 
 10    |-if isinstance(b, bool) or isinstance(a, int) or isinstance(a, float):  # SIM101
-   10 |+if isinstance(a, (int, float)) or isinstance(b, bool):  # SIM101
+   10 |+if isinstance(b, bool) or isinstance(a, (int, float)):  # SIM101
 11 11 |     pass
 12 12 | 
 13 13 | if isinstance(a, int) or isinstance(b, bool) or isinstance(a, float):  # SIM101
 
-SIM101.py:13:4: SIM101 [*] Multiple `isinstance` calls for `a`, merge into a single call
-   |
-11 |     pass
-12 | 
-13 | if isinstance(a, int) or isinstance(b, bool) or isinstance(a, float):  # SIM101
-   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SIM101
-14 |     pass
-   |
-   = help: Merge `isinstance` calls for `a`
-
-ℹ Suggested fix
-10 10 | if isinstance(b, bool) or isinstance(a, int) or isinstance(a, float):  # SIM101
-11 11 |     pass
-12 12 | 
-13    |-if isinstance(a, int) or isinstance(b, bool) or isinstance(a, float):  # SIM101
-   13 |+if isinstance(a, (int, float)) or isinstance(b, bool):  # SIM101
-14 14 |     pass
-15 15 | 
-16 16 | if (isinstance(a, int) or isinstance(a, float)) and isinstance(b, bool):  # SIM101
-
 SIM101.py:16:5: SIM101 [*] Multiple `isinstance` calls for `a`, merge into a single call
    |
 14 |     pass
@@ -146,4 +126,44 @@ SIM101.py:22:4: SIM101 Multiple `isinstance` calls for expression, merge into a
    |
    = help: Merge `isinstance` calls
 
+SIM101.py:38:4: SIM101 [*] Multiple `isinstance` calls for `a`, merge into a single call
+   |
+36 |     pass
+37 | 
+38 | if x or isinstance(a, int) or isinstance(a, float):
+   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SIM101
+39 |     pass
+   |
+   = help: Merge `isinstance` calls for `a`
+
+ℹ Suggested fix
+35 35 | if isinstance(a, int) or unrelated_condition or isinstance(a, float):
+36 36 |     pass
+37 37 | 
+38    |-if x or isinstance(a, int) or isinstance(a, float):
+   38 |+if x or isinstance(a, (int, float)):
+39 39 |     pass
+40 40 | 
+41 41 | if x or y or isinstance(a, int) or isinstance(a, float) or z:
+
+SIM101.py:41:4: SIM101 [*] Multiple `isinstance` calls for `a`, merge into a single call
+   |
+39 |     pass
+40 | 
+41 | if x or y or isinstance(a, int) or isinstance(a, float) or z:
+   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SIM101
+42 |     pass
+   |
+   = help: Merge `isinstance` calls for `a`
+
+ℹ Suggested fix
+38 38 | if x or isinstance(a, int) or isinstance(a, float):
+39 39 |     pass
+40 40 | 
+41    |-if x or y or isinstance(a, int) or isinstance(a, float) or z:
+   41 |+if x or y or isinstance(a, (int, float)) or z:
+42 42 |     pass
+43 43 | 
+44 44 | def f():
+