Skip to content

Commit

Permalink
Include comparisons to builtin types in type-comparison rule
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Aug 4, 2023
1 parent d3aa8b4 commit 597abb8
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 44 deletions.
3 changes: 3 additions & 0 deletions crates/ruff/resources/test/fixtures/pycodestyle/E721.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,3 +58,6 @@
types = StrEnum
if x == types.X:
pass

#: E721
assert type(res) is int
91 changes: 47 additions & 44 deletions crates/ruff/src/rules/pycodestyle/rules/type_comparison.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -44,55 +46,56 @@ 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
.push(Diagnostic::new(TypeComparison, expr.range()));
}
}
}
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()));
}
}
_ => {}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
|


0 comments on commit 597abb8

Please sign in to comment.