Preserve quotes in generated f-strings

[ntBre]

Summary

This is another follow-up to #15726 and #15778, extending the quote-preserving behavior to f-strings and deleting the now-unused Generator::quote field.

Details

I also made one unrelated change to rules/flynt/helpers.rs to remove a to_string call for making a Box<str> and tweaked some arguments to some of the Generator::unparse_f_string methods to make the code easier to follow, in my opinion. Happy to revert especially the latter of these if needed.

Unfortunately this still does not fix the issue in #9660, which appears to be more of an escaping issue than a quote-preservation issue. After #15726, the result is now a = f'# {"".join([])}' if 1 else "" instead of a = f"# {''.join([])}" if 1 else "" (single quotes on the outside now), but we still don't have the desired behavior of double quotes everywhere on Python 3.12+. I added a test for this but split it off into another branch since it ended up being unaddressed here, but my dbg! statements showed the correct preferred quotes going into UnicodeEscape::with_preferred_quote.

Test Plan

Existing rule and Generator tests.

[github-actions]

ruff-ecosystem results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+5 -5 violations, +0 -0 fixes in 2 projects; 53 projects unchanged)

bokeh/bokeh (+4 -4 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --no-preview --select ALL

+ examples/server/api/flask_embed.py:26:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f"{new}D").mean()` instead of `if`-`else`-block
- examples/server/api/flask_embed.py:26:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f'{new}D').mean()` instead of `if`-`else`-block
+ examples/server/api/flask_gunicorn_embed.py:41:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f"{new}D").mean()` instead of `if`-`else`-block
- examples/server/api/flask_gunicorn_embed.py:41:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f'{new}D').mean()` instead of `if`-`else`-block
+ examples/server/api/standalone_embed.py:18:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f"{new}D").mean()` instead of `if`-`else`-block
- examples/server/api/standalone_embed.py:18:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f'{new}D').mean()` instead of `if`-`else`-block
+ examples/server/api/tornado_embed.py:29:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f"{new}D").mean()` instead of `if`-`else`-block
- examples/server/api/tornado_embed.py:29:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f'{new}D').mean()` instead of `if`-`else`-block

zulip/zulip (+1 -1 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --no-preview --select ALL

+ scripts/lib/sharding.py:65:21: SIM108 Use ternary operator `host = shard if "." in shard else f"{shard}.{external_host}"` instead of `if`-`else`-block
- scripts/lib/sharding.py:65:21: SIM108 Use ternary operator `host = shard if "." in shard else f'{shard}.{external_host}'` instead of `if`-`else`-block

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
SIM108	10	5	5	0	0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+5 -5 violations, +0 -0 fixes in 2 projects; 53 projects unchanged)

bokeh/bokeh (+4 -4 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ examples/server/api/flask_embed.py:26:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f"{new}D").mean()` instead of `if`-`else`-block
- examples/server/api/flask_embed.py:26:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f'{new}D').mean()` instead of `if`-`else`-block
+ examples/server/api/flask_gunicorn_embed.py:41:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f"{new}D").mean()` instead of `if`-`else`-block
- examples/server/api/flask_gunicorn_embed.py:41:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f'{new}D').mean()` instead of `if`-`else`-block
+ examples/server/api/standalone_embed.py:18:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f"{new}D").mean()` instead of `if`-`else`-block
- examples/server/api/standalone_embed.py:18:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f'{new}D').mean()` instead of `if`-`else`-block
+ examples/server/api/tornado_embed.py:29:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f"{new}D").mean()` instead of `if`-`else`-block
- examples/server/api/tornado_embed.py:29:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f'{new}D').mean()` instead of `if`-`else`-block

zulip/zulip (+1 -1 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ scripts/lib/sharding.py:65:21: SIM108 Use ternary operator `host = shard if "." in shard else f"{shard}.{external_host}"` instead of `if`-`else`-block
- scripts/lib/sharding.py:65:21: SIM108 Use ternary operator `host = shard if "." in shard else f'{shard}.{external_host}'` instead of `if`-`else`-block

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
SIM108	10	5	5	0	0

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

[AlexWaygood]

Nice! Would it be possible to add some new lint rule test fixtures that show a behaviour change from this PR? Maybe a new snippet or two for one of the flynt rules?

[ntBre]

Maybe a new snippet or two for one of the flynt rules?

This is trickier than I expected. For both of these rules, I just used Checker::default_fstring_flags instead of passing them along from an existing string. For the flynt rule that's reasonable since it's creating an f-string where one didn't exist before, but the RUF030 rule should actually preserve them. I think I need to restructure the rule code a bit to make this possible, though.

[dhruvmanila]

but we still don't have the desired behavior of double quotes everywhere on Python 3.12+. I added a test for this but split it off into another branch since it ended up being unaddressed here, but my dbg! statements showed the correct preferred quotes going into UnicodeEscape::with_preferred_quote.

We should make sure that this doesn't introduce any incompatibility with the f-string style in the formatter (I think it shouldn't) as the formatter prefers alternating quote style instead of using the same quotes in Python 3.12+ (https://docs.astral.sh/ruff/formatter/#f-string-formatting).

[dhruvmanila]

Unfortunately this still does not fix the issue in #9660, which appears to be more of an escaping issue than a quote-preservation issue.

Sorry, I think I missed this issue before but I was wondering whether that should actually be considered a bug or not because the formatter prefers using alternating quote style. I think this should not be a bug because if the issue author is running the formatter after the linter, then the quotes will be changed back which might seem confusing.

[AlexWaygood]

Hmm, @dhruvmanila I still would consider #9660 a bug I think. I agree that if a lint autofix is generating a new f-string completely from scratch, it should prefer to use alternating quotes like the formatter does. But that's not what is happening in #9660 -- in #9660, it's replacing an existing f-string with a new f-string, but the new f-string has a different quoting style to the old f-string. That seems like an unnecessary stylistic change for the autofix to make, considering that the user might not even have any autoformatter enabled on their code. If they're using the Ruff formatter, then the issue doesn't arise in the first place, because their existing f-string would probably already have alternating quotes. (Or, if it's an f-string that they've just added and haven't yet formatted, they can run the formatter immediately after they run the linter in order to fix it up.)

Basically, my opinion is that where possible lint autofixes should aim to have no opinion at all on quoting styles, and should leave that to the formatter. Instead, they should as much as possible try to preserve quoting styles when doing autofixes.

[AlexWaygood]

This is trickier than I expected. For both of these rules, I just used Checker::default_fstring_flags instead of passing them along from an existing string. For the flynt rule that's reasonable since it's creating an f-string where one didn't exist before, but the RUF030 rule should actually preserve them. I think I need to restructure the rule code a bit to make this possible, though.

From the ecosystem report in #15794 (comment), it looks like there's some changes to SIM108 suggested autofixes -- you could maybe add some f-string snippets to the SIM108 fixtures, if the flynt rules are hard to add tests for?

[ntBre]

@AlexWaygood Thanks for the SIM108 suggestion, I added two cases for that: one with double quotes and one with single quotes. I was looking for rules that explicitly interacted with f-strings, but I see now that just about any rule that uses the Generator and can apply to an expression with f-strings would have worked, which is exciting on its own!

@dhruvmanila What do you think about Alex's comments? I tend to agree with preserving whatever the user had and then letting the formatter clean it up afterward. We still have some time to decide either way, though. I was going to work on preserving prefixes and triple quotes before taking a stab at that issue anyway.

[AlexWaygood]

🎉