FURB189: subclass-builtin

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

@@ -0,0 +1,41 @@
+# 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
+
+class SetOnceDict(SetOnceMappingMixin, dict):
+    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"
+
+# negatives
+class C:
+    pass
+
+class I(int):
+    pass

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, arguments.as_deref());
+            }
         }
         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,118 @@
+use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
+use ruff_macros::{derive_message_formats, violation};
+use ruff_python_ast::Arguments;
+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, .. } = self;
+        format!("Subclass of `{subclass}`")
+    }
+
+    fn fix_title(&self) -> String {
+        let SubclassBuiltin {
+            subclass,
+            replacement,
+        } = self;
+        format!("Replace subclass `{subclass}` with `{replacement}`")
+    }
+}
+
+enum Builtins {
+    Str,
+    List,
+    Dict,
+}
+
+/// FURB189
+pub(crate) fn subclass_builtin(checker: &mut Checker, arguments: Option<&Arguments>) {
+    let Some(Arguments { args, .. }) = arguments else {
+        return;
+    };
+
+    if args.len() == 0 {
+        return;
+    }
+
+    for base in args {
+        for symbol_type in [Builtins::Dict, Builtins::Str, Builtins::List] {
+            let symbol = match symbol_type {
+                Builtins::Dict => "dict",
+                Builtins::List => "list",
+                Builtins::Str => "str",
+            };
+            if checker.semantic().match_builtin_expr(base, symbol) {
+                let replacement_symbol = match symbol_type {
+                    Builtins::Dict => "UserDict",
+                    Builtins::List => "UserList",
+                    Builtins::Str => "UserStr",
+                };
+
+                let mut diagnostic = Diagnostic::new(
+                    SubclassBuiltin {
+                        subclass: symbol.to_string(),
+                        replacement: replacement_symbol.to_string(),
+                    },
+                    base.range(),
+                );
+                diagnostic.try_set_fix(|| {
+                    let (import_edit, binding) = checker.importer().get_or_import_symbol(
+                        &ImportRequest::import_from("collections", replacement_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);
+
+                // inheritance of these builtins is mutually exclusive
+                continue;
+            }
+        }
+    }
+}

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

@@ -0,0 +1,134 @@
+---
+source: crates/ruff_linter/src/rules/refurb/mod.rs
+---
+FURB189.py:17:9: FURB189 [*] Subclass of `dict`
+   |
+16 | # positives
+17 | class D(dict):
+   |         ^^^^ FURB189
+18 |     pass
+   |
+   = help: Replace subclass `dict` with `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 [*] Subclass of `list`
+   |
+18 |     pass
+19 | 
+20 | class L(list):
+   |         ^^^^ FURB189
+21 |     pass
+   |
+   = help: Replace subclass `list` with `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 [*] Subclass of `str`
+   |
+21 |     pass
+22 | 
+23 | class S(str):
+   |         ^^^ FURB189
+24 |     pass
+   |
+   = help: Replace subclass `str` with `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 | class SetOnceDict(SetOnceMappingMixin, dict):
+
+FURB189.py:26:40: FURB189 [*] Subclass of `dict`
+   |
+24 |     pass
+25 | 
+26 | class SetOnceDict(SetOnceMappingMixin, dict):
+   |                                        ^^^^ FURB189
+27 |     pass
+   |
+   = help: Replace subclass `dict` with `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__ = ()
+--------------------------------------------------------------------------------
+23 23 | class S(str):
+24 24 |     pass
+25 25 | 
+26    |-class SetOnceDict(SetOnceMappingMixin, dict):
+   26 |+class SetOnceDict(SetOnceMappingMixin, UserDict):
+27 27 |     pass
+28 28 | 
+29 29 | class ActivityState(str, Enum, metaclass=CaseInsensitiveEnumMeta):
+
+FURB189.py:29:21: FURB189 [*] Subclass of `str`
+   |
+27 |     pass
+28 | 
+29 | class ActivityState(str, Enum, metaclass=CaseInsensitiveEnumMeta):
+   |                     ^^^ FURB189
+30 |     """Activity state. This is an optional property and if not provided, the state will be Active by
+31 |     default.
+   |
+   = help: Replace subclass `str` with `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__ = ()
+--------------------------------------------------------------------------------
+26 26 | class SetOnceDict(SetOnceMappingMixin, dict):
+27 27 |     pass
+28 28 | 
+29    |-class ActivityState(str, Enum, metaclass=CaseInsensitiveEnumMeta):
+   29 |+class ActivityState(UserStr, Enum, metaclass=CaseInsensitiveEnumMeta):
+30 30 |     """Activity state. This is an optional property and if not provided, the state will be Active by
+31 31 |     default.
+32 32 |     """