[refurb] Implement isinstance-type-none (FURB168)

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

@@ -0,0 +1,51 @@
+foo: object
+
+# Errors.
+
+if isinstance(foo, type(None)):
+    pass
+
+if isinstance(foo, (type(None))):
+    pass
+
+if isinstance(foo, (type(None), type(None), type(None))):
+    pass
+
+if isinstance(foo, None | None):
+    pass
+
+if isinstance(foo, (None | None)):
+    pass
+
+if isinstance(foo, None | type(None)):
+    pass
+
+if isinstance(foo, (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
+
+
+# Okay.
+
+if isinstance(foo, int):
+    pass
+
+if isinstance(foo, (int)):
+    pass
+
+if isinstance(foo, (int, str)):
+    pass
+
+if isinstance(foo, (int, type(None), str)):
+    pass
+
+# This is a TypeError, which the rule ignores.
+if isinstance(foo, None):
+    pass
+
+# This is also a TypeError, which the rule ignores.
+if isinstance(foo, (None,)):
+    pass

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

@@ -908,6 +908,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
             if checker.enabled(Rule::ExceptionWithoutExcInfo) {
                 flake8_logging::rules::exception_without_exc_info(checker, call);
             }
+            if checker.enabled(Rule::IsinstanceTypeNone) {
+                refurb::rules::isinstance_type_none(checker, expr, func, args);
+            }
             if checker.enabled(Rule::ImplicitCwd) {
                 refurb::rules::no_implicit_cwd(checker, call);
             }

crates/ruff_linter/src/codes.rs

@@ -935,6 +935,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Refurb, "140") => (RuleGroup::Preview, rules::refurb::rules::ReimplementedStarmap),
         (Refurb, "145") => (RuleGroup::Preview, rules::refurb::rules::SliceCopy),
         (Refurb, "148") => (RuleGroup::Preview, rules::refurb::rules::UnnecessaryEnumerate),
+        (Refurb, "168") => (RuleGroup::Stable, rules::refurb::rules::IsinstanceTypeNone),
         (Refurb, "171") => (RuleGroup::Preview, rules::refurb::rules::SingleItemMembershipTest),
         (Refurb, "177") => (RuleGroup::Preview, rules::refurb::rules::ImplicitCwd),
 

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

@@ -24,6 +24,7 @@ mod tests {
     #[test_case(Rule::PrintEmptyString, Path::new("FURB105.py"))]
     #[test_case(Rule::ImplicitCwd, Path::new("FURB177.py"))]
     #[test_case(Rule::SingleItemMembershipTest, Path::new("FURB171.py"))]
+    #[test_case(Rule::IsinstanceTypeNone, Path::new("FURB168.py"))]
     fn rules(rule_code: Rule, path: &Path) -> Result<()> {
         let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
         let diagnostics = test_path(

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

@@ -0,0 +1,140 @@
+use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
+use ruff_python_ast::{self as ast, Expr, Operator};
+
+use ruff_macros::{derive_message_formats, violation};
+use ruff_python_codegen::Generator;
+use ruff_text_size::{Ranged, TextRange};
+
+use crate::checkers::ast::Checker;
+
+/// ## What it does
+/// Checks for uses of `isinstance` that check if an object is of type `None`.
+///
+/// ## Why is this bad?
+/// There is only ever one instance of `None`, so it is more efficient and
+/// readable to use the `is` operator to check if an object is `None`.
+///
+/// ## Example
+/// ```python
+/// isinstance(foo, type(None))
+/// ```
+///
+/// Use instead:
+/// ```python
+/// foo is None
+/// ```
+///
+/// ## 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)
+/// - [Python documentation: `type`](https://docs.python.org/3/library/functions.html#type)
+/// - [Python documentation: Identity comparisons](https://docs.python.org/3/reference/expressions.html#is-not)
+#[violation]
+pub struct IsinstanceTypeNone;
+
+impl Violation for IsinstanceTypeNone {
+    const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
+
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        format!("Prefer `is` operator over `isinstance` to check if an object is `None`")
+    }
+
+    fn fix_title(&self) -> Option<String> {
+        Some("Replace with `is` operator".to_string())
+    }
+}
+
+/// FURB168
+pub(crate) fn isinstance_type_none(checker: &mut Checker, expr: &Expr, func: &Expr, args: &[Expr]) {
+    if let Expr::Name(ast::ExprName { id, .. }) = func {
+        if id.as_str() != "isinstance" {
+            return;
+        }
+        if !checker.semantic().is_builtin(id) {
+            return;
+        }
+        if let Some(types) = args.get(1) {
+            if is_none(types) {
+                let object_name = match &args[0] {
+                    Expr::Name(ast::ExprName { id, .. }) => id.as_str(),
+                    _ => return,
+                };
+                let mut diagnostic = Diagnostic::new(IsinstanceTypeNone, expr.range());
+                let replacement = generate_is_none_replacement(object_name, checker.generator());
+                diagnostic.set_fix(Fix::safe_edit(Edit::replacement(
+                    replacement,
+                    expr.start(),
+                    expr.end(),
+                )));
+                checker.diagnostics.push(diagnostic);
+            }
+        }
+    }
+}
+
+/// 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 {
+        match expr {
+            // Ex) `None`
+            // Note: `isinstance` only accepts `None` as a type when used with
+            // the union operator, so we need to check if we're in a union.
+            Expr::Constant(ast::ExprConstant { value, .. }) if in_union_context => value.is_none(),
+
+            // 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" {
+                        if let Some(Expr::Constant(ast::ExprConstant { value, .. })) =
+                            arguments.args.get(0)
+                        {
+                            return value.is_none();
+                        }
+                    }
+                }
+                false
+            }
+
+            // Ex) `(type(None),)`
+            Expr::Tuple(ast::ExprTuple { elts, .. }) => elts.iter().all(|e| inner(e, false)),
+
+            // Ex) `type(None) | type(None)`
+            Expr::BinOp(ast::ExprBinOp {
+                left,
+                op: Operator::BitOr,
+                right,
+                ..
+            }) => inner(left, true) && inner(right, true),
+
+            // Otherwise, return false.
+            _ => false,
+        }
+    }
+    inner(expr, false)
+}
+
+/// Format a code snippet comparing `name` to `None`.
+/// Ex) `name is None`
+fn generate_is_none_replacement(name: &str, generator: Generator) -> String {
+    // Construct `name`.
+    let var = ast::ExprName {
+        id: name.to_string(),
+        ctx: ast::ExprContext::Load,
+        range: TextRange::default(),
+    };
+    // Construct `name is None`.
+    let compare = ast::ExprCompare {
+        left: Box::new(var.into()),
+        ops: vec![ast::CmpOp::Is],
+        comparators: vec![ast::Expr::Constant(ast::ExprConstant {
+            value: ast::Constant::None,
+            range: TextRange::default(),
+        })],
+        range: TextRange::default(),
+    };
+    generator.expr(&compare.into())
+}

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

