Redirect PLR1701 to SIM101

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

@@ -48,3 +48,7 @@ def isinstance(a, b):
 
     if isinstance(a, int) or isinstance(a, float):
         pass
+
+# Regression test for: https://github.com/astral-sh/ruff/issues/7455#issuecomment-1722460483
+if(isinstance(a, int)) or (isinstance(a, float)):
+    pass

crates/ruff_linter/src/checkers/ast/analyze/expression.rs

@@ -1515,16 +1515,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
                 refurb::rules::reimplemented_starmap(checker, &generator.into());
             }
         }
-        Expr::BoolOp(
-            bool_op @ ast::ExprBoolOp {
-                op,
-                values,
-                range: _,
-            },
-        ) => {
-            if checker.enabled(Rule::RepeatedIsinstanceCalls) {
-                pylint::rules::repeated_isinstance_calls(checker, expr, *op, values);
-            }
+        Expr::BoolOp(bool_op) => {
             if checker.enabled(Rule::MultipleStartsEndsWith) {
                 flake8_pie::rules::multiple_starts_ends_with(checker, expr);
             }

crates/ruff_linter/src/codes.rs

@@ -248,7 +248,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Pylint, "R0915") => (RuleGroup::Stable, rules::pylint::rules::TooManyStatements),
         (Pylint, "R0916") => (RuleGroup::Preview, rules::pylint::rules::TooManyBooleanExpressions),
         (Pylint, "R0917") => (RuleGroup::Preview, rules::pylint::rules::TooManyPositional),
-        (Pylint, "R1701") => (RuleGroup::Stable, rules::pylint::rules::RepeatedIsinstanceCalls),
+        (Pylint, "R1701") => (RuleGroup::Removed, rules::pylint::rules::RepeatedIsinstanceCalls),
         (Pylint, "R1702") => (RuleGroup::Preview, rules::pylint::rules::TooManyNestedBlocks),
         (Pylint, "R1704") => (RuleGroup::Preview, rules::pylint::rules::RedefinedArgumentFromLocal),
         (Pylint, "R1706") => (RuleGroup::Removed, rules::pylint::rules::AndOrTernary),

crates/ruff_linter/src/rule_redirects.rs

@@ -103,6 +103,8 @@ static REDIRECTS: Lazy<HashMap<&'static str, &'static str>> = Lazy::new(|| {
         ("TRY200", "B904"),
         ("PGH001", "S307"),
         ("PGH002", "G010"),
+        // Removed in v0.5
+        ("PLR1701", "SIM101"),
         // Test redirect by exact code
         #[cfg(any(feature = "test-rules", test))]
         ("RUF940", "RUF950"),

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

@@ -15,6 +15,7 @@ use ruff_python_codegen::Generator;
 use ruff_python_semantic::SemanticModel;
 
 use crate::checkers::ast::Checker;
+use crate::fix::edits::pad;
 
 /// ## What it does
 /// Checks for multiple `isinstance` calls on the same target.
@@ -404,7 +405,7 @@ pub(crate) fn duplicate_isinstance_call(checker: &mut Checker, expr: &Expr) {
                     .collect();
 
                 // Generate a single `isinstance` call.
-                let node = ast::ExprTuple {
+                let tuple = ast::ExprTuple {
                     // Flatten all the types used across the `isinstance` calls.
                     elts: types
                         .iter()
@@ -421,39 +422,45 @@ pub(crate) fn duplicate_isinstance_call(checker: &mut Checker, expr: &Expr) {
                     range: TextRange::default(),
                     parenthesized: true,
                 };
-                let node1 = ast::ExprName {
-                    id: "isinstance".into(),
-                    ctx: ExprContext::Load,
-                    range: TextRange::default(),
-                };
-                let node2 = ast::ExprCall {
-                    func: Box::new(node1.into()),
+                let isinstance_call = ast::ExprCall {
+                    func: Box::new(
+                        ast::ExprName {
+                            id: "isinstance".into(),
+                            ctx: ExprContext::Load,
+                            range: TextRange::default(),
+                        }
+                        .into(),
+                    ),
                     arguments: Arguments {
-                        args: Box::from([target.clone(), node.into()]),
+                        args: Box::from([target.clone(), tuple.into()]),
                         keywords: Box::from([]),
                         range: TextRange::default(),
                     },
                     range: TextRange::default(),
-                };
-                let call = node2.into();
+                }
+                .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 {
+                let bool_op = ast::ExprBoolOp {
                     op: BoolOp::Or,
-                    values: before.chain(iter::once(call)).chain(after).collect(),
+                    values: before
+                        .chain(iter::once(isinstance_call))
+                        .chain(after)
+                        .collect(),
                     range: TextRange::default(),
-                };
-                let bool_op = node.into();
+                }
+                .into();
+                let fixed_source = checker.generator().expr(&bool_op);
 
                 // Populate the `Fix`. Replace the _entire_ `BoolOp`. Note that if we have
                 // multiple duplicates, the fixes will conflict.
                 diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
-                    checker.generator().expr(&bool_op),
+                    pad(fixed_source, expr.range(), checker.locator()),
                     expr.range(),
                 )));
             }

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

@@ -166,4 +166,19 @@ SIM101.py:41:4: SIM101 [*] Multiple `isinstance` calls for `a`, merge into a sin
 43 43 | 
 44 44 | def f():
 
+SIM101.py:53:3: SIM101 [*] Multiple `isinstance` calls for `a`, merge into a single call
+   |
+52 | # Regression test for: https://github.com/astral-sh/ruff/issues/7455#issuecomment-1722460483
+53 | if(isinstance(a, int)) or (isinstance(a, float)):
+   |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SIM101
+54 |     pass
+   |
+   = help: Merge `isinstance` calls for `a`
 
+ℹ Unsafe fix
+50 50 |         pass
+51 51 | 
+52 52 | # Regression test for: https://github.com/astral-sh/ruff/issues/7455#issuecomment-1722460483
+53    |-if(isinstance(a, int)) or (isinstance(a, float)):
+   53 |+if isinstance(a, (int, float)):
+54 54 |     pass

crates/ruff_linter/src/rules/pylint/mod.rs

@@ -39,10 +39,6 @@ mod tests {
     #[test_case(Rule::CollapsibleElseIf, Path::new("collapsible_else_if.py"))]
     #[test_case(Rule::CompareToEmptyString, Path::new("compare_to_empty_string.py"))]
     #[test_case(Rule::ComparisonOfConstant, Path::new("comparison_of_constant.py"))]
-    #[test_case(
-        Rule::RepeatedIsinstanceCalls,
-        Path::new("repeated_isinstance_calls.py")
-    )]
     #[test_case(Rule::ComparisonWithItself, Path::new("comparison_with_itself.py"))]
     #[test_case(Rule::EqWithoutHash, Path::new("eq_without_hash.py"))]
     #[test_case(Rule::EmptyComment, Path::new("empty_comment.py"))]
@@ -229,17 +225,6 @@ mod tests {
         Ok(())
     }
 
-    #[test]
-    fn repeated_isinstance_calls() -> Result<()> {
-        let diagnostics = test_path(
-            Path::new("pylint/repeated_isinstance_calls.py"),
-            &LinterSettings::for_rule(Rule::RepeatedIsinstanceCalls)
-                .with_target_version(PythonVersion::Py39),
-        )?;
-        assert_messages!(diagnostics);
-        Ok(())
-    }
-
     #[test]
     fn continue_in_finally() -> Result<()> {
         let diagnostics = test_path(

crates/ruff_linter/src/rules/pylint/rules/repeated_isinstance_calls.rs

@@ -1,18 +1,11 @@
-use itertools::Itertools;
-use ruff_python_ast::{self as ast, Arguments, BoolOp, Expr};
-use rustc_hash::{FxHashMap, FxHashSet};
-
-use crate::fix::edits::pad;
-use crate::fix::snippet::SourceCodeSnippet;
-use ruff_diagnostics::{AlwaysFixableViolation, Applicability, Diagnostic, Edit, Fix};
+use ruff_diagnostics::AlwaysFixableViolation;
 use ruff_macros::{derive_message_formats, violation};
-use ruff_python_ast::hashable::HashableExpr;
-use ruff_text_size::Ranged;
-
-use crate::checkers::ast::Checker;
 
-use crate::settings::types::PythonVersion;
+use crate::fix::snippet::SourceCodeSnippet;
 
+/// ## Removed
+/// This rule is identical to [SIM101] which should be used instead.
+///
 /// ## What it does
 /// Checks for repeated `isinstance` calls on the same object.
 ///
@@ -53,11 +46,14 @@ use crate::settings::types::PythonVersion;
 ///
 /// ## References
 /// - [Python documentation: `isinstance`](https://docs.python.org/3/library/functions.html#isinstance)
+///
+/// [SIM101]: https://docs.astral.sh/ruff/rules/duplicate-isinstance-call/
 #[violation]
 pub struct RepeatedIsinstanceCalls {
     expression: SourceCodeSnippet,
 }
 
+// PLR1701
 impl AlwaysFixableViolation for RepeatedIsinstanceCalls {
     #[derive_message_formats]
     fn message(&self) -> String {
@@ -78,88 +74,3 @@ impl AlwaysFixableViolation for RepeatedIsinstanceCalls {
         }
     }
 }
-
-/// PLR1701
-pub(crate) fn repeated_isinstance_calls(
-    checker: &mut Checker,
-    expr: &Expr,
-    op: BoolOp,
-    values: &[Expr],
-) {
-    if !op.is_or() {
-        return;
-    }
-
-    let mut obj_to_types: FxHashMap<HashableExpr, (usize, FxHashSet<HashableExpr>)> =
-        FxHashMap::default();
-    for value in values {
-        let Expr::Call(ast::ExprCall {
-            func,
-            arguments: Arguments { args, .. },
-            ..
-        }) = value
-        else {
-            continue;
-        };
-        let [obj, types] = &args[..] else {
-            continue;
-        };
-        if !checker.semantic().match_builtin_expr(func, "isinstance") {
-            continue;
-        }
-        let (num_calls, matches) = obj_to_types
-            .entry(obj.into())
-            .or_insert_with(|| (0, FxHashSet::default()));
-
-        *num_calls += 1;
-        matches.extend(match types {
-            Expr::Tuple(ast::ExprTuple { elts, .. }) => {
-                elts.iter().map(HashableExpr::from_expr).collect()
-            }
-            _ => {
-                vec![types.into()]
-            }
-        });
-    }
-
-    for (obj, (num_calls, types)) in obj_to_types {
-        if num_calls > 1 && types.len() > 1 {
-            let call = merged_isinstance_call(
-                &checker.generator().expr(obj.as_expr()),
-                types
-                    .iter()
-                    .map(HashableExpr::as_expr)
-                    .map(|expr| checker.generator().expr(expr))
-                    .sorted(),
-                checker.settings.target_version,
-            );
-            let mut diagnostic = Diagnostic::new(
-                RepeatedIsinstanceCalls {
-                    expression: SourceCodeSnippet::new(call.clone()),
-                },
-                expr.range(),
-            );
-            diagnostic.set_fix(Fix::applicable_edit(
-                Edit::range_replacement(pad(call, expr.range(), checker.locator()), expr.range()),
-                if checker.settings.target_version >= PythonVersion::Py310 {
-                    Applicability::Unsafe
-                } else {
-                    Applicability::Safe
-                },
-            ));
-            checker.diagnostics.push(diagnostic);
-        }
-    }
-}
-
-fn merged_isinstance_call(
-    obj: &str,
-    types: impl IntoIterator<Item = String>,
-    target_version: PythonVersion,
-) -> String {
-    if target_version >= PythonVersion::Py310 {
-        format!("isinstance({}, {})", obj, types.into_iter().join(" | "))
-    } else {
-        format!("isinstance({}, ({}))", obj, types.into_iter().join(", "))
-    }
-}