[`refurb`] Avoid `None | None` as well as better detection and fix (`FURB168`) (#15779)

crates/ruff_linter/resources/test/fixtures/refurb/FURB168.py

@@ -5,28 +5,45 @@ foo: object
 if isinstance(foo, type(None)):
     pass
 
-if isinstance(foo, (type(None))):
+if isinstance(foo and bar, type(None)):
     pass
 
 if isinstance(foo, (type(None), type(None), type(None))):
     pass
 
-if isinstance(foo, None | None):
+if isinstance(foo, type(None)) is True:
     pass
 
-if isinstance(foo, (None | None)):
+if -isinstance(foo, type(None)):
     pass
 
 if isinstance(foo, None | type(None)):
     pass
 
-if isinstance(foo, (None | type(None))):
+if isinstance(foo, type(None) | type(None)):
     pass
 
 # A bit contrived, but is both technically valid and equivalent to the above.
 if isinstance(foo, (type(None) | ((((type(None))))) | ((None | type(None))))):
     pass
 
+if isinstance(
+    foo,  # Comment
+    None
+):
+    ...
+
+from typing import Union
+
+if isinstance(foo, Union[None]):
+    ...
+
+if isinstance(foo, Union[None, None]):
+    ...
+
+if isinstance(foo, Union[None, type(None)]):
+    ...
+
 
 # Okay.
 
@@ -42,6 +59,12 @@ if isinstance(foo, (int, str)):
 if isinstance(foo, (int, type(None), str)):
     pass
 
+if isinstance(foo, str | None):
+    pass
+
+if isinstance(foo, Union[None, str]):
+    ...
+
 # This is a TypeError, which the rule ignores.
 if isinstance(foo, None):
     pass
@@ -49,3 +72,16 @@ if isinstance(foo, None):
 # This is also a TypeError, which the rule ignores.
 if isinstance(foo, (None,)):
     pass
+
+if isinstance(foo, None | None):
+    pass
+
+if isinstance(foo, (type(None) | ((((type(None))))) | ((None | None | type(None))))):
+    pass
+
+# https://github.com/astral-sh/ruff/issues/15776
+def _():
+    def type(*args): ...
+
+    if isinstance(foo, type(None)):
+        ...

crates/ruff_linter/src/rules/refurb/rules/isinstance_type_none.rs

@@ -1,12 +1,11 @@
-use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
+use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, Violation};
-use ruff_python_ast::{self as ast, Expr, Operator};
+use ruff_python_ast::{self as ast, CmpOp, Expr, Operator};
 
 use ruff_macros::{derive_message_formats, ViolationMetadata};
-use ruff_text_size::Ranged;
+use ruff_python_semantic::SemanticModel;
+use ruff_text_size::TextRange;
 
 use crate::checkers::ast::Checker;
-use crate::fix::edits::pad;
-use crate::rules::refurb::helpers::generate_none_identity_comparison;
 
 /// ## What it does
 /// Checks for uses of `isinstance` that check if an object is of type `None`.
@@ -25,6 +24,9 @@ use crate::rules::refurb::helpers::generate_none_identity_comparison;
 /// obj is None
 /// ```
 ///
+/// ## Fix safety
+/// The fix will be marked as unsafe if there are any comments within the call.
+///
 /// ## References
 /// - [Python documentation: `isinstance`](https://docs.python.org/3/library/functions.html#isinstance)
 /// - [Python documentation: `None`](https://docs.python.org/3/library/constants.html#None)
