From 3ec2ee02c4b9134c69a5aac86a6e2f4f7cb2685b Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 10 Oct 2023 20:32:30 -0400 Subject: [PATCH] Allow is and is not for type comparisons --- .../test/fixtures/pycodestyle/E721.py | 71 +++++++---- .../ruff_linter/src/rules/pycodestyle/mod.rs | 19 +++ .../pycodestyle/rules/type_comparison.rs | 115 +++++++++++++++--- ...les__pycodestyle__tests__E721_E721.py.snap | 115 ++++++++---------- ...destyle__tests__preview__E721_E721.py.snap | 112 +++++++++++++++++ 5 files changed, 328 insertions(+), 104 deletions(-) create mode 100644 crates/ruff_linter/src/rules/pycodestyle/snapshots/ruff_linter__rules__pycodestyle__tests__preview__E721_E721.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pycodestyle/E721.py b/crates/ruff_linter/resources/test/fixtures/pycodestyle/E721.py index 6eacc0cb0c9ab9..26776e816b73ee 100644 --- a/crates/ruff_linter/resources/test/fixtures/pycodestyle/E721.py +++ b/crates/ruff_linter/resources/test/fixtures/pycodestyle/E721.py @@ -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 @@ -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 @@ -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): diff --git a/crates/ruff_linter/src/rules/pycodestyle/mod.rs b/crates/ruff_linter/src/rules/pycodestyle/mod.rs index b46ca90532c8df..ddb1c09f9ee4e3 100644 --- a/crates/ruff_linter/src/rules/pycodestyle/mod.rs +++ b/crates/ruff_linter/src/rules/pycodestyle/mod.rs @@ -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}; @@ -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( diff --git a/crates/ruff_linter/src/rules/pycodestyle/rules/type_comparison.rs b/crates/ruff_linter/src/rules/pycodestyle/rules/type_comparison.rs index b8ff329a3ee5bf..1a7c3a67c7d5c4 100644 --- a/crates/ruff_linter/src/rules/pycodestyle/rules/type_comparison.rs +++ b/crates/ruff_linter/src/rules/pycodestyle/rules/type_comparison.rs @@ -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 /// ``` /// @@ -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() @@ -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(), + )); } } } @@ -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, .. }) => { @@ -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, + } +} diff --git a/crates/ruff_linter/src/rules/pycodestyle/snapshots/ruff_linter__rules__pycodestyle__tests__E721_E721.py.snap b/crates/ruff_linter/src/rules/pycodestyle/snapshots/ruff_linter__rules__pycodestyle__tests__E721_E721.py.snap index c1947fa971a73a..e5b81848e86fe0 100644 --- a/crates/ruff_linter/src/rules/pycodestyle/snapshots/ruff_linter__rules__pycodestyle__tests__E721_E721.py.snap +++ b/crates/ruff_linter/src/rules/pycodestyle/snapshots/ruff_linter__rules__pycodestyle__tests__E721_E721.py.snap @@ -17,7 +17,7 @@ E721.py:5:4: E721 Do not compare types, use `isinstance()` 5 | if type(res) != type(""): | ^^^^^^^^^^^^^^^^^^^^^ E721 6 | pass -7 | #: E721 +7 | #: Okay | E721.py:15:4: E721 Do not compare types, use `isinstance()` @@ -34,7 +34,7 @@ E721.py:18:8: E721 Do not compare types, use `isinstance()` | 16 | pass 17 | #: E721 -18 | assert type(res) == type(False) +18 | assert type(res) == type(False) or type(res) == type(None) | ^^^^^^^^^^^^^^^^^^^^^^^^ E721 19 | #: E721 20 | assert type(res) == type([]) @@ -42,7 +42,7 @@ E721.py:18:8: E721 Do not compare types, use `isinstance()` E721.py:20:8: E721 Do not compare types, use `isinstance()` | -18 | assert type(res) == type(False) +18 | assert type(res) == type(False) or type(res) == type(None) 19 | #: E721 20 | assert type(res) == type([]) | ^^^^^^^^^^^^^^^^^^^^^ E721 @@ -77,97 +77,84 @@ E721.py:26:8: E721 Do not compare types, use `isinstance()` 26 | assert type(res) == type((0)) | ^^^^^^^^^^^^^^^^^^^^^^ E721 27 | #: E721 -28 | assert type(res) != type((1,)) +28 | assert type(res) != type((1, )) | E721.py:28:8: E721 Do not compare types, use `isinstance()` | 26 | assert type(res) == type((0)) 27 | #: E721 -28 | assert type(res) != type((1,)) - | ^^^^^^^^^^^^^^^^^^^^^^^ E721 -29 | #: E721 -30 | assert type(res) is type((1,)) +28 | assert type(res) != type((1, )) + | ^^^^^^^^^^^^^^^^^^^^^^^^ E721 +29 | #: Okay +30 | assert type(res) is type((1, )) | E721.py:30:8: E721 Do not compare types, use `isinstance()` | -28 | assert type(res) != type((1,)) -29 | #: E721 -30 | assert type(res) is type((1,)) - | ^^^^^^^^^^^^^^^^^^^^^^^ E721 -31 | #: E721 -32 | assert type(res) is not type((1,)) +28 | assert type(res) != type((1, )) +29 | #: Okay +30 | assert type(res) is type((1, )) + | ^^^^^^^^^^^^^^^^^^^^^^^^ E721 +31 | #: Okay +32 | assert type(res) is not type((1, )) | E721.py:32:8: E721 Do not compare types, use `isinstance()` | -30 | assert type(res) is type((1,)) -31 | #: E721 -32 | assert type(res) is not type((1,)) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ E721 +30 | assert type(res) is type((1, )) +31 | #: Okay +32 | assert type(res) is not type((1, )) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ E721 33 | #: E211 E721 -34 | assert type(res) == type( +34 | assert type(res) == type ([2, ]) | E721.py:34:8: E721 Do not compare types, use `isinstance()` | -32 | assert type(res) is not type((1,)) -33 | #: E211 E721 -34 | assert type(res) == type( - | ________^ -35 | | [ -36 | | 2, -37 | | ] -38 | | ) - | |_^ E721 -39 | #: E201 E201 E202 E721 -40 | assert type(res) == type(()) - | - -E721.py:40:8: E721 Do not compare types, use `isinstance()` - | -38 | ) -39 | #: E201 E201 E202 E721 -40 | assert type(res) == type(()) - | ^^^^^^^^^^^^^^^^^^^^^ E721 -41 | #: E201 E202 E721 -42 | assert type(res) == type((0,)) +32 | assert type(res) is not type((1, )) +33 | #: E211 E721 +34 | assert type(res) == type ([2, ]) + | ^^^^^^^^^^^^^^^^^^^^^^^^^ E721 +35 | #: E201 E201 E202 E721 +36 | assert type(res) == type( ( ) ) | -E721.py:42:8: E721 Do not compare types, use `isinstance()` +E721.py:36:8: E721 Do not compare types, use `isinstance()` | -40 | assert type(res) == type(()) -41 | #: E201 E202 E721 -42 | assert type(res) == type((0,)) - | ^^^^^^^^^^^^^^^^^^^^^^^ E721 -43 | -44 | #: Okay +34 | assert type(res) == type ([2, ]) +35 | #: E201 E201 E202 E721 +36 | assert type(res) == type( ( ) ) + | ^^^^^^^^^^^^^^^^^^^^^^^^ E721 +37 | #: E201 E202 E721 +38 | assert type(res) == type( (0, ) ) | -E721.py:63:8: E721 Do not compare types, use `isinstance()` +E721.py:38:8: E721 Do not compare types, use `isinstance()` | -62 | #: E721 -63 | assert type(res) is int - | ^^^^^^^^^^^^^^^^ E721 +36 | assert type(res) == type( ( ) ) +37 | #: E201 E202 E721 +38 | assert type(res) == type( (0, ) ) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ E721 +39 | #: | -E721.py:69:12: E721 Do not compare types, use `isinstance()` +E721.py:96:12: E721 Do not compare types, use `isinstance()` | -67 | def asdf(self, value: str | None): -68 | #: E721 -69 | if type(value) is str: +94 | def asdf(self, value: str | None): +95 | #: E721 +96 | if type(value) is str: | ^^^^^^^^^^^^^^^^^^ E721 -70 | ... +97 | ... | -E721.py:79:12: E721 Do not compare types, use `isinstance()` - | -77 | def asdf(self, value: str | None): -78 | #: E721 -79 | if type(value) is str: - | ^^^^^^^^^^^^^^^^^^ E721 -80 | ... - | +E721.py:106:12: E721 Do not compare types, use `isinstance()` + | +104 | def asdf(self, value: str | None): +105 | #: E721 +106 | if type(value) is str: + | ^^^^^^^^^^^^^^^^^^ E721 +107 | ... + | diff --git a/crates/ruff_linter/src/rules/pycodestyle/snapshots/ruff_linter__rules__pycodestyle__tests__preview__E721_E721.py.snap b/crates/ruff_linter/src/rules/pycodestyle/snapshots/ruff_linter__rules__pycodestyle__tests__preview__E721_E721.py.snap new file mode 100644 index 00000000000000..0c75d706f540d7 --- /dev/null +++ b/crates/ruff_linter/src/rules/pycodestyle/snapshots/ruff_linter__rules__pycodestyle__tests__preview__E721_E721.py.snap @@ -0,0 +1,112 @@ +--- +source: crates/ruff_linter/src/rules/pycodestyle/mod.rs +--- +E721.py:2:4: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks + | +1 | #: E721 +2 | if type(res) == type(42): + | ^^^^^^^^^^^^^^^^^^^^^ E721 +3 | pass +4 | #: E721 + | + +E721.py:5:4: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks + | +3 | pass +4 | #: E721 +5 | if type(res) != type(""): + | ^^^^^^^^^^^^^^^^^^^^^ E721 +6 | pass +7 | #: Okay + | + +E721.py:18:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks + | +16 | pass +17 | #: E721 +18 | assert type(res) == type(False) or type(res) == type(None) + | ^^^^^^^^^^^^^^^^^^^^^^^^ E721 +19 | #: E721 +20 | assert type(res) == type([]) + | + +E721.py:20:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks + | +18 | assert type(res) == type(False) or type(res) == type(None) +19 | #: E721 +20 | assert type(res) == type([]) + | ^^^^^^^^^^^^^^^^^^^^^ E721 +21 | #: E721 +22 | assert type(res) == type(()) + | + +E721.py:22:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks + | +20 | assert type(res) == type([]) +21 | #: E721 +22 | assert type(res) == type(()) + | ^^^^^^^^^^^^^^^^^^^^^ E721 +23 | #: E721 +24 | assert type(res) == type((0,)) + | + +E721.py:24:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks + | +22 | assert type(res) == type(()) +23 | #: E721 +24 | assert type(res) == type((0,)) + | ^^^^^^^^^^^^^^^^^^^^^^^ E721 +25 | #: E721 +26 | assert type(res) == type((0)) + | + +E721.py:26:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks + | +24 | assert type(res) == type((0,)) +25 | #: E721 +26 | assert type(res) == type((0)) + | ^^^^^^^^^^^^^^^^^^^^^^ E721 +27 | #: E721 +28 | assert type(res) != type((1, )) + | + +E721.py:28:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks + | +26 | assert type(res) == type((0)) +27 | #: E721 +28 | assert type(res) != type((1, )) + | ^^^^^^^^^^^^^^^^^^^^^^^^ E721 +29 | #: Okay +30 | assert type(res) is type((1, )) + | + +E721.py:34:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks + | +32 | assert type(res) is not type((1, )) +33 | #: E211 E721 +34 | assert type(res) == type ([2, ]) + | ^^^^^^^^^^^^^^^^^^^^^^^^^ E721 +35 | #: E201 E201 E202 E721 +36 | assert type(res) == type( ( ) ) + | + +E721.py:36:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks + | +34 | assert type(res) == type ([2, ]) +35 | #: E201 E201 E202 E721 +36 | assert type(res) == type( ( ) ) + | ^^^^^^^^^^^^^^^^^^^^^^^^ E721 +37 | #: E201 E202 E721 +38 | assert type(res) == type( (0, ) ) + | + +E721.py:38:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks + | +36 | assert type(res) == type( ( ) ) +37 | #: E201 E202 E721 +38 | assert type(res) == type( (0, ) ) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ E721 +39 | #: + | + +