[refurb] Implement subclass-builtin (FURB189)

crates/ruff_linter/resources/test/fixtures/refurb/FURB189.py

@@ -0,0 +1,42 @@
+# setup
+from enum import Enum, EnumMeta
+from collections import UserList as UL
+
+class SetOnceMappingMixin:
+    __slots__ = ()
+    def __setitem__(self, key, value):
+        if key in self:
+            raise KeyError(str(key) + ' already set')
+        return super().__setitem__(key, value)
+
+
+class CaseInsensitiveEnumMeta(EnumMeta):
+    pass
+
+# positives
+class D(dict):
+    pass
+
+class L(list):
+    pass
+
+class S(str):
+    pass
+
+# currently not detected
+class SetOnceDict(SetOnceMappingMixin, dict):
+    pass
+
+# negatives
+class C:
+    pass
+
+class I(int):
+    pass
+
+class ActivityState(str, Enum, metaclass=CaseInsensitiveEnumMeta):
+    """Activity state. This is an optional property and if not provided, the state will be Active by
+    default.
+    """
+    ACTIVE = "Active"
+    INACTIVE = "Inactive"

crates/ruff_linter/src/checkers/ast/analyze/statement.rs

@@ -549,6 +549,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
             if checker.enabled(Rule::WhitespaceAfterDecorator) {
                 pycodestyle::rules::whitespace_after_decorator(checker, decorator_list);
             }