@@ -48,37 +50,35 @@ impl Violation for IsinstanceTypeNone {
 
 /// FURB168
 pub(crate) fn isinstance_type_none(checker: &mut Checker, call: &ast::ExprCall) {
-    let Some(types) = call.arguments.find_positional(1) else {
+    let semantic = checker.semantic();
+    let (func, arguments) = (&call.func, &call.arguments);
+
+    if !arguments.keywords.is_empty() {
+        return;
+    }
+
+    let [expr, types] = arguments.args.as_ref() else {
         return;
     };
-    if !checker
+
-        .semantic()
+    if !semantic.match_builtin_expr(func, "isinstance") {
-        .match_builtin_expr(&call.func, "isinstance")
-    {
         return;
     }
-    if is_none(types) {
+
-        let Some(Expr::Name(ast::ExprName {
+    if !is_none(types, semantic) {
-            id: object_name, ..
+        return;
-        })) = call.arguments.find_positional(0)
-        else {
-            return;
-        };
-        let mut diagnostic = Diagnostic::new(IsinstanceTypeNone, call.range());
-        let replacement =
-            generate_none_identity_comparison(object_name.clone(), false, checker.generator());
-        diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
-            pad(replacement, call.range(), checker.locator()),
-            call.range(),
-        )));
-        checker.diagnostics.push(diagnostic);
     }
+
+    let fix = replace_with_identity_check(expr, call.range, checker);
+    let diagnostic = Diagnostic::new(IsinstanceTypeNone, call.range);
+
+    checker.diagnostics.push(diagnostic.with_fix(fix));
 }
 
 /// Returns `true` if the given expression is equivalent to checking if the
 /// object type is `None` when used with the `isinstance` builtin.
-fn is_none(expr: &Expr) -> bool {
+fn is_none(expr: &Expr, semantic: &SemanticModel) -> bool {
-    fn inner(expr: &Expr, in_union_context: bool) -> bool {
+    fn inner(expr: &Expr, in_union_context: bool, semantic: &SemanticModel) -> bool {
         match expr {
             // Ex) `None`
             // Note: `isinstance` only accepts `None` as a type when used with
@@ -88,17 +88,20 @@ fn is_none(expr: &Expr) -> bool {
             // Ex) `type(None)`
             Expr::Call(ast::ExprCall {
                 func, arguments, ..
-            }) if arguments.len() == 1 => {
+            }) => {
-                if let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() {
+                if !semantic.match_builtin_expr(func, "type") {
-                    if id.as_str() == "type" {
+                    return false;
-                        return matches!(arguments.args.first(), Some(Expr::NoneLiteral(_)));
-                    }
                 }
-                false
+
+                if !arguments.keywords.is_empty() {
+                    return false;
+                }
+
+                matches!(arguments.args.as_ref(), [Expr::NoneLiteral(_)])
             }
 
             // Ex) `(type(None),)`
-            Expr::Tuple(tuple) => tuple.iter().all(|element| inner(element, false)),
+            Expr::Tuple(tuple) => tuple.iter().all(|element| inner(element, false, semantic)),
 
             // Ex) `type(None) | type(None)`
             Expr::BinOp(ast::ExprBinOp {
@@ -106,11 +109,60 @@ fn is_none(expr: &Expr) -> bool {
                 op: Operator::BitOr,
                 right,
                 ..
-            }) => inner(left, true) && inner(right, true),
+            }) => {
+                // `None | None` is a `TypeError` at runtime
+                if left.is_none_literal_expr() && right.is_none_literal_expr() {
+                    return false;
+                }
+
+                inner(left, true, semantic) && inner(right, true, semantic)
+            }
+
+            // Ex) `Union[None, ...]`
+            Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => {
+                if !semantic.match_typing_expr(value, "Union") {
+                    return false;
+                }
+
+                match slice.as_ref() {
+                    Expr::Tuple(ast::ExprTuple { elts, .. }) => {
+                        elts.iter().all(|element| inner(element, true, semantic))
+                    }
+                    slice => inner(slice, true, semantic),
+                }
+            }
 
             // Otherwise, return false.
             _ => false,
         }
     }
-    inner(expr, false)
+    inner(expr, false, semantic)
+}
+
+fn replace_with_identity_check(expr: &Expr, range: TextRange, checker: &Checker) -> Fix {
+    let (semantic, generator) = (checker.semantic(), checker.generator());
+
+    let new_expr = Expr::Compare(ast::ExprCompare {
+        left: expr.clone().into(),
+        ops: [CmpOp::Is].into(),
+        comparators: [ast::ExprNoneLiteral::default().into()].into(),
+        range: TextRange::default(),
+    });
+
+    let new_content = generator.expr(&new_expr);
+    let new_content = if semantic.current_expression_parent().is_some() {
+        format!("({new_content})")
+    } else {
+        new_content
+    };
+
+    let applicability = if checker.comment_ranges().intersects(range) {
+        Applicability::Unsafe
+    } else {
+        Applicability::Safe
+    };
+
+    let edit = Edit::range_replacement(new_content, range);
+
+    Fix::applicable_edit(edit, applicability)
 }

crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB168_FURB168.py.snap

@@ -19,14 +19,14 @@ FURB168.py:5:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if a
   5 |+if foo is None:
 6 6 |     pass
 7 7 | 
-8 8 | if isinstance(foo, (type(None))):
+8 8 | if isinstance(foo and bar, type(None)):
 
 FURB168.py:8:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if an object is `None`
   |
 6 |     pass
 7 |
-8 | if isinstance(foo, (type(None))):
+8 | if isinstance(foo and bar, type(None)):
-  |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
+  |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
 9 |     pass
   |
   = help: Replace with `is` operator
@@ -35,8 +35,8 @@ FURB168.py:8:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if a
 5 5 | if isinstance(foo, type(None)):
 6 6 |     pass
 7 7 | 
-8   |-if isinstance(foo, (type(None))):
+8   |-if isinstance(foo and bar, type(None)):
-  8 |+if foo is None:
+  8 |+if (foo and bar) is None:
 9 9 |     pass
 10 10 | 
 11 11 | if isinstance(foo, (type(None), type(None), type(None))):
@@ -52,21 +52,21 @@ FURB168.py:11:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if
    = help: Replace with `is` operator
 
 ℹ Safe fix
-8  8  | if isinstance(foo, (type(None))):
+8  8  | if isinstance(foo and bar, type(None)):
 9  9  |     pass
 10 10 | 
 11    |-if isinstance(foo, (type(None), type(None), type(None))):
    11 |+if foo is None:
 12 12 |     pass
 13 13 | 
-14 14 | if isinstance(foo, None | None):
+14 14 | if isinstance(foo, type(None)) is True:
 
 FURB168.py:14:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if an object is `None`
    |
 12 |     pass
 13 |
-14 | if isinstance(foo, None | None):
+14 | if isinstance(foo, type(None)) is True:
-   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
+   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
 15 |     pass
    |
    = help: Replace with `is` operator
@@ -75,28 +75,28 @@ FURB168.py:14:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if
 11 11 | if isinstance(foo, (type(None), type(None), type(None))):
 12 12 |     pass
 13 13 | 
-14    |-if isinstance(foo, None | None):
+14    |-if isinstance(foo, type(None)) is True:
-   14 |+if foo is None:
+   14 |+if (foo is None) is True:
 15 15 |     pass
 16 16 | 
-17 17 | if isinstance(foo, (None | None)):
+17 17 | if -isinstance(foo, type(None)):
 
-FURB168.py:17:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if an object is `None`
+FURB168.py:17:5: FURB168 [*] Prefer `is` operator over `isinstance` to check if an object is `None`
    |
 15 |     pass
 16 |
-17 | if isinstance(foo, (None | None)):
+17 | if -isinstance(foo, type(None)):
-   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
 18 |     pass
    |
    = help: Replace with `is` operator
 
 ℹ Safe fix
-14 14 | if isinstance(foo, None | None):
+14 14 | if isinstance(foo, type(None)) is True:
 15 15 |     pass
 16 16 | 
-17    |-if isinstance(foo, (None | None)):
+17    |-if -isinstance(foo, type(None)):
-   17 |+if foo is None:
+   17 |+if -(foo is None):
 18 18 |     pass
 19 19 | 
 20 20 | if isinstance(foo, None | type(None)):
@@ -112,21 +112,21 @@ FURB168.py:20:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if
    = help: Replace with `is` operator
 
 ℹ Safe fix
-17 17 | if isinstance(foo, (None | None)):
+17 17 | if -isinstance(foo, type(None)):
 18 18 |     pass
 19 19 | 
 20    |-if isinstance(foo, None | type(None)):
    20 |+if foo is None:
 21 21 |     pass
 22 22 | 
-23 23 | if isinstance(foo, (None | type(None))):
+23 23 | if isinstance(foo, type(None) | type(None)):
 
 FURB168.py:23:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if an object is `None`
    |
 21 |     pass
 22 |
-23 | if isinstance(foo, (None | type(None))):
+23 | if isinstance(foo, type(None) | type(None)):
-   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
+   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
 24 |     pass
    |
    = help: Replace with `is` operator
@@ -135,7 +135,7 @@ FURB168.py:23:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if
 20 20 | if isinstance(foo, None | type(None)):
 21 21 |     pass
 22 22 | 
-23    |-if isinstance(foo, (None | type(None))):
+23    |-if isinstance(foo, type(None) | type(None)):
    23 |+if foo is None:
 24 24 |     pass
 25 25 | 
@@ -158,4 +158,64 @@ FURB168.py:27:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if
    27 |+if foo is None:
 28 28 |     pass
 29 29 | 
-30 30 |
+30 30 | if isinstance(
+
+FURB168.py:38:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if an object is `None`
+   |
+36 | from typing import Union
+37 |
+38 | if isinstance(foo, Union[None]):
+   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
+39 |     ...
+   |
+   = help: Replace with `is` operator
+
+ℹ Safe fix
+35 35 | 
+36 36 | from typing import Union
+37 37 | 
+38    |-if isinstance(foo, Union[None]):
+   38 |+if foo is None:
+39 39 |     ...
+40 40 | 
+41 41 | if isinstance(foo, Union[None, None]):
+
+FURB168.py:41:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if an object is `None`
+   |
+39 |     ...
+40 |
+41 | if isinstance(foo, Union[None, None]):
+   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
+42 |     ...
+   |
+   = help: Replace with `is` operator
+
+ℹ Safe fix
+38 38 | if isinstance(foo, Union[None]):
+39 39 |     ...
+40 40 | 
+41    |-if isinstance(foo, Union[None, None]):
+   41 |+if foo is None:
+42 42 |     ...
+43 43 | 
+44 44 | if isinstance(foo, Union[None, type(None)]):
+
+FURB168.py:44:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if an object is `None`
+   |
+42 |     ...
+43 |
+44 | if isinstance(foo, Union[None, type(None)]):
+   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
+45 |     ...
+   |
+   = help: Replace with `is` operator
+
+ℹ Safe fix
+41 41 | if isinstance(foo, Union[None, None]):
+42 42 |     ...
+43 43 | 
+44    |-if isinstance(foo, Union[None, type(None)]):
+   44 |+if foo is None:
+45 45 |     ...
+46 46 | 
+47 47 |