Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[refurb] Implement metaclass_abcmeta (FURB180) #9658

Merged
merged 3 commits into from
Jan 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/refurb/FURB180.py
Original file line number Diff line number Diff line change
@@ -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
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/refurb/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
104 changes: 104 additions & 0 deletions crates/ruff_linter/src/rules/refurb/rules/metaclass_abcmeta.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
use itertools::Itertools;

use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
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).
///
/// ## Why is this bad?
///
/// 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
/// 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 {
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")
}) 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;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split this condition out of the find_position, I found it a little easier to read them when enforced separately.


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 {
// 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].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],
)
})
});

checker.diagnostics.push(diagnostic);
}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/refurb/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;
Expand All @@ -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;
Expand Down
Original file line number Diff line number Diff line change
@@ -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 |


1 change: 1 addition & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading