[refurb] Implement type-none-comparison (FURB169)

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

@@ -0,0 +1,67 @@
+foo = None
+
+# Error.
+
+type(foo) is type(None)
+
+type(None) is type(foo)
+
+type(None) is type(None)
+
+type(foo) is not type(None)
+
+type(None) is not type(foo)
+
+type(None) is not type(None)
+
+type(foo) == type(None)
+
+type(None) == type(foo)
+
+type(None) == type(None)
+
+type(foo) != type(None)
+
+type(None) != type(foo)
+
+type(None) != type(None)
+
+# Ok.
+
+foo is None
+
+foo is not None
+
+None is foo
+
+None is not foo
+
+None is None
+
+None is not None
+
+foo is type(None)
+
+type(foo) is None
+
+type(None) is None
+
+foo is not type(None)
+
+type(foo) is not None
+
+type(None) is not None
+
+foo == type(None)
+
+type(foo) == None
+
+type(None) == None
+
+foo != type(None)
+
+type(foo) != None
+
+type(None) != None
+
+type(foo) > type(None)

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

@@ -1235,6 +1235,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
                     comparators,
                 );
             }
+            if checker.enabled(Rule::TypeNoneComparison) {
+                refurb::rules::type_none_comparison(checker, compare);
+            }
             if checker.enabled(Rule::SingleItemMembershipTest) {
                 refurb::rules::single_item_membership_test(checker, expr, left, ops, comparators);
             }

crates/ruff_linter/src/codes.rs

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

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

@@ -34,3 +34,30 @@ pub(super) fn generate_method_call(name: &str, method: &str, generator: Generato
     };
     generator.stmt(&stmt.into())
 }
+
+/// Format a code snippet comparing `name` to `None` (e.g., `name is None`).
+pub(super) fn generate_none_identity_comparison(
+    name: &str,
+    negate: bool,
+    generator: Generator,
+) -> String {
+    // Construct `name`.
+    let var = ast::ExprName {
+        id: name.to_string(),
+        ctx: ast::ExprContext::Load,
+        range: TextRange::default(),
+    };
+    // Construct `name is None` or `name is not None`.
+    let op = if negate {
+        ast::CmpOp::IsNot
+    } else {
+        ast::CmpOp::Is
+    };
+    let compare = ast::ExprCompare {
+        left: Box::new(var.into()),
+        ops: vec![op],
+        comparators: vec![ast::Expr::NoneLiteral(ast::ExprNoneLiteral::default())],
+        range: TextRange::default(),
+    };
+    generator.expr(&compare.into())
+}

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

