Skip to content

Commit

Permalink
Add "set discard" check (#74):
Browse files Browse the repository at this point in the history
When removing keys from a `set`, you can use `.remove()`, which assumes the
key already exists. If you just want to remove the key regardless of
whether it is already in the set or not, use `.discard()` instead.

One potential case that this check does not pick up on is the following:

```python
try:
    nums.remove(123)
except KeyError:
    pass
```

Since `.remove()` will throw a `KeyError` if the key doesn't exist, a valid
way of silencing this would be the above code. This is identical to the "with
suppress" check, and I don't like the interaction that all of these checks
are having with it. This also means that the following code should be checked:

```python
with suppress(KeyError):
    nums.remove(123)
```

Though adding 3 different styles for the same check is a lot of work, and
requires additional maintainence.

Also, this check (and many others) do not pick up on member expressions, such
as `x.y.z.remove(123)`. This will be addressed soon.
  • Loading branch information
dosisod authored Oct 17, 2022
1 parent 6588a56 commit a4d13c7
Show file tree
Hide file tree
Showing 4 changed files with 105 additions and 1 deletion.
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[tool.poetry]
name = "refurb"
version = "1.3.0"
version = "1.4.0"
description = "A tool for refurbish and modernize Python codebases"
authors = ["dosisod"]
license = "GPL-3.0-only"
Expand Down
69 changes: 69 additions & 0 deletions refurb/checks/builtin/set_discard.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
from dataclasses import dataclass

from mypy.nodes import (
Block,
CallExpr,
ComparisonExpr,
ExpressionStmt,
IfStmt,
MemberExpr,
NameExpr,
Var,
)

from refurb.error import Error


@dataclass
class ErrorInfo(Error):
"""
If you want to remove a value from a set regardless of whether it exists or
not, use the `discard()` method instead of `remove()`:
Bad:
```
nums = set((123, 456))
if 123 in nums:
nums.remove(123)
```
Good:
```
nums = set((123, 456))
nums.discard(123)
```
"""

code = 132
msg: str = "Replace `if x in s: s.remove(x)` with `s.discard(x)`"


def check(node: IfStmt, errors: list[Error]) -> None:
match node:
case IfStmt(
expr=[ComparisonExpr(operators=["in"], operands=[lhs, rhs])],
body=[
Block(
body=[
ExpressionStmt(
expr=CallExpr(
callee=MemberExpr(
expr=NameExpr(node=Var(type=ty)) as expr,
name="remove",
),
args=[arg],
)
)
]
)
],
) if (
str(lhs) == str(arg)
and str(rhs) == str(expr)
and str(ty).startswith("builtins.set[")
):
errors.append(ErrorInfo(node.line, node.column))
34 changes: 34 additions & 0 deletions test/data/err_132.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
s = set()

# these should match

if "x" in s:
s.remove("x")

# these should not

if "x" in s:
s.remove("y")

s.discard("x")

s2 = set()

if "x" in s:
s2.remove("x")

if "x" in s:
s.remove("x")
print("removed item")

class Container:
def remove(self, item) -> None:
return

def __contains__(self, other) -> bool:
return True

c = Container()

if "x" in c:
c.remove("x")
1 change: 1 addition & 0 deletions test/data/err_132.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
test/data/err_132.py:5:1 [FURB132]: Replace `if x in s: s.remove(x)` with `s.discard(x)`

0 comments on commit a4d13c7

Please sign in to comment.