diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB189.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB189.py new file mode 100644 index 0000000000000..9ac8e83b38e07 --- /dev/null +++ b/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" diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index d2ee3cd6b6a17..e26d9e8cb3f7c 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/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) { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index a082c17557187..be27d11fc6b1d 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/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 diff --git a/crates/ruff_linter/src/rules/refurb/mod.rs b/crates/ruff_linter/src/rules/refurb/mod.rs index 640773c3e72fb..c03451eae57b1 100644 --- a/crates/ruff_linter/src/rules/refurb/mod.rs +++ b/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( diff --git a/crates/ruff_linter/src/rules/refurb/rules/mod.rs b/crates/ruff_linter/src/rules/refurb/rules/mod.rs index e71ded4315d23..426b864aa9a3a 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/mod.rs +++ b/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; diff --git a/crates/ruff_linter/src/rules/refurb/rules/subclass_builtin.rs b/crates/ruff_linter/src/rules/refurb/rules/subclass_builtin.rs new file mode 100644 index 0000000000000..c560156dff175 --- /dev/null +++ b/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 { + 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", + } + } +} diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB189_FURB189.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB189_FURB189.py.snap new file mode 100644 index 0000000000000..5378d2df19ca2 --- /dev/null +++ b/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 diff --git a/ruff.schema.json b/ruff.schema.json index 53d4be452c5a6..36868446991e7 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3253,6 +3253,7 @@ "FURB181", "FURB187", "FURB188", + "FURB189", "FURB19", "FURB192", "G",