@@ -25,6 +25,7 @@ mod tests {
     #[test_case(Rule::ImplicitCwd, Path::new("FURB177.py"))]
     #[test_case(Rule::SingleItemMembershipTest, Path::new("FURB171.py"))]
     #[test_case(Rule::IsinstanceTypeNone, Path::new("FURB168.py"))]
+    #[test_case(Rule::TypeNoneComparison, Path::new("FURB169.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

@@ -2,11 +2,11 @@ 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 ruff_text_size::Ranged;
 
 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`.
@@ -69,7 +69,8 @@ pub(crate) fn isinstance_type_none(checker: &mut Checker, call: &ast::ExprCall)
             return;
         };
         let mut diagnostic = Diagnostic::new(IsinstanceTypeNone, call.range());
-        let replacement = generate_replacement(object_name, checker.generator());
+        let replacement =
+            generate_none_identity_comparison(object_name, false, checker.generator());
         diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
             pad(replacement, call.range(), checker.locator()),
             call.range(),
@@ -117,21 +118,3 @@ fn is_none(expr: &Expr) -> bool {
     }
     inner(expr, false)
 }
-
-/// Format a code snippet comparing `name` to `None` (e.g., `name is None`).
-fn generate_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::NoneLiteral(ast::ExprNoneLiteral::default())],
-        range: TextRange::default(),
-    };
-    generator.expr(&compare.into())
-}

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

@@ -8,6 +8,7 @@ pub(crate) use reimplemented_starmap::*;
 pub(crate) use repeated_append::*;
 pub(crate) use single_item_membership_test::*;
 pub(crate) use slice_copy::*;
+pub(crate) use type_none_comparison::*;
 pub(crate) use unnecessary_enumerate::*;
 
 mod check_and_remove_from_set;
@@ -20,4 +21,5 @@ mod reimplemented_starmap;
 mod repeated_append;
 mod single_item_membership_test;
 mod slice_copy;
+mod type_none_comparison;
 mod unnecessary_enumerate;

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

@@ -0,0 +1,153 @@
+use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
+use ruff_macros::{derive_message_formats, violation};
+use ruff_python_ast::{self as ast, CmpOp, Expr};
+use ruff_python_semantic::SemanticModel;
+use ruff_text_size::Ranged;
+
+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 `type` that compare the type of an object to the type of
+/// `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
+/// type(obj) is type(None)
+/// ```
+///
+/// Use instead:
+/// ```python
+/// obj 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 TypeNoneComparison {
+    object: String,
+    comparison: Comparison,
+}
+
+impl Violation for TypeNoneComparison {
+    const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
+
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        let TypeNoneComparison { object, .. } = self;
+        format!("Compare the identities of `{object}` and `None` instead of their respective types")
+    }
+
+    fn fix_title(&self) -> Option<String> {
+        let TypeNoneComparison { object, comparison } = self;
+        match comparison {
+            Comparison::Is | Comparison::Eq => Some(format!("Replace with `{object} is None`")),
+            Comparison::IsNot | Comparison::NotEq => {
+                Some(format!("Replace with `{object} is not None`"))
+            }
+        }
+    }
+}
+
+/// FURB169
+pub(crate) fn type_none_comparison(checker: &mut Checker, compare: &ast::ExprCompare) {
+    let ([op], [right]) = (compare.ops.as_slice(), compare.comparators.as_slice()) else {
+        return;
+    };
+
+    // Ensure that the comparison is an identity or equality test.
+    let comparison = match op {
+        CmpOp::Is => Comparison::Is,
+        CmpOp::IsNot => Comparison::IsNot,
+        CmpOp::Eq => Comparison::Eq,
+        CmpOp::NotEq => Comparison::NotEq,
+        _ => return,
+    };
+
+    // Get the objects whose types are being compared.
+    let Some(left_arg) = type_call_arg(&compare.left, checker.semantic()) else {
+        return;
+    };
+    let Some(right_arg) = type_call_arg(right, checker.semantic()) else {
+        return;
+    };
+
+    // If one of the objects is `None`, get the other object; else, return.
+    let other_arg = match (
+        left_arg.is_none_literal_expr(),
+        right_arg.is_none_literal_expr(),
+    ) {
+        (true, false) => right_arg,
+        (false, true) => left_arg,
+        // If both are `None`, just pick one.
+        (true, true) => left_arg,
+        _ => return,
+    };
+
+    // Get the name of the other object (or `None` if both were `None`).
+    let other_arg_name = match other_arg {
+        Expr::Name(ast::ExprName { id, .. }) => id.as_str(),
+        Expr::NoneLiteral { .. } => "None",
+        _ => return,
+    };
+
+    let mut diagnostic = Diagnostic::new(
+        TypeNoneComparison {
+            object: other_arg_name.to_string(),
+            comparison,
+        },
+        compare.range(),
+    );
+    diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
+        pad(
+            match comparison {
+                Comparison::Is | Comparison::Eq => {
+                    generate_none_identity_comparison(other_arg_name, false, checker.generator())
+                }
+                Comparison::IsNot | Comparison::NotEq => {
+                    generate_none_identity_comparison(other_arg_name, true, checker.generator())
+                }
+            },
+            compare.range(),
+            checker.locator(),
+        ),
+        compare.range(),
+    )));
+    checker.diagnostics.push(diagnostic);
+}
+
+/// Returns the object passed to the function, if the expression is a call to
+/// `type` with a single argument.
+fn type_call_arg<'a>(expr: &'a Expr, semantic: &'a SemanticModel) -> Option<&'a Expr> {
+    // The expression must be a single-argument call to `type`.
+    let ast::ExprCall {
+        func, arguments, ..
+    } = expr.as_call_expr()?;
+    if arguments.len() != 1 {
+        return None;
+    }
+
+    // The function itself must be the builtin `type`.
+    let ast::ExprName { id, .. } = func.as_name_expr()?;
+    if id.as_str() != "type" || !semantic.is_builtin(id) {
+        return None;
+    }
+
+    arguments.find_positional(0)
+}
+
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+enum Comparison {
+    Is,
+    IsNot,
+    Eq,
+    NotEq,
+}