Implement consider-merging-isinstance (#1009)

README.md

@@ -751,6 +751,7 @@ For more, see [Pylint](https://pypi.org/project/pylint/2.15.7/) on PyPI.
 
 | Code | Name | Message | Fix |
 | ---- | ---- | ------- | --- |
+| PLR1701 | ConsiderMergingIsinstance | Consider merging these isinstance calls: `isinstance(..., (...))` |  |
 | PLC3002 | UnnecessaryDirectLambdaCall | Lambda expression called directly. Execute the expression inline instead. |  |
 | PLR0206 | PropertyWithParameters | Cannot have defined parameters for properties |  |
 | PLE1142 | AwaitOutsideAsync | `await` should be used within an async function |  |

resources/test/fixtures/pylint/consider_merging_isinstance.py

@@ -0,0 +1,37 @@
+"""Checks use of consider-merging-isinstance"""
+# pylint:disable=line-too-long, simplifiable-condition
+
+
+def isinstances():
+    "Examples of isinstances"
+    var = range(10)
+
+    # merged
+    if isinstance(var[1], (int, float)):
+        pass
+    result = isinstance(var[2], (int, float))
+
+    # not merged
+    if isinstance(var[3], int) or isinstance(var[3], float) or isinstance(var[3], list) and True:  # [consider-merging-isinstance]
+        pass
+    result = isinstance(var[4], int) or isinstance(var[4], float) or isinstance(var[5], list) and False  # [consider-merging-isinstance]
+
+    result = isinstance(var[5], int) or True or isinstance(var[5], float)  # [consider-merging-isinstance]
+
+    infered_isinstance = isinstance
+    result = infered_isinstance(var[6], int) or infered_isinstance(var[6], float) or infered_isinstance(var[6], list) and False   # [consider-merging-isinstance]
+    result = isinstance(var[10], str) or isinstance(var[10], int) and var[8] * 14 or isinstance(var[10], float) and var[5] * 14.4 or isinstance(var[10], list)   # [consider-merging-isinstance]
+    result = isinstance(var[11], int) or isinstance(var[11], int) or isinstance(var[11], float)   # [consider-merging-isinstance]
+
+    result = isinstance(var[20])
+    result = isinstance()
+
+    # Combination merged and not merged
+    result = isinstance(var[12], (int, float)) or isinstance(var[12], list)  # [consider-merging-isinstance]
+
+    # not merged but valid
+    result = isinstance(var[5], int) and var[5] * 14 or isinstance(var[5], float) and var[5] * 14.4
+    result = isinstance(var[7], int) or not isinstance(var[7], float)
+    result = isinstance(var[6], int) or isinstance(var[7], float)
+    result = isinstance(var[6], int) or isinstance(var[7], int)
+    return result

src/check_ast.rs

@@ -2063,6 +2063,11 @@ where
                 }
                 self.push_scope(Scope::new(ScopeKind::Generator));
             }
+            ExprKind::BoolOp { op, values } => {
+                if self.settings.enabled.contains(&CheckCode::PLR1701) {
+                    pylint::plugins::consider_merging_isinstance(self, expr, op, values);
+                }
+            }
             _ => {}
         };
 

src/checks.rs

