Improve error messages:

More error messages now include proper names for expressions. For example, if you write `pathlib.Path()` instead of `Path()`, error messages will now include the `pathlib.` part, which Refurb previously would not include. This should help give better context for error messages.
dosisod · Oct 13, 2023 · 19f80c8 · 19f80c8
1 parent dd01007
commit 19f80c8
Show file tree

Hide file tree

Showing 11 changed files with 83 additions and 27 deletions.
diff --git a/refurb/checks/common.py b/refurb/checks/common.py
@@ -281,3 +281,14 @@ def is_type_none_call(node: Expression) -> bool:
             return True
 
     return False
+
+
+def stringify(node: Node) -> str:
+    match node:
+        case MemberExpr(expr=expr, name=name):
+            return f"{stringify(expr)}.{name}"
+
+        case NameExpr(name=name):
+            return name
+
+    return str(node)  # pragma: no cover
diff --git a/refurb/checks/decimal/simplify_ctor.py b/refurb/checks/decimal/simplify_ctor.py
@@ -1,6 +1,6 @@
 from dataclasses import dataclass
 
-from mypy.nodes import CallExpr, NameExpr, RefExpr, StrExpr
+from mypy.nodes import CallExpr, MemberExpr, NameExpr, RefExpr, StrExpr
 
 from refurb.error import Error
 
@@ -43,7 +43,7 @@ class ErrorInfo(Error):
 def check(node: CallExpr, errors: list[Error]) -> None:
     match node:
         case CallExpr(
-            callee=RefExpr(fullname="_decimal.Decimal"),
+            callee=RefExpr(fullname="_decimal.Decimal") as ref,
             args=[arg],
         ):
             match arg:
@@ -59,14 +59,22 @@ def check(node: CallExpr, errors: list[Error]) -> None:
                     except ValueError:
                         return
 
-                    msg = f'Replace `Decimal("{old}")` with `Decimal({new})`'
+                    func_name = stringify_decimal_expr(ref)
+
+                    msg = f'Replace `{func_name}("{old}")` with `{func_name}({new})`'  # noqa: E501
 
                     errors.append(ErrorInfo.from_node(node, msg))
 
                 case CallExpr(
                     callee=NameExpr(fullname="builtins.float"),
                     args=[StrExpr(value=value)],
                 ) if value.lower() in FLOAT_LITERALS:
-                    msg = f'Replace `Decimal(float("{value}"))` with `Decimal("{value}")'  # noqa: E501
+                    func_name = stringify_decimal_expr(ref)
+
+                    msg = f'Replace `{func_name}(float("{value}"))` with `{func_name}("{value}")`'  # noqa: E501
 
                     errors.append(ErrorInfo.from_node(node, msg))
+
+
+def stringify_decimal_expr(node: RefExpr) -> str:
+    return "decimal.Decimal" if isinstance(node, MemberExpr) else "Decimal"
diff --git a/refurb/checks/functools/use_cache.py b/refurb/checks/functools/use_cache.py
@@ -1,6 +1,13 @@
 from dataclasses import dataclass
 
-from mypy.nodes import ArgKind, CallExpr, Decorator, NameExpr, RefExpr
+from mypy.nodes import (
+    ArgKind,
+    CallExpr,
+    Decorator,
+    MemberExpr,
+    NameExpr,
+    RefExpr,
+)
 
 from refurb.error import Error
 from refurb.settings import Settings
@@ -47,11 +54,17 @@ def check(node: Decorator, errors: list[Error], settings: Settings) -> None:
         case Decorator(
             decorators=[
                 CallExpr(
-                    callee=RefExpr(fullname="functools.lru_cache"),
+                    callee=RefExpr(fullname="functools.lru_cache") as ref,
                     arg_names=["maxsize"],
                     arg_kinds=[ArgKind.ARG_NAMED],
                     args=[NameExpr(fullname="builtins.None")],
                 )
             ]
         ):
-            errors.append(ErrorInfo.from_node(node))
+            prefix = "functools." if isinstance(ref, MemberExpr) else ""
+            old = f"@{prefix}lru_cache(maxsize=None)"
+            new = f"@{prefix}cache"
+
+            msg = f"Replace `{old}` with `{new}`"
+
+            errors.append(ErrorInfo.from_node(node, msg))
diff --git a/refurb/checks/pathlib/simplify_ctor.py b/refurb/checks/pathlib/simplify_ctor.py
@@ -2,6 +2,7 @@
 
 from mypy.nodes import CallExpr, RefExpr, StrExpr
 
+from refurb.checks.common import stringify
 from refurb.error import Error
 
 
