Skip to content

Commit

Permalink
Allow is and is not for direct type comparisons (#7905)
Browse files Browse the repository at this point in the history
## Summary

This PR updates our E721 implementation and semantics to match the
updated `pycodestyle` logic, which I think is an improvement.
Specifically, we now allow `type(obj) is int` for exact type
comparisons, which were previously impossible. So now, we're largely
just linting against code like `type(obj) == int`.

This change is gated to preview mode.

Closes #7904.

## Test Plan

Updated the test fixture and ensured parity with latest Flake8.
  • Loading branch information
charliermarsh authored Oct 20, 2023
1 parent f6d6200 commit df807ff
Show file tree
Hide file tree
Showing 5 changed files with 328 additions and 104 deletions.
71 changes: 49 additions & 22 deletions crates/ruff_linter/resources/test/fixtures/pycodestyle/E721.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,18 @@
#: E721
if type(res) != type(""):
pass
#: E721
#: Okay
import types

if res == types.IntType:
pass
#: E721
#: Okay
import types

if type(res) is not types.ListType:
pass
#: E721
assert type(res) == type(False)
assert type(res) == type(False) or type(res) == type(None)
#: E721
assert type(res) == type([])
#: E721
Expand All @@ -25,21 +25,18 @@
#: E721
assert type(res) == type((0))
#: E721
assert type(res) != type((1,))
#: E721
assert type(res) is type((1,))
#: E721
assert type(res) is not type((1,))
assert type(res) != type((1, ))
#: Okay
assert type(res) is type((1, ))
#: Okay
assert type(res) is not type((1, ))
#: E211 E721
assert type(res) == type(
[
2,
]
)
assert type(res) == type ([2, ])
#: E201 E201 E202 E721
assert type(res) == type(())
assert type(res) == type( ( ) )
#: E201 E202 E721
assert type(res) == type((0,))
assert type(res) == type( (0, ) )
#:

#: Okay
import types
Expand All @@ -50,18 +47,48 @@
pass
if isinstance(res, types.MethodType):
pass
if type(a) != type(b) or type(a) == type(ccc):
#: Okay
def func_histype(a, b, c):
pass
#: E722
try:
pass
except:
pass
#: E722
try:
pass
except Exception:
pass
except:
pass
#: E722 E203 E271
try:
pass
except :
pass
#: Okay
fake_code = """"
try:
do_something()
except:
pass
"""
try:
pass
except Exception:
pass
#: Okay
from . import custom_types as types

assert type(res) == type(None)
red = types.ColorTypeRED
red is types.ColorType.RED
#: Okay
from . import compute_type

types = StrEnum
if x == types.X:
if compute_type(foo) == 5:
pass

#: E721
assert type(res) is int


class Foo:
def asdf(self, value: str | None):
Expand Down
19 changes: 19 additions & 0 deletions crates/ruff_linter/src/rules/pycodestyle/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ mod tests {

use crate::line_width::LineLength;
use crate::registry::Rule;
use crate::settings::types::PreviewMode;
use crate::test::test_path;
use crate::{assert_messages, settings};

Expand Down Expand Up @@ -61,6 +62,24 @@ mod tests {
Ok(())
}

#[test_case(Rule::TypeComparison, Path::new("E721.py"))]
fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!(
"preview__{}_{}",
rule_code.noqa_code(),
path.to_string_lossy()
);
let diagnostics = test_path(
Path::new("pycodestyle").join(path).as_path(),
&settings::LinterSettings {
preview: PreviewMode::Enabled,
..settings::LinterSettings::for_rule(rule_code)
},
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
}

#[test]
fn w292_4() -> Result<()> {
let diagnostics = test_path(
Expand Down
115 changes: 97 additions & 18 deletions crates/ruff_linter/src/rules/pycodestyle/rules/type_comparison.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,26 +3,31 @@ 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};
use ruff_python_semantic::SemanticModel;
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
use crate::settings::types::PreviewMode;

/// ## What it does
/// Checks for object type comparisons without using `isinstance()`.
/// Checks for object type comparisons using `==` and other comparison
/// operators.
///
/// ## Why is this bad?
/// Do not compare types directly.
/// Unlike a direct type comparison, `isinstance` will also check if an object
/// is an instance of a class or a subclass thereof.
///
/// 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`.
/// Under [preview mode](https://docs.astral.sh/ruff/preview), this rule also
/// allows for direct type comparisons using `is` and `is not`, to check for
/// exact type equality (while still forbidding comparisons using `==` and
/// `!=`).
///
/// ## Example
/// ```python
/// if type(obj) is type(1):
/// if type(obj) == type(1):
/// pass
///
/// if type(obj) is int:
/// if type(obj) == int:
/// pass
/// ```
///
Expand All @@ -32,17 +37,31 @@ use crate::checkers::ast::Checker;
/// pass
/// ```
#[violation]
pub struct TypeComparison;
pub struct TypeComparison {
preview: PreviewMode,
}

impl Violation for TypeComparison {
#[derive_message_formats]
fn message(&self) -> String {
format!("Do not compare types, use `isinstance()`")
match self.preview {
PreviewMode::Disabled => format!("Do not compare types, use `isinstance()`"),
PreviewMode::Enabled => format!(
"Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks"
),
}
}
}

/// E721
pub(crate) fn type_comparison(checker: &mut Checker, compare: &ast::ExprCompare) {
match checker.settings.preview {
PreviewMode::Disabled => deprecated_type_comparison(checker, compare),
PreviewMode::Enabled => preview_type_comparison(checker, compare),
}
}

fn deprecated_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()
Expand Down Expand Up @@ -82,9 +101,12 @@ pub(crate) fn type_comparison(checker: &mut Checker, compare: &ast::ExprCompare)
.first()
.is_some_and(|arg| !arg.is_name_expr() && !is_const_none(arg))
{
checker
.diagnostics
.push(Diagnostic::new(TypeComparison, compare.range()));
checker.diagnostics.push(Diagnostic::new(
TypeComparison {
preview: PreviewMode::Disabled,
},
compare.range(),
));
}
}
}
Expand All @@ -95,9 +117,12 @@ pub(crate) fn type_comparison(checker: &mut Checker, compare: &ast::ExprCompare)
.resolve_call_path(value.as_ref())
.is_some_and(|call_path| matches!(call_path.as_slice(), ["types", ..]))
{
checker
.diagnostics
.push(Diagnostic::new(TypeComparison, compare.range()));
checker.diagnostics.push(Diagnostic::new(
TypeComparison {
preview: PreviewMode::Disabled,
},
compare.range(),
));
}
}
Expr::Name(ast::ExprName { id, .. }) => {
Expand All @@ -115,12 +140,66 @@ pub(crate) fn type_comparison(checker: &mut Checker, compare: &ast::ExprCompare)
| "set"
) && checker.semantic().is_builtin(id)
{
checker
.diagnostics
.push(Diagnostic::new(TypeComparison, compare.range()));
checker.diagnostics.push(Diagnostic::new(
TypeComparison {
preview: PreviewMode::Disabled,
},
compare.range(),
));
}
}
_ => {}
}
}
}

