diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB169.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB169.py new file mode 100644 index 0000000000000..5a8f74539b066 --- /dev/null +++ b/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) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index e8970a2fabb84..49631b85d5281 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -1238,6 +1238,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); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 5139a47358eaf..59591fa2cc978 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -945,6 +945,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), diff --git a/crates/ruff_linter/src/rules/refurb/helpers.rs b/crates/ruff_linter/src/rules/refurb/helpers.rs index e432d5b7b8a7f..031a58c62a437 100644 --- a/crates/ruff_linter/src/rules/refurb/helpers.rs +++ b/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()) +} diff --git a/crates/ruff_linter/src/rules/refurb/mod.rs b/crates/ruff_linter/src/rules/refurb/mod.rs index 69bd8b669038b..476bf48287d14 100644 --- a/crates/ruff_linter/src/rules/refurb/mod.rs +++ b/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( 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 65399c5b79510..b4a6c558cea99 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 @@ -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()) -} diff --git a/crates/ruff_linter/src/rules/refurb/rules/mod.rs b/crates/ruff_linter/src/rules/refurb/rules/mod.rs index 519373ad3f2df..868ea4a68e7ab 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/mod.rs +++ b/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; diff --git a/crates/ruff_linter/src/rules/refurb/rules/type_none_comparison.rs b/crates/ruff_linter/src/rules/refurb/rules/type_none_comparison.rs new file mode 100644 index 0000000000000..da1f3695a742b --- /dev/null +++ b/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 { + 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, +} diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB169_FURB169.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB169_FURB169.py.snap new file mode 100644 index 0000000000000..b643992510891 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB169_FURB169.py.snap @@ -0,0 +1,256 @@ +--- +source: crates/ruff_linter/src/rules/refurb/mod.rs +--- +FURB169.py:5:1: FURB169 [*] Compare the identities of `foo` and `None` instead of their respective types + | +3 | # Error. +4 | +5 | type(foo) is type(None) + | ^^^^^^^^^^^^^^^^^^^^^^^ FURB169 +6 | +7 | type(None) is type(foo) + | + = help: Replace with `foo is None` + +ℹ Fix +2 2 | +3 3 | # Error. +4 4 | +5 |-type(foo) is type(None) + 5 |+foo is None +6 6 | +7 7 | type(None) is type(foo) +8 8 | + +FURB169.py:7:1: FURB169 [*] Compare the identities of `foo` and `None` instead of their respective types + | +5 | type(foo) is type(None) +6 | +7 | type(None) is type(foo) + | ^^^^^^^^^^^^^^^^^^^^^^^ FURB169 +8 | +9 | type(None) is type(None) + | + = help: Replace with `foo is None` + +ℹ Fix +4 4 | +5 5 | type(foo) is type(None) +6 6 | +7 |-type(None) is type(foo) + 7 |+foo is None +8 8 | +9 9 | type(None) is type(None) +10 10 | + +FURB169.py:9:1: FURB169 [*] Compare the identities of `None` and `None` instead of their respective types + | + 7 | type(None) is type(foo) + 8 | + 9 | type(None) is type(None) + | ^^^^^^^^^^^^^^^^^^^^^^^^ FURB169 +10 | +11 | type(foo) is not type(None) + | + = help: Replace with `None is None` + +ℹ Fix +6 6 | +7 7 | type(None) is type(foo) +8 8 | +9 |-type(None) is type(None) + 9 |+None is None +10 10 | +11 11 | type(foo) is not type(None) +12 12 | + +FURB169.py:11:1: FURB169 [*] Compare the identities of `foo` and `None` instead of their respective types + | + 9 | type(None) is type(None) +10 | +11 | type(foo) is not type(None) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB169 +12 | +13 | type(None) is not type(foo) + | + = help: Replace with `foo is not None` + +ℹ Fix +8 8 | +9 9 | type(None) is type(None) +10 10 | +11 |-type(foo) is not type(None) + 11 |+foo is not None +12 12 | +13 13 | type(None) is not type(foo) +14 14 | + +FURB169.py:13:1: FURB169 [*] Compare the identities of `foo` and `None` instead of their respective types + | +11 | type(foo) is not type(None) +12 | +13 | type(None) is not type(foo) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB169 +14 | +15 | type(None) is not type(None) + | + = help: Replace with `foo is not None` + +ℹ Fix +10 10 | +11 11 | type(foo) is not type(None) +12 12 | +13 |-type(None) is not type(foo) + 13 |+foo is not None +14 14 | +15 15 | type(None) is not type(None) +16 16 | + +FURB169.py:15:1: FURB169 [*] Compare the identities of `None` and `None` instead of their respective types + | +13 | type(None) is not type(foo) +14 | +15 | type(None) is not type(None) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB169 +16 | +17 | type(foo) == type(None) + | + = help: Replace with `None is not None` + +ℹ Fix +12 12 | +13 13 | type(None) is not type(foo) +14 14 | +15 |-type(None) is not type(None) + 15 |+None is not None +16 16 | +17 17 | type(foo) == type(None) +18 18 | + +FURB169.py:17:1: FURB169 [*] Compare the identities of `foo` and `None` instead of their respective types + | +15 | type(None) is not type(None) +16 | +17 | type(foo) == type(None) + | ^^^^^^^^^^^^^^^^^^^^^^^ FURB169 +18 | +19 | type(None) == type(foo) + | + = help: Replace with `foo is None` + +ℹ Fix +14 14 | +15 15 | type(None) is not type(None) +16 16 | +17 |-type(foo) == type(None) + 17 |+foo is None +18 18 | +19 19 | type(None) == type(foo) +20 20 | + +FURB169.py:19:1: FURB169 [*] Compare the identities of `foo` and `None` instead of their respective types + | +17 | type(foo) == type(None) +18 | +19 | type(None) == type(foo) + | ^^^^^^^^^^^^^^^^^^^^^^^ FURB169 +20 | +21 | type(None) == type(None) + | + = help: Replace with `foo is None` + +ℹ Fix +16 16 | +17 17 | type(foo) == type(None) +18 18 | +19 |-type(None) == type(foo) + 19 |+foo is None +20 20 | +21 21 | type(None) == type(None) +22 22 | + +FURB169.py:21:1: FURB169 [*] Compare the identities of `None` and `None` instead of their respective types + | +19 | type(None) == type(foo) +20 | +21 | type(None) == type(None) + | ^^^^^^^^^^^^^^^^^^^^^^^^ FURB169 +22 | +23 | type(foo) != type(None) + | + = help: Replace with `None is None` + +ℹ Fix +18 18 | +19 19 | type(None) == type(foo) +20 20 | +21 |-type(None) == type(None) + 21 |+None is None +22 22 | +23 23 | type(foo) != type(None) +24 24 | + +FURB169.py:23:1: FURB169 [*] Compare the identities of `foo` and `None` instead of their respective types + | +21 | type(None) == type(None) +22 | +23 | type(foo) != type(None) + | ^^^^^^^^^^^^^^^^^^^^^^^ FURB169 +24 | +25 | type(None) != type(foo) + | + = help: Replace with `foo is not None` + +ℹ Fix +20 20 | +21 21 | type(None) == type(None) +22 22 | +23 |-type(foo) != type(None) + 23 |+foo is not None +24 24 | +25 25 | type(None) != type(foo) +26 26 | + +FURB169.py:25:1: FURB169 [*] Compare the identities of `foo` and `None` instead of their respective types + | +23 | type(foo) != type(None) +24 | +25 | type(None) != type(foo) + | ^^^^^^^^^^^^^^^^^^^^^^^ FURB169 +26 | +27 | type(None) != type(None) + | + = help: Replace with `foo is not None` + +ℹ Fix +22 22 | +23 23 | type(foo) != type(None) +24 24 | +25 |-type(None) != type(foo) + 25 |+foo is not None +26 26 | +27 27 | type(None) != type(None) +28 28 | + +FURB169.py:27:1: FURB169 [*] Compare the identities of `None` and `None` instead of their respective types + | +25 | type(None) != type(foo) +26 | +27 | type(None) != type(None) + | ^^^^^^^^^^^^^^^^^^^^^^^^ FURB169 +28 | +29 | # Ok. + | + = help: Replace with `None is not None` + +ℹ Fix +24 24 | +25 25 | type(None) != type(foo) +26 26 | +27 |-type(None) != type(None) + 27 |+None is not None +28 28 | +29 29 | # Ok. +30 30 | + + diff --git a/ruff.schema.json b/ruff.schema.json index adad852f9cede..0d06727c770d1 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2822,6 +2822,7 @@ "FURB148", "FURB16", "FURB168", + "FURB169", "FURB17", "FURB171", "FURB177",