Improve error messages

dosisod · Jan 17, 2024 · 771df6f · 771df6f
1 parent 3bf4058
commit 771df6f
Show file tree

Hide file tree

Showing 17 changed files with 157 additions and 121 deletions.
diff --git a/refurb/checks/builtin/no_del.py b/refurb/checks/builtin/no_del.py
@@ -2,6 +2,7 @@
 
 from mypy.nodes import DelStmt, IndexExpr, NameExpr, SliceExpr, Var
 
+from refurb.checks.common import stringify
 from refurb.error import Error
 
 
@@ -37,14 +38,17 @@ class ErrorInfo(Error):
     name = "no-del"
     code = 131
     categories = ("builtin", "readability")
-    msg: str = "Replace `del x[:]` with `x.clear()`"
 
 
 def check(node: DelStmt, errors: list[Error]) -> None:
     match node:
-        case DelStmt(expr=IndexExpr(base=NameExpr(node=Var(type=ty)), index=index)) if str(
+        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):
-                    errors.append(ErrorInfo.from_node(node))
+                    expr = stringify(expr)  # type: ignore
+
+                    msg = f"Replace `del {expr}[:]` with `{expr}.clear()`"
+
+                    errors.append(ErrorInfo.from_node(node, msg))
diff --git a/refurb/checks/builtin/no_ignored_dict_items.py b/refurb/checks/builtin/no_ignored_dict_items.py
@@ -12,7 +12,7 @@
     Var,
 )
 
-from refurb.checks.common import check_for_loop_like, is_name_unused_in_contexts
+from refurb.checks.common import check_for_loop_like, is_name_unused_in_contexts, stringify
 from refurb.error import Error
 
 
@@ -67,21 +67,27 @@ def check_dict_items_call(
             TupleExpr(items=[NameExpr() as key, NameExpr() as value]),
             CallExpr(
                 callee=MemberExpr(
-                    expr=NameExpr(node=Var(type=ty)),
+                    expr=NameExpr(node=Var(type=ty)) as dict_expr,
                     name="items",
                 )
             ),
         ) if str(ty).startswith("builtins.dict["):
-            check_unused_key_or_value(key, value, contexts, errors)
+            check_unused_key_or_value(key, value, contexts, errors, dict_expr)
 
 
 def check_unused_key_or_value(
-    key: NameExpr, value: NameExpr, contexts: list[Node], errors: list[Error]
+    key: NameExpr,
+    value: NameExpr,
+    contexts: list[Node],
+    errors: list[Error],
+    dict_expr: NameExpr,
 ) -> None:
     if is_name_unused_in_contexts(key, contexts):
-        errors.append(
-            ErrorInfo.from_node(key, "Key is unused, use `for value in d.values()` instead")
-        )
+        msg = f"Key is unused, use `for {stringify(value)} in {stringify(dict_expr)}.values()` instead"  # noqa: E501
+
+        errors.append(ErrorInfo.from_node(key, msg))
 
     if is_name_unused_in_contexts(value, contexts):
-        errors.append(ErrorInfo.from_node(value, "Value is unused, use `for key in d` instead"))
+        msg = f"Value is unused, use `for {stringify(key)} in {stringify(dict_expr)}` instead"
+
+        errors.append(ErrorInfo.from_node(value, msg))
diff --git a/refurb/checks/builtin/no_ignored_enumerate.py b/refurb/checks/builtin/no_ignored_enumerate.py
@@ -11,7 +11,7 @@
     Var,
 )
 
-from refurb.checks.common import check_for_loop_like, is_name_unused_in_contexts
+from refurb.checks.common import check_for_loop_like, is_name_unused_in_contexts, stringify
 from refurb.error import Error
 
 
@@ -66,22 +66,28 @@ def check_enumerate_call(
             TupleExpr(items=[NameExpr() as index, NameExpr() as value]),
             CallExpr(
                 callee=NameExpr(fullname="builtins.enumerate"),
-                args=[NameExpr(node=Var(type=ty))],
+                args=[NameExpr(node=Var(type=ty)) as enumerate_arg],
             ),
         ) if is_sequence_type(str(ty)):
