[refurb] Do not add abc.ABC if already present (FURB180) (#22234)

<!--
Thank you for contributing to Ruff/ty! To help us out with reviewing,
please consider the following:

- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title? (Please prefix
with `[ty]` for ty pull
  requests.)
- Does this pull request include references to any relevant issues?
-->

## Summary

According to the `FURB180` rule, `abc.ABC` should be used instead of
`metaclass=abc.ABCMeta`. This issue can be fixed automatically, but if
the class being checked already contains `abc.ABC`, it would be added a
second time.

Fixes: #17162

## Test Plan

I ran `cargo test refurb`.  
For checking the "fix" I used this python file:
<details>

<summary>Python file (input)</summary>

```python
import abc
from abc import abstractmethod, ABCMeta, ABC
from abc import ABC as ABCAnotherName

class ParentClass:
    ...

from abc import ABC, ABCMeta

class Foo(metaclass=ABCMeta):
    ...
    
class Foo2(abc.ABC, ParentClass, metaclass=ABCMeta):
    ...
    
class Foo3(ParentClass, ABC, metaclass=ABCMeta):
    ...
    
class Foo4(ABCAnotherName, metaclass=ABCMeta):
    ...
```

</details>

The output after applying the fix is so:
<details>
<summary>Python file (output)</summary>

```python
import abc
from abc import abstractmethod, ABCMeta, ABC
from abc import ABC as ABCAnotherName

class ParentClass:
    ...

from abc import ABC, ABCMeta

class Foo(ABC):
    ...
    
class Foo2(abc.ABC, ParentClass, ):
    ...
    
class Foo3(ParentClass, ABC, ):
    ...
    
class Foo4(ABCAnotherName, ):
    ...
```

</details>

Note: Trailing commas may remain after the automatic fix. It is unclear
whether this is an issue.

---------

Co-authored-by: me <me@example.com>
Co-authored-by: Brent Westbrook <brentrwestbrook@gmail.com>
Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com>

Author: 3 people

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

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)
         })
     });
 }

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