From dbe31262aa873690724b6c01db704228fe48f9e2 Mon Sep 17 00:00:00 2001 From: dosisod <39638017+dosisod@users.noreply.github.com> Date: Wed, 31 Jan 2024 22:19:14 -0800 Subject: [PATCH] Reimplement FURB131: 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. --- docs/checks.md | 16 +++++------ refurb/checks/builtin/no_del.py | 51 +++++++++++++++++++++------------ refurb/checks/common.py | 4 +++ test/data/err_131.py | 16 +++++++++-- test/data/err_131.txt | 3 +- 5 files changed, 59 insertions(+), 31 deletions(-) diff --git a/docs/checks.md b/docs/checks.md index 6bf25f4..d834e9d 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -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() ``` diff --git a/refurb/checks/builtin/no_del.py b/refurb/checks/builtin/no_del.py index 6fe35ad..3bd2543 100644 --- a/refurb/checks/builtin/no_del.py +++ b/refurb/checks/builtin/no_del.py @@ -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 @@ -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)) diff --git a/refurb/checks/common.py b/refurb/checks/common.py index de3c311..5ab9555 100644 --- a/refurb/checks/common.py +++ b/refurb/checks/common.py @@ -10,6 +10,7 @@ ComparisonExpr, ComplexExpr, ConditionalExpr, + DelStmt, DictExpr, DictionaryComprehension, Expression, @@ -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 diff --git a/test/data/err_131.py b/test/data/err_131.py index 80bef79..d5ee130 100644 --- a/test/data/err_131.py +++ b/test/data/err_131.py @@ -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] = [] diff --git a/test/data/err_131.txt b/test/data/err_131.txt index a58bfbf..203efd1 100644 --- a/test/data/err_131.txt +++ b/test/data/err_131.txt @@ -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()`