+            if checker.enabled(Rule::SubclassBuiltin) {
+                refurb::rules::subclass_builtin(checker, class_def);
+            }
         }
         Stmt::Import(ast::StmtImport { names, range: _ }) => {
             if checker.enabled(Rule::MultipleImportsOnOneLine) {

crates/ruff_linter/src/codes.rs

@@ -1072,6 +1072,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Refurb, "181") => (RuleGroup::Stable, rules::refurb::rules::HashlibDigestHex),
         (Refurb, "187") => (RuleGroup::Stable, rules::refurb::rules::ListReverseCopy),
         (Refurb, "188") => (RuleGroup::Preview, rules::refurb::rules::SliceToRemovePrefixOrSuffix),
+        (Refurb, "189") => (RuleGroup::Preview, rules::refurb::rules::SubclassBuiltin),
         (Refurb, "192") => (RuleGroup::Preview, rules::refurb::rules::SortedMinMax),
 
         // flake8-logging

crates/ruff_linter/src/rules/refurb/mod.rs

@@ -48,6 +48,7 @@ mod tests {
     #[test_case(Rule::FStringNumberFormat, Path::new("FURB116.py"))]
     #[test_case(Rule::SortedMinMax, Path::new("FURB192.py"))]
     #[test_case(Rule::SliceToRemovePrefixOrSuffix, Path::new("FURB188.py"))]
+    #[test_case(Rule::SubclassBuiltin, Path::new("FURB189.py"))]
     fn rules(rule_code: Rule, path: &Path) -> Result<()> {
         let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
         let diagnostics = test_path(

crates/ruff_linter/src/rules/refurb/rules/mod.rs

@@ -26,6 +26,7 @@ pub(crate) use single_item_membership_test::*;
 pub(crate) use slice_copy::*;
 pub(crate) use slice_to_remove_prefix_or_suffix::*;
 pub(crate) use sorted_min_max::*;
+pub(crate) use subclass_builtin::*;
 pub(crate) use type_none_comparison::*;
 pub(crate) use unnecessary_enumerate::*;
 pub(crate) use unnecessary_from_float::*;
@@ -60,6 +61,7 @@ mod single_item_membership_test;
 mod slice_copy;
 mod slice_to_remove_prefix_or_suffix;
 mod sorted_min_max;
+mod subclass_builtin;
 mod type_none_comparison;
 mod unnecessary_enumerate;
 mod unnecessary_from_float;

crates/ruff_linter/src/rules/refurb/rules/subclass_builtin.rs

@@ -0,0 +1,130 @@
+use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
+use ruff_macros::{derive_message_formats, violation};
+use ruff_python_ast::{Arguments, StmtClassDef};
+use ruff_text_size::Ranged;
+
+use crate::{checkers::ast::Checker, importer::ImportRequest};
+
+/// ## What it does
+/// Checks for subclasses of `dict`, `list` or `str`.
+///
+/// ## Why is this bad?
+/// Subclassing `dict`, `list`, or `str` objects can be error prone, use the
+/// `UserDict`, `UserList`, and `UserStr` objects from the `collections` module
+/// instead.
+///
+/// ## Example
+/// ```python
+/// class CaseInsensitiveDict(dict): ...
+/// ```
+///
+/// Use instead:
+/// ```python
+/// from collections import UserDict
+///
+///
+/// class CaseInsensitiveDict(UserDict): ...
+/// ```
+///
+/// ## Fix safety
+/// This fix is marked as unsafe because `isinstance()` checks for `dict`,
+/// `list`, and `str` types will fail when using the corresponding User class.
+/// If you need to pass custom `dict` or `list` objects to code you don't
+/// control, ignore this check. If you do control the code, consider using
+/// the following type checks instead:
+///
+/// * `dict` -> `collections.abc.MutableMapping`
+/// * `list` -> `collections.abc.MutableSequence`
+/// * `str` -> No such conversion exists
+///
+/// ## References
+///
+/// - [Python documentation: `collections`](https://docs.python.org/3/library/collections.html)
+#[violation]
+pub struct SubclassBuiltin {
+    subclass: String,
+    replacement: String,
+}
+
+impl AlwaysFixableViolation for SubclassBuiltin {
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        let SubclassBuiltin {
+            subclass,
+            replacement,
+        } = self;
+        format!(
+            "Subclassing `{subclass}` can be error prone, use `collections.{replacement}` instead"
+        )
+    }
+
+    fn fix_title(&self) -> String {
+        let SubclassBuiltin { replacement, .. } = self;
+        format!("Replace with `collections.{replacement}`")
+    }
+}
+
+/// FURB189
+pub(crate) fn subclass_builtin(checker: &mut Checker, class: &StmtClassDef) {
+    let Some(Arguments { args: bases, .. }) = class.arguments.as_deref() else {
+        return;
+    };
+
+    let [base] = &**bases else {
+        return;
+    };
+
+    let Some(symbol) = checker.semantic().resolve_builtin_symbol(base) else {
+        return;
+    };
+
+    let Some(supported_builtin) = SupportedBuiltins::from_symbol(symbol) else {
+        return;
+    };
+
+    let user_symbol = supported_builtin.user_symbol();
+
+    let mut diagnostic = Diagnostic::new(
+        SubclassBuiltin {
+            subclass: symbol.to_string(),
+            replacement: user_symbol.to_string(),
+        },
+        base.range(),
+    );
+    diagnostic.try_set_fix(|| {
+        let (import_edit, binding) = checker.importer().get_or_import_symbol(
+            &ImportRequest::import_from("collections", user_symbol),
+            base.start(),
+            checker.semantic(),
+        )?;
+        let other_edit = Edit::range_replacement(binding, base.range());
+        Ok(Fix::unsafe_edits(import_edit, [other_edit]))
+    });
+    checker.diagnostics.push(diagnostic);
+}
+
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
+enum SupportedBuiltins {
+    Str,
+    List,
+    Dict,
+}
+
+impl SupportedBuiltins {
+    fn from_symbol(value: &str) -> Option<SupportedBuiltins> {
+        match value {
+            "str" => Some(Self::Str),
+            "dict" => Some(Self::Dict),
+            "list" => Some(Self::List),
+            _ => None,
+        }
+    }
+
+    const fn user_symbol(self) -> &'static str {
+        match self {
+            SupportedBuiltins::Dict => "UserDict",
+            SupportedBuiltins::List => "UserList",
+            SupportedBuiltins::Str => "UserStr",
+        }
+    }
+}

crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB189_FURB189.py.snap

@@ -0,0 +1,77 @@
+---
+source: crates/ruff_linter/src/rules/refurb/mod.rs
+---
+FURB189.py:17:9: FURB189 [*] Subclassing `dict` can be error prone, use `collections.UserDict` instead
+   |
+16 | # positives
+17 | class D(dict):
+   |         ^^^^ FURB189
+18 |     pass
+   |
+   = help: Replace with `collections.UserDict`
+
+ℹ Unsafe fix
+1  1  | # setup
+2  2  | from enum import Enum, EnumMeta
+3     |-from collections import UserList as UL
+   3  |+from collections import UserList as UL, UserDict
+4  4  | 
+5  5  | class SetOnceMappingMixin:
+6  6  |     __slots__ = ()
+--------------------------------------------------------------------------------
+14 14 |     pass
+15 15 | 
+16 16 | # positives
+17    |-class D(dict):
+   17 |+class D(UserDict):
+18 18 |     pass
+19 19 | 
+20 20 | class L(list):
+
+FURB189.py:20:9: FURB189 [*] Subclassing `list` can be error prone, use `collections.UserList` instead
+   |
+18 |     pass
+19 | 
+20 | class L(list):
+   |         ^^^^ FURB189
+21 |     pass
+   |
+   = help: Replace with `collections.UserList`
+
+ℹ Unsafe fix
+17 17 | class D(dict):
+18 18 |     pass
+19 19 | 
+20    |-class L(list):
+   20 |+class L(UL):
+21 21 |     pass
+22 22 | 
+23 23 | class S(str):
+
+FURB189.py:23:9: FURB189 [*] Subclassing `str` can be error prone, use `collections.UserStr` instead
+   |
+21 |     pass
+22 | 
+23 | class S(str):
+   |         ^^^ FURB189
+24 |     pass
+   |
+   = help: Replace with `collections.UserStr`
+
+ℹ Unsafe fix
+1  1  | # setup
+2  2  | from enum import Enum, EnumMeta
+3     |-from collections import UserList as UL
+   3  |+from collections import UserList as UL, UserStr
+4  4  | 
+5  5  | class SetOnceMappingMixin:
+6  6  |     __slots__ = ()
+--------------------------------------------------------------------------------
+20 20 | class L(list):
+21 21 |     pass
+22 22 | 
+23    |-class S(str):
+   23 |+class S(UserStr):
+24 24 |     pass
+25 25 | 
+26 26 | # currently not detected