Skip to content

Commit

Permalink
[pycodestyle] Remove deprecated functionality from `type-comparison…
Browse files Browse the repository at this point in the history 
…` (`E721`) (#11220)

## Summary

Stabilizes `E721` behavior implemented in #7905.

The functionality change in `E721` was implemented in #7905, released in
[v0.1.2](https://github.com/astral-sh/ruff/releases/tag/v0.1.2). And
seems functionally stable since #9676, without an explicit release but
would correspond to
[v0.2.0](https://github.com/astral-sh/ruff/releases/tag/v0.2.0). So the
deprecated functionally should be removable in the next minor release.

resolves: #6465
  • Loading branch information
@augustelalande
augustelalande authored Jun 25, 2024
1 parent ae6d966 commit 2a28e97
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 317 deletions.
1 change: 0 additions & 1 deletion crates/ruff_linter/src/rules/pycodestyle/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ mod tests {
}

#[test_case(Rule::IsLiteral, Path::new("constant_literals.py"))]
#[test_case(Rule::TypeComparison, Path::new("E721.py"))]
#[test_case(Rule::ModuleImportNotAtTopOfFile, Path::new("E402_2.py"))]
#[test_case(Rule::RedundantBackslash, Path::new("E502.py"))]
#[test_case(Rule::TooManyNewlinesAtEndOfFile, Path::new("W391_0.py"))]
Expand Down
118 changes: 7 additions & 111 deletions crates/ruff_linter/src/rules/pycodestyle/rules/type_comparison.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ 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 using `==` and other comparison
Expand Down Expand Up @@ -37,119 +36,19 @@ use crate::settings::types::PreviewMode;
/// pass
/// ```
#[violation]
pub struct TypeComparison {
preview: PreviewMode,
}
pub struct TypeComparison;

impl Violation for TypeComparison {
#[derive_message_formats]
fn message(&self) -> String {
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"
),
}
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()
.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 semantic = checker.semantic();

if !semantic.match_builtin_expr(func, "type") {
continue;
}

// Right-hand side must be, e.g., `type(1)` or `int`.
match right {
Expr::Call(ast::ExprCall {
func, arguments, ..
}) => {
// Ex) `type(obj) is type(1)`
if semantic.match_builtin_expr(func, "type") {
// Allow comparison for types which are not obvious.
if arguments
.args
.first()
.is_some_and(|arg| !arg.is_name_expr() && !arg.is_none_literal_expr())
{
checker.diagnostics.push(Diagnostic::new(
TypeComparison {
preview: PreviewMode::Disabled,
},
compare.range(),
));
}
}
}
Expr::Attribute(ast::ExprAttribute { value, .. }) => {
// Ex) `type(obj) is types.NoneType`
if semantic
.resolve_qualified_name(value.as_ref())
.is_some_and(|qualified_name| {
matches!(qualified_name.segments(), ["types", ..])
})
{
checker.diagnostics.push(Diagnostic::new(
TypeComparison {
preview: PreviewMode::Disabled,
},
compare.range(),
));
}
}
Expr::Name(ast::ExprName { id, .. }) => {
// Ex) `type(obj) is int`
if matches!(
id.as_str(),
"int"
| "str"
| "float"
| "bool"
| "complex"
| "bytes"
| "list"
| "dict"
| "set"
| "memoryview"
) && semantic.has_builtin_binding(id)
{
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()
Expand All @@ -165,12 +64,9 @@ pub(crate) fn preview_type_comparison(checker: &mut Checker, compare: &ast::Expr
}

// Disallow the comparison.
checker.diagnostics.push(Diagnostic::new(
TypeComparison {
preview: PreviewMode::Enabled,
},
compare.range(),
));
checker
.diagnostics
.push(Diagnostic::new(TypeComparison, compare.range()));
}
}
}
Expand Down
85 changes: 28 additions & 57 deletions ...src/rules/pycodestyle/snapshots/ruff_linter__rules__pycodestyle__tests__E721_E721.py.snap
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
---
source: crates/ruff_linter/src/rules/pycodestyle/mod.rs
---
E721.py:2:4: E721 Do not compare types, use `isinstance()`
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):
Expand All @@ -10,7 +10,7 @@ E721.py:2:4: E721 Do not compare types, use `isinstance()`
4 | #: E721
|

E721.py:5:4: E721 Do not compare types, use `isinstance()`
E721.py:5:4: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
|
3 | pass
4 | #: E721
Expand All @@ -20,7 +20,7 @@ E721.py:5:4: E721 Do not compare types, use `isinstance()`
7 | #: E721
|

E721.py:8:4: E721 Do not compare types, use `isinstance()`
E721.py:8:4: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
|
6 | pass
7 | #: E721
Expand All @@ -30,17 +30,7 @@ E721.py:8:4: E721 Do not compare types, use `isinstance()`
10 | #: Okay
|

E721.py:18:4: E721 Do not compare types, use `isinstance()`
|
16 | import types
17 |
18 | if type(res) is not types.ListType:
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ E721
19 | pass
20 | #: E721
|

E721.py:21:8: E721 Do not compare types, use `isinstance()`
E721.py:21:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
|
19 | pass
20 | #: E721
Expand All @@ -50,7 +40,7 @@ E721.py:21:8: E721 Do not compare types, use `isinstance()`
23 | assert type(res) == type([])
|

E721.py:23:8: E721 Do not compare types, use `isinstance()`
E721.py:23:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
|
21 | assert type(res) == type(False) or type(res) == type(None)
22 | #: E721
Expand All @@ -60,7 +50,7 @@ E721.py:23:8: E721 Do not compare types, use `isinstance()`
25 | assert type(res) == type(())
|

E721.py:25:8: E721 Do not compare types, use `isinstance()`
E721.py:25:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
|
23 | assert type(res) == type([])
24 | #: E721
Expand All @@ -70,7 +60,7 @@ E721.py:25:8: E721 Do not compare types, use `isinstance()`
27 | assert type(res) == type((0,))
|

E721.py:27:8: E721 Do not compare types, use `isinstance()`
E721.py:27:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
|
25 | assert type(res) == type(())
26 | #: E721
Expand All @@ -80,7 +70,7 @@ E721.py:27:8: E721 Do not compare types, use `isinstance()`
29 | assert type(res) == type((0))
|

E721.py:29:8: E721 Do not compare types, use `isinstance()`
E721.py:29:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
|
27 | assert type(res) == type((0,))
28 | #: E721
Expand All @@ -90,7 +80,7 @@ E721.py:29:8: E721 Do not compare types, use `isinstance()`
31 | assert type(res) != type((1, ))
|

E721.py:31:8: E721 Do not compare types, use `isinstance()`
E721.py:31:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
|
29 | assert type(res) == type((0))
30 | #: E721
Expand All @@ -100,27 +90,7 @@ E721.py:31:8: E721 Do not compare types, use `isinstance()`
33 | assert type(res) is type((1, ))
|

E721.py:33:8: E721 Do not compare types, use `isinstance()`
|
31 | assert type(res) != type((1, ))
32 | #: Okay
33 | assert type(res) is type((1, ))
| ^^^^^^^^^^^^^^^^^^^^^^^^ E721
34 | #: Okay
35 | assert type(res) is not type((1, ))
|

E721.py:35:8: E721 Do not compare types, use `isinstance()`
|
33 | assert type(res) is type((1, ))
34 | #: Okay
35 | assert type(res) is not type((1, ))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ E721
36 | #: E211 E721
37 | assert type(res) == type ([2, ])
|

E721.py:37:8: E721 Do not compare types, use `isinstance()`
E721.py:37:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
|
35 | assert type(res) is not type((1, ))
36 | #: E211 E721
Expand All @@ -130,7 +100,7 @@ E721.py:37:8: E721 Do not compare types, use `isinstance()`
39 | assert type(res) == type( ( ) )
|

E721.py:39:8: E721 Do not compare types, use `isinstance()`
E721.py:39:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
|
37 | assert type(res) == type ([2, ])
38 | #: E201 E201 E202 E721
Expand All @@ -140,7 +110,7 @@ E721.py:39:8: E721 Do not compare types, use `isinstance()`
41 | assert type(res) == type( (0, ) )
|

E721.py:41:8: E721 Do not compare types, use `isinstance()`
E721.py:41:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
|
39 | assert type(res) == type( ( ) )
40 | #: E201 E202 E721
Expand All @@ -149,25 +119,26 @@ E721.py:41:8: E721 Do not compare types, use `isinstance()`
42 | #:
|

E721.py:107:12: E721 Do not compare types, use `isinstance()`
|
105 | def asdf(self, value: str | None):
106 | #: E721
107 | if type(value) is str:
| ^^^^^^^^^^^^^^^^^^ E721
108 | ...
|
E721.py:59:4: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
|
57 | pass
58 | #: E721
59 | if type(res) == type:
| ^^^^^^^^^^^^^^^^^ E721
60 | pass
61 | #: Okay
|

E721.py:117:12: E721 Do not compare types, use `isinstance()`
E721.py:140:1: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
|
115 | def asdf(self, value: str | None):
116 | #: E721
117 | if type(value) is str:
| ^^^^^^^^^^^^^^^^^^ E721
118 | ...
139 | #: E721
140 | dtype == float
| ^^^^^^^^^^^^^^ E721
141 |
142 | import builtins
|

E721.py:144:4: E721 Do not compare types, use `isinstance()`
E721.py:144:4: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
|
142 | import builtins
143 |
Expand Down
Loading

0 comments on commit 2a28e97

Please sign in to comment.