Implement duplicate isinstance detection (SIM101) (#1673)

See: #998.

README.md

@@ -967,6 +967,7 @@ For more, see [flake8-simplify](https://pypi.org/project/flake8-simplify/0.19.3/
 
 | Code | Name | Message | Fix |
 | ---- | ---- | ------- | --- |
+| SIM101 | DuplicateIsinstanceCall | Multiple `isinstance` calls for `...`, merge into a single call | 🛠 |
 | SIM102 | NestedIfStatements | Use a single `if` statement instead of nested `if` statements |  |
 | SIM105 | UseContextlibSuppress | Use `contextlib.suppress(...)` instead of try-except-pass |  |
 | SIM107 | ReturnInTryExceptFinally | Don't use `return` in `try`/`except` and `finally` |  |

resources/test/fixtures/flake8_simplify/SIM101.py

@@ -0,0 +1,23 @@
+if isinstance(a, int) or isinstance(a, float):  # SIM101
+    pass
+
+if isinstance(a, (int, float)) or isinstance(a, bool):  # SIM101
+    pass
+
+if isinstance(a, int) or isinstance(a, float) or isinstance(b, bool):  # SIM101
+    pass
+
+if isinstance(b, bool) or isinstance(a, int) or isinstance(a, float):  # SIM101
+    pass
+
+if isinstance(a, int) or isinstance(b, bool) or isinstance(a, float):  # SIM101
+    pass
+
+if (isinstance(a, int) or isinstance(a, float)) and isinstance(b, bool):  # SIM101
+    pass
+
+if isinstance(a, int) and isinstance(b, bool) or isinstance(a, float):
+    pass
+
+if isinstance(a, bool) or isinstance(b, str):
+    pass

ruff.schema.json

@@ -936,6 +936,7 @@
         "SIM",
         "SIM1",
         "SIM10",
+        "SIM101",
         "SIM102",
         "SIM105",
         "SIM107",

src/checkers/ast.rs

@@ -2710,6 +2710,9 @@ where
                 if self.settings.enabled.contains(&CheckCode::PLR1701) {
                     pylint::plugins::merge_isinstance(self, expr, op, values);
                 }
+                if self.settings.enabled.contains(&CheckCode::SIM101) {
+                    flake8_simplify::plugins::duplicate_isinstance_call(self, expr);
+                }
                 if self.settings.enabled.contains(&CheckCode::SIM220) {
                     flake8_simplify::plugins::a_and_not_a(self, expr);
                 }

src/flake8_simplify/mod.rs

@@ -12,6 +12,7 @@ mod tests {
     use crate::registry::CheckCode;
     use crate::settings;
 
+    #[test_case(CheckCode::SIM101, Path::new("SIM101.py"); "SIM101")]
     #[test_case(CheckCode::SIM102, Path::new("SIM102.py"); "SIM102")]
     #[test_case(CheckCode::SIM105, Path::new("SIM105.py"); "SIM105")]
     #[test_case(CheckCode::SIM107, Path::new("SIM107.py"); "SIM107")]

src/flake8_simplify/plugins/ast_bool_op.rs

@@ -1,10 +1,18 @@
-use rustpython_ast::{Boolop, Constant, Expr, ExprKind, Unaryop};
+use std::iter;
 
+use itertools::Either::{Left, Right};
+use rustc_hash::FxHashMap;
+use rustpython_ast::{Boolop, Constant, Expr, ExprContext, ExprKind, Unaryop};
+
+use crate::ast::helpers::create_expr;
 use crate::ast::types::Range;
 use crate::autofix::Fix;
 use crate::checkers::ast::Checker;
 use crate::registry::{Check, CheckCode, CheckKind};
+use crate::source_code_generator::SourceCodeGenerator;
+use crate::source_code_style::SourceCodeStyleDetector;
 
+/// Return `true` if two `Expr` instances are equivalent names.
 fn is_same_expr<'a>(a: &'a Expr, b: &'a Expr) -> Option<&'a str> {
     if let (ExprKind::Name { id: a, .. }, ExprKind::Name { id: b, .. }) = (&a.node, &b.node) {
         if a == b {
@@ -14,6 +22,132 @@ fn is_same_expr<'a>(a: &'a Expr, b: &'a Expr) -> Option<&'a str> {
     None
 }
 
+/// Generate source code from an `Expr`.
+fn to_source(expr: &Expr, stylist: &SourceCodeStyleDetector) -> String {
+    let mut generator = SourceCodeGenerator::new(
+        stylist.indentation(),
+        stylist.quote(),
+        stylist.line_ending(),
+    );
+    generator.unparse_expr(expr, 0);
+    generator.generate().unwrap()
+}
+
+/// SIM101
+pub fn duplicate_isinstance_call(checker: &mut Checker, expr: &Expr) {
+    let ExprKind::BoolOp { op: Boolop::Or, values } = &expr.node else {
+        return;
+    };
+
+    // Locate duplicate `isinstance` calls, represented as a map from argument name to indices
+    // of the relevant `Expr` instances in `values`.
+    let mut duplicates = FxHashMap::default();
+    for (index, call) in values.iter().enumerate() {
+        // Verify that this is an `isinstance` call.
+        let ExprKind::Call { func, args, keywords } = &call.node else {
+            continue;
+        };
+        if args.len() != 2 {
+            continue;
+        }
+        if !keywords.is_empty() {
+            continue;
+        }
+        let ExprKind::Name { id: func_name, .. } = &func.node else {
+            continue;
+        };
+        if func_name != "isinstance" {
+            continue;
+        }
+
+        // Collect the name of the argument.
+        let ExprKind::Name { id: arg_name, .. } = &args[0].node else {
+            continue;
+        };
+        duplicates
+            .entry(arg_name.as_str())
+            .or_insert_with(Vec::new)
+            .push(index);
+    }
+
+    // Generate a `Check` for each duplicate.
+    for (arg_name, indices) in duplicates {
+        if indices.len() > 1 {
+            let mut check = Check::new(
+                CheckKind::DuplicateIsinstanceCall(arg_name.to_string()),
+                Range::from_located(expr),
+            );
+            if checker.patch(&CheckCode::SIM101) {
+                // Grab the types used in each duplicate `isinstance` call.
+                let types: Vec<&Expr> = indices
+                    .iter()
+                    .map(|index| &values[*index])
+                    .map(|expr| {
+                        let ExprKind::Call { args, ..} = &expr.node else {
+                            unreachable!("Indices should only contain `isinstance` calls")
+                        };
+                        args.get(1).expect("`isinstance` should have two arguments")
+                    })
+                    .collect();
+
+                // Flatten all the types used across the `isinstance` cals.
+                let elts: Vec<&Expr> = types
+                    .iter()
+                    .flat_map(|value| {
+                        if let ExprKind::Tuple { elts, .. } = &value.node {
+                            Left(elts.iter())
+                        } else {
+                            Right(iter::once(*value))
+                        }
+                    })
+                    .collect();
+
+                // Generate a single `isinstance` call.
+                let call = create_expr(ExprKind::Call {
+                    func: Box::new(create_expr(ExprKind::Name {
+                        id: "isinstance".to_string(),
+                        ctx: ExprContext::Load,
+                    })),
+                    args: vec![
+                        create_expr(ExprKind::Name {
+                            id: arg_name.to_string(),
+                            ctx: ExprContext::Load,
+                        }),
+                        create_expr(ExprKind::Tuple {
+                            elts: elts.into_iter().map(Clone::clone).collect(),
+                            ctx: ExprContext::Load,
+                        }),
+                    ],
+                    keywords: vec![],
+                });
+
+                // Generate the combined `BoolOp`.
+                let bool_op = create_expr(ExprKind::BoolOp {
+                    op: Boolop::Or,
+                    values: iter::once(call)
+                        .chain(
+                            values
+                                .iter()
+                                .enumerate()
+                                .filter(|(index, _)| !indices.contains(index))
+                                .map(|(_, elt)| elt.clone()),
+                        )
+                        .collect(),
+                });
+
+                // Populate the `Fix`. Replace the _entire_ `BoolOp`. Note that if we have
+                // multiple duplicates, the fixes will conflict.
+                check.amend(Fix::replacement(
+                    to_source(&bool_op, checker.style),
+                    expr.location,
+                    expr.end_location.unwrap(),
+                ));
+            }
+            checker.add_check(check);
+        }
+    }
+}
+
 /// SIM220
 pub fn a_and_not_a(checker: &mut Checker, expr: &Expr) {
     let ExprKind::BoolOp { op: Boolop::And, values, } = &expr.node else {

src/flake8_simplify/plugins/mod.rs

@@ -1,4 +1,4 @@
-pub use ast_bool_op::{a_and_not_a, a_or_not_a, and_false, or_true};
+pub use ast_bool_op::{a_and_not_a, a_or_not_a, and_false, duplicate_isinstance_call, or_true};
 pub use ast_for::convert_loop_to_any_all;
 pub use ast_if::nested_if_statements;
 pub use ast_with::multiple_with_statements;

src/flake8_simplify/snapshots/ruff__flake8_simplify__tests__SIM101_SIM101.py.snap

@@ -0,0 +1,107 @@
+---
+source: src/flake8_simplify/mod.rs
+expression: checks
+---
+- kind:
+    DuplicateIsinstanceCall: a
+  location:
+    row: 1
+    column: 3
+  end_location:
+    row: 1
+    column: 45
+  fix:
+    content: "isinstance(a, (int, float))"
+    location:
+      row: 1
+      column: 3
+    end_location:
+      row: 1
+      column: 45
+  parent: ~
+- kind:
+    DuplicateIsinstanceCall: a
+  location:
+    row: 4
+    column: 3
+  end_location:
+    row: 4
+    column: 53
+  fix:
+    content: "isinstance(a, (int, float, bool))"
+    location:
+      row: 4
+      column: 3
+    end_location:
+      row: 4
+      column: 53
+  parent: ~
+- kind:
+    DuplicateIsinstanceCall: a
+  location:
+    row: 7
+    column: 3
+  end_location:
+    row: 7
+    column: 68
+  fix:
+    content: "isinstance(a, (int, float)) or isinstance(b, bool)"
+    location:
+      row: 7
+      column: 3
+    end_location:
+      row: 7
+      column: 68
+  parent: ~
+- kind:
+    DuplicateIsinstanceCall: a
+  location:
+    row: 10
+    column: 3
+  end_location:
+    row: 10
+    column: 68
+  fix:
+    content: "isinstance(a, (int, float)) or isinstance(b, bool)"
+    location:
+      row: 10
+      column: 3
+    end_location:
+      row: 10
+      column: 68
+  parent: ~
+- kind:
+    DuplicateIsinstanceCall: a
+  location:
+    row: 13
+    column: 3
+  end_location:
+    row: 13
+    column: 68
+  fix:
+    content: "isinstance(a, (int, float)) or isinstance(b, bool)"
+    location:
+      row: 13
+      column: 3
+    end_location:
+      row: 13
+      column: 68
+  parent: ~
+- kind:
+    DuplicateIsinstanceCall: a
+  location:
+    row: 16
+    column: 4
+  end_location:
+    row: 16
+    column: 46
+  fix:
+    content: "isinstance(a, (int, float))"
+    location:
+      row: 16
+      column: 4
+    end_location:
+      row: 16
+      column: 46
+  parent: ~
+

src/registry.rs

@@ -218,6 +218,7 @@ pub enum CheckCode {
     YTT302,
     YTT303,
     // flake8-simplify
+    SIM101,
     SIM102,
     SIM105,
     SIM107,
@@ -959,6 +960,7 @@ pub enum CheckKind {
     SysVersionCmpStr10,
     SysVersionSlice1Referenced,
     // flake8-simplify
+    DuplicateIsinstanceCall(String),
     AAndNotA(String),
     AOrNotA(String),
     AndFalse,
@@ -1400,6 +1402,7 @@ impl CheckCode {
             // flake8-blind-except
             CheckCode::BLE001 => CheckKind::BlindExcept("Exception".to_string()),
             // flake8-simplify
+            CheckCode::SIM101 => CheckKind::DuplicateIsinstanceCall("...".to_string()),
             CheckCode::SIM102 => CheckKind::NestedIfStatements,
             CheckCode::SIM105 => CheckKind::UseContextlibSuppress("...".to_string()),
             CheckCode::SIM107 => CheckKind::ReturnInTryExceptFinally,
@@ -1946,6 +1949,7 @@ impl CheckCode {
             CheckCode::S324 => CheckCategory::Flake8Bandit,
             CheckCode::S506 => CheckCategory::Flake8Bandit,
             // flake8-simplify
+            CheckCode::SIM101 => CheckCategory::Flake8Simplify,
             CheckCode::SIM102 => CheckCategory::Flake8Simplify,
             CheckCode::SIM105 => CheckCategory::Flake8Simplify,
             CheckCode::SIM107 => CheckCategory::Flake8Simplify,
@@ -2204,6 +2208,7 @@ impl CheckKind {
             CheckKind::SysVersionCmpStr10 => &CheckCode::YTT302,
             CheckKind::SysVersionSlice1Referenced => &CheckCode::YTT303,
             // flake8-simplify
+            CheckKind::DuplicateIsinstanceCall(..) => &CheckCode::SIM101,
             CheckKind::AAndNotA(..) => &CheckCode::SIM220,
             CheckKind::AOrNotA(..) => &CheckCode::SIM221,
             CheckKind::AndFalse => &CheckCode::SIM223,
@@ -2972,6 +2977,9 @@ impl CheckKind {
                 "`sys.version[:1]` referenced (python10), use `sys.version_info`".to_string()
             }
             // flake8-simplify
+            CheckKind::DuplicateIsinstanceCall(name) => {
+                format!("Multiple `isinstance` calls for `{name}`, merge into a single call")
+            }
             CheckKind::UseContextlibSuppress(exception) => {
                 format!("Use `contextlib.suppress({exception})` instead of try-except-pass")
             }
@@ -3641,6 +3649,7 @@ impl CheckKind {
                 | CheckKind::DoNotAssignLambda(..)
                 | CheckKind::DupeClassFieldDefinitions(..)
                 | CheckKind::DuplicateHandlerException(..)
+                | CheckKind::DuplicateIsinstanceCall(..)
                 | CheckKind::EndsInPeriod
                 | CheckKind::EndsInPunctuation
                 | CheckKind::FStringMissingPlaceholders
@@ -3778,6 +3787,9 @@ impl CheckKind {
                 Some(format!("Remove duplicate field definition for `{name}`"))
             }
             CheckKind::DuplicateHandlerException(..) => Some("De-duplicate exceptions".to_string()),
+            CheckKind::DuplicateIsinstanceCall(name) => {
+                Some(format!("Merge `isinstance` calls for `{name}`"))
+            }
             CheckKind::EndsInPeriod => Some("Add period".to_string()),
             CheckKind::EndsInPunctuation => Some("Add closing punctuation".to_string()),
             CheckKind::ExtraneousScopeFunction => Some("Remove `scope=` argument".to_string()),