diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB168.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB168.py new file mode 100644 index 0000000000000..36a8081f5a4f3 --- /dev/null +++ b/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 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 13ad26bed7933..bf3eb63f3e98e 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/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, call); + } if checker.enabled(Rule::ImplicitCwd) { refurb::rules::no_implicit_cwd(checker, call); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 1c836a092be94..8bc65b6ab0820 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/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::Preview, rules::refurb::rules::IsinstanceTypeNone), (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/mod.rs b/crates/ruff_linter/src/rules/refurb/mod.rs index fe50c29bcd7e3..69bd8b669038b 100644 --- a/crates/ruff_linter/src/rules/refurb/mod.rs +++ b/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( 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 new file mode 100644 index 0000000000000..8d40c3ea08a35 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/rules/isinstance_type_none.rs @@ -0,0 +1,144 @@ +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; +use crate::fix::edits::pad; + +/// ## 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(obj, 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 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 { + Some("Replace with `is` operator".to_string()) + } +} + +/// FURB168 +pub(crate) fn isinstance_type_none(checker: &mut Checker, call: &ast::ExprCall) { + let Expr::Name(ast::ExprName { id, .. }) = call.func.as_ref() else { + return; + }; + if id.as_str() != "isinstance" { + return; + } + if !checker.semantic().is_builtin(id) { + return; + } + let Some(types) = call.arguments.find_positional(1) else { + 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_replacement(object_name, checker.generator()); + diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( + pad(replacement, call.range(), checker.locator()), + call.range(), + ))); + 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(|elt| inner(elt, 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` (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::Constant(ast::ExprConstant { + value: ast::Constant::None, + range: TextRange::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 096ebc10b68d6..519373ad3f2df 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/mod.rs +++ b/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; 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 new file mode 100644 index 0000000000000..51505c574d40f --- /dev/null +++ b/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 | + + diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index 0f7924531a388..1adcd7c68c995 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -1134,6 +1134,7 @@ mod tests { Rule::AssignmentInAssert, Rule::DirectLoggerInstantiation, Rule::InvalidGetLoggerArgument, + Rule::IsinstanceTypeNone, Rule::ManualDictComprehension, Rule::ReimplementedStarmap, Rule::SliceCopy, diff --git a/ruff.schema.json b/ruff.schema.json index 2c3d3fcf1476c..7b0d663172f35 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2820,6 +2820,8 @@ "FURB140", "FURB145", "FURB148", + "FURB16", + "FURB168", "FURB17", "FURB171", "FURB177",