Skip to content

Commit

Permalink
Add no-copy-with-merge check:
Browse files Browse the repository at this point in the history
This was a small nitpick I found myself doing when I was merging two dicts
together. Can't say how common this pattern is, though I did get a few hits on
grep.app.
  • Loading branch information
dosisod committed Jan 5, 2024
1 parent ac56b62 commit 68fb0a0
Show file tree
Hide file tree
Showing 4 changed files with 119 additions and 0 deletions.
23 changes: 23 additions & 0 deletions docs/checks.md
Original file line number Diff line number Diff line change
Expand Up @@ -2232,4 +2232,27 @@ def process(file_name: str):
.withColumn('service_type', F.lit('green'))
)
return df
```

## FURB185: `no-copy-with-merge`

Categories: `readability`

You don't need to call `.clone()` on a dict/set when using it in a union
since the original dict/set is not modified.

Bad:

```python
d = {"a": 1}

merged = d.clone() | {"b": 2}
```

Good:

```python
d = {"a": 1}

merged = d | {"b": 2}
```
67 changes: 67 additions & 0 deletions refurb/checks/readability/no_copy_with_merge.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
from dataclasses import dataclass

from mypy.nodes import CallExpr, Expression, MemberExpr, OpExpr, RefExpr, Var

from refurb.checks.common import stringify
from refurb.error import Error


@dataclass
class ErrorInfo(Error):
"""
You don't need to call `.clone()` on a dict/set when using it in a union
since the original dict/set is not modified.
Bad:
```
d = {"a": 1}
merged = d.clone() | {"b": 2}
```
Good:
```
d = {"a": 1}
merged = d | {"b": 2}
```
"""

name = "no-copy-with-merge"
categories = ("readability",)
code = 185


UNIONABLE_TYPES = ("builtins.dict[", "builtins.set[")


ignored_nodes = set[int]()


def check_expr(expr: Expression, errors: list[Error]) -> None:
if id(expr) in ignored_nodes:
return

match expr:
case CallExpr(
callee=MemberExpr(
expr=RefExpr(node=Var(type=ty)) as ref,
name="copy",
),
args=[],
) if str(ty).startswith(UNIONABLE_TYPES):
msg = f"Replace `{stringify(ref)}.clone()` with `{stringify(ref)}`"

errors.append(ErrorInfo.from_node(expr, msg))

case OpExpr(left=lhs, op="|", right=rhs):
check_expr(lhs, errors)
check_expr(rhs, errors)

ignored_nodes.add(id(expr))


def check(node: OpExpr, errors: list[Error]) -> None:
check_expr(node, errors)
23 changes: 23 additions & 0 deletions test/data/err_185.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
x = {}
y = set()

# these should match
_ = x.copy() | {}
_ = {} | x.copy()

_ = y.copy() | set()
_ = set() | y.copy()

_ = x.copy() | {} | x.copy()



class C:
def copy(self) -> dict:
return {}

c = C()


# these should not
_ = c.copy() | {}
6 changes: 6 additions & 0 deletions test/data/err_185.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
test/data/err_185.py:5:5 [FURB185]: Replace `x.clone()` with `x`
test/data/err_185.py:6:10 [FURB185]: Replace `x.clone()` with `x`
test/data/err_185.py:8:5 [FURB185]: Replace `y.clone()` with `y`
test/data/err_185.py:9:13 [FURB185]: Replace `y.clone()` with `y`
test/data/err_185.py:11:5 [FURB185]: Replace `x.clone()` with `x`
test/data/err_185.py:11:21 [FURB185]: Replace `x.clone()` with `x`

0 comments on commit 68fb0a0

Please sign in to comment.