Implement E721 (#193)

README.md

@@ -124,14 +124,14 @@ ruff's goal is to achieve feature-parity with Flake8 when used (1) without any p
 stylistic checks; limiting to Python 3 obviates the need for certain compatibility checks.)
 
 Under those conditions, Flake8 implements about 60 rules, give or take. At time of writing, ruff
-implements 40 rules. (Note that these 40 rules likely cover a disproportionate share of errors:
+implements 41 rules. (Note that these 41 rules likely cover a disproportionate share of errors:
 unused imports, undefined variables, etc.)
 
 The unimplemented rules are tracked in #170, and include:
 
 - 14 rules related to string `.format` calls.
-- 1 rule related to parsing and syntax.
+- 4 logical rules.
-- 6 logical rules.
+- 1 rule related to parsing.
 
 Beyond rule-set parity, ruff suffers from the following limitations vis-à-vis Flake8:
 
@@ -149,6 +149,7 @@ Beyond rule-set parity, ruff suffers from the following limitations vis-à-vis F
 | E712 | TrueFalseComparison | Comparison to `True` should be `cond is True` |
 | E713 | NotInTest | Test for membership should be `not in` |
 | E714 | NotIsTest | Test for object identity should be `is not` |
+| E721 | TypeComparison | do not compare types, use `isinstance()` |
 | E722 | DoNotUseBareExcept | Do not use bare `except` |
 | E731 | DoNotAssignLambda | Do not assign a lambda expression, use a def |
 | E741 | AmbiguousVariableName | ambiguous variable name '...' |

examples/generate_rules_table.rs

@@ -36,6 +36,7 @@ fn main() {
         CheckKind::TooManyExpressionsInStarredAssignment,
         CheckKind::TrueFalseComparison(true, RejectedCmpop::Eq),
         CheckKind::TwoStarredExpressions,
+        CheckKind::TypeComparison,
         CheckKind::UndefinedExport("...".to_string()),
         CheckKind::UndefinedLocal("...".to_string()),
         CheckKind::UndefinedName("...".to_string()),

resources/test/fixtures/E721.py

@@ -0,0 +1,46 @@
+#: E721
+if type(res) == type(42):
+    pass
+#: E721
+if type(res) != type(""):
+    pass
+#: E721
+import types
+
+if res == types.IntType:
+    pass
+#: E721
+import types
+
+if type(res) is not types.ListType:
+    pass
+#: E721
+assert type(res) == type(False) or type(res) == type(None)
+#: E721
+assert type(res) == type([])
+#: E721
+assert type(res) == type(())
+#: E721
+assert type(res) == type((0,))
+#: 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,))
+#: E211 E721
+assert type(res) == type(
+    [
+        2,
+    ]
+)
+#: E201 E201 E202 E721
+assert type(res) == type(())
+#: E201 E202 E721
+assert type(res) == type((0,))
+
+assert type(res) == type(res)
+assert type(res) == type(int)
+assert type(res) == type()

resources/test/fixtures/pyproject.toml

@@ -8,6 +8,7 @@ select = [
     "E712",
     "E713",
     "E714",
+    "E721",
     "E722",
     "E731",
     "E741",

src/ast/checks.rs

@@ -476,6 +476,46 @@ pub fn check_is_literal(
     checks
 }
 
+/// Check TypeComparison compliance.
+pub fn check_type_comparison(
+    ops: &Vec<Cmpop>,
+    comparators: &Vec<Expr>,
+    location: Location,
+) -> Vec<Check> {
+    let mut checks: Vec<Check> = vec![];
+
+    for (op, right) in izip!(ops, comparators) {
+        if matches!(op, Cmpop::Is | Cmpop::IsNot | Cmpop::Eq | Cmpop::NotEq) {
+            match &right.node {
+                ExprKind::Call { func, args, .. } => {
+                    if let ExprKind::Name { id, .. } = &func.node {
+                        // Ex) type(False)
+                        if id == "type" {
+                            if let Some(arg) = args.first() {
+                                // Allow comparison for types which are not obvious.
+                                if !matches!(arg.node, ExprKind::Name { .. }) {
+                                    checks.push(Check::new(CheckKind::TypeComparison, location));
+                                }
+                            }
+                        }
+                    }
+                }
+                ExprKind::Attribute { value, .. } => {
+                    if let ExprKind::Name { id, .. } = &value.node {
+                        // Ex) types.IntType
+                        if id == "types" {
+                            checks.push(Check::new(CheckKind::TypeComparison, location));
+                        }
+                    }
+                }
+                _ => {}
+            }
+        }
+    }
+
+    checks
+}
+
 /// Check TwoStarredExpressions and TooManyExpressionsInStarredAssignment compliance.
 pub fn check_starred_expressions(
     elts: &[Expr],

src/check_ast.rs

@@ -746,6 +746,14 @@ where
                         expr.location,
                     ));
                 }
+
+                if self.settings.select.contains(&CheckCode::E721) {
+                    self.checks.extend(checks::check_type_comparison(
+                        ops,
+                        comparators,
+                        expr.location,
+                    ));
+                }
             }
             ExprKind::Constant {
                 value: Constant::Str(value),
@@ -813,7 +821,7 @@ where
                 } else if match_name_or_attr(func, "NamedTuple") {
                     self.visit_expr(func);
 
-                    // NamedTuple("a", [("a", int)])
+                    // Ex) NamedTuple("a", [("a", int)])
                     if args.len() > 1 {
                         match &args[1].node {
                             ExprKind::List { elts, .. } | ExprKind::Tuple { elts, .. } => {
@@ -837,7 +845,7 @@ where
                         }
                     }
 
-                    // NamedTuple("a", a=int)
+                    // Ex) NamedTuple("a", a=int)
                     for keyword in keywords {
                         let KeywordData { value, .. } = &keyword.node;
                         self.visit_annotation(value);
@@ -845,7 +853,7 @@ where
                 } else if match_name_or_attr(func, "TypedDict") {
                     self.visit_expr(func);
 
-                    // TypedDict("a", {"a": int})
+                    // Ex) TypedDict("a", {"a": int})
                     if args.len() > 1 {
                         if let ExprKind::Dict { keys, values } = &args[1].node {
                             for key in keys {
@@ -859,7 +867,7 @@ where
                         }
                     }
 
-                    // TypedDict("a", a=int)
+                    // Ex) TypedDict("a", a=int)
                     for keyword in keywords {
                         let KeywordData { value, .. } = &keyword.node;
                         self.visit_annotation(value);

src/checks.rs

@@ -14,6 +14,7 @@ pub enum CheckCode {
     E712,
     E713,
     E714,
+    E721,
     E722,
     E731,
     E741,
@@ -60,8 +61,9 @@ impl FromStr for CheckCode {
             "E711" => Ok(CheckCode::E711),
             "E712" => Ok(CheckCode::E712),
             "E713" => Ok(CheckCode::E713),
-            "E722" => Ok(CheckCode::E722),
             "E714" => Ok(CheckCode::E714),
+            "E721" => Ok(CheckCode::E721),
+            "E722" => Ok(CheckCode::E722),
             "E731" => Ok(CheckCode::E731),
             "E741" => Ok(CheckCode::E741),
             "E742" => Ok(CheckCode::E742),
@@ -86,6 +88,7 @@ impl FromStr for CheckCode {
             "F704" => Ok(CheckCode::F704),
             "F706" => Ok(CheckCode::F706),
             "F707" => Ok(CheckCode::F707),
+            "F722" => Ok(CheckCode::F722),
             "F821" => Ok(CheckCode::F821),
             "F822" => Ok(CheckCode::F822),
             "F823" => Ok(CheckCode::F823),
@@ -108,6 +111,7 @@ impl CheckCode {
             CheckCode::E712 => "E712",
             CheckCode::E713 => "E713",
             CheckCode::E714 => "E714",
+            CheckCode::E721 => "E721",
             CheckCode::E722 => "E722",
             CheckCode::E731 => "E731",
             CheckCode::E741 => "E741",
@@ -203,6 +207,7 @@ pub enum CheckKind {
     TooManyExpressionsInStarredAssignment,
     TrueFalseComparison(bool, RejectedCmpop),
     TwoStarredExpressions,
+    TypeComparison,
     UndefinedExport(String),
     UndefinedLocal(String),
     UndefinedName(String),
@@ -251,6 +256,7 @@ impl CheckKind {
             }
             CheckKind::TrueFalseComparison(_, _) => "TrueFalseComparison",
             CheckKind::TwoStarredExpressions => "TwoStarredExpressions",
+            CheckKind::TypeComparison => "TypeComparison",
             CheckKind::UndefinedExport(_) => "UndefinedExport",
             CheckKind::UndefinedLocal(_) => "UndefinedLocal",
             CheckKind::UndefinedName(_) => "UndefinedName",
@@ -297,6 +303,7 @@ impl CheckKind {
             CheckKind::TooManyExpressionsInStarredAssignment => &CheckCode::F621,
             CheckKind::TrueFalseComparison(_, _) => &CheckCode::E712,
             CheckKind::TwoStarredExpressions => &CheckCode::F622,
+            CheckKind::TypeComparison => &CheckCode::E721,
             CheckKind::UndefinedExport(_) => &CheckCode::F822,
             CheckKind::UndefinedLocal(_) => &CheckCode::F823,
             CheckKind::UndefinedName(_) => &CheckCode::F821,
@@ -409,6 +416,7 @@ impl CheckKind {
                 },
             },
             CheckKind::TwoStarredExpressions => "two starred expressions in assignment".to_string(),
+            CheckKind::TypeComparison => "do not compare types, use `isinstance()`".to_string(),
             CheckKind::UndefinedExport(name) => {
                 format!("Undefined name `{name}` in `__all__`")
             }

src/linter.rs

@@ -240,6 +240,108 @@ mod tests {
         Ok(())
     }
 
+    #[test]
+    fn e721() -> Result<()> {
+        let mut actual = check_path(
+            Path::new("./resources/test/fixtures/E721.py"),
+            &settings::Settings {
+                line_length: 88,
+                exclude: vec![],
+                select: BTreeSet::from([CheckCode::E721]),
+            },
+            &fixer::Mode::Generate,
+        )?;
+        actual.sort_by_key(|check| check.location);
+        let expected = vec![
+            Check {
+                kind: CheckKind::TypeComparison,
+                location: Location::new(2, 14),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::TypeComparison,
+                location: Location::new(5, 14),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::TypeComparison,
+                location: Location::new(10, 8),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::TypeComparison,
+                location: Location::new(15, 14),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::TypeComparison,
+                location: Location::new(18, 18),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::TypeComparison,
+                location: Location::new(18, 46),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::TypeComparison,
+                location: Location::new(20, 18),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::TypeComparison,
+                location: Location::new(22, 18),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::TypeComparison,
+                location: Location::new(24, 18),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::TypeComparison,
+                location: Location::new(26, 18),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::TypeComparison,
+                location: Location::new(28, 18),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::TypeComparison,
+                location: Location::new(30, 18),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::TypeComparison,
+                location: Location::new(32, 18),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::TypeComparison,
+                location: Location::new(34, 18),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::TypeComparison,
+                location: Location::new(40, 18),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::TypeComparison,
+                location: Location::new(42, 18),
+                fix: None,
+            },
+        ];
+        assert_eq!(actual.len(), expected.len());
+        for i in 0..actual.len() {
+            assert_eq!(actual[i], expected[i]);
+        }
+
+        Ok(())
+    }
+
     #[test]
     fn e722() -> Result<()> {
         let mut actual = check_path(

src/pyproject.rs

@@ -266,6 +266,7 @@ other-attribute = 1
                     CheckCode::E712,
                     CheckCode::E713,
                     CheckCode::E714,
+                    CheckCode::E721,
                     CheckCode::E722,
                     CheckCode::E731,
                     CheckCode::E741,

src/settings.rs

@@ -49,6 +49,7 @@ impl Settings {
                     CheckCode::E712,
                     CheckCode::E713,
                     CheckCode::E714,
+                    CheckCode::E721,
                     CheckCode::E722,
                     CheckCode::E731,
                     CheckCode::E741,