Merge pull request #328 from jakkdl/suggest_r_format_specifier

Add B028 - suggest !r for quote-wrapped variables in f-strings

Author: Zac-HD

Diff for: README.rst

@@ -169,6 +169,8 @@ limitations make it difficult.
 
 **B027**: Empty method in abstract base class, but has no abstract decorator. Consider adding @abstractmethod.
 
+**B028**: Consider replacing ``f"'{foo}'"`` with ``f"{foo!r}"`` which is both easier to read and will escape quotes inside ``foo`` if that would appear. The check tries to filter out any format specs that are invalid together with ``!r``. If you're using other conversion flags then e.g. ``f"'{foo!a}'"`` can be replaced with ``f"{ascii(foo)!r}"``. Not currently implemented for python<3.8 or ``str.format()`` calls.
+
 Opinionated warnings
 ~~~~~~~~~~~~~~~~~~~~
 
@@ -310,6 +312,7 @@ Future
 ~~~~~~~~~
 
 * Add B906: ``visit_`` function with no further calls to a ``visit`` function. (#313)
+* Add B028: Suggest ``!r`` when formatted value in f-string is surrounded by quotes. (#319)
 
 22.12.6
 ~~~~~~~~~

Diff for: bugbear.py

@@ -4,6 +4,7 @@
 import logging
 import math
 import re
+import sys
 from collections import namedtuple
 from contextlib import suppress
 from functools import lru_cache, partial
@@ -448,6 +449,10 @@ def visit_With(self, node):
         self.check_for_b022(node)
         self.generic_visit(node)
 
+    def visit_JoinedStr(self, node):
+        self.check_for_b028(node)
+        self.generic_visit(node)
+
     def check_for_b005(self, node):
         if node.func.attr not in B005.methods:
             return  # method name doesn't match
@@ -1009,6 +1014,116 @@ def check_for_b906(self, node: ast.FunctionDef):
         else:
             self.errors.append(B906(node.lineno, node.col_offset))
 
+    def check_for_b028(self, node: ast.JoinedStr):  # noqa: C901
+        # AST structure of strings in f-strings in 3.7 is different enough this
+        # implementation doesn't work
+        if sys.version_info <= (3, 7):
+            return  # pragma: no cover
+
+        def myunparse(node: ast.AST) -> str:  # pragma: no cover
+            if sys.version_info >= (3, 9):
+                return ast.unparse(node)
+            if isinstance(node, ast.Name):
+                return node.id
+            if isinstance(node, ast.Attribute):
+                return myunparse(node.value) + "." + node.attr
+            if isinstance(node, ast.Constant):
+                return repr(node.value)
+            if isinstance(node, ast.Call):
+                return myunparse(node.func) + "()"  # don't bother with arguments
+
+            # as a last resort, just give the type name
+            return type(node).__name__
+
+        quote_marks = "'\""
+        current_mark = None
+        variable = None
+        for value in node.values:
+            # check for quote mark after pre-marked variable
+            if (
+                current_mark is not None
+                and variable is not None
+                and isinstance(value, ast.Constant)
+                and isinstance(value.value, str)
+                and value.value[0] == current_mark
+            ):
+                self.errors.append(
+                    B028(
+                        variable.lineno,
+                        variable.col_offset,
+                        vars=(myunparse(variable.value),),
+                    )
+                )
+                current_mark = variable = None
+                # don't continue with length>1, so we can detect a new pre-mark
+                # in the same string as a post-mark, e.g. `"{foo}" "{bar}"`
+                if len(value.value) == 1:
+                    continue
+
+            # detect pre-mark
+            if (
+                isinstance(value, ast.Constant)
+                and isinstance(value.value, str)
+                and value.value[-1] in quote_marks
+            ):
+                current_mark = value.value[-1]
+                variable = None
+                continue
+
+            # detect variable, if there's a pre-mark
+            if (
+                current_mark is not None
+                and variable is None
+                and isinstance(value, ast.FormattedValue)
+                and value.conversion != ord("r")
+            ):
+                # check if the format spec shows that this is numeric
+                # or otherwise hard/impossible to convert to `!r`
+                if (
+                    isinstance(value.format_spec, ast.JoinedStr)
+                    and value.format_spec.values  # empty format spec is fine
+                ):
+                    if (
+                        # if there's variables in the format_spec, skip
+                        len(value.format_spec.values) > 1
+                        or not isinstance(value.format_spec.values[0], ast.Constant)
+                    ):
+                        current_mark = variable = None
+                        continue
+                    format_specifier = value.format_spec.values[0].value
+
+                    # if second character is an align, first character is a fill
+                    # char - strip it
+                    if len(format_specifier) > 1 and format_specifier[1] in "<>=^":
+                        format_specifier = format_specifier[1:]
+
+                    # strip out precision digits, so the only remaining ones are
+                    # width digits, which will misplace the quotes
+                    format_specifier = re.sub(r"\.\d*", "", format_specifier)
+
+                    # skip if any invalid characters in format spec
+                    invalid_characters = "".join(
+                        (
+                            "=",  # align character only valid for numerics
+                            "+- ",  # sign
+                            "0123456789",  # width digits
+                            "z",  # coerce negative zero floating point to positive
+                            "#",  # alternate form
+                            "_,",  # thousands grouping
+                            "bcdeEfFgGnoxX%",  # various number specifiers
+                        )
+                    )
+                    if set(format_specifier).intersection(invalid_characters):
+                        current_mark = variable = None
+                        continue
+
+                # otherwise save value as variable
+                variable = value
+                continue
+
+            # if no pre-mark or variable detected, reset state
+            current_mark = variable = None
+
 
 def compose_call_path(node):
     if isinstance(node, ast.Attribute):
@@ -1370,6 +1485,12 @@ def visit_Lambda(self, node):
         " decorator. Consider adding @abstractmethod."
     )
 )
+B028 = Error(
+    message=(
+        "B028 {!r} is manually surrounded by quotes, consider using the `!r` conversion"
+        " flag."
+    )
+)
 
 # Warnings disabled by default.
 B901 = Error(

Diff for: tests/b028.py

@@ -0,0 +1,114 @@
+def foo():
+    return "hello"
+
+
+var = var2 = "hello"
+
+# warnings
+f"begin '{var}' end"
+f"'{var}' end"
+f"begin '{var}'"
+
+f'begin "{var}" end'
+f'"{var}" end'
+f'begin "{var}"'
+
+f'a "{"hello"}" b'
+f'a "{foo()}" b'
+
+# fmt: off
+k = (f'"' # error emitted on this line since all values are assigned the same lineno
+     f'{var}'
+     f'"'
+     f'"')
+
+k = (f'"' # error emitted on this line
+     f'{var}'
+     '"'
+     f'"')
+# fmt: on
+
+f'{"hello"}"{var}"'  # warn for var and not hello
+f'"{var}"{"hello"}'  # warn for var and not hello
+f'"{var}" and {"hello"} and "{var2}"'  # warn for var and var2
+f'"{var}" and "{var2}"'  # warn for both
+f'"{var}""{var2}"'  # warn for both
+
+# check that pre-quote & variable is reset if no post-quote is found
+f'"{var}abc "{var2}"'  # warn on var2
+
+# check formatting on different contained types
+f'"{var}"'
+f'"{var.__str__}"'
+f'"{var.__str__.__repr__}"'
+f'"{3+5}"'
+f'"{foo()}"'
+f'"{None}"'
+f'"{...}"'  # although f'"{...!r}"' == 'Ellipsis'
+f'"{True}"'
+
+# alignment specifier
+f'"{var:<}"'
+f'"{var:>}"'
+f'"{var:^}"'
+f'"{var:5<}"'
+
+# explicit string specifier
+f'"{var:s}"'
+
+# empty format string
+f'"{var:}"'
+
+# These all currently give warnings, but could be considered false alarms
+# multiple quote marks
+f'"""{var}"""'
+# str conversion specified
+f'"{var!s}"'
+# two variables fighting over the same quote mark
+f'"{var}"{var2}"'  # currently gives warning on the first one
+
+
+# ***no warnings*** #
+
+# padding inside quotes
+f'"{var:5}"'
+
+# quote mark not immediately adjacent
+f'" {var} "'
+f'"{var} "'
+f'" {var}"'
+
+# mixed quote marks
+f"'{var}\""
+
+# repr specifier already given
+f'"{var!r}"'
+
+# two variables in a row with no quote mark inbetween
+f'"{var}{var}"'
+
+# don't crash on non-string constants
+f'5{var}"'
+f"\"{var}'"
+
+# sign option (only valid for number types)
+f'"{var:+}"'
+
+# integer presentation type specified
+f'"{var:b}"'
+f'"{var:x}"'
+
+# float presentation type
+f'"{var:e%}"'
+
+# alignment specifier invalid for strings
+f'"{var:=}"'
+
+# other types and combinations are tested in test_b028_format_specifier_permutations
+
+# don't attempt to parse complex format specs
+f'"{var:{var}}"'
+f'"{var:5{var}}"'
+
+# even if explicit string type (not implemented)
+f'"{var:{var}s}"'