From 1ceac2e60ad7b9b9b63770512dd0da9918786a66 Mon Sep 17 00:00:00 2001 From: dosisod <39638017+dosisod@users.noreply.github.com> Date: Wed, 17 Jan 2024 23:34:19 -0800 Subject: [PATCH] Add FURB187 `use-reverse` --- docs/checks.md | 27 ++++++ refurb/checks/common.py | 7 +- refurb/checks/readability/use_reverse.py | 105 +++++++++++++++++++++++ test/data/err_187.py | 43 ++++++++++ test/data/err_187.txt | 3 + 5 files changed, 184 insertions(+), 1 deletion(-) create mode 100644 refurb/checks/readability/use_reverse.py create mode 100644 test/data/err_187.py create mode 100644 test/data/err_187.txt diff --git a/docs/checks.md b/docs/checks.md index ad6aa67..9bb11e6 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -2300,4 +2300,31 @@ Good: names = ["Bob", "Alice", "Charlie"] names.sort() +``` + +## FURB187: `use-reverse` + +Categories: `performance` `readability` + +Don't use `x[::-1]` or `reversed(x)` to reverse a list and reassign it to +itself, use the faster in-place `.reverse()` method instead. + +Bad: + +```python +names = ["Bob", "Alice", "Charlie"] + +names = reverse(names) +# or +names = list(reverse(names)) +# or +names = names[::-1] +``` + +Good: + +```python +names = ["Bob", "Alice", "Charlie"] + +names.reverse() ``` \ No newline at end of file diff --git a/refurb/checks/common.py b/refurb/checks/common.py index 8895cd5..098801d 100644 --- a/refurb/checks/common.py +++ b/refurb/checks/common.py @@ -3,6 +3,7 @@ from mypy.nodes import ( ArgKind, + AssignmentStmt, Block, BytesExpr, CallExpr, @@ -105,7 +106,7 @@ def check_for_loop_like( def unmangle_name(name: str | None) -> str: - return (name or "").replace("'", "") + return (name or "").rstrip("'*") def is_equivalent(lhs: Node | None, rhs: Node | None) -> bool: @@ -396,6 +397,10 @@ def _stringify(node: Node) -> str: return f"{{{inner}}}" + # TODO: support multiple lvalues + case AssignmentStmt(lvalues=[lhs], rvalue=rhs): + return f"{stringify(lhs)} = {stringify(rhs)}" + raise ValueError diff --git a/refurb/checks/readability/use_reverse.py b/refurb/checks/readability/use_reverse.py new file mode 100644 index 0000000..4b32bcf --- /dev/null +++ b/refurb/checks/readability/use_reverse.py @@ -0,0 +1,105 @@ +from dataclasses import dataclass + +from mypy.nodes import ( + AssignmentStmt, + CallExpr, + IndexExpr, + IntExpr, + NameExpr, + SliceExpr, + UnaryExpr, + Var, +) + +from refurb.checks.common import stringify, unmangle_name +from refurb.error import Error + + +@dataclass +class ErrorInfo(Error): + """ + Don't use `x[::-1]` or `reversed(x)` to reverse a list and reassign it to + itself, use the faster in-place `.reverse()` method instead. + + Bad: + + ``` + names = ["Bob", "Alice", "Charlie"] + + names = reverse(names) + # or + names = list(reverse(names)) + # or + names = names[::-1] + ``` + + Good: + + ``` + names = ["Bob", "Alice", "Charlie"] + + names.reverse() + ``` + """ + + name = "use-reverse" + categories = ("performance", "readability") + code = 187 + + +def check(node: AssignmentStmt, errors: list[Error]) -> None: + match node: + case AssignmentStmt( + lvalues=[NameExpr(fullname=assign_name) as assign_ref], + rvalue=CallExpr( + callee=NameExpr(fullname="builtins.reversed"), + args=[NameExpr(fullname=reverse_name, node=Var(type=ty))], + ), + ) if ( + unmangle_name(assign_name) == unmangle_name(reverse_name) + and str(ty).startswith("builtins.list[") + ): + pass + + case AssignmentStmt( + lvalues=[NameExpr(fullname=assign_name) as assign_ref], + rvalue=CallExpr( + callee=NameExpr(fullname="builtins.list"), + args=[ + CallExpr( + callee=NameExpr(fullname="builtins.reversed"), + args=[NameExpr(fullname=reverse_name, node=Var(type=ty))], + ), + ], + ), + ) if ( + unmangle_name(assign_name) == unmangle_name(reverse_name) + and str(ty).startswith("builtins.list[") + ): + pass + + case AssignmentStmt( + lvalues=[NameExpr(fullname=assign_name) as assign_ref], + rvalue=IndexExpr( + base=NameExpr(fullname=reverse_name, node=Var(type=ty)), + index=SliceExpr( + begin_index=None, + end_index=None, + stride=UnaryExpr(op="-", expr=IntExpr(value=1)), + ), + ), + ) if ( + unmangle_name(assign_name) == unmangle_name(reverse_name) + and str(ty).startswith("builtins.list[") + ): + pass + + case _: + return + + old = stringify(node) + new = f"{stringify(assign_ref)}.reverse()" + + msg = f"Replace `{old}` with `{new}`" + + errors.append(ErrorInfo.from_node(node, msg)) diff --git a/test/data/err_187.py b/test/data/err_187.py new file mode 100644 index 0000000..668efa0 --- /dev/null +++ b/test/data/err_187.py @@ -0,0 +1,43 @@ +# these should match + +# using functions to ensure `l` doesn't change type +def a(): + l = [] + l = reversed(l) + +def b(): + l = [] + l = list(reversed(l)) + +def c(): + l = [] + l = l[::-1] + + +# these should not + +def d(): + l = [] + _ = reversed(l) + +def e(): + l = [] + l = l[::-2] + l = l[1:] + l = l[1::-1] + l = l[:1:-1] + +def f(): + d = {} + + # dont warn since d is a dict and does not have a .reverse() method + d = reversed(d) + +def g(): + l = "abc"[::-1] + +def h(): + l = reversed([1, 2, 3]) + +def i(): + l = list(reversed([1, 2, 3])) diff --git a/test/data/err_187.txt b/test/data/err_187.txt new file mode 100644 index 0000000..3eb6d7c --- /dev/null +++ b/test/data/err_187.txt @@ -0,0 +1,3 @@ +test/data/err_187.py:6:5 [FURB187]: Replace `l = reversed(l)` with `l.reverse()` +test/data/err_187.py:10:5 [FURB187]: Replace `l = list(reversed(l))` with `l.reverse()` +test/data/err_187.py:14:5 [FURB187]: Replace `l = l[::-1]` with `l.reverse()`