From 4118df098b6cfb434011d5537a5c02586660b0e5 Mon Sep 17 00:00:00 2001 From: Aleksei Latyshev Date: Sat, 27 Jan 2024 21:38:02 +0100 Subject: [PATCH 1/2] [`refurb`] Implement `metaclass_abcmeta` (`FURB180`) --- .../resources/test/fixtures/refurb/FURB180.py | 58 +++++++++++++ .../src/checkers/ast/analyze/statement.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/refurb/mod.rs | 1 + .../rules/refurb/rules/metaclass_abcmeta.rs | 87 +++++++++++++++++++ .../ruff_linter/src/rules/refurb/rules/mod.rs | 2 + ...es__refurb__tests__FURB180_FURB180.py.snap | 81 +++++++++++++++++ ruff.schema.json | 1 + 8 files changed, 234 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/refurb/FURB180.py create mode 100644 crates/ruff_linter/src/rules/refurb/rules/metaclass_abcmeta.rs create mode 100644 crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB180_FURB180.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB180.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB180.py new file mode 100644 index 0000000000000..33576061fc23a --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB180.py @@ -0,0 +1,58 @@ +import abc +from abc import abstractmethod, ABCMeta + + +# Errors + +class A0(metaclass=abc.ABCMeta): + @abstractmethod + def foo(self): pass + + +class A1(metaclass=ABCMeta): + @abstractmethod + def foo(self): pass + + +class B0: + def __init_subclass__(cls, **kwargs): + super().__init_subclass__() + + +class B1: + pass + + +class A2(B0, B1, metaclass=ABCMeta): + @abstractmethod + def foo(self): pass + + +class A3(B0, before_metaclass=1, metaclass=abc.ABCMeta): + pass + + +# OK + +class Meta(type): + def __new__(cls, *args, **kwargs): + return super().__new__(cls, *args) + + +class A4(metaclass=Meta, no_metaclass=ABCMeta): + @abstractmethod + def foo(self): pass + + +class A5(metaclass=Meta): + pass + + +class A6(abc.ABC): + @abstractmethod + def foo(self): pass + + +class A7(B0, abc.ABC, B1): + @abstractmethod + def foo(self): pass diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 481e52a008340..fae6b0b527b99 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -516,6 +516,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::BadDunderMethodName) { pylint::rules::bad_dunder_method_name(checker, body); } + if checker.enabled(Rule::MetaClassABCMeta) { + refurb::rules::metaclass_abcmeta(checker, class_def); + } } Stmt::Import(ast::StmtImport { names, range: _ }) => { if checker.enabled(Rule::MultipleImportsOnOneLine) { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 6f802c79bd1e4..ded0fe7eeff7e 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -1001,6 +1001,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Refurb, "169") => (RuleGroup::Preview, rules::refurb::rules::TypeNoneComparison), (Refurb, "171") => (RuleGroup::Preview, rules::refurb::rules::SingleItemMembershipTest), (Refurb, "177") => (RuleGroup::Preview, rules::refurb::rules::ImplicitCwd), + (Refurb, "180") => (RuleGroup::Preview, rules::refurb::rules::MetaClassABCMeta), (Refurb, "181") => (RuleGroup::Preview, rules::refurb::rules::HashlibDigestHex), // flake8-logging diff --git a/crates/ruff_linter/src/rules/refurb/mod.rs b/crates/ruff_linter/src/rules/refurb/mod.rs index fde53bda5f2c5..26ba58041933b 100644 --- a/crates/ruff_linter/src/rules/refurb/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/mod.rs @@ -32,6 +32,7 @@ mod tests { #[test_case(Rule::IsinstanceTypeNone, Path::new("FURB168.py"))] #[test_case(Rule::TypeNoneComparison, Path::new("FURB169.py"))] #[test_case(Rule::RedundantLogBase, Path::new("FURB163.py"))] + #[test_case(Rule::MetaClassABCMeta, Path::new("FURB180.py"))] #[test_case(Rule::HashlibDigestHex, Path::new("FURB181.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); diff --git a/crates/ruff_linter/src/rules/refurb/rules/metaclass_abcmeta.rs b/crates/ruff_linter/src/rules/refurb/rules/metaclass_abcmeta.rs new file mode 100644 index 0000000000000..043c8823f481d --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/rules/metaclass_abcmeta.rs @@ -0,0 +1,87 @@ +use crate::checkers::ast::Checker; +use crate::importer::ImportRequest; +use itertools::Itertools; +use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::helpers::map_callable; +use ruff_python_ast::StmtClassDef; +use ruff_text_size::{Ranged, TextRange}; + +/// ## What it does +/// Checks for uses of `metaclass=abc.ABCMeta` to define Abstract Base Classes (ABCs). +/// +/// ## Why is this bad? +/// Inheritance from the ABC wrapper class is semantically the same thing, but more succinct. +/// +/// ## Example +/// ```python +/// class C(metaclass=ABCMeta): +/// pass +/// ``` +/// +/// Use instead: +/// ```python +/// class C(ABC): +/// pass +/// ``` +/// +/// ## References +/// - [Python documentation: `abc.ABC`](https://docs.python.org/3/library/abc.html#abc.ABC) +/// - [Python documentation: `abc.ABCMeta`](https://docs.python.org/3/library/abc.html#abc.ABCMeta) +#[violation] +pub struct MetaClassABCMeta; + +impl AlwaysFixableViolation for MetaClassABCMeta { + #[derive_message_formats] + fn message(&self) -> String { + format!("Use of metaclass=abc.ABCMeta to define Abstract Base Class") + } + + fn fix_title(&self) -> String { + "Replace with abc.ABC".into() + } +} + +pub(crate) fn metaclass_abcmeta(checker: &mut Checker, class_def: &StmtClassDef) { + let Some((position, keyword)) = class_def.keywords().iter().find_position(|&keyword| { + keyword + .arg + .as_ref() + .is_some_and(|arg| arg.as_str() == "metaclass") + && checker + .semantic() + .resolve_call_path(map_callable(&keyword.value)) + .is_some_and(|call_path| matches!(call_path.as_slice(), ["abc", "ABCMeta"])) + }) else { + return; + }; + + let mut diagnostic = Diagnostic::new(MetaClassABCMeta, keyword.range); + diagnostic.try_set_fix(|| { + let (import_edit, binding) = checker.importer().get_or_import_symbol( + &ImportRequest::import("abc", "ABC"), + keyword.range.start(), + checker.semantic(), + )?; + Ok(if position == 0 { + Fix::safe_edits( + Edit::range_replacement(binding, keyword.range), + [import_edit], + ) + } else { + // When the `abc.ABCMeta` is not the first keyword, put `abc.ABC` before the first keyword + Fix::safe_edits( + Edit::range_deletion(TextRange::new( + class_def.keywords()[position - 1].range.end(), + keyword.range.end(), + )), + [ + Edit::insertion(format!("{binding}, "), class_def.keywords()[0].start()), + import_edit, + ], + ) + }) + }); + + checker.diagnostics.push(diagnostic); +} diff --git a/crates/ruff_linter/src/rules/refurb/rules/mod.rs b/crates/ruff_linter/src/rules/refurb/rules/mod.rs index ea9fbceef645f..532003ee143df 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/mod.rs @@ -6,6 +6,7 @@ pub(crate) use if_expr_min_max::*; pub(crate) use implicit_cwd::*; pub(crate) use isinstance_type_none::*; pub(crate) use math_constant::*; +pub(crate) use metaclass_abcmeta::*; pub(crate) use print_empty_string::*; pub(crate) use read_whole_file::*; pub(crate) use redundant_log_base::*; @@ -26,6 +27,7 @@ mod if_expr_min_max; mod implicit_cwd; mod isinstance_type_none; mod math_constant; +mod metaclass_abcmeta; mod print_empty_string; mod read_whole_file; mod redundant_log_base; diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB180_FURB180.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB180_FURB180.py.snap new file mode 100644 index 0000000000000..80f9982ef31f6 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB180_FURB180.py.snap @@ -0,0 +1,81 @@ +--- +source: crates/ruff_linter/src/rules/refurb/mod.rs +--- +FURB180.py:7:10: FURB180 [*] Use of metaclass=abc.ABCMeta to define Abstract Base Class + | +5 | # Errors +6 | +7 | class A0(metaclass=abc.ABCMeta): + | ^^^^^^^^^^^^^^^^^^^^^ FURB180 +8 | @abstractmethod +9 | def foo(self): pass + | + = help: Replace with abc.ABC + +ℹ Safe fix +4 4 | +5 5 | # Errors +6 6 | +7 |-class A0(metaclass=abc.ABCMeta): + 7 |+class A0(abc.ABC): +8 8 | @abstractmethod +9 9 | def foo(self): pass +10 10 | + +FURB180.py:12:10: FURB180 [*] Use of metaclass=abc.ABCMeta to define Abstract Base Class + | +12 | class A1(metaclass=ABCMeta): + | ^^^^^^^^^^^^^^^^^ FURB180 +13 | @abstractmethod +14 | def foo(self): pass + | + = help: Replace with abc.ABC + +ℹ Safe fix +9 9 | def foo(self): pass +10 10 | +11 11 | +12 |-class A1(metaclass=ABCMeta): + 12 |+class A1(abc.ABC): +13 13 | @abstractmethod +14 14 | def foo(self): pass +15 15 | + +FURB180.py:26:18: FURB180 [*] Use of metaclass=abc.ABCMeta to define Abstract Base Class + | +26 | class A2(B0, B1, metaclass=ABCMeta): + | ^^^^^^^^^^^^^^^^^ FURB180 +27 | @abstractmethod +28 | def foo(self): pass + | + = help: Replace with abc.ABC + +ℹ Safe fix +23 23 | pass +24 24 | +25 25 | +26 |-class A2(B0, B1, metaclass=ABCMeta): + 26 |+class A2(B0, B1, abc.ABC): +27 27 | @abstractmethod +28 28 | def foo(self): pass +29 29 | + +FURB180.py:31:34: FURB180 [*] Use of metaclass=abc.ABCMeta to define Abstract Base Class + | +31 | class A3(B0, before_metaclass=1, metaclass=abc.ABCMeta): + | ^^^^^^^^^^^^^^^^^^^^^ FURB180 +32 | pass + | + = help: Replace with abc.ABC + +ℹ Safe fix +28 28 | def foo(self): pass +29 29 | +30 30 | +31 |-class A3(B0, before_metaclass=1, metaclass=abc.ABCMeta): + 31 |+class A3(B0, abc.ABC, before_metaclass=1): +32 32 | pass +33 33 | +34 34 | + + diff --git a/ruff.schema.json b/ruff.schema.json index b347dcd00af5e..dbebae5df0613 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2956,6 +2956,7 @@ "FURB171", "FURB177", "FURB18", + "FURB180", "FURB181", "G", "G0", From 4b64a171c3edb68a3b02f827a97ad5d172fbd42b Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 31 Jan 2024 17:24:41 -0500 Subject: [PATCH 2/2] Tweak message and docs --- .../rules/refurb/rules/metaclass_abcmeta.rs | 57 ++++++++++++------- ...es__refurb__tests__FURB180_FURB180.py.snap | 16 +++--- 2 files changed, 45 insertions(+), 28 deletions(-) diff --git a/crates/ruff_linter/src/rules/refurb/rules/metaclass_abcmeta.rs b/crates/ruff_linter/src/rules/refurb/rules/metaclass_abcmeta.rs index 043c8823f481d..c5700635c9f76 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/metaclass_abcmeta.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/metaclass_abcmeta.rs @@ -1,17 +1,23 @@ -use crate::checkers::ast::Checker; -use crate::importer::ImportRequest; use itertools::Itertools; + use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::helpers::map_callable; use ruff_python_ast::StmtClassDef; use ruff_text_size::{Ranged, TextRange}; +use crate::checkers::ast::Checker; +use crate::importer::ImportRequest; + /// ## What it does -/// Checks for uses of `metaclass=abc.ABCMeta` to define Abstract Base Classes (ABCs). +/// Checks for uses of `metaclass=abc.ABCMeta` to define abstract base classes +/// (ABCs). /// /// ## Why is this bad? -/// Inheritance from the ABC wrapper class is semantically the same thing, but more succinct. +/// +/// Instead of `class C(metaclass=abc.ABCMeta): ...`, use `class C(ABC): ...` +/// to define an abstract base class. Inheriting from the `ABC` wrapper class +/// is semantically identical to setting `metaclass=abc.ABCMeta`, but more +/// succinct. /// /// ## Example /// ```python @@ -34,52 +40,63 @@ pub struct MetaClassABCMeta; impl AlwaysFixableViolation for MetaClassABCMeta { #[derive_message_formats] fn message(&self) -> String { - format!("Use of metaclass=abc.ABCMeta to define Abstract Base Class") + format!("Use of `metaclass=abc.ABCMeta` to define abstract base class") } fn fix_title(&self) -> String { - "Replace with abc.ABC".into() + format!("Replace with `abc.ABC`") } } +/// FURB180 pub(crate) fn metaclass_abcmeta(checker: &mut Checker, class_def: &StmtClassDef) { + // Identify the `metaclass` keyword. let Some((position, keyword)) = class_def.keywords().iter().find_position(|&keyword| { keyword .arg .as_ref() .is_some_and(|arg| arg.as_str() == "metaclass") - && checker - .semantic() - .resolve_call_path(map_callable(&keyword.value)) - .is_some_and(|call_path| matches!(call_path.as_slice(), ["abc", "ABCMeta"])) }) else { return; }; + // Determine whether it's assigned to `abc.ABCMeta`. + if !checker + .semantic() + .resolve_call_path(&keyword.value) + .is_some_and(|call_path| matches!(call_path.as_slice(), ["abc", "ABCMeta"])) + { + return; + } + let mut diagnostic = Diagnostic::new(MetaClassABCMeta, keyword.range); + diagnostic.try_set_fix(|| { let (import_edit, binding) = checker.importer().get_or_import_symbol( &ImportRequest::import("abc", "ABC"), keyword.range.start(), checker.semantic(), )?; - Ok(if position == 0 { - Fix::safe_edits( - Edit::range_replacement(binding, keyword.range), - [import_edit], - ) - } else { - // When the `abc.ABCMeta` is not the first keyword, put `abc.ABC` before the first keyword + Ok(if position > 0 { + // When the `abc.ABCMeta` is not the first keyword, put `abc.ABC` before the first + // keyword. Fix::safe_edits( + // Delete from the previous argument, to the end of the `metaclass` argument. Edit::range_deletion(TextRange::new( - class_def.keywords()[position - 1].range.end(), - keyword.range.end(), + class_def.keywords()[position - 1].end(), + keyword.end(), )), + // Insert `abc.ABC` before the first keyword. [ Edit::insertion(format!("{binding}, "), class_def.keywords()[0].start()), import_edit, ], ) + } else { + Fix::safe_edits( + Edit::range_replacement(binding, keyword.range), + [import_edit], + ) }) }); diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB180_FURB180.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB180_FURB180.py.snap index 80f9982ef31f6..0d683880efd9d 100644 --- a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB180_FURB180.py.snap +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB180_FURB180.py.snap @@ -1,7 +1,7 @@ --- source: crates/ruff_linter/src/rules/refurb/mod.rs --- -FURB180.py:7:10: FURB180 [*] Use of metaclass=abc.ABCMeta to define Abstract Base Class +FURB180.py:7:10: FURB180 [*] Use of `metaclass=abc.ABCMeta` to define abstract base class | 5 | # Errors 6 | @@ -10,7 +10,7 @@ FURB180.py:7:10: FURB180 [*] Use of metaclass=abc.ABCMeta to define Abstract Bas 8 | @abstractmethod 9 | def foo(self): pass | - = help: Replace with abc.ABC + = help: Replace with `abc.ABC` ℹ Safe fix 4 4 | @@ -22,14 +22,14 @@ FURB180.py:7:10: FURB180 [*] Use of metaclass=abc.ABCMeta to define Abstract Bas 9 9 | def foo(self): pass 10 10 | -FURB180.py:12:10: FURB180 [*] Use of metaclass=abc.ABCMeta to define Abstract Base Class +FURB180.py:12:10: FURB180 [*] Use of `metaclass=abc.ABCMeta` to define abstract base class | 12 | class A1(metaclass=ABCMeta): | ^^^^^^^^^^^^^^^^^ FURB180 13 | @abstractmethod 14 | def foo(self): pass | - = help: Replace with abc.ABC + = help: Replace with `abc.ABC` ℹ Safe fix 9 9 | def foo(self): pass @@ -41,14 +41,14 @@ FURB180.py:12:10: FURB180 [*] Use of metaclass=abc.ABCMeta to define Abstract Ba 14 14 | def foo(self): pass 15 15 | -FURB180.py:26:18: FURB180 [*] Use of metaclass=abc.ABCMeta to define Abstract Base Class +FURB180.py:26:18: FURB180 [*] Use of `metaclass=abc.ABCMeta` to define abstract base class | 26 | class A2(B0, B1, metaclass=ABCMeta): | ^^^^^^^^^^^^^^^^^ FURB180 27 | @abstractmethod 28 | def foo(self): pass | - = help: Replace with abc.ABC + = help: Replace with `abc.ABC` ℹ Safe fix 23 23 | pass @@ -60,13 +60,13 @@ FURB180.py:26:18: FURB180 [*] Use of metaclass=abc.ABCMeta to define Abstract Ba 28 28 | def foo(self): pass 29 29 | -FURB180.py:31:34: FURB180 [*] Use of metaclass=abc.ABCMeta to define Abstract Base Class +FURB180.py:31:34: FURB180 [*] Use of `metaclass=abc.ABCMeta` to define abstract base class | 31 | class A3(B0, before_metaclass=1, metaclass=abc.ABCMeta): | ^^^^^^^^^^^^^^^^^^^^^ FURB180 32 | pass | - = help: Replace with abc.ABC + = help: Replace with `abc.ABC` ℹ Safe fix 28 28 | def foo(self): pass