@@ -33,10 +34,12 @@ def check(node: CallExpr, errors: list[Error]) -> None:
     match node:
         case CallExpr(
             args=[StrExpr(value="." | "" as value)],
-            callee=RefExpr(fullname="pathlib.Path"),
+            callee=RefExpr(fullname="pathlib.Path") as ref,
         ):
+            func_name = stringify(ref)
+
             errors.append(
                 ErrorInfo.from_node(
-                    node, f'Replace `Path("{value}")` with `Path()`'
+                    node, f'Replace `{func_name}("{value}")` with `Path()`'
                 )
             )
diff --git a/refurb/checks/secrets/simplify_token_function.py b/refurb/checks/secrets/simplify_token_function.py
@@ -10,6 +10,7 @@
     SliceExpr,
 )
 
+from refurb.checks.common import stringify
 from refurb.error import Error
 
 
@@ -45,13 +46,13 @@ def check(node: CallExpr | IndexExpr, errors: list[Error]) -> None:
         case CallExpr(
             callee=MemberExpr(
                 expr=CallExpr(
-                    callee=RefExpr(fullname=fullname),
+                    callee=RefExpr(fullname="secrets.token_bytes") as ref,
                     args=token_args,
                 ),
                 name="hex",
             ),
             args=[],
-        ) if fullname == "secrets.token_bytes":
+        ):
             match token_args:
                 case [IntExpr(value=value)]:
                     arg = str(value)
@@ -66,15 +67,21 @@ def check(node: CallExpr | IndexExpr, errors: list[Error]) -> None:
                     return
 
             new_arg = "" if arg == "None" else arg
+            prefix = "secrets." if isinstance(ref, MemberExpr) else ""
+            old = f"{prefix}token_bytes({arg}).hex()"
+            new = f"{prefix}token_hex({new_arg})"
 
-            msg = f"Replace `token_bytes({arg}).hex()` with `token_hex({new_arg})`"  # noqa: E501
+            msg = f"Replace `{old}` with `{new}`"
 
             errors.append(ErrorInfo.from_node(node, msg))
 
         # Detects `token_xyz()[:x]`
         case IndexExpr(
             base=CallExpr(
-                callee=RefExpr(fullname=fullname, name=name),  # type: ignore
+                callee=RefExpr(
+                    fullname=fullname,
+                    name=name,  # type: ignore[misc]
+                ) as ref,
                 args=[] | [NameExpr(fullname="builtins.None")] as args,
             ),
             index=SliceExpr(
@@ -84,8 +91,9 @@ def check(node: CallExpr | IndexExpr, errors: list[Error]) -> None:
             ),
         ) if fullname in ("secrets.token_hex", "secrets.token_bytes"):
             arg = "None" if args else ""
+            func_name = stringify(ref)
 
-            old = f"{name}({arg})[:{size}]"
+            old = f"{func_name}({arg})[:{size}]"
 
             # size must be multiple of 2 for hex functions since each hex digit
             # takes up 2 bytes.
@@ -95,7 +103,7 @@ def check(node: CallExpr | IndexExpr, errors: list[Error]) -> None:
 
                 size //= 2
 
-            new = f"{name}({size})"
+            new = f"{func_name}({size})"
 
             msg = f"Replace `{old}` with `{new}`"
 

diff --git a/test/data/err_134.txt b/test/data/err_134.txt
@@ -1,2 +1,2 @@
 test/data/err_134.py:6:2 [FURB134]: Replace `@lru_cache(maxsize=None)` with `@cache`
-test/data/err_134.py:10:2 [FURB134]: Replace `@lru_cache(maxsize=None)` with `@cache`
+test/data/err_134.py:10:2 [FURB134]: Replace `@functools.lru_cache(maxsize=None)` with `@functools.cache`
diff --git a/test/data/err_143.py b/test/data/err_143.py
@@ -23,6 +23,17 @@
 _ = i or 0
 
 
+class C:
+    x: int
+    def __init__(self) -> None:
+        # x could be anything here
+        self.x = 123
+
+c = C()
+
+_ = c.x or 0
+
+
 # these should not
 
 _ = s or set((1, 2, 3))

diff --git a/test/data/err_153.txt b/test/data/err_153.txt
@@ -1,3 +1,3 @@
 test/data/err_153.py:6:5 [FURB153]: Replace `Path(".")` with `Path()`
 test/data/err_153.py:7:5 [FURB153]: Replace `Path("")` with `Path()`
