Skip to content

Commit

Permalink
Reimplement FURB131:
Browse files Browse the repository at this point in the history
FURB131 (previously `no-del`) has been broken for a while, basically since it's
initial creation. #239 fixed some of this, but introduced improper semantics by
implying you could use `del x[:]` when `x` is a `dict`, which is not true. This
means that FURB131 only really applies to `list` types now (or any sequence
that implements `.clear()`, though this isn't supported yet). I recently came
across a similar idiom for clearing an array, `x[:] = []`, which is about twice
as slow as `x.clear()`, and similar in nature to `del x[:]`. This new update
was what prompted me to fix all the broken logic.

With that in mind, this check has been renamed to `use-clear` since it no
longer is about the `del` statement, but about using slices to clear a list.
This shouldn't cause any issues since the name of checks are only used for
display purposes. This will break any hyperlinks to FURB131 in the docs, though
you'll still end up on the same page so I'm not too concerned about that.
  • Loading branch information
dosisod committed Feb 1, 2024
1 parent d72ecfd commit dbe3126
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 31 deletions.
16 changes: 7 additions & 9 deletions docs/checks.md
Original file line number Diff line number Diff line change
Expand Up @@ -833,32 +833,30 @@ if "key" in d:
...
```

## FURB131: `no-del`
## FURB131: `use-clear`

Categories: `builtin` `readability`

The `del` statement is commonly used for popping single elements from dicts
and lists, though a slice can be used to remove a range of elements
instead. When removing all elements via a slice, use the faster and more
succinct `.clear()` method instead.
Slice expressions can be used to replace part a list without reassigning
it. If you want to clear all the elements out of a list while maintaining
the same reference, don't use `del x[:]` or `x[:] = []`, use the faster
`x.clear()` method instead.

Bad:

```python
names = {"key": "value"}
nums = [1, 2, 3]

del names[:]
del nums[:]
# or
nums[:] = []
```

Good:

```python
names = {"key": "value"}
nums = [1, 2, 3]

names.clear()
nums.clear()
```

Expand Down
51 changes: 32 additions & 19 deletions refurb/checks/builtin/no_del.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from dataclasses import dataclass

from mypy.nodes import DelStmt, IndexExpr, NameExpr, SliceExpr, Var
from mypy.nodes import AssignmentStmt, DelStmt, IndexExpr, ListExpr, RefExpr, SliceExpr, Var

from refurb.checks.common import stringify
from refurb.error import Error
Expand All @@ -9,46 +9,59 @@
@dataclass
class ErrorInfo(Error):
"""
The `del` statement is commonly used for popping single elements from dicts
and lists, though a slice can be used to remove a range of elements
instead. When removing all elements via a slice, use the faster and more
succinct `.clear()` method instead.
Slice expressions can be used to replace part a list without reassigning
it. If you want to clear all the elements out of a list while maintaining
the same reference, don't use `del x[:]` or `x[:] = []`, use the faster
`x.clear()` method instead.
Bad:
```
names = {"key": "value"}
nums = [1, 2, 3]
del names[:]
del nums[:]
# or
nums[:] = []
```
Good:
```
names = {"key": "value"}
nums = [1, 2, 3]
names.clear()
nums.clear()
```
"""

name = "no-del"
name = "use-clear"
code = 131
categories = ("builtin", "readability")


def check(node: DelStmt, errors: list[Error]) -> None:
def check(node: DelStmt | AssignmentStmt, errors: list[Error]) -> None:
match node:
case DelStmt(expr=IndexExpr(base=NameExpr(node=Var(type=ty)) as expr, index=index)) if str(
ty
).startswith(("builtins.dict[", "builtins.list[")):
match index:
case SliceExpr(begin_index=None, end_index=None):
expr = stringify(expr) # type: ignore
case DelStmt(
expr=IndexExpr(
base=RefExpr(node=Var(type=ty)) as name,
index=SliceExpr(begin_index=None, end_index=None, stride=None),
),
) if str(ty).startswith("builtins.list["):
pass

msg = f"Replace `del {expr}[:]` with `{expr}.clear()`"
case AssignmentStmt(
lvalues=[
IndexExpr(
base=RefExpr(node=Var(type=ty)) as name,
index=SliceExpr(begin_index=None, end_index=None, stride=None),
),
],
rvalue=ListExpr(items=[]),
) if str(ty).startswith("builtins.list["):
pass

errors.append(ErrorInfo.from_node(node, msg))
case _:
return

msg = f"Replace `{stringify(node)}` with `{stringify(name)}.clear()`"

errors.append(ErrorInfo.from_node(node, msg))
4 changes: 4 additions & 0 deletions refurb/checks/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
ComparisonExpr,
ComplexExpr,
ConditionalExpr,
DelStmt,
DictExpr,
DictionaryComprehension,
Expression,
Expand Down Expand Up @@ -420,6 +421,9 @@ def _stringify(node: Node) -> str:
case ConditionalExpr(if_expr=if_true, cond=cond, else_expr=if_false):
return f"{_stringify(if_true)} if {_stringify(cond)} else {_stringify(if_false)}"

case DelStmt(expr=expr):
return f"del {stringify(expr)}"

raise ValueError


Expand Down
16 changes: 14 additions & 2 deletions test/data/err_131.py
Original file line number Diff line number Diff line change
@@ -1,17 +1,29 @@
names = {"key": "value"}
nums = [1, 2, 3]

# these should match

del nums[:]

nums[:] = []


# these should not

names = {"key": "value"}

del names["key"]
del names[:] # type: ignore

del nums[0]

x = 1
del x

del nums[1:2]
del nums[1:]
del nums[:1]
del nums[::1]

nums[:] = [1, 2, 3]
nums[1:] = []
nums[:1] = []
nums[::1] = []
3 changes: 2 additions & 1 deletion test/data/err_131.txt
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
test/data/err_131.py:6:1 [FURB131]: Replace `del nums[:]` with `nums.clear()`
test/data/err_131.py:5:1 [FURB131]: Replace `del nums[:]` with `nums.clear()`
test/data/err_131.py:7:1 [FURB131]: Replace `nums[:] = []` with `nums.clear()`

0 comments on commit dbe3126

Please sign in to comment.