Mark repeated-isinstance-calls as unsafe on Python 3.10 and later (#…

…11622) ## Summary Closes #11616.
astral-sh · May 30, 2024 · 685d11a · 685d11a
1 parent dcabd04
commit 685d11a
Show file tree

Hide file tree

Showing 3 changed files with 25 additions and 16 deletions.
diff --git a/crates/ruff_linter/src/rules/pylint/rules/repeated_isinstance_calls.rs b/crates/ruff_linter/src/rules/pylint/rules/repeated_isinstance_calls.rs
@@ -4,7 +4,7 @@ use rustc_hash::{FxHashMap, FxHashSet};
 
 use crate::fix::edits::pad;
 use crate::fix::snippet::SourceCodeSnippet;
-use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
+use ruff_diagnostics::{AlwaysFixableViolation, Applicability, Diagnostic, Edit, Fix};
 use ruff_macros::{derive_message_formats, violation};
 use ruff_python_ast::hashable::HashableExpr;
 use ruff_text_size::Ranged;
@@ -20,6 +20,15 @@ use crate::settings::types::PythonVersion;
 /// Repeated `isinstance` calls on the same object can be merged into a
 /// single call.
 ///
+/// ## Fix safety
+/// This rule's fix is marked as unsafe on Python 3.10 and later, as combining
+/// multiple `isinstance` calls with a binary operator (`|`) will fail at
+/// runtime if any of the operands are themselves tuples.
+///
+/// For example, given `TYPES = (dict, list)`, then
+/// `isinstance(None, TYPES | set | float)` will raise a `TypeError` at runtime,
+/// while `isinstance(None, set | float)` will not.
+///
 /// ## Example
 /// ```python
 /// def is_number(x):
@@ -130,10 +139,14 @@ pub(crate) fn repeated_isinstance_calls(
                 },
                 expr.range(),
             );
-            diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
-                pad(call, expr.range(), checker.locator()),
-                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);
         }
     }

diff --git a/...nt/snapshots/ruff_linter__rules__pylint__tests__PLR1701_repeated_isinstance_calls.py.snap b/...nt/snapshots/ruff_linter__rules__pylint__tests__PLR1701_repeated_isinstance_calls.py.snap
@@ -11,7 +11,7 @@ repeated_isinstance_calls.py:15:8: PLR1701 [*] Merge `isinstance` calls: `isinst
    |
    = help: Replace with `isinstance(var[3], float | int)`
 
-ℹ Safe fix
+ℹ Unsafe fix
 12 12 |     result = isinstance(var[2], (int, float))
 13 13 | 
 14 14 |     # not merged
@@ -32,7 +32,7 @@ repeated_isinstance_calls.py:17:14: PLR1701 [*] Merge `isinstance` calls: `isins
    |
    = help: Replace with `isinstance(var[4], float | int)`
 
-ℹ Safe fix
+ℹ Unsafe fix
 14 14 |     # not merged
 15 15 |     if isinstance(var[3], int) or isinstance(var[3], float) or isinstance(var[3], list) and True:  # [consider-merging-isinstance]
 16 16 |         pass
@@ -53,7 +53,7 @@ repeated_isinstance_calls.py:19:14: PLR1701 [*] Merge `isinstance` calls: `isins
    |
    = help: Replace with `isinstance(var[5], float | int)`
 
-ℹ Safe fix
+ℹ Unsafe fix
 16 16 |         pass
 17 17 |     result = isinstance(var[4], int) or isinstance(var[4], float) or isinstance(var[5], list) and False  # [consider-merging-isinstance]
 18 18 | 
@@ -73,7 +73,7 @@ repeated_isinstance_calls.py:23:14: PLR1701 [*] Merge `isinstance` calls: `isins
    |
    = help: Replace with `isinstance(var[10], list | str)`
 
-ℹ Safe fix
+ℹ Unsafe fix
 20 20 | 
 21 21 |     inferred_isinstance = isinstance
 22 22 |     result = inferred_isinstance(var[6], int) or inferred_isinstance(var[6], float) or inferred_isinstance(var[6], list) and False   # [consider-merging-isinstance]
@@ -94,7 +94,7 @@ repeated_isinstance_calls.py:24:14: PLR1701 [*] Merge `isinstance` calls: `isins
    |
    = help: Replace with `isinstance(var[11], float | int)`
 
-ℹ Safe fix
+ℹ Unsafe fix
 21 21 |     inferred_isinstance = isinstance
 22 22 |     result = inferred_isinstance(var[6], int) or inferred_isinstance(var[6], float) or inferred_isinstance(var[6], list) and False   # [consider-merging-isinstance]
 23 23 |     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]
@@ -114,7 +114,7 @@ repeated_isinstance_calls.py:30:14: PLR1701 [*] Merge `isinstance` calls: `isins
    |
    = help: Replace with `isinstance(var[12], float | int | list)`
 
-ℹ Safe fix
+ℹ Unsafe fix
 27 27 |     result = isinstance()
 28 28 | 
 29 29 |     # Combination merged and not merged
@@ -133,12 +133,10 @@ repeated_isinstance_calls.py:42:3: PLR1701 [*] Merge `isinstance` calls: `isinst
    |
    = help: Replace with `isinstance(self.k, float | int)`
 
-ℹ Safe fix
+ℹ Unsafe fix
 39 39 | 
 40 40 | 
 41 41 | # Regression test for: https://github.com/astral-sh/ruff/issues/7455#issuecomment-1722460483
 42    |-if(isinstance(self.k, int)) or (isinstance(self.k, float)):
    42 |+if isinstance(self.k, float | int):
 43 43 |     ...
-
-
diff --git a/.../rules/pylint/snapshots/ruff_linter__rules__pylint__tests__repeated_isinstance_calls.snap b/.../rules/pylint/snapshots/ruff_linter__rules__pylint__tests__repeated_isinstance_calls.snap
@@ -140,5 +140,3 @@ repeated_isinstance_calls.py:42:3: PLR1701 [*] Merge `isinstance` calls: `isinst
 42    |-if(isinstance(self.k, int)) or (isinstance(self.k, float)):
    42 |+if isinstance(self.k, (float, int)):
 43 43 |     ...
-
-