From c0cb73ab1632ad2de9be0b771e4bedc006e4b34d Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 14 Sep 2022 21:10:29 -0400 Subject: [PATCH] Implement E721 (#193) --- README.md | 7 +- examples/generate_rules_table.rs | 1 + resources/test/fixtures/E721.py | 46 +++++++++++ resources/test/fixtures/pyproject.toml | 1 + src/ast/checks.rs | 40 ++++++++++ src/check_ast.rs | 16 +++- src/checks.rs | 10 ++- src/linter.rs | 102 +++++++++++++++++++++++++ src/pyproject.rs | 1 + src/settings.rs | 1 + 10 files changed, 217 insertions(+), 8 deletions(-) create mode 100644 resources/test/fixtures/E721.py diff --git a/README.md b/README.md index 6fe207e29f..ff8df2981d 100644 --- a/README.md +++ b/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. -- 6 logical rules. +- 4 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 '...' | diff --git a/examples/generate_rules_table.rs b/examples/generate_rules_table.rs index 99ec9c85f2..d288d28e57 100644 --- a/examples/generate_rules_table.rs +++ b/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()), diff --git a/resources/test/fixtures/E721.py b/resources/test/fixtures/E721.py new file mode 100644 index 0000000000..9b379fefc8 --- /dev/null +++ b/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() diff --git a/resources/test/fixtures/pyproject.toml b/resources/test/fixtures/pyproject.toml index 44a265aa18..22e6c7e2a2 100644 --- a/resources/test/fixtures/pyproject.toml +++ b/resources/test/fixtures/pyproject.toml @@ -8,6 +8,7 @@ select = [ "E712", "E713", "E714", + "E721", "E722", "E731", "E741", diff --git a/src/ast/checks.rs b/src/ast/checks.rs index 9fea732712..81baaf50d4 100644 --- a/src/ast/checks.rs +++ b/src/ast/checks.rs @@ -476,6 +476,46 @@ pub fn check_is_literal( checks } +/// Check TypeComparison compliance. +pub fn check_type_comparison( + ops: &Vec, + comparators: &Vec, + location: Location, +) -> Vec { + let mut checks: Vec = 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], diff --git a/src/check_ast.rs b/src/check_ast.rs index f163a51173..628ab3fadd 100644 --- a/src/check_ast.rs +++ b/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); diff --git a/src/checks.rs b/src/checks.rs index d79e50276d..cafd8d6419 100644 --- a/src/checks.rs +++ b/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__`") } diff --git a/src/linter.rs b/src/linter.rs index fab8ceed09..cd2f447882 100644 --- a/src/linter.rs +++ b/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( diff --git a/src/pyproject.rs b/src/pyproject.rs index 28ed17daf8..48b09c71fa 100644 --- a/src/pyproject.rs +++ b/src/pyproject.rs @@ -266,6 +266,7 @@ other-attribute = 1 CheckCode::E712, CheckCode::E713, CheckCode::E714, + CheckCode::E721, CheckCode::E722, CheckCode::E731, CheckCode::E741, diff --git a/src/settings.rs b/src/settings.rs index 4a379ee50a..502d5a84f5 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -49,6 +49,7 @@ impl Settings { CheckCode::E712, CheckCode::E713, CheckCode::E714, + CheckCode::E721, CheckCode::E722, CheckCode::E731, CheckCode::E741,