-            check_unused_index_or_value(index, value, contexts, errors)
+            check_unused_index_or_value(index, value, contexts, errors, enumerate_arg)
 
 
 def check_unused_index_or_value(
-    index: NameExpr, value: NameExpr, contexts: list[Node], errors: list[Error]
+    index: NameExpr,
+    value: NameExpr,
+    contexts: list[Node],
+    errors: list[Error],
+    enumerate_arg: NameExpr,
 ) -> None:
     if is_name_unused_in_contexts(index, contexts):
-        errors.append(ErrorInfo.from_node(index, "Index is unused, use `for x in y` instead"))
+        msg = f"Index is unused, use `for {stringify(value)} in {stringify(enumerate_arg)}` instead"  # noqa: E501
+
+        errors.append(ErrorInfo.from_node(index, msg))
 
     if is_name_unused_in_contexts(value, contexts):
-        errors.append(
-            ErrorInfo.from_node(value, "Value is unused, use `for x in range(len(y))` instead")
-        )
+        msg = f"Value is unused, use `for {stringify(index)} in range(len({stringify(enumerate_arg)}))` instead"  # noqa: E501
+
+        errors.append(ErrorInfo.from_node(value, msg))
 
 
 # TODO: allow for any type that supports the Sequence protocol

diff --git a/refurb/checks/iterable/implicit_readlines.py b/refurb/checks/iterable/implicit_readlines.py
@@ -2,6 +2,7 @@
 
 from mypy.nodes import CallExpr, Expression, ForStmt, GeneratorExpr, MemberExpr, NameExpr, Var
 
+from refurb.checks.common import stringify
 from refurb.error import Error
 
 
@@ -31,29 +32,28 @@ class ErrorInfo(Error):
 
     name = "simplify-readlines"
     code = 129
-    msg: str = "Replace `f.readlines()` with `f`"
     categories = ("builtin", "readability")
 
 
-def get_readline_file_object(expr: Expression) -> NameExpr | None:
+def check_for_readline_object(expr: Expression, errors: list[Error]) -> NameExpr | None:
     match expr:
         case CallExpr(
             callee=MemberExpr(expr=NameExpr(node=Var(type=ty)) as f, name="readlines"),
             args=[],
         ) if str(ty) in {"io.TextIOWrapper", "io.BufferedReader"}:
-            return f
+            f_name = stringify(f)
+
+            msg = f"Replace `{f_name}.readlines()` with `{f_name}`"
+
+            errors.append(ErrorInfo.from_node(f, msg))
 
     return None
 
 
 def check(node: ForStmt | GeneratorExpr, errors: list[Error]) -> None:
     if isinstance(node, ForStmt):
-        if f := get_readline_file_object(node.expr):
-            errors.append(ErrorInfo.from_node(f))
+        check_for_readline_object(node.expr, errors)
 
     else:
-        errors.extend(
-            ErrorInfo.from_node(f)
-            for expr in node.sequences
-            if (f := get_readline_file_object(expr))
-        )
+        for expr in node.sequences:
+            check_for_readline_object(expr, errors)
diff --git a/refurb/checks/logical/use_or.py b/refurb/checks/logical/use_or.py
@@ -2,7 +2,7 @@
 
 from mypy.nodes import ConditionalExpr
 
-from refurb.checks.common import is_equivalent
+from refurb.checks.common import is_equivalent, stringify
 from refurb.error import Error
 
 
@@ -29,10 +29,17 @@ class ErrorInfo(Error):
 
     name = "use-or-oper"
     code = 110
-    msg: str = "Replace `x if x else y` with `x or y`"
     categories = ("logical", "readability")
 
 
 def check(node: ConditionalExpr, errors: list[Error]) -> None:
     if is_equivalent(node.if_expr, node.cond):
-        errors.append(ErrorInfo.from_node(node))
+        if_true = stringify(node.if_expr)
+        if_false = stringify(node.else_expr)
+
+        old = f"{if_true} if {if_true} else {if_false}"
+        new = f"{if_true} or {if_false}"
+
+        msg = f"Replace `{old}` with `{new}`"
+
+        errors.append(ErrorInfo.from_node(node, msg))
diff --git a/refurb/checks/readability/in_keys.py b/refurb/checks/readability/in_keys.py
@@ -2,6 +2,7 @@
 
 from mypy.nodes import CallExpr, ComparisonExpr, MemberExpr, NameExpr, Var
 
+from refurb.checks.common import stringify
 from refurb.error import Error
 
 