@@ -90,7 +90,8 @@ pub enum CheckCode {
     F831,
     F841,
     F901,
-    // pylint errors
+    // pylint
+    PLR1701,
     PLC3002,
     PLR0206,
     PLE1142,
@@ -541,7 +542,8 @@ pub enum CheckKind {
     UnusedImport(String, bool),
     UnusedVariable(String),
     YieldOutsideFunction(DeferralKeyword),
-    // pylint errors
+    // pylint
+    ConsiderMergingIsinstance(String, Vec<String>),
     UnnecessaryDirectLambdaCall,
     PropertyWithParameters,
     AwaitOutsideAsync,
@@ -828,10 +830,13 @@ impl CheckCode {
             CheckCode::F831 => CheckKind::DuplicateArgumentName,
             CheckCode::F841 => CheckKind::UnusedVariable("...".to_string()),
             CheckCode::F901 => CheckKind::RaiseNotImplemented,
-            // pylint errors
+            // pylint
             CheckCode::PLC3002 => CheckKind::UnnecessaryDirectLambdaCall,
-            CheckCode::PLR0206 => CheckKind::PropertyWithParameters,
             CheckCode::PLE1142 => CheckKind::AwaitOutsideAsync,
+            CheckCode::PLR0206 => CheckKind::PropertyWithParameters,
+            CheckCode::PLR1701 => {
+                CheckKind::ConsiderMergingIsinstance("...".to_string(), vec!["...".to_string()])
+            }
             // flake8-builtins
             CheckCode::A001 => CheckKind::BuiltinVariableShadowing("...".to_string()),
             CheckCode::A002 => CheckKind::BuiltinArgumentShadowing("...".to_string()),
@@ -1244,8 +1249,9 @@ impl CheckCode {
             CheckCode::N818 => CheckCategory::PEP8Naming,
             CheckCode::PGH001 => CheckCategory::PygrepHooks,
             CheckCode::PLC3002 => CheckCategory::Pylint,
-            CheckCode::PLR0206 => CheckCategory::Pylint,
             CheckCode::PLE1142 => CheckCategory::Pylint,
+            CheckCode::PLR0206 => CheckCategory::Pylint,
+            CheckCode::PLR1701 => CheckCategory::Pylint,
             CheckCode::Q000 => CheckCategory::Flake8Quotes,
             CheckCode::Q001 => CheckCategory::Flake8Quotes,
             CheckCode::Q002 => CheckCategory::Flake8Quotes,
@@ -1357,10 +1363,11 @@ impl CheckKind {
             // pycodestyle warnings
             CheckKind::NoNewLineAtEndOfFile => &CheckCode::W292,
             CheckKind::InvalidEscapeSequence(_) => &CheckCode::W605,
-            // pylint errors
-            CheckKind::UnnecessaryDirectLambdaCall => &CheckCode::PLC3002,
-            CheckKind::PropertyWithParameters => &CheckCode::PLR0206,
+            // pylint
             CheckKind::AwaitOutsideAsync => &CheckCode::PLE1142,
+            CheckKind::ConsiderMergingIsinstance(..) => &CheckCode::PLR1701,
+            CheckKind::PropertyWithParameters => &CheckCode::PLR0206,
+            CheckKind::UnnecessaryDirectLambdaCall => &CheckCode::PLC3002,
             // flake8-builtins
             CheckKind::BuiltinVariableShadowing(_) => &CheckCode::A001,
             CheckKind::BuiltinArgumentShadowing(_) => &CheckCode::A002,
@@ -1732,7 +1739,11 @@ impl CheckKind {
             CheckKind::InvalidEscapeSequence(char) => {
                 format!("Invalid escape sequence: '\\{char}'")
             }
-            // pylint errors
+            // pylint
+            CheckKind::ConsiderMergingIsinstance(obj, types) => {
+                let types = types.join(", ");
+                format!("Consider merging these isinstance calls: `isinstance({obj}, ({types}))`")
+            }
             CheckKind::UnnecessaryDirectLambdaCall => "Lambda expression called directly. Execute \
                                                        the expression inline instead."
                 .to_string(),

src/checks_gen.rs

@@ -297,6 +297,10 @@ pub enum CheckCodePrefix {
     PLR02,
     PLR020,
     PLR0206,
+    PLR1,
+    PLR17,
+    PLR170,
+    PLR1701,
     Q,
     Q0,
     Q00,
@@ -1176,11 +1180,15 @@ impl CheckCodePrefix {
             CheckCodePrefix::PLE11 => vec![CheckCode::PLE1142],
             CheckCodePrefix::PLE114 => vec![CheckCode::PLE1142],
             CheckCodePrefix::PLE1142 => vec![CheckCode::PLE1142],
-            CheckCodePrefix::PLR => vec![CheckCode::PLR0206],
+            CheckCodePrefix::PLR => vec![CheckCode::PLR1701, CheckCode::PLR0206],
             CheckCodePrefix::PLR0 => vec![CheckCode::PLR0206],
             CheckCodePrefix::PLR02 => vec![CheckCode::PLR0206],
             CheckCodePrefix::PLR020 => vec![CheckCode::PLR0206],
             CheckCodePrefix::PLR0206 => vec![CheckCode::PLR0206],
+            CheckCodePrefix::PLR1 => vec![CheckCode::PLR1701],
+            CheckCodePrefix::PLR17 => vec![CheckCode::PLR1701],
+            CheckCodePrefix::PLR170 => vec![CheckCode::PLR1701],
+            CheckCodePrefix::PLR1701 => vec![CheckCode::PLR1701],
             CheckCodePrefix::Q => vec![
                 CheckCode::Q000,
                 CheckCode::Q001,
@@ -1662,6 +1670,10 @@ impl CheckCodePrefix {
             CheckCodePrefix::PLR02 => SuffixLength::Two,
             CheckCodePrefix::PLR020 => SuffixLength::Three,
             CheckCodePrefix::PLR0206 => SuffixLength::Four,
+            CheckCodePrefix::PLR1 => SuffixLength::One,
+            CheckCodePrefix::PLR17 => SuffixLength::Two,
+            CheckCodePrefix::PLR170 => SuffixLength::Three,
+            CheckCodePrefix::PLR1701 => SuffixLength::Four,
             CheckCodePrefix::Q => SuffixLength::Zero,
             CheckCodePrefix::Q0 => SuffixLength::One,
             CheckCodePrefix::Q00 => SuffixLength::Two,

src/pylint/mod.rs

@@ -12,8 +12,9 @@ mod tests {
     use crate::Settings;
 
     #[test_case(CheckCode::PLC3002, Path::new("unnecessary_direct_lambda_call.py"); "PLC3002")]
-    #[test_case(CheckCode::PLR0206, Path::new("property_with_parameters.py"); "PLR0206")]
     #[test_case(CheckCode::PLE1142, Path::new("await_outside_async.py"); "PLE1142")]
+    #[test_case(CheckCode::PLR0206, Path::new("property_with_parameters.py"); "PLR0206")]
+    #[test_case(CheckCode::PLR1701, Path::new("consider_merging_isinstance.py"); "PLR1701")]
     fn checks(check_code: CheckCode, path: &Path) -> Result<()> {
         let snapshot = format!("{}", path.to_string_lossy());
         let mut checks = test_path(

src/pylint/plugins.rs

@@ -1,4 +1,6 @@
-use rustpython_ast::{Arguments, Expr, ExprKind, Stmt};
+use itertools::Itertools;
+use rustc_hash::{FxHashMap, FxHashSet};
+use rustpython_ast::{Arguments, Boolop, Expr, ExprKind, Stmt};
 
 use crate::ast::types::{FunctionScope, Range, ScopeKind};
 use crate::check_ast::Checker;
@@ -15,6 +17,26 @@ pub fn unnecessary_direct_lambda_call(checker: &mut Checker, expr: &Expr, func:
     }
 }
 
+/// PLE1142
+pub fn await_outside_async(checker: &mut Checker, expr: &Expr) {
+    if !checker
+        .current_scopes()
+        .find_map(|scope| {
+            if let ScopeKind::Function(FunctionScope { async_, .. }) = &scope.kind {
+                Some(*async_)
+            } else {
+                None
+            }
+        })
+        .unwrap_or(true)
+    {
+        checker.add_check(Check::new(
+            CheckKind::AwaitOutsideAsync,
+            Range::from_located(expr),
+        ));
+    }
+}
+
 /// PLR0206
 pub fn property_with_parameters(
     checker: &mut Checker,
@@ -43,22 +65,44 @@ pub fn property_with_parameters(
     }
 }
 
-/// PLE1142
-pub fn await_outside_async(checker: &mut Checker, expr: &Expr) {
-    if !checker
-        .current_scopes()
-        .find_map(|scope| {
-            if let ScopeKind::Function(FunctionScope { async_, .. }) = &scope.kind {
-                Some(*async_)
-            } else {
-                None
+/// PLR1701
+pub fn consider_merging_isinstance(
+    checker: &mut Checker,
+    expr: &Expr,
+    op: &Boolop,
+    values: &[Expr],
+) {
+    if !matches!(op, Boolop::Or) || !checker.is_builtin("isinstance") {
+        return;
+    }
+
+    let mut obj_to_types: FxHashMap<String, FxHashSet<String>> = FxHashMap::default();
+    for value in values {
+        if let ExprKind::Call { func, args, .. } = &value.node {
+            if matches!(&func.node, ExprKind::Name { id, .. } if id == "isinstance") {
+                if let [obj, types] = &args[..] {
+                    obj_to_types
+                        .entry(obj.to_string())
+                        .or_insert_with(FxHashSet::default)
+                        .extend(match &types.node {
+                            ExprKind::Tuple { elts, .. } => {
+                                elts.iter().map(std::string::ToString::to_string).collect()
+                            }
+                            _ => {
+                                vec![types.to_string()]
+                            }
+                        });
+                }
             }
-        })
-        .unwrap_or(true)
-    {
-        checker.add_check(Check::new(
-            CheckKind::AwaitOutsideAsync,
-            Range::from_located(expr),
-        ));
+        }
+    }
+
+    for (obj, types) in obj_to_types {
+        if types.len() > 1 {
+            checker.add_check(Check::new(
+                CheckKind::ConsiderMergingIsinstance(obj, types.into_iter().sorted().collect()),
+                Range::from_located(expr),
+            ));
+        }
     }
 }

src/pylint/snapshots/ruff__pylint__tests__consider_merging_isinstance.py.snap

@@ -0,0 +1,78 @@
+---
+source: src/pylint/mod.rs
+expression: checks
+---
+- kind:
+    ConsiderMergingIsinstance:
+      - "var[3]"
+      - - float
+        - int
+  location:
+    row: 15
+    column: 31
+  end_location:
+    row: 15
+    column: 96
+  fix: ~
+- kind:
+    ConsiderMergingIsinstance:
+      - "var[4]"
+      - - float
+        - int
+  location:
+    row: 17
+    column: 37
+  end_location:
+    row: 17
+    column: 103
+  fix: ~
+- kind:
+    ConsiderMergingIsinstance:
+      - "var[5]"
+      - - float
+        - int
+  location:
+    row: 19
+    column: 37
+  end_location:
+    row: 19
+    column: 73
+  fix: ~
+- kind:
+    ConsiderMergingIsinstance:
+      - "var[10]"
+      - - list
+        - str
+  location:
+    row: 23
+    column: 38
+  end_location:
+    row: 23
+    column: 158
+  fix: ~
+- kind:
+    ConsiderMergingIsinstance:
+      - "var[11]"
+      - - float
+        - int
+  location:
+    row: 24
+    column: 38
+  end_location:
+    row: 24
+    column: 95
+  fix: ~
+- kind:
+    ConsiderMergingIsinstance:
+      - "var[12]"
+      - - float
+        - int
+        - list
+  location:
+    row: 30
+    column: 47
+  end_location:
+    row: 30
+    column: 75
+  fix: ~
+