Match left-hand side types() call in types-comparison (#6326)

Follow-up to https://github.com/astral-sh/ruff/pull/6325, to avoid false
positives in cases like:

```python
if x == int:
    ...
```

Which is valid, since we don't know that we're comparing the type _of_
something -- we're comparing the type objects directly.
This commit is contained in:
Charlie Marsh 2023-08-03 23:01:23 -04:00 committed by GitHub
parent 8cddb6c08d
commit 6da527170f
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 37 additions and 28 deletions

View file

@ -1134,12 +1134,14 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
flake8_simplify::rules::double_negation(checker, expr, *op, operand);
}
}
Expr::Compare(ast::ExprCompare {
left,
ops,
comparators,
range: _,
}) => {
Expr::Compare(
compare @ ast::ExprCompare {
left,
ops,
comparators,
range: _,
},
) => {
let check_none_comparisons = checker.enabled(Rule::NoneComparison);
let check_true_false_comparisons = checker.enabled(Rule::TrueFalseComparison);
if check_none_comparisons || check_true_false_comparisons {
@ -1157,7 +1159,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
pyflakes::rules::invalid_literal_comparison(checker, left, ops, comparators, expr);
}
if checker.enabled(Rule::TypeComparison) {
pycodestyle::rules::type_comparison(checker, expr, ops, comparators);
pycodestyle::rules::type_comparison(checker, compare);
}
if checker.any_enabled(&[
Rule::SysVersionCmpStr3,

View file

@ -1,3 +1,4 @@
use itertools::Itertools;
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::is_const_none;
@ -40,16 +41,32 @@ impl Violation for TypeComparison {
}
/// E721
pub(crate) fn type_comparison(
checker: &mut Checker,
expr: &Expr,
ops: &[CmpOp],
comparators: &[Expr],
) {
for (op, right) in ops.iter().zip(comparators) {
pub(crate) fn type_comparison(checker: &mut Checker, compare: &ast::ExprCompare) {
for ((left, right), op) in std::iter::once(compare.left.as_ref())
.chain(compare.comparators.iter())
.tuple_windows()
.zip(compare.ops.iter())
{
if !matches!(op, CmpOp::Is | CmpOp::IsNot | CmpOp::Eq | CmpOp::NotEq) {
continue;
}
// Left-hand side must be, e.g., `type(obj)`.
let Expr::Call(ast::ExprCall {
func, ..
}) = left else {
continue;
};
let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() else {
continue;
};
if !(id == "type" && checker.semantic().is_builtin("type")) {
continue;
}
// Right-hand side must be, e.g., `type(1)` or `int`.
match right {
Expr::Call(ast::ExprCall {
func, arguments, ..
@ -68,7 +85,7 @@ pub(crate) fn type_comparison(
{
checker
.diagnostics
.push(Diagnostic::new(TypeComparison, expr.range()));
.push(Diagnostic::new(TypeComparison, compare.range()));
}
}
}
@ -76,12 +93,12 @@ pub(crate) fn type_comparison(
// Ex) `type(obj) is types.NoneType`
if checker
.semantic()
.resolve_call_path(value)
.resolve_call_path(value.as_ref())
.is_some_and(|call_path| matches!(call_path.as_slice(), ["types", ..]))
{
checker
.diagnostics
.push(Diagnostic::new(TypeComparison, expr.range()));
.push(Diagnostic::new(TypeComparison, compare.range()));
}
}
Expr::Name(ast::ExprName { id, .. }) => {
@ -93,7 +110,7 @@ pub(crate) fn type_comparison(
{
checker
.diagnostics
.push(Diagnostic::new(TypeComparison, expr.range()));
.push(Diagnostic::new(TypeComparison, compare.range()));
}
}
_ => {}

View file

@ -20,16 +20,6 @@ E721.py:5:4: E721 Do not compare types, use `isinstance()`
7 | #: E721
|
E721.py:10:4: E721 Do not compare types, use `isinstance()`
|
8 | import types
9 |
10 | if res == types.IntType:
| ^^^^^^^^^^^^^^^^^^^^ E721
11 | pass
12 | #: E721
|
E721.py:15:4: E721 Do not compare types, use `isinstance()`
|
13 | import types