Skip to content

Commit

Permalink
Add no-subclass-builtin check (#324):
Browse files Browse the repository at this point in the history
Closes #323.

I am disabling this check by default for 2 reasons:

* `isinstance(x, dict)` fails for `UserDict` objects (including `list`/`str`)
* Changing `dict` to `UserDict` is not a quick fix, and you will probably have
  to change your class definition to get the most out of `UserDict` (ie,
  removing reimplemented methods, something Refurb can't easily detect).

Teams that want to enforce this can enable this option, but overall, it is hard
to deduce what is/is not proper usage of classes subclassing `dict`.
  • Loading branch information
dosisod authored Jan 31, 2024
1 parent a9a4edd commit d72ecfd
Show file tree
Hide file tree
Showing 6 changed files with 125 additions and 1 deletion.
5 changes: 5 additions & 0 deletions docs/categories.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ Checks that have the `builtin` category cover a few different topics:
* Statements such as `del`
* File system related operations such as `open()` and `readlines()`

## `collections`

These checks are for the [collections](https://docs.python.org/3/library/collections.html)
standard library module.

## `control-flow`

These checks deal with the control flow of a program, such as optimizing usage
Expand Down
35 changes: 34 additions & 1 deletion docs/checks.md
Original file line number Diff line number Diff line change
Expand Up @@ -2364,4 +2364,37 @@ Good:
```python
def strip_txt_extension(filename: str) -> str:
return filename.removesuffix(".txt")
```
```

## FURB189: `no-subclass-builtin`

Categories: `collections`

Subclassing `dict`, `list`, or `str` objects can be error prone, use the
`UserDict`, `UserList`, and `UserStr` objects from the `collections` module
instead.

Bad:

```python
class CaseInsensitiveDict(dict):
...
```

Good:

```python
from collections import UserDict

class CaseInsensitiveDict(UserDict):
...
```

Note: `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
Empty file.
64 changes: 64 additions & 0 deletions refurb/checks/collections/no_subclass_builtin.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
from dataclasses import dataclass

from mypy.nodes import ClassDef, NameExpr

from refurb.error import Error


@dataclass
class ErrorInfo(Error):
"""
Subclassing `dict`, `list`, or `str` objects can be error prone, use the
`UserDict`, `UserList`, and `UserStr` objects from the `collections` module
instead.
Bad:
```
class CaseInsensitiveDict(dict):
...
```
Good:
```
from collections import UserDict
class CaseInsensitiveDict(UserDict):
...
```
Note: `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
"""

enabled = False
name = "no-subclass-builtin"
code = 189
categories = ("collections",)


CLASS_MAPPING = {
"builtins.dict": "UserDict",
"builtins.list": "UserList",
"builtins.str": "UserStr",
}


def check(node: ClassDef, errors: list[Error]) -> None:
match node:
case ClassDef(
name=class_name,
base_type_exprs=[NameExpr(fullname=fullname, name=builtin_name)],
) if new_class := CLASS_MAPPING.get(fullname):
old = f"class {class_name}({builtin_name}):"
new = f"class {class_name}({new_class}):"
msg = f"Replace `{old}` with `{new}`"

errors.append(ErrorInfo.from_node(node, msg))
19 changes: 19 additions & 0 deletions test/data/err_189.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# these will match

class D(dict):
pass

class L(list):
pass

class S(str):
pass


# these will not

class C:
pass

class I(int):
pass
3 changes: 3 additions & 0 deletions test/data/err_189.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
test/data/err_189.py:3:1 [FURB189]: Replace `class D(dict):` with `class D(UserDict):`
test/data/err_189.py:6:1 [FURB189]: Replace `class L(list):` with `class L(UserList):`
test/data/err_189.py:9:1 [FURB189]: Replace `class S(str):` with `class S(UserStr):`

0 comments on commit d72ecfd

Please sign in to comment.