Fix issues with SIM101 (adjacent isinstance() calls) (#7798)

- Only trigger for immediately adjacent isinstance() calls with the same target - Preserve order of or conditions Two existing tests changed: - One was incorrectly reordering the or conditions, and is now correct. - Another was combining two non-adjacent isinstance() calls. It's safe enough in that example, but this isn't safe to do in general, and it feels low-value to come up with a heuristic for when it is safe, so it seems better to not combine the calls in that case. Fixes #7797
astral-sh · Oct 4, 2023 · 7b4fb4f · 7b4fb4f
1 parent 5d49d26
commit 7b4fb4f
Show file tree

Hide file tree

Showing 3 changed files with 112 additions and 68 deletions.
diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM101.py b/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

diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_bool_op.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_bool_op.rs
@@ -5,14 +5,14 @@ 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};
 use ruff_python_ast::comparable::ComparableExpr;
 use ruff_python_ast::helpers::{contains_effect, Truthiness};
 use ruff_python_ast::parenthesize::parenthesized_range;
 use ruff_python_codegen::Generator;
+use ruff_python_semantic::SemanticModel;
 
 use crate::checkers::ast::Checker;
 use crate::registry::AsRule;
@@ -299,6 +299,42 @@ fn is_same_expr<'a>(a: &'a Expr, b: &'a Expr) -> Option<&'a str> {
     None
 }
 
+/// If `call` is an `isinstance()` call, return its target.
+fn isinstance_target<'a>(call: &'a Expr, semantic: &'a SemanticModel) -> Option<&'a Expr> {
+    // Verify that this is an `isinstance` call.
+    let Expr::Call(ast::ExprCall {
+        func,
+        arguments:
+            Arguments {
+                args,
+                keywords,
+                range: _,
+            },
+        range: _,
+    }) = &call
+    else {
+        return None;
+    };
+    if args.len() != 2 {
+        return None;
+    }
+    if !keywords.is_empty() {
+        return None;
+    }
+    let Expr::Name(ast::ExprName { id: func_name, .. }) = func.as_ref() else {
+        return None;
+    };
+    if func_name != "isinstance" {
+        return None;
+    }
+    if !semantic.is_builtin("isinstance") {
+        return None;
+    }
+
+    // Collect the target (e.g., `obj` in `isinstance(obj, int)`).
+    Some(&args[0])
+}
+
 /// SIM101
 pub(crate) fn duplicate_isinstance_call(checker: &mut Checker, expr: &Expr) {
     let Expr::BoolOp(ast::ExprBoolOp {
@@ -310,50 +346,32 @@ 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 {
-            func,
-            arguments:
-                Arguments {
-                    args,
-                    keywords,
-                    range: _,
-                },
-            range: _,
-        }) = &call
-        else {
+        let Some(target) = isinstance_target(call, checker.semantic()) else {
+            last_target_option = None;
             continue;
         };
-        if args.len() != 2 {
-            continue;
-        }
-        if !keywords.is_empty() {
-            continue;
-        }
-        let Expr::Name(ast::ExprName { id: func_name, .. }) = func.as_ref() else {
-            continue;
-        };
-        if func_name != "isinstance" {
-            continue;
-        }
-        if !checker.semantic().is_builtin("isinstance") {
-            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 +447,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();

diff --git a/...ake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM101_SIM101.py.snap b/...ake8_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():
+