@@ -43,10 +44,14 @@ def check(node: ComparisonExpr, errors: list[Error]) -> None:
                 _,
                 CallExpr(
                     callee=MemberExpr(
-                        expr=NameExpr(node=Var(type=ty)),
+                        expr=NameExpr(node=Var(type=ty)) as obj,
                         name="keys",
                     ),
                 ) as expr,
             ],
         ) if str(ty).startswith("builtins.dict"):
-            errors.append(ErrorInfo.from_node(expr, f"Replace `{oper} d.keys()` with `{oper} d`"))
+            obj_name = stringify(obj)
+
+            msg = f"Replace `{oper} {obj_name}.keys()` with `{oper} {obj_name}`"
+
+            errors.append(ErrorInfo.from_node(expr, msg))
diff --git a/refurb/checks/readability/no_is_bool_compare.py b/refurb/checks/readability/no_is_bool_compare.py
@@ -2,6 +2,7 @@
 
 from mypy.nodes import ComparisonExpr, Expression, NameExpr, Var
 
+from refurb.checks.common import stringify
 from refurb.error import Error
 
 
@@ -64,12 +65,16 @@ def check(node: ComparisonExpr, errors: list[Error]) -> None:
             operands=[NameExpr() as lhs, NameExpr() as rhs],
         ):
             if is_bool_literal(lhs) and is_bool_variable(rhs):
-                old = f"{lhs.name} {oper} x"
-                new = "x" if is_truthy(oper, lhs.name) else "not x"
+                expr = stringify(rhs)
+
+                old = f"{lhs.name} {oper} {expr}"
+                new = expr if is_truthy(oper, lhs.name) else f"not {expr}"
 
             elif is_bool_variable(lhs) and is_bool_literal(rhs):
-                old = f"x {oper} {rhs.name}"
-                new = "x" if is_truthy(oper, rhs.name) else "not x"
+                expr = stringify(lhs)
+
+                old = f"{expr} {oper} {rhs.name}"
+                new = expr if is_truthy(oper, rhs.name) else f"not {expr}"
 
             else:
                 return

diff --git a/refurb/checks/string/fstring_number.py b/refurb/checks/string/fstring_number.py
@@ -2,6 +2,7 @@
 
 from mypy.nodes import CallExpr, IndexExpr, IntExpr, NameExpr, SliceExpr
 
+from refurb.checks.common import stringify
 from refurb.error import Error
 
 
@@ -42,15 +43,14 @@ class ErrorInfo(Error):
 def check(node: IndexExpr, errors: list[Error]) -> None:
     match node:
         case IndexExpr(
-            base=CallExpr(callee=NameExpr() as name_node),
+            base=CallExpr(callee=NameExpr() as name_node, args=[arg]),
             index=SliceExpr(begin_index=IntExpr(value=2), end_index=None),
         ) if name_node.fullname in FUNC_CONVERSIONS:
+            arg = stringify(arg)  # type: ignore
+
             format = FUNC_CONVERSIONS[name_node.fullname or ""]
-            fstring = f'f"{{num:{format}}}"'
-
-            errors.append(
-                ErrorInfo.from_node(
-                    node,
-                    f"Replace `{name_node.name}(num)[2:]` with `{fstring}`",
-                )
-            )
+            fstring = f'f"{{{arg}:{format}}}"'
+
+            msg = f"Replace `{stringify(node)}` with `{fstring}`"
+
+            errors.append(ErrorInfo.from_node(node, msg))
diff --git a/refurb/checks/string/use_fstring_fmt.py b/refurb/checks/string/use_fstring_fmt.py
@@ -2,6 +2,7 @@
 
 from mypy.nodes import CallExpr, MemberExpr, NameExpr, StrExpr
 
+from refurb.checks.common import stringify
 from refurb.error import Error
 
 
@@ -38,14 +39,14 @@ class ErrorInfo(Error):
 
 
 CONVERSIONS = {
-    "builtins.str": "x",
-    "builtins.repr": "x!r",
-    "builtins.ascii": "x!a",
-    "builtins.bin": "x:#b",
-    "builtins.oct": "x:#o",
-    "builtins.hex": "x:#x",
-    "builtins.chr": "x:c",
-    "builtins.format": "x",
+    "builtins.str": "",
+    "builtins.repr": "!r",
+    "builtins.ascii": "!a",
+    "builtins.bin": ":#b",
+    "builtins.oct": ":#o",
+    "builtins.hex": ":#x",
+    "builtins.chr": ":c",
+    "builtins.format": "",
 }
 
 
