Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow is and is not for direct type comparisons #7905

Merged
merged 1 commit into from
Oct 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Comment on lines -22 to 28
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the "Use instead" section needs to contain the following:

if type(obj) is 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
Loading