From fdb32330a95f59d806e790940ea8ae668ff86556 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 4 Oct 2022 14:31:52 -0400 Subject: [PATCH] Implement __metaclass__ = type removal (#324) --- README.md | 2 + resources/test/fixtures/U001.py | 13 ++++ src/ast/checks.rs | 20 ++++++ src/check_ast.rs | 36 +++++++++- src/checks.rs | 71 +++++++++++++------- src/linter.rs | 12 ++++ src/snapshots/ruff__linter__tests__u001.snap | 37 ++++++++++ 7 files changed, 165 insertions(+), 26 deletions(-) create mode 100644 resources/test/fixtures/U001.py create mode 100644 src/snapshots/ruff__linter__tests__u001.snap diff --git a/README.md b/README.md index afd954d23e..a9bb4561f3 100644 --- a/README.md +++ b/README.md @@ -213,6 +213,7 @@ ruff also implements some of the most popular Flake8 plugins natively, including - [`flake8-builtins`](https://pypi.org/project/flake8-builtins/) - [`flake8-super`](https://pypi.org/project/flake8-super/) - [`flake8-print`](https://pypi.org/project/flake8-print/) +- [`pyupgrade`](https://pypi.org/project/pyupgrade/) (partial) Beyond rule-set parity, ruff suffers from the following limitations vis-à-vis Flake8: @@ -276,6 +277,7 @@ The 🛠 emoji indicates that a rule is automatically fixable by the `--fix` com | SPR001 | SuperCallWithParameters | Use `super()` instead of `super(__class__, self)` | | 🛠 | | T201 | PrintFound | `print` found | | 🛠 | | T203 | PPrintFound | `pprint` found | | 🛠 | +| U001 | UselessMetaclassType | `__metaclass__ = type` is implied | | 🛠 | | R001 | UselessObjectInheritance | Class `...` inherits from object | | 🛠 | | R002 | NoAssertEquals | `assertEquals` is deprecated, use `assertEqual` instead | | 🛠 | | M001 | UnusedNOQA | Unused `noqa` directive | | 🛠 | diff --git a/resources/test/fixtures/U001.py b/resources/test/fixtures/U001.py new file mode 100644 index 0000000000..7e83caa89e --- /dev/null +++ b/resources/test/fixtures/U001.py @@ -0,0 +1,13 @@ +class A: + __metaclass__ = type + + +class B: + __metaclass__ = type + + def __init__(self) -> None: + pass + + +class C(metaclass=type): + pass diff --git a/src/ast/checks.rs b/src/ast/checks.rs index ca2d263222..5e5b927d6d 100644 --- a/src/ast/checks.rs +++ b/src/ast/checks.rs @@ -116,6 +116,26 @@ pub fn check_do_not_assign_lambda(value: &Expr, location: Range) -> Option, + value: &Expr, + location: Range, +) -> Option { + if targets.len() == 1 { + if let ExprKind::Name { id, .. } = targets.first().map(|expr| &expr.node).unwrap() { + if id == "__metaclass__" { + if let ExprKind::Name { id, .. } = &value.node { + if id == "type" { + return Some(Check::new(CheckKind::UselessMetaclassType, location)); + } + } + } + } + } + None +} + fn is_ambiguous_name(name: &str) -> bool { name == "l" || name == "I" || name == "O" } diff --git a/src/check_ast.rs b/src/check_ast.rs index ddb9b96479..37c000eea3 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -19,7 +19,6 @@ use crate::ast::types::{ }; use crate::ast::visitor::{walk_excepthandler, Visitor}; use crate::ast::{checks, operations, visitor}; -use crate::autofix::fixes::remove_stmt; use crate::autofix::{fixer, fixes}; use crate::checks::{Check, CheckCode, CheckKind}; use crate::python::builtins::{BUILTINS, MAGIC_GLOBALS}; @@ -622,7 +621,7 @@ where } } } - StmtKind::Assign { value, .. } => { + StmtKind::Assign { targets, value, .. } => { if self.settings.select.contains(&CheckCode::E731) { if let Some(check) = checks::check_do_not_assign_lambda( value, @@ -631,6 +630,37 @@ where self.checks.push(check); } } + if self.settings.select.contains(&CheckCode::U001) { + if let Some(mut check) = checks::check_useless_metaclass_type( + targets, + value, + self.locate_check(Range::from_located(stmt)), + ) { + if matches!(self.autofix, fixer::Mode::Generate | fixer::Mode::Apply) { + let context = self.binding_context(); + let deleted: Vec<&Stmt> = self + .deletions + .iter() + .map(|index| self.parents[*index]) + .collect(); + + match fixes::remove_stmt( + self.parents[context.defined_by], + context.defined_in.map(|index| self.parents[index]), + &deleted, + ) { + Ok(fix) => { + if fix.content.is_empty() || fix.content == "pass" { + self.deletions.insert(context.defined_by); + } + check.amend(fix) + } + Err(e) => error!("Failed to fix unused imports: {}", e), + } + } + self.checks.push(check); + } + } } StmtKind::AnnAssign { value, .. } => { if self.settings.select.contains(&CheckCode::E731) { @@ -797,7 +827,7 @@ where .map(|index| self.parents[*index]) .collect(); - match remove_stmt( + match fixes::remove_stmt( self.parents[context.defined_by], context.defined_in.map(|index| self.parents[index]), &deleted, diff --git a/src/checks.rs b/src/checks.rs index 4e7fc606d9..2ae31b107e 100644 --- a/src/checks.rs +++ b/src/checks.rs @@ -53,7 +53,7 @@ pub const DEFAULT_CHECK_CODES: [CheckCode; 42] = [ CheckCode::F901, ]; -pub const ALL_CHECK_CODES: [CheckCode; 51] = [ +pub const ALL_CHECK_CODES: [CheckCode; 52] = [ // pycodestyle CheckCode::E402, CheckCode::E501, @@ -107,11 +107,13 @@ pub const ALL_CHECK_CODES: [CheckCode; 51] = [ // flake8-print CheckCode::T201, CheckCode::T203, - // Meta - CheckCode::M001, + // pyupgrade + CheckCode::U001, // Refactor CheckCode::R001, CheckCode::R002, + // Meta + CheckCode::M001, ]; #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize, Hash, PartialOrd, Ord)] @@ -169,6 +171,8 @@ pub enum CheckCode { // flake8-print T201, T203, + // pyupgrade + U001, // Refactor R001, R002, @@ -231,6 +235,8 @@ impl FromStr for CheckCode { "A003" => Ok(CheckCode::A003), // flake8-super "SPR001" => Ok(CheckCode::SPR001), + // pyupgrade + "U001" => Ok(CheckCode::U001), // Refactor "R001" => Ok(CheckCode::R001), "R002" => Ok(CheckCode::R002), @@ -297,6 +303,8 @@ impl CheckCode { // flake8-print CheckCode::T201 => "T201", CheckCode::T203 => "T203", + // pyupgrade + CheckCode::U001 => "U001", // Refactor CheckCode::R001 => "R001", CheckCode::R002 => "R002", @@ -372,6 +380,8 @@ impl CheckCode { // flake8-print CheckCode::T201 => CheckKind::PrintFound, CheckCode::T203 => CheckKind::PPrintFound, + // pyupgrade + CheckCode::U001 => CheckKind::UselessMetaclassType, // Refactor CheckCode::R001 => CheckKind::UselessObjectInheritance("...".to_string()), CheckCode::R002 => CheckKind::NoAssertEquals, @@ -439,6 +449,7 @@ pub enum CheckKind { UnusedImport(String), UnusedNOQA(Option), UnusedVariable(String), + UselessMetaclassType, UselessObjectInheritance(String), YieldOutsideFunction, // flake8-builtin @@ -483,7 +494,6 @@ impl CheckKind { CheckKind::ModuleImportNotAtTopOfFile => "ModuleImportNotAtTopOfFile", CheckKind::MultiValueRepeatedKeyLiteral => "MultiValueRepeatedKeyLiteral", CheckKind::MultiValueRepeatedKeyVariable(_) => "MultiValueRepeatedKeyVariable", - CheckKind::NoAssertEquals => "NoAssertEquals", CheckKind::NoneComparison(_) => "NoneComparison", CheckKind::NotInTest => "NotInTest", CheckKind::NotIsTest => "NotIsTest", @@ -497,9 +507,7 @@ impl CheckKind { CheckKind::UndefinedLocal(_) => "UndefinedLocal", CheckKind::UndefinedName(_) => "UndefinedName", CheckKind::UnusedImport(_) => "UnusedImport", - CheckKind::UnusedNOQA(_) => "UnusedNOQA", CheckKind::UnusedVariable(_) => "UnusedVariable", - CheckKind::UselessObjectInheritance(_) => "UselessObjectInheritance", CheckKind::YieldOutsideFunction => "YieldOutsideFunction", // flake8-builtins CheckKind::BuiltinVariableShadowing(_) => "BuiltinVariableShadowing", @@ -510,6 +518,13 @@ impl CheckKind { // flake8-print CheckKind::PrintFound => "PrintFound", CheckKind::PPrintFound => "PPrintFound", + // pyupgrade + CheckKind::UselessMetaclassType => "UselessMetaclassType", + // Refactor + CheckKind::NoAssertEquals => "NoAssertEquals", + CheckKind::UselessObjectInheritance(_) => "UselessObjectInheritance", + // Meta + CheckKind::UnusedNOQA(_) => "UnusedNOQA", } } @@ -542,7 +557,6 @@ impl CheckKind { CheckKind::ModuleImportNotAtTopOfFile => &CheckCode::E402, CheckKind::MultiValueRepeatedKeyLiteral => &CheckCode::F601, CheckKind::MultiValueRepeatedKeyVariable(_) => &CheckCode::F602, - CheckKind::NoAssertEquals => &CheckCode::R002, CheckKind::NoneComparison(_) => &CheckCode::E711, CheckKind::NotInTest => &CheckCode::E713, CheckKind::NotIsTest => &CheckCode::E714, @@ -557,9 +571,7 @@ impl CheckKind { CheckKind::UndefinedLocal(_) => &CheckCode::F823, CheckKind::UndefinedName(_) => &CheckCode::F821, CheckKind::UnusedImport(_) => &CheckCode::F401, - CheckKind::UnusedNOQA(_) => &CheckCode::M001, CheckKind::UnusedVariable(_) => &CheckCode::F841, - CheckKind::UselessObjectInheritance(_) => &CheckCode::R001, CheckKind::YieldOutsideFunction => &CheckCode::F704, // flake8-builtins CheckKind::BuiltinVariableShadowing(_) => &CheckCode::A001, @@ -570,6 +582,13 @@ impl CheckKind { // flake8-print CheckKind::PrintFound => &CheckCode::T201, CheckKind::PPrintFound => &CheckCode::T203, + // pyupgrade + CheckKind::UselessMetaclassType => &CheckCode::U001, + // Refactor + CheckKind::NoAssertEquals => &CheckCode::R002, + CheckKind::UselessObjectInheritance(_) => &CheckCode::R001, + // Meta + CheckKind::UnusedNOQA(_) => &CheckCode::M001, } } @@ -646,9 +665,6 @@ impl CheckKind { CheckKind::MultiValueRepeatedKeyVariable(name) => { format!("Dictionary key `{name}` repeated") } - CheckKind::NoAssertEquals => { - "`assertEquals` is deprecated, use `assertEqual` instead".to_string() - } CheckKind::NoneComparison(op) => match op { RejectedCmpop::Eq => "Comparison to `None` should be `cond is None`".to_string(), RejectedCmpop::NotEq => { @@ -700,16 +716,10 @@ impl CheckKind { CheckKind::UnusedVariable(name) => { format!("Local variable `{name}` is assigned to but never used") } - CheckKind::UselessObjectInheritance(name) => { - format!("Class `{name}` inherits from object") - } CheckKind::YieldOutsideFunction => { "`yield` or `yield from` statement outside of a function/method".to_string() } - CheckKind::UnusedNOQA(code) => match code { - None => "Unused `noqa` directive".to_string(), - Some(code) => format!("Unused `noqa` directive for: {code}"), - }, + // flake8-builtins CheckKind::BuiltinVariableShadowing(name) => { format!("Variable `{name}` is shadowing a python builtin") @@ -727,6 +737,20 @@ impl CheckKind { // flake8-print CheckKind::PrintFound => "`print` found".to_string(), CheckKind::PPrintFound => "`pprint` found".to_string(), + // pyupgrade + CheckKind::UselessMetaclassType => "`__metaclass__ = type` is implied".to_string(), + // Refactor + CheckKind::NoAssertEquals => { + "`assertEquals` is deprecated, use `assertEqual` instead".to_string() + } + CheckKind::UselessObjectInheritance(name) => { + format!("Class `{name}` inherits from object") + } + // Meta + CheckKind::UnusedNOQA(code) => match code { + None => "Unused `noqa` directive".to_string(), + Some(code) => format!("Unused `noqa` directive for: {code}"), + }, } } @@ -735,12 +759,13 @@ impl CheckKind { matches!( self, CheckKind::NoAssertEquals - | CheckKind::UselessObjectInheritance(_) - | CheckKind::UnusedNOQA(_) + | CheckKind::PPrintFound + | CheckKind::PrintFound | CheckKind::SuperCallWithParameters | CheckKind::UnusedImport(_) - | CheckKind::PrintFound - | CheckKind::PPrintFound + | CheckKind::UnusedNOQA(_) + | CheckKind::UselessMetaclassType + | CheckKind::UselessObjectInheritance(_) ) } } diff --git a/src/linter.rs b/src/linter.rs index f465bf30a0..efb436aed2 100644 --- a/src/linter.rs +++ b/src/linter.rs @@ -803,4 +803,16 @@ mod tests { insta::assert_yaml_snapshot!(checks); Ok(()) } + + #[test] + fn u001() -> Result<()> { + let mut checks = check_path( + Path::new("./resources/test/fixtures/U001.py"), + &settings::Settings::for_rule(CheckCode::U001), + &fixer::Mode::Generate, + )?; + checks.sort_by_key(|check| check.location); + insta::assert_yaml_snapshot!(checks); + Ok(()) + } } diff --git a/src/snapshots/ruff__linter__tests__u001.snap b/src/snapshots/ruff__linter__tests__u001.snap new file mode 100644 index 0000000000..a74ef4c609 --- /dev/null +++ b/src/snapshots/ruff__linter__tests__u001.snap @@ -0,0 +1,37 @@ +--- +source: src/linter.rs +expression: checks +--- +- kind: UselessMetaclassType + location: + row: 2 + column: 5 + end_location: + row: 2 + column: 25 + fix: + content: pass + location: + row: 2 + column: 5 + end_location: + row: 2 + column: 25 + applied: false +- kind: UselessMetaclassType + location: + row: 6 + column: 5 + end_location: + row: 6 + column: 25 + fix: + content: "" + location: + row: 6 + column: 1 + end_location: + row: 7 + column: 1 + applied: false +