diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB180.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB180.py index 33576061fc23a7..ad75ebb62c9810 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB180.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB180.py @@ -56,3 +56,45 @@ def foo(self): pass class A7(B0, abc.ABC, B1): @abstractmethod def foo(self): pass + + +# Regression tests for https://github.com/astral-sh/ruff/issues/17162 +class A8(abc.ABC, metaclass=ABCMeta): # FURB180 + @abstractmethod + def foo(self): + pass + + +def a9(): + from abc import ABC + + class A9(ABC, metaclass=ABCMeta): # FURB180 + @abstractmethod + def foo(self): + pass + + +def a10(): + from abc import ABC as ABCAlternativeName + + class A10(ABCAlternativeName, metaclass=ABCMeta): # FURB180 + @abstractmethod + def foo(self): + pass + + +class MyMetaClass(abc.ABC): ... + + +class A11(MyMetaClass, metaclass=ABCMeta): # FURB180 + @abstractmethod + def foo(self): + pass + + +class A12( + keyword_argument=1, + # comment + metaclass=abc.ABCMeta, +): # FURB180 + pass \ No newline at end of file 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 7e5a432968334e..0202126ea8d850 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/metaclass_abcmeta.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/metaclass_abcmeta.rs @@ -1,11 +1,12 @@ use itertools::Itertools; - use ruff_diagnostics::Applicability; use ruff_macros::{ViolationMetadata, derive_message_formats}; use ruff_python_ast::StmtClassDef; -use ruff_text_size::{Ranged, TextRange}; +use ruff_python_semantic::analyze; +use ruff_text_size::Ranged; use crate::checkers::ast::Checker; +use crate::fix::edits::{Parentheses, remove_argument}; use crate::importer::ImportRequest; use crate::{AlwaysFixableViolation, Edit, Fix}; @@ -42,6 +43,9 @@ use crate::{AlwaysFixableViolation, Edit, Fix}; /// The rule's fix is unsafe if the class has base classes. This is because the base classes might /// be validating the class's other base classes (e.g., `typing.Protocol` does this) or otherwise /// alter runtime behavior if more base classes are added. +/// The rule's fix will also be marked as unsafe if the class has +/// comments in its argument list that could be deleted. +/// /// /// ## References /// - [Python documentation: `abc.ABC`](https://docs.python.org/3/library/abc.html#abc.ABC) @@ -63,6 +67,11 @@ impl AlwaysFixableViolation for MetaClassABCMeta { /// FURB180 pub(crate) fn metaclass_abcmeta(checker: &Checker, class_def: &StmtClassDef) { + // Determine whether the class definition contains at least one argument. + let Some(arguments) = &class_def.arguments.as_ref() else { + return; + }; + // Identify the `metaclass` keyword. let Some((position, keyword)) = class_def.keywords().iter().find_position(|&keyword| { keyword @@ -82,10 +91,18 @@ pub(crate) fn metaclass_abcmeta(checker: &Checker, class_def: &StmtClassDef) { return; } - let applicability = if class_def.bases().is_empty() { - Applicability::Safe - } else { + let applicability = if !class_def.bases().is_empty() { + // The class has base arguments (not just a `metaclass` keyword). + // Applying the fix may change semantics, so it is considered unsafe. + Applicability::Unsafe + } else if checker.comment_ranges().intersects(arguments.range()) { + // The `metaclass` keyword overlaps with a comment. + // Applying the fix would remove or alter the comment, so it is unsafe. Applicability::Unsafe + } else { + // An empty class definition with no overlapping comments. + // Applying the fix is considered safe. + Applicability::Safe }; let mut diagnostic = checker.report_diagnostic(MetaClassABCMeta, keyword.range); @@ -95,28 +112,47 @@ pub(crate) fn metaclass_abcmeta(checker: &Checker, class_def: &StmtClassDef) { keyword.range.start(), checker.semantic(), )?; + + // Check the `abc.ABC` is in base classes. + let has_abc = analyze::class::any_qualified_base_class( + class_def, + checker.semantic(), + &|qualified_name| matches!(qualified_name.segments(), ["abc", "ABC"]), + ); + + let delete_metaclass_keyword = remove_argument( + keyword, + arguments, + Parentheses::Remove, + checker.source(), + checker.tokens(), + )?; + Ok(if position > 0 { - // When the `abc.ABCMeta` is not the first keyword, put `abc.ABC` before the first - // keyword. - Fix::applicable_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, - ], - applicability, - ) + // When the `abc.ABCMeta` is not the first keyword and `abc.ABC` is not + // in base classes put `abc.ABC` before the first keyword argument. + if has_abc { + Fix::applicable_edit(delete_metaclass_keyword, applicability) + } else { + Fix::applicable_edits( + delete_metaclass_keyword, + [ + Edit::insertion(format!("{binding}, "), class_def.keywords()[0].start()), + import_edit, + ], + applicability, + ) + } } else { - Fix::applicable_edits( - Edit::range_replacement(binding, keyword.range), - [import_edit], - applicability, - ) + let edit_action = if has_abc { + // Class already inherits the `abc.ABC`, delete the `metaclass` keyword only. + delete_metaclass_keyword + } else { + // Replace `metaclass` keyword by `abc.ABC`. + Edit::range_replacement(binding, keyword.range) + }; + + Fix::applicable_edits(edit_action, [import_edit], applicability) }) }); } 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 be994f1cc5d438..a36dfa46aeaf9d 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 @@ -75,3 +75,106 @@ help: Replace with `abc.ABC` 33 | 34 | note: This is an unsafe fix and may change runtime behavior + +FURB180 [*] Use of `metaclass=abc.ABCMeta` to define abstract base class + --> FURB180.py:62:19 + | +61 | # Regression tests for https://github.com/astral-sh/ruff/issues/17162 +62 | class A8(abc.ABC, metaclass=ABCMeta): # FURB180 + | ^^^^^^^^^^^^^^^^^ +63 | @abstractmethod +64 | def foo(self): + | +help: Replace with `abc.ABC` +59 | +60 | +61 | # Regression tests for https://github.com/astral-sh/ruff/issues/17162 + - class A8(abc.ABC, metaclass=ABCMeta): # FURB180 +62 + class A8(abc.ABC): # FURB180 +63 | @abstractmethod +64 | def foo(self): +65 | pass +note: This is an unsafe fix and may change runtime behavior + +FURB180 [*] Use of `metaclass=abc.ABCMeta` to define abstract base class + --> FURB180.py:71:19 + | +69 | from abc import ABC +70 | +71 | class A9(ABC, metaclass=ABCMeta): # FURB180 + | ^^^^^^^^^^^^^^^^^ +72 | @abstractmethod +73 | def foo(self): + | +help: Replace with `abc.ABC` +68 | def a9(): +69 | from abc import ABC +70 | + - class A9(ABC, metaclass=ABCMeta): # FURB180 +71 + class A9(ABC): # FURB180 +72 | @abstractmethod +73 | def foo(self): +74 | pass +note: This is an unsafe fix and may change runtime behavior + +FURB180 [*] Use of `metaclass=abc.ABCMeta` to define abstract base class + --> FURB180.py:80:35 + | +78 | from abc import ABC as ABCAlternativeName +79 | +80 | class A10(ABCAlternativeName, metaclass=ABCMeta): # FURB180 + | ^^^^^^^^^^^^^^^^^ +81 | @abstractmethod +82 | def foo(self): + | +help: Replace with `abc.ABC` +77 | def a10(): +78 | from abc import ABC as ABCAlternativeName +79 | + - class A10(ABCAlternativeName, metaclass=ABCMeta): # FURB180 +80 + class A10(ABCAlternativeName): # FURB180 +81 | @abstractmethod +82 | def foo(self): +83 | pass +note: This is an unsafe fix and may change runtime behavior + +FURB180 [*] Use of `metaclass=abc.ABCMeta` to define abstract base class + --> FURB180.py:89:24 + | +89 | class A11(MyMetaClass, metaclass=ABCMeta): # FURB180 + | ^^^^^^^^^^^^^^^^^ +90 | @abstractmethod +91 | def foo(self): + | +help: Replace with `abc.ABC` +86 | class MyMetaClass(abc.ABC): ... +87 | +88 | + - class A11(MyMetaClass, metaclass=ABCMeta): # FURB180 +89 + class A11(MyMetaClass): # FURB180 +90 | @abstractmethod +91 | def foo(self): +92 | pass +note: This is an unsafe fix and may change runtime behavior + +FURB180 [*] Use of `metaclass=abc.ABCMeta` to define abstract base class + --> FURB180.py:98:5 + | + 96 | keyword_argument=1, + 97 | # comment + 98 | metaclass=abc.ABCMeta, + | ^^^^^^^^^^^^^^^^^^^^^ + 99 | ): # FURB180 +100 | pass + | +help: Replace with `abc.ABC` +93 | +94 | +95 | class A12( + - keyword_argument=1, + - # comment + - metaclass=abc.ABCMeta, +96 + abc.ABC, keyword_argument=1, +97 | ): # FURB180 +98 | pass +note: This is an unsafe fix and may change runtime behavior