[flake8-bugbear] Implement quoted-fstring-value (B907)

[alexcdennis]

Summary

Implements rule B907 quoted-fstring-value.

Tracking Issue: #3758

From the bugbear documentation

B907: 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.

Changes from original

The original rule in bugbear was marked as optional/opinionated (PyCQA/flake8-bugbear#333) because there were several complaints about limitations (PyCQA/flake8-bugbear#329, PyCQA/flake8-bugbear#331, PyCQA/flake8-bugbear#332, #1893 (comment)).

The cause of all these limitations was that the suggested fix (to always add !r) was too opinionated and could changed the semantics of the output.

As such, this implementation diverges from the original by changing the framing in the documentation and error messages to remove all reference to the problematic suggested fix. Instead, it opts for a more nuanced approach which requires the user to consider their own fix on a case-by-case basis, while also providing an opt out (the user may add a noop to the code).

Test Plan

cargo nextest run

Sources

Implementation inspired by original bugbear implementation
Test cases include all test cases from the original bugbear test file, plus extras

Related/References

ruff

#1893
#2954 (comment)

bugbear

PyCQA/flake8-bugbear#319

[charliermarsh]

It seems like the check in bugbear suffered from a lot of false positives (and hence was moved to the opinionated section). Do you know how many of those are present in the implementation here? What are the limitations of the rule?

[alexcdennis]

This rule has all of the same limitations as the original, since it is the same algorithm.

All of the limitations that I have seen suggested fall under the same issue: this rule may suggests a fix (adding !r) that changes the meaning of the output.
(1) If the value is not a string, adding !r changes the output (PyCQA/flake8-bugbear#329, #1893 (comment))
(2) If the value is a string where there is a different meaning between double quotes and single quotes, then !r can change this meaning (eg an SQL query PyCQA/flake8-bugbear#331) (since !r considers double and single quotes to be interchangeable and will substitute one for the other at unexpected times)

Under the current framing (that adding !r is the correct fix), neither of these limitations can be overcome.
(1) Detecting whether or not the value is a string requires type inference
(2) This is no static rule that can understand the intent of the programmer for the arbitrary semantics of the string output

However, the real cause of both of these issues is the original framing that the proposed fix (adding !r) is correct in all situations. Thus, in order to overcome these limitations, we need to change this framing by removing that proposed fix altogether. (It is not clear to me that there is a fix that is correct in all situations)

For this reason, I originally tried to word the message as a suggestion for the user to consider if this is a problem on a case-by-case basis instead of as something that the user must fix. I think this still requires more rewording in order to make this clearer.

edit: rewritten in order to hopefully improve clarity

[alexcdennis]

I have changed the framing around the rule by changing the suggested fix. I believe that this overcomes all previously suggested limitations, as discussed above.

Summary:
Instead of the original suggested fix of "always add !r", the new suggested fix is "consider on a case-by-case basis; if you want to signal that this is intended behavior, add a noop"

Open questions:
Is the new documentation/error message too wordy/nuanced/confusing?
If so, is there a way to reword it to make it clearer?