diff --git a/crates/ruff/resources/test/fixtures/pycodestyle/E721.py b/crates/ruff/resources/test/fixtures/pycodestyle/E721.py index a220f58c3d8e4..5f07803832eca 100644 --- a/crates/ruff/resources/test/fixtures/pycodestyle/E721.py +++ b/crates/ruff/resources/test/fixtures/pycodestyle/E721.py @@ -58,3 +58,6 @@ types = StrEnum if x == types.X: pass + +#: E721 +assert type(res) is int diff --git a/crates/ruff/src/rules/pycodestyle/rules/type_comparison.rs b/crates/ruff/src/rules/pycodestyle/rules/type_comparison.rs index 553d21319ef0b..1d65bcd9215c1 100644 --- a/crates/ruff/src/rules/pycodestyle/rules/type_comparison.rs +++ b/crates/ruff/src/rules/pycodestyle/rules/type_comparison.rs @@ -1,31 +1,33 @@ -use itertools::izip; -use ruff_python_ast::{self as ast, Arguments, CmpOp, Constant, Expr, Ranged}; - use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::helpers::is_const_none; +use ruff_python_ast::{self as ast, CmpOp, Expr, Ranged}; use crate::checkers::ast::Checker; /// ## What it does -/// Checks for object type comparisons without using isinstance(). +/// Checks for object type comparisons without using `isinstance()`. /// /// ## Why is this bad? /// Do not compare types directly. -/// When checking if an object is a instance of a certain type, keep in mind that it might -/// be subclassed. E.g. `bool` inherits from `int` or `Exception` inherits from `BaseException`. +/// +/// When checking if an object is a instance of a certain type, keep in mind +/// that it might be subclassed. For example, `bool` inherits from `int`, and +/// `Exception` inherits from `BaseException`. /// /// ## Example /// ```python /// if type(obj) is type(1): /// pass +/// +/// if type(obj) is int: +/// pass /// ``` /// /// Use instead: /// ```python /// if isinstance(obj, int): /// pass -/// if type(a1) is type(b1): -/// pass /// ``` #[violation] pub struct TypeComparison; @@ -44,48 +46,25 @@ pub(crate) fn type_comparison( ops: &[CmpOp], comparators: &[Expr], ) { - for (op, right) in izip!(ops, comparators) { + for (op, right) in ops.iter().zip(comparators) { if !matches!(op, CmpOp::Is | CmpOp::IsNot | CmpOp::Eq | CmpOp::NotEq) { continue; } match right { Expr::Call(ast::ExprCall { - func, - arguments: Arguments { args, .. }, - .. + func, arguments, .. }) => { - if let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() { - // Ex) `type(False)` - if id == "type" && checker.semantic().is_builtin("type") { - if let Some(arg) = args.first() { - // Allow comparison for types which are not obvious. - if !matches!( - arg, - Expr::Name(_) - | Expr::Constant(ast::ExprConstant { - value: Constant::None, - kind: None, - range: _ - }) - ) { - checker - .diagnostics - .push(Diagnostic::new(TypeComparison, expr.range())); - } - } - } - } - } - Expr::Attribute(ast::ExprAttribute { value, .. }) => { - if let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() { - // Ex) `types.NoneType` - if id == "types" - && checker - .semantic() - .resolve_call_path(value) - .is_some_and(|call_path| { - call_path.first().is_some_and(|module| *module == "types") - }) + // Ex) `type(obj) is type(1)` + let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() else { + continue; + }; + + if id == "type" && checker.semantic().is_builtin("type") { + // Allow comparison for types which are not obvious. + if arguments + .args + .first() + .is_some_and(|arg| !arg.is_name_expr() && !is_const_none(arg)) { checker .diagnostics @@ -93,6 +72,30 @@ pub(crate) fn type_comparison( } } } + Expr::Attribute(ast::ExprAttribute { value, .. }) => { + // Ex) `type(obj) is types.NoneType` + if checker + .semantic() + .resolve_call_path(value) + .is_some_and(|call_path| matches!(call_path.as_slice(), ["types", ..])) + { + checker + .diagnostics + .push(Diagnostic::new(TypeComparison, expr.range())); + } + } + Expr::Name(ast::ExprName { id, .. }) => { + // Ex) `type(obj) is int` + if matches!( + id.as_str(), + "int" | "str" | "float" | "bool" | "complex" | "bytes" + ) && checker.semantic().is_builtin(id) + { + checker + .diagnostics + .push(Diagnostic::new(TypeComparison, expr.range())); + } + } _ => {} } } diff --git a/crates/ruff/src/rules/pycodestyle/snapshots/ruff__rules__pycodestyle__tests__E721_E721.py.snap b/crates/ruff/src/rules/pycodestyle/snapshots/ruff__rules__pycodestyle__tests__E721_E721.py.snap index 764b9511eb9cb..897cff081a1d6 100644 --- a/crates/ruff/src/rules/pycodestyle/snapshots/ruff__rules__pycodestyle__tests__E721_E721.py.snap +++ b/crates/ruff/src/rules/pycodestyle/snapshots/ruff__rules__pycodestyle__tests__E721_E721.py.snap @@ -155,4 +155,11 @@ E721.py:42:8: E721 Do not compare types, use `isinstance()` 44 | #: Okay | +E721.py:63:8: E721 Do not compare types, use `isinstance()` + | +62 | #: E721 +63 | assert type(res) is int + | ^^^^^^^^^^^^^^^^ E721 + | +