pub(crate) fn preview_type_comparison(checker: &mut Checker, compare: &ast::ExprCompare) {
for (left, right) in std::iter::once(compare.left.as_ref())
.chain(compare.comparators.iter())
.tuple_windows()
.zip(compare.ops.iter())
.filter(|(_, op)| matches!(op, CmpOp::Eq | CmpOp::NotEq))
.map(|((left, right), _)| (left, right))
{
if is_type(left, checker.semantic()) || is_type(right, checker.semantic()) {
checker.diagnostics.push(Diagnostic::new(
TypeComparison {
preview: PreviewMode::Enabled,
},
compare.range(),
));
}
}
}

/// Returns `true` if the [`Expr`] is known to evaluate to a type (e.g., `int`, or `type(1)`).
fn is_type(expr: &Expr, semantic: &SemanticModel) -> bool {
match expr {
Expr::Call(ast::ExprCall {
func, arguments, ..
}) => {
// Ex) `type(obj) == type(1)`
let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() else {
return false;
};

if !(id == "type" && semantic.is_builtin("type")) {
return false;
};

// Allow comparison for types which are not obvious.
arguments
.args
.first()
.is_some_and(|arg| !arg.is_name_expr() && !is_const_none(arg))
}
Expr::Name(ast::ExprName { id, .. }) => {
// Ex) `type(obj) == int`
matches!(
id.as_str(),
"int" | "str" | "float" | "bool" | "complex" | "bytes" | "list" | "dict" | "set"
) && semantic.is_builtin(id)
}
_ => false,
}
}
Loading

0 comments on commit df807ff

Please sign in to comment.