diff --git a/refurb/checks/builtin/no_ignored_dict_items.py b/refurb/checks/builtin/no_ignored_dict_items.py index f711bca..a8577b9 100644 --- a/refurb/checks/builtin/no_ignored_dict_items.py +++ b/refurb/checks/builtin/no_ignored_dict_items.py @@ -14,9 +14,8 @@ from refurb.checks.common import ( check_for_loop_like, - get_mypy_type, + is_mapping, is_name_unused_in_contexts, - is_same_type, stringify, ) from refurb.error import Error @@ -75,7 +74,7 @@ def check_dict_items_call( callee=MemberExpr(expr=dict_expr, name="items"), args=[], ), - ) if is_same_type(get_mypy_type(dict_expr), dict): + ) if is_mapping(dict_expr): check_unused_key_or_value(key, value, contexts, errors, dict_expr) diff --git a/refurb/checks/common.py b/refurb/checks/common.py index eca01a9..b5c5666 100644 --- a/refurb/checks/common.py +++ b/refurb/checks/common.py @@ -721,3 +721,35 @@ def mypy_type_to_python_type(ty: Type | SymbolNode | None) -> type | None: return SIMPLE_TYPES.get(fullname) # type: ignore return None # pragma: no cover + + +MAPPING_TYPES = ( + dict, + "collections.ChainMap", + "collections.Counter", + "collections.OrderedDict", + "collections.UserDict", + "collections.abc.Mapping", + "collections.abc.MutableMapping", + "collections.defaultdict", + "typing.Mapping", + "typing.MutableMapping", +) + + +# TODO: support any Mapping subclass +def is_mapping(expr: Expression) -> bool: + return is_mapping_type(get_mypy_type(expr)) + + +def is_mapping_type(ty: Type | SymbolNode | None) -> bool: + return is_same_type(ty, *MAPPING_TYPES) + + +def is_sized(node: Expression) -> bool: + return is_sized_type(get_mypy_type(node)) + + +# TODO: support any Sized subclass +def is_sized_type(ty: Type | SymbolNode | None) -> bool: + return is_mapping_type(ty) or is_same_type(ty, list, tuple, set, frozenset, str) diff --git a/refurb/checks/readability/in_keys.py b/refurb/checks/readability/in_keys.py index 812efbb..454a84e 100644 --- a/refurb/checks/readability/in_keys.py +++ b/refurb/checks/readability/in_keys.py @@ -2,7 +2,7 @@ from mypy.nodes import CallExpr, ComparisonExpr, MemberExpr -from refurb.checks.common import get_mypy_type, is_same_type, stringify +from refurb.checks.common import is_mapping, stringify from refurb.error import Error @@ -47,7 +47,7 @@ def check(node: ComparisonExpr, errors: list[Error]) -> None: args=[], ) as expr, ], - ) if is_same_type(get_mypy_type(dict_expr), dict): + ) if is_mapping(dict_expr): dict_expr = stringify(dict_expr) # type: ignore msg = f"Replace `{oper} {stringify(expr)}` with `{oper} {dict_expr}`" diff --git a/refurb/checks/readability/no_len_cmp.py b/refurb/checks/readability/no_len_cmp.py index 050fe1b..43cccce 100644 --- a/refurb/checks/readability/no_len_cmp.py +++ b/refurb/checks/readability/no_len_cmp.py @@ -7,7 +7,6 @@ ConditionalExpr, DictExpr, DictionaryComprehension, - Expression, GeneratorExpr, IfStmt, IntExpr, @@ -20,7 +19,7 @@ WhileStmt, ) -from refurb.checks.common import get_mypy_type, is_same_type, stringify +from refurb.checks.common import is_sized, stringify from refurb.error import Error from refurb.visitor import METHOD_NODE_MAPPINGS, TraverserVisitor @@ -61,16 +60,9 @@ class ErrorInfo(Error): categories = ("iterable", "truthy") -def is_builtin_container_like(node: Expression) -> bool: - return is_same_type(get_mypy_type(node), list, tuple, dict, set, frozenset, str) - - def is_len_call(node: CallExpr) -> bool: match node: - case CallExpr( - callee=NameExpr(fullname="builtins.len"), - args=[arg], - ) if is_builtin_container_like(arg): + case CallExpr(callee=NameExpr(fullname="builtins.len"), args=[arg]) if is_sized(arg): return True return False @@ -130,7 +122,7 @@ def visit_comparison_expr(self, node: ComparisonExpr) -> None: case ComparisonExpr( operators=["==" | "!=" as oper], operands=[lhs, (ListExpr(items=[]) | DictExpr(items=[]))], - ) if is_builtin_container_like(lhs): + ) if is_sized(lhs): old = stringify(node) new = stringify(lhs) diff --git a/refurb/checks/readability/use_dict_union.py b/refurb/checks/readability/use_dict_union.py index 92e2d7a..e6be367 100644 --- a/refurb/checks/readability/use_dict_union.py +++ b/refurb/checks/readability/use_dict_union.py @@ -1,9 +1,9 @@ from dataclasses import dataclass from itertools import groupby -from mypy.nodes import ArgKind, CallExpr, DictExpr, Expression, RefExpr +from mypy.nodes import ArgKind, CallExpr, DictExpr, RefExpr -from refurb.checks.common import get_mypy_type, is_same_type, stringify +from refurb.checks.common import is_mapping, stringify from refurb.error import Error from refurb.settings import Settings @@ -38,20 +38,6 @@ def add_defaults(settings: dict[str, str]) -> dict[str, str]: categories = ("dict", "readability") -MAPPING_TYPES = ( - dict, - "collections.ChainMap", - "collections.Counter", - "collections.OrderedDict", - "collections.defaultdict", - "collections.UserDict", -) - - -def is_builtin_mapping(expr: Expression) -> bool: - return is_same_type(get_mypy_type(expr), *MAPPING_TYPES) - - def check(node: DictExpr | CallExpr, errors: list[Error], settings: Settings) -> None: if settings.get_python_version() < (3, 9): return # pragma: no cover @@ -83,7 +69,7 @@ def check(node: DictExpr | CallExpr, errors: list[Error], settings: Settings) -> if is_star: _, star_expr = pair - if not is_builtin_mapping(star_expr): + if not is_mapping(star_expr): return old.append(f"**{stringify(star_expr)}") @@ -121,7 +107,7 @@ def check(node: DictExpr | CallExpr, errors: list[Error], settings: Settings) -> return if kind == ArgKind.ARG_STAR2: - if not is_builtin_mapping(arg): + if not is_mapping(arg): return stringified_arg = stringify(arg) diff --git a/test/data/err_115.py b/test/data/err_115.py index fa675b5..4b7dd09 100644 --- a/test/data/err_115.py +++ b/test/data/err_115.py @@ -83,6 +83,13 @@ class C: assert c.l == [] +from collections.abc import Mapping + +def mapping_check(m: Mapping[str, str]): + if len(m) == 0: + pass + + # these should not if len(nums) == 1: ... diff --git a/test/data/err_115.txt b/test/data/err_115.txt index 04e3ce6..c803e5c 100644 --- a/test/data/err_115.txt +++ b/test/data/err_115.txt @@ -38,3 +38,4 @@ test/data/err_115.py:73:8 [FURB115]: Replace `len(nums)` with `nums` test/data/err_115.py:74:8 [FURB115]: Replace `len(nums)` with `nums` test/data/err_115.py:80:8 [FURB115]: Replace `C().l == []` with `not C().l` test/data/err_115.py:83:8 [FURB115]: Replace `c.l == []` with `not c.l` +test/data/err_115.py:89:8 [FURB115]: Replace `len(m) == 0` with `not m` diff --git a/test/data/err_130.py b/test/data/err_130.py index d2f7de2..4dbca0c 100644 --- a/test/data/err_130.py +++ b/test/data/err_130.py @@ -19,6 +19,13 @@ class C: pass +from collections.abc import Mapping + +def mapping_check(m: Mapping[str, str]): + if x in m.keys(): + pass + + # these should not if "key" in d: diff --git a/test/data/err_130.txt b/test/data/err_130.txt index 6df96df..7c6f1e7 100644 --- a/test/data/err_130.txt +++ b/test/data/err_130.txt @@ -2,3 +2,4 @@ test/data/err_130.py:5:13 [FURB130]: Replace `in d.keys()` with `in d` test/data/err_130.py:8:17 [FURB130]: Replace `not in d.keys()` with `not in d` test/data/err_130.py:12:9 [FURB130]: Replace `in d.keys()` with `in d` test/data/err_130.py:18:9 [FURB130]: Replace `in C().d.keys()` with `in C().d` +test/data/err_130.py:25:13 [FURB130]: Replace `in m.keys()` with `in m` diff --git a/test/data/err_135.py b/test/data/err_135.py index bbe150f..8b69d0a 100644 --- a/test/data/err_135.py +++ b/test/data/err_135.py @@ -52,6 +52,13 @@ def f6(): print(k, v) +from collections.abc import Mapping + +def mapping_check(m: Mapping[str, str]): + for k, v in m.items(): + pass + + # these should not def f7(): diff --git a/test/data/err_135.txt b/test/data/err_135.txt index c54959c..6892416 100644 --- a/test/data/err_135.txt +++ b/test/data/err_135.txt @@ -12,3 +12,5 @@ test/data/err_135.py:39:15 [FURB135]: Value is unused, use `for k in c.d` instea test/data/err_135.py:40:12 [FURB135]: Key is unused, use `for v in c.d.values()` instead test/data/err_135.py:49:9 [FURB135]: Key is unused, use `for v in d.values()` instead test/data/err_135.py:49:12 [FURB135]: Value is unused, use `for k in d` instead +test/data/err_135.py:58:9 [FURB135]: Key is unused, use `for v in m.values()` instead +test/data/err_135.py:58:12 [FURB135]: Value is unused, use `for k in m` instead diff --git a/test/data/err_173.py b/test/data/err_173.py index 49ad3ea..24d9143 100644 --- a/test/data/err_173.py +++ b/test/data/err_173.py @@ -47,6 +47,15 @@ class Wrapper: _ = {**Wrapper().d, **x} +from collections.abc import Mapping, MutableMapping + + +def mapping_test(m: Mapping[str, str]): + _ = dict(**m) + +def mutable_mapping_test(m: MutableMapping[str, str]): + _ = dict(**m) + # these should not diff --git a/test/data/err_173.txt b/test/data/err_173.txt index 45fde7c..8eb0e9b 100644 --- a/test/data/err_173.txt +++ b/test/data/err_173.txt @@ -22,3 +22,5 @@ test/data/err_173.py:41:5 [FURB173]: Replace `dict(**x, a=1, b=2)` with `x | {"a test/data/err_173.py:42:5 [FURB173]: Replace `dict(**x, **y, a=1, b=2)` with `x | y | {"a": 1, "b": 2}` test/data/err_173.py:43:5 [FURB173]: Replace `dict(**x, **{})` with `x | {}` test/data/err_173.py:48:5 [FURB173]: Replace `{**Wrapper().d, **x}` with `Wrapper().d | x` +test/data/err_173.py:54:9 [FURB173]: Replace `dict(**m)` with `{**m}` +test/data/err_173.py:57:9 [FURB173]: Replace `dict(**m)` with `{**m}`