@@ -58,10 +59,12 @@ def check(node: CallExpr, errors: list[Error]) -> None:
             match inner:
                 case CallExpr(
                     callee=NameExpr(fullname=fullname) as func,
-                    args=[_],
+                    args=[arg],
                 ) if fullname in CONVERSIONS:
-                    func_name = f"{{{func.name}(x)}}"
-                    conversion = f"{{{CONVERSIONS[fullname or '']}}}"  # noqa: FURB143, E501
+                    arg = stringify(arg)  # type: ignore
+
+                    func_name = f"{{{func.name}({arg})}}"
+                    conversion = f"{{{arg}{CONVERSIONS[fullname or '']}}}"  # noqa: FURB143
 
                     errors.append(
                         ErrorInfo.from_node(node, f"Replace `{func_name}` with `{conversion}`")

diff --git a/test/data/bug_equivalent_nodes.txt b/test/data/bug_equivalent_nodes.txt
@@ -1,12 +1,12 @@
-test/data/bug_equivalent_nodes.py:21:5 [FURB110]: Replace `x if x else y` with `x or y`
-test/data/bug_equivalent_nodes.py:30:5 [FURB110]: Replace `x if x else y` with `x or y`
-test/data/bug_equivalent_nodes.py:42:5 [FURB110]: Replace `x if x else y` with `x or y`
-test/data/bug_equivalent_nodes.py:49:5 [FURB110]: Replace `x if x else y` with `x or y`
-test/data/bug_equivalent_nodes.py:56:5 [FURB110]: Replace `x if x else y` with `x or y`
-test/data/bug_equivalent_nodes.py:63:5 [FURB110]: Replace `x if x else y` with `x or y`
-test/data/bug_equivalent_nodes.py:70:5 [FURB110]: Replace `x if x else y` with `x or y`
-test/data/bug_equivalent_nodes.py:77:5 [FURB110]: Replace `x if x else y` with `x or y`
-test/data/bug_equivalent_nodes.py:84:5 [FURB110]: Replace `x if x else y` with `x or y`
-test/data/bug_equivalent_nodes.py:91:5 [FURB110]: Replace `x if x else y` with `x or y`
-test/data/bug_equivalent_nodes.py:98:5 [FURB110]: Replace `x if x else y` with `x or y`
-test/data/bug_equivalent_nodes.py:105:5 [FURB110]: Replace `x if x else y` with `x or y`
+test/data/bug_equivalent_nodes.py:21:5 [FURB110]: Replace `bob.name if bob.name else "alice"` with `bob.name or "alice"`
+test/data/bug_equivalent_nodes.py:30:5 [FURB110]: Replace `nums[0] if nums[0] else 123` with `nums[0] or 123`
+test/data/bug_equivalent_nodes.py:42:5 [FURB110]: Replace `f(1) if f(1) else 2` with `f(1) or 2`
+test/data/bug_equivalent_nodes.py:49:5 [FURB110]: Replace `[1, 2, 3] if [1, 2, 3] else []` with `[1, 2, 3] or []`
+test/data/bug_equivalent_nodes.py:56:5 [FURB110]: Replace `[x, 4, 5, 6] if [x, 4, 5, 6] else []` with `[x, 4, 5, 6] or []`
+test/data/bug_equivalent_nodes.py:63:5 [FURB110]: Replace `not False if not False else False` with `not False or False`
+test/data/bug_equivalent_nodes.py:70:5 [FURB110]: Replace `1 + 2 if 1 + 2 else 3` with `1 + 2 or 3`
+test/data/bug_equivalent_nodes.py:77:5 [FURB110]: Replace `1 < 2 if 1 < 2 else 3` with `1 < 2 or 3`
+test/data/bug_equivalent_nodes.py:84:5 [FURB110]: Replace `nums[1:] if nums[1:] else nums` with `nums[1:] or nums`
+test/data/bug_equivalent_nodes.py:91:5 [FURB110]: Replace `{"k": "v"} if {"k": "v"} else {}` with `{"k": "v"} or {}`
+test/data/bug_equivalent_nodes.py:98:5 [FURB110]: Replace `(1, 2, 3) if (1, 2, 3) else ()` with `(1, 2, 3) or ()`
+test/data/bug_equivalent_nodes.py:105:5 [FURB110]: Replace `{1, 2, 3} if {1, 2, 3} else set()` with `{1, 2, 3} or set()`
diff --git a/test/data/err_110.txt b/test/data/err_110.txt
@@ -1,4 +1,4 @@
 test/data/err_110.py:10:5 [FURB110]: Replace `x if x else y` with `x or y`
-test/data/err_110.py:11:5 [FURB110]: Replace `x if x else y` with `x or y`
+test/data/err_110.py:11:5 [FURB110]: Replace `True if True else y` with `True or y`
 test/data/err_110.py:13:5 [FURB110]: Replace `x if x else y` with `x or y`
-test/data/err_110.py:18:5 [FURB110]: Replace `x if x else y` with `x or y`
+test/data/err_110.py:18:5 [FURB110]: Replace `f() if f() else y` with `f() or y`
diff --git a/test/data/err_116.txt b/test/data/err_116.txt
@@ -1,3 +1,3 @@
-test/data/err_116.py:3:5 [FURB116]: Replace `bin(num)[2:]` with `f"{num:b}"`
-test/data/err_116.py:4:5 [FURB116]: Replace `oct(num)[2:]` with `f"{num:o}"`
-test/data/err_116.py:5:5 [FURB116]: Replace `hex(num)[2:]` with `f"{num:x}"`
+test/data/err_116.py:3:5 [FURB116]: Replace `bin(1234)[2:]` with `f"{1234:b}"`
+test/data/err_116.py:4:5 [FURB116]: Replace `oct(1234)[2:]` with `f"{1234:o}"`
+test/data/err_116.py:5:5 [FURB116]: Replace `hex(1234)[2:]` with `f"{1234:x}"`
diff --git a/test/data/err_119.txt b/test/data/err_119.txt
@@ -1,8 +1,8 @@
-test/data/err_119.py:3:1 [FURB119]: Replace `{str(x)}` with `{x}`
-test/data/err_119.py:5:1 [FURB119]: Replace `{repr(x)}` with `{x!r}`
-test/data/err_119.py:7:1 [FURB119]: Replace `{ascii(x)}` with `{x!a}`
-test/data/err_119.py:9:1 [FURB119]: Replace `{bin(x)}` with `{x:#b}`
-test/data/err_119.py:11:1 [FURB119]: Replace `{oct(x)}` with `{x:#o}`
-test/data/err_119.py:13:1 [FURB119]: Replace `{hex(x)}` with `{x:#x}`
-test/data/err_119.py:15:1 [FURB119]: Replace `{chr(x)}` with `{x:c}`
-test/data/err_119.py:17:1 [FURB119]: Replace `{format(x)}` with `{x}`
+test/data/err_119.py:3:1 [FURB119]: Replace `{str("hello world")}` with `{"hello world"}`
+test/data/err_119.py:5:1 [FURB119]: Replace `{repr(123)}` with `{123!r}`
+test/data/err_119.py:7:1 [FURB119]: Replace `{ascii("hello world")}` with `{"hello world"!a}`
+test/data/err_119.py:9:1 [FURB119]: Replace `{bin(12)}` with `{12:#b}`
+test/data/err_119.py:11:1 [FURB119]: Replace `{oct(511)}` with `{511:#o}`
+test/data/err_119.py:13:1 [FURB119]: Replace `{hex(255)}` with `{255:#x}`
+test/data/err_119.py:15:1 [FURB119]: Replace `{chr(65)}` with `{65:c}`
+test/data/err_119.py:17:1 [FURB119]: Replace `{format("hello world")}` with `{"hello world"}`
diff --git a/test/data/err_131.txt b/test/data/err_131.txt
@@ -1 +1 @@
-test/data/err_131.py:6:1 [FURB131]: Replace `del x[:]` with `x.clear()`
+test/data/err_131.py:6:1 [FURB131]: Replace `del nums[:]` with `nums.clear()`
Original file line number	Diff line number	Diff line change
		@@ -1 +1 @@
		test/data/err_131.py:6:1 [FURB131]: Replace `del x[:]` with `x.clear()`
		test/data/err_131.py:6:1 [FURB131]: Replace `del nums[:]` with `nums.clear()`