diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB168.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB168.py index 36a8081f5a4f3..17238d520c4c9 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB168.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB168.py @@ -5,28 +5,45 @@ 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, 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 @@ # 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)): + ... diff --git a/crates/ruff_linter/src/rules/refurb/rules/isinstance_type_none.rs b/crates/ruff_linter/src/rules/refurb/rules/isinstance_type_none.rs index e32acd64ccd83..3354a06e8b891 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/isinstance_type_none.rs +++ b/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_python_ast::{self as ast, Expr, Operator}; +use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, Violation}; +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() - .match_builtin_expr(&call.func, "isinstance") - { + + if !semantic.match_builtin_expr(func, "isinstance") { return; } - if is_none(types) { - let Some(Expr::Name(ast::ExprName { - id: object_name, .. - })) = 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); + + if !is_none(types, semantic) { + return; } + + 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 inner(expr: &Expr, in_union_context: bool) -> bool { +fn is_none(expr: &Expr, semantic: &SemanticModel) -> 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 id.as_str() == "type" { - return matches!(arguments.args.first(), Some(Expr::NoneLiteral(_))); - } + }) => { + if !semantic.match_builtin_expr(func, "type") { + return false; + } + + if !arguments.keywords.is_empty() { + return false; } - 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) } diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB168_FURB168.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB168_FURB168.py.snap index 4afe7602fb212..c9b6165ff5a6e 100644 --- a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB168_FURB168.py.snap +++ b/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))): - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168 +8 | if isinstance(foo and bar, type(None)): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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 foo is None: +8 |-if isinstance(foo and bar, type(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): - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168 +14 | if isinstance(foo, type(None)) is True: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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 foo is None: +14 |-if isinstance(foo, type(None)) is True: + 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)): - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168 +17 | if -isinstance(foo, type(None)): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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 foo is None: +17 |-if -isinstance(foo, type(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))): - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168 +23 | if isinstance(foo, type(None) | type(None)): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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 |