-test/data/err_153.py:8:5 [FURB153]: Replace `Path(".")` with `Path()`
+test/data/err_153.py:8:5 [FURB153]: Replace `pathlib.Path(".")` with `Path()`
diff --git a/test/data/err_157.py b/test/data/err_157.py
@@ -18,6 +18,7 @@
 _ = Decimal(float("NaN"))
 _ = Decimal(float("nan"))
 _ = decimal.Decimal("0")
+_ = decimal.Decimal(float("nan"))
 
 
 # these should not

diff --git a/test/data/err_157.txt b/test/data/err_157.txt
@@ -5,11 +5,12 @@ test/data/err_157.py:9:5 [FURB157]: Replace `Decimal("-1234")` with `Decimal(-12
 test/data/err_157.py:10:5 [FURB157]: Replace `Decimal("1234")` with `Decimal(1234)`
 test/data/err_157.py:11:5 [FURB157]: Replace `Decimal("01234")` with `Decimal(1234)`
 test/data/err_157.py:12:5 [FURB157]: Replace `Decimal("\r\n\r 1234")` with `Decimal(1234)`
-test/data/err_157.py:13:5 [FURB157]: Replace `Decimal(float("Infinity"))` with `Decimal("Infinity")
-test/data/err_157.py:14:5 [FURB157]: Replace `Decimal(float("-Infinity"))` with `Decimal("-Infinity")
-test/data/err_157.py:15:5 [FURB157]: Replace `Decimal(float("inf"))` with `Decimal("inf")
-test/data/err_157.py:16:5 [FURB157]: Replace `Decimal(float("-inf"))` with `Decimal("-inf")
-test/data/err_157.py:17:5 [FURB157]: Replace `Decimal(float("-INF"))` with `Decimal("-INF")
-test/data/err_157.py:18:5 [FURB157]: Replace `Decimal(float("NaN"))` with `Decimal("NaN")
-test/data/err_157.py:19:5 [FURB157]: Replace `Decimal(float("nan"))` with `Decimal("nan")
-test/data/err_157.py:20:5 [FURB157]: Replace `Decimal("0")` with `Decimal(0)`
+test/data/err_157.py:13:5 [FURB157]: Replace `Decimal(float("Infinity"))` with `Decimal("Infinity")`
+test/data/err_157.py:14:5 [FURB157]: Replace `Decimal(float("-Infinity"))` with `Decimal("-Infinity")`
+test/data/err_157.py:15:5 [FURB157]: Replace `Decimal(float("inf"))` with `Decimal("inf")`
+test/data/err_157.py:16:5 [FURB157]: Replace `Decimal(float("-inf"))` with `Decimal("-inf")`
+test/data/err_157.py:17:5 [FURB157]: Replace `Decimal(float("-INF"))` with `Decimal("-INF")`
+test/data/err_157.py:18:5 [FURB157]: Replace `Decimal(float("NaN"))` with `Decimal("NaN")`
+test/data/err_157.py:19:5 [FURB157]: Replace `Decimal(float("nan"))` with `Decimal("nan")`
+test/data/err_157.py:20:5 [FURB157]: Replace `decimal.Decimal("0")` with `decimal.Decimal(0)`
+test/data/err_157.py:21:5 [FURB157]: Replace `decimal.Decimal(float("nan"))` with `decimal.Decimal("nan")`
diff --git a/test/data/err_174.txt b/test/data/err_174.txt
@@ -1,9 +1,9 @@
 test/data/err_174.py:6:1 [FURB174]: Replace `token_bytes(32).hex()` with `token_hex(32)`
 test/data/err_174.py:7:1 [FURB174]: Replace `token_bytes(None).hex()` with `token_hex()`
 test/data/err_174.py:8:1 [FURB174]: Replace `token_bytes().hex()` with `token_hex()`
-test/data/err_174.py:9:1 [FURB174]: Replace `token_bytes().hex()` with `token_hex()`
+test/data/err_174.py:9:1 [FURB174]: Replace `secrets.token_bytes().hex()` with `secrets.token_hex()`
 test/data/err_174.py:11:1 [FURB174]: Replace `token_bytes()[:8]` with `token_bytes(8)`
 test/data/err_174.py:12:1 [FURB174]: Replace `token_hex()[:8]` with `token_hex(4)`
 test/data/err_174.py:13:1 [FURB174]: Replace `token_bytes(None)[:8]` with `token_bytes(8)`
 test/data/err_174.py:14:1 [FURB174]: Replace `token_hex(None)[:8]` with `token_hex(4)`
-test/data/err_174.py:15:1 [FURB174]: Replace `token_bytes()[:8]` with `token_bytes(8)`
+test/data/err_174.py:15:1 [FURB174]: Replace `secrets.token_bytes()[:8]` with `secrets.token_bytes(8)`