@@ -1,6 +1,7 @@
 pub(crate) use check_and_remove_from_set::*;
 pub(crate) use delete_full_slice::*;
 pub(crate) use implicit_cwd::*;
+pub(crate) use isinstance_type_none::*;
 pub(crate) use print_empty_string::*;
 pub(crate) use read_whole_file::*;
 pub(crate) use reimplemented_starmap::*;
@@ -12,6 +13,7 @@ pub(crate) use unnecessary_enumerate::*;
 mod check_and_remove_from_set;
 mod delete_full_slice;
 mod implicit_cwd;
+mod isinstance_type_none;
 mod print_empty_string;
 mod read_whole_file;
 mod reimplemented_starmap;

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

@@ -0,0 +1,163 @@
+---
+source: crates/ruff_linter/src/rules/refurb/mod.rs
+---
+FURB168.py:5:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if an object is `None`
+  |
+3 | # Errors.
+4 | 
+5 | if isinstance(foo, type(None)):
+  |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
+6 |     pass
+  |
+  = help: Replace with `is` operator
+
+ℹ Fix
+2 2 | 
+3 3 | # Errors.
+4 4 | 
+5   |-if isinstance(foo, type(None)):
+  5 |+if foo is None:
+6 6 |     pass
+7 7 | 
+8 8 | if isinstance(foo, (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
+9 |     pass
+  |
+  = help: Replace with `is` operator
+
+ℹ Fix
+5 5 | if isinstance(foo, type(None)):
+6 6 |     pass
+7 7 | 
+8   |-if isinstance(foo, (type(None))):
+  8 |+if foo is None:
+9 9 |     pass
+10 10 | 
+11 11 | if isinstance(foo, (type(None), type(None), type(None))):
+
+FURB168.py:11:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if an object is `None`
+   |
+ 9 |     pass
+10 | 
+11 | if isinstance(foo, (type(None), type(None), type(None))):
+   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
+12 |     pass
+   |
+   = help: Replace with `is` operator
+
+ℹ Fix
+8  8  | if isinstance(foo, (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):
+
+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
+15 |     pass
+   |
+   = help: Replace with `is` operator
+
+ℹ Fix
+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:
+15 15 |     pass
+16 16 | 
+17 17 | if isinstance(foo, (None | None)):
+
+FURB168.py:17:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if an object is `None`
+   |
+15 |     pass
+16 | 
+17 | if isinstance(foo, (None | None)):
+   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
+18 |     pass
+   |
+   = help: Replace with `is` operator
+
+ℹ Fix
+14 14 | if isinstance(foo, None | None):
+15 15 |     pass
+16 16 | 
+17    |-if isinstance(foo, (None | None)):
+   17 |+if foo is None:
+18 18 |     pass
+19 19 | 
+20 20 | if isinstance(foo, None | type(None)):
+
+FURB168.py:20:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if an object is `None`
+   |
+18 |     pass
+19 | 
+20 | if isinstance(foo, None | type(None)):
+   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
+21 |     pass
+   |
+   = help: Replace with `is` operator
+
+ℹ Fix
+17 17 | if isinstance(foo, (None | 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))):
+
+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
+24 |     pass
+   |
+   = help: Replace with `is` operator
+
+ℹ Fix
+20 20 | if isinstance(foo, None | type(None)):
+21 21 |     pass
+22 22 | 
+23    |-if isinstance(foo, (None | type(None))):
+   23 |+if foo is None:
+24 24 |     pass
+25 25 | 
+26 26 | # A bit contrived, but is both technically valid and equivalent to the above.
+
+FURB168.py:27:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if an object is `None`
+   |
+26 | # A bit contrived, but is both technically valid and equivalent to the above.
+27 | if isinstance(foo, (type(None) | ((((type(None))))) | ((None | type(None))))):
+   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
+28 |     pass
+   |
+   = help: Replace with `is` operator
+
+ℹ Fix
+24 24 |     pass
+25 25 | 
+26 26 | # A bit contrived, but is both technically valid and equivalent to the above.
+27    |-if isinstance(foo, (type(None) | ((((type(None))))) | ((None | type(None))))):
+   27 |+if foo is None:
+28 28 |     pass
+29 29 | 
+30 30 | 
+
+