PIE782: False positive when f-string contains numeric formatting

[jdufresne]

Tested on flake8-pie-0.4.2

test.py

v = 3.14159
print(f"{v:0.2f}")

$ python3 test.py
3.14
$ flake8 --select PIE782 test.py
test.py:2:7: PIE782: Unnecessary f-string. You can safely remove the `f` prefix.

But the f is not safe to remove.

[sbdchd]

Yeah we ran into this as well.

Turns out the heuristic for checking whether a f-string is necessary wasn't as simple as originally thought.

[peterjc]

Probably not unexpected, but d has the same issue:

v = 3.14159
print(f"{v:0.2f}")
level = 123
print(f"Level is {level:d}")

$ flake8 --select PIE pie.py 
pie.py:2:7: PIE782: Unnecessary f-string. You can safely remove the `f` prefix.
pie.py:4:7: PIE782: Unnecessary f-string. You can safely remove the `f` prefix.

In this case these were conversions from things like print("Level is %i" % level) using flynt,
https://github.com/ikamensh/flynt

[sbdchd]

I think removing the rule would be easier / simpler than trying to fix the heuristic

[peterjc]

Would the rule be useful if it was something as crude as f"string" without at least one { and at least one } character present?

[peterjc]

OK, I see now that the ast does not easily give you the literal value.

Currently you look at any JoinedStr and if it contains a FormattedValue it is fine, otherwise raise PIE782:

https://github.com/sbdchd/flake8-pie/blob/v0.2.1/flake8_pie.py#L32

I'm not familiar with the ast module, but think I see where the false positives come from - the format specifiers themselves:

>>> import ast
>>> def walk(node, indent=4):
...     print(" " * indent, str(node), node.s if hasattr(node, "s") else "-")
...     if hasattr(node, "values"):
...         for child in node.values:
...             walk(child, indent + 4)
... 
>>> for node in ast.walk(ast.parse("print(f'Hello {name:d}')")): walk(node)
... 
     <_ast.Module object at 0x7f89600ccb90> -
     <_ast.Expr object at 0x7f89600cca10> -
     <_ast.Call object at 0x7f89600cc790> -
     <_ast.Name object at 0x7f89600cc650> -
     <_ast.JoinedStr object at 0x7f89600cc990> -
         <_ast.Str object at 0x7f89600cca50> Hello 
         <_ast.FormattedValue object at 0x7f89600cc810> -
     <_ast.Load object at 0x7f89600bf5d0> -
     <_ast.Str object at 0x7f89600cca50> Hello 
     <_ast.FormattedValue object at 0x7f89600cc810> -
     <_ast.Name object at 0x7f89600cc690> -
     <_ast.JoinedStr object at 0x7f89600ccc10> -
         <_ast.Str object at 0x7f89600cc950> d
     <_ast.Load object at 0x7f89600bf5d0> -
     <_ast.Str object at 0x7f89600cc950> d
>>> for node in ast.walk(ast.parse("print(f'Hello {name}')")): walk(node)
... 
     <_ast.Module object at 0x7f89600cc690> -
     <_ast.Expr object at 0x7f89600cc810> -
     <_ast.Call object at 0x7f89600cca50> -
     <_ast.Name object at 0x7f89600cc610> -
     <_ast.JoinedStr object at 0x7f89600cc990> -
         <_ast.Str object at 0x7f89600cc650> Hello 
         <_ast.FormattedValue object at 0x7f89600cc790> -
     <_ast.Load object at 0x7f89600bf5d0> -
     <_ast.Str object at 0x7f89600cc650> Hello 
     <_ast.FormattedValue object at 0x7f89600cc790> -
     <_ast.Name object at 0x7f89600cca10> -
     <_ast.Load object at 0x7f89600bf5d0> -
>>> for node in ast.walk(ast.parse("print(f'Hello world')")): walk(node)
... 
     <_ast.Module object at 0x7f89600cc790> -
     <_ast.Expr object at 0x7f89600cc650> -
     <_ast.Call object at 0x7f89600ccc10> -
     <_ast.Name object at 0x7f89600cc990> -
     <_ast.JoinedStr object at 0x7f89600cc610> -
         <_ast.Str object at 0x7f89600cca50> Hello world
     <_ast.Load object at 0x7f89600bf5d0> -
     <_ast.Str object at 0x7f89600cca50> Hello world
>>> for node in ast.walk(ast.parse("print('Hello world')")): walk(node)
... 
     <_ast.Module object at 0x7f89600cc990> -
     <_ast.Expr object at 0x7f89600ccc10> -
     <_ast.Call object at 0x7f89600cc650> -
     <_ast.Name object at 0x7f89600cc790> -
     <_ast.Str object at 0x7f89600cc810> Hello world
     <_ast.Load object at 0x7f89600bf5d0> -

The f-string insertion points look like JoinedStr containing both Str and FormattedValue.

The false positives are from the format specifiers themselves (here d), which are a JoinedStr containing only Str - just like a pointless f-string using just a plain string literal (e.g. f'Hello world').

It looks like any format specifier would trigger the false positive:

>>> for node in ast.walk(ast.parse("print(f'Hello {client:formal}')")): walk(node)
... 
     <_ast.Module object at 0x7f89600cc650> -
     <_ast.Expr object at 0x7f89600ccc10> -
     <_ast.Call object at 0x7f89600cc990> -
     <_ast.Name object at 0x7f89600cca50> -
     <_ast.JoinedStr object at 0x7f89600cca10> -
         <_ast.Str object at 0x7f89600cc690> Hello 
         <_ast.FormattedValue object at 0x7f89600cc950> -
     <_ast.Load object at 0x7f89600bf5d0> -
     <_ast.Str object at 0x7f89600cc690> Hello 
     <_ast.FormattedValue object at 0x7f89600cc950> -
     <_ast.Name object at 0x7f89600ccb90> -
     <_ast.JoinedStr object at 0x7f89600cc710> -
         <_ast.Str object at 0x7f89600ccbd0> formal
     <_ast.Load object at 0x7f89600bf5d0> -
     <_ast.Str object at 0x7f89600ccbd0> formal

Indeed it does:

class Client:
    def __init__(self, id, polite, casual):
        self.polite = polite
        self.casual = casual
    def __format__(self, format_spec):
        if format_spec == "polite":
            return self.polite
        elif format_spec == "casual":
            return self.casual
        else:
            raise ValueError(f"{format_spec} is not a format defined by Client object")

customer = Client(123, "Peter", "Pete")

print(f"Hello {customer:polite}, may we call you {customer:casual}?")
print(f"Thanks!")  # pointless f-string

$ python /tmp/pie.py 
Hello Peter, may we call you Pete?
Thanks!

$ flake8 --select PIE /tmp/pie.py 
/tmp/pie.py:15:7: PIE782: Unnecessary f-string. You can safely remove the `f` prefix.
/tmp/pie.py:15:7: PIE782: Unnecessary f-string. You can safely remove the `f` prefix.
/tmp/pie.py:16:7: PIE782: Unnecessary f-string. You can safely remove the `f` prefix.

[peterjc]

I've spent a while looking into the AST tree, but failed to spot how to solve this.

Removing the PIE782 warning may be the pragmatic way forward :(

[sbdchd]

I've removed this rule in the latest version https://pypi.org/project/flake8-pie/0.5.0/

edit: feel free to let me know if there are any problems

[peterjc]

Slightly on a tangent, but I've started working on a plugin to flag the different Python string formatting styles:

https://github.com/peterjc/flake8-sfs
https://pypi.org/project/flake8-sfs/

I would ideally include pointless f-strings as a special case, and will try to let you know if I do manage to solve it one day (or some kind person contributes a solution).

[peterjc]

Just to note that pointless f-strings (like f"Hello") are detected by pyflakes 2.2.0 (released 2020-04-08) onwards, and available in flake8 as F541 f-string is missing placeholders.

This is also available in ruff https://docs.astral.sh/ruff/rules/f-string-missing-placeholders/