new rule - fstring syntax in non-fstring

[DetachHead]

asdf = 1
"value is {asdf}" # the user probably meant to make this an f string

to avoid false positives, it should only complain when the expression inside the {} refers to an existing variable. that way, most usages of the format function won't trigger false positives:

x = "{foo}" # no error because `foo` variable does not exist
x.format(foo="bar")

[dhruvmanila]

Interesting! Although not sure how to reliably detect this. One way could be to pass the string as a f-string to the parser and check if there are any ExprFormattedValue node in it.

[charliermarsh]

I'm worried about false positives on this one.

[DetachHead]

i feel like they would be rare as long as it only complains when the expression inside the {} refers to an existing variable

[MichaReiser]

I would find this useful. Coming from Rust and JavaScript, I wrote "{value}" too many times to then notice my debug print statements are completely useless. So I went back and added the f in front of my strings.

[dhruvmanila]

I'm worried about false positives on this one.

Can you give an example where there would be false positives? Maybe we could only highlight when the node within {...} is an identifier and skip everything else.

[zanieb]

My common use false positive

x = "{foo}"
x.format(foo="bar")

[DetachHead]

@zanieb in that case it shouldn't trigger a false positive if the rule works how i suggested in #8151 (comment), because there's no variable in scope called foo.

imo the format method should only be used instead of f-strings in cases where the variables being formatted are not in scope. i've updated the OP to clarify this

[zanieb]

I agree requiring the variables to be in scope is a reasonable heuristic for avoiding false positives. I'm not sure I can come up with a common one that meets that requirement.

I found https://github.com/PrefectHQ/prefect/blob/f654a1b23eb5662afb5b8bfd08c9c86668e817ee/tests/agent/test_agent_queue_selection.py#L36 but that probably ought to be an f-string anyway. We can probably hint something else when format is being called instead of using a f-string.

[dhruvmanila]

We could just skip if the said string is part of a large .format call or % operator.

I think then we could support this as:

1. Skip if this string contains { / }
2. Skip if it's part of .format call or % operator
3. Parse the string with the f prefix
4. Visit each ExprFormattedValue node. If there are none, return
5. Create a diagnostic message only if all of the expression inside the formatted node is
   a. An identifier
   b. It's defined in the scope

What do you think? @zanieb

[peterjc]

I'd welcome this. Cross reference peterjc/flake8-sfs#5

See also the opposite request, f-strings without any variables in them - briefly implemented as flake8-pie's PIE782 code but withdrawn due to false positives. Cross reference peterjc/flake8-sfs#3 and sbdchd/flake8-pie#24

[dhruvmanila]

See also the opposite request, f-strings without any variables in them

We do support this - https://docs.astral.sh/ruff/rules/f-string-missing-placeholders/ :)

[hauntsaninja]

pyanalyze supports this check: https://github.com/quora/pyanalyze/blob/647c64e8c9b4eaa81a5309d8e76e72bafe61d687/pyanalyze/name_check_visitor.py#L3096

Here are the heuristics it uses to avoid false positives:

• At least one name is used in the expression and all names used inside the expression are bound
• The string literal is not immediately .format()-ed
• It's not standalone expression (i.e. doesn't look like a docstring)
• It's not in a call that passes kwargs corresponding to all names (to avoid false positives with interpolation functions that work like .format)

[charliermarsh]

I'm supportive of adding this with the above heuristics.