[pyupgrade] Fix Unicode escape sequence handling in format string parsing

[danparizher]

Summary

Fixes #19771

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[ntBre]

Thanks for looking into this!

I'm a bit wary of changing this parsing code, but I had a few nits if we do go this route. We'd probably also want @MichaReiser to take a look here.

This code isn't super widely used, only occurring in ~3 rules or so, so it's not as foundational as I thought initially, but we should still be careful.

[ntBre]

I don't think we need to collect here:

Suggested change

	let chars: Vec<char> = cur_text.chars().collect();
	for (i, &c) in chars.iter().enumerate().skip(3) {
	for (i, c) in cur_text.chars().iter().enumerate().skip(3) {

[ntBre]

I don't think we need to count the braces, but we need to return some kind of error if we encounter another open brace, based on a quick test in Python. Maybe there's a way to make this work, but it seems to me that braces aren't allowed inside of a \N{...} escape:

>>> "\N{{angle}}".format(angle="angle")
  File "<python-input-0>", line 1
    "\N{{angle}}".format(angle="angle")
    ^^^^^^^^^^^^^
SyntaxError: (unicode error) 'unicodeescape' codec can't decode bytes in position 0-9: unknown Unicode character name

[ntBre]

If we push_str("\\N") at the start, we can just push individual characters as we go instead of bumping end_pos.

[ntBre]

Ah yeah, this is an error in CPython, so this is the case I think we should return an Err in.

>>> "\N{LATIN {SMALL} LETTER A}"
  File "<python-input-2>", line 1
    "\N{LATIN {SMALL} LETTER A}"
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
SyntaxError: (unicode error) 'unicodeescape' codec can't decode bytes in position 0-15: unknown Unicode character name

[MichaReiser]

I don't think I'll have time anytime soon to review this in detail and I'm also not familiar with this code or the logic around it. @ntBre you're right to be wary. There have been plenty of bugs around f-string handling, https://github.com/astral-sh/ruff/issues?q=sort%3Aupdated-desc%20is%3Aissue%20state%3Aclosed%20UP032

[MichaReiser]

These seems guaranteed by the starts_with call on line 597`

[MichaReiser]

It feels a bit silly to update result_string only to undo the changes right after. Can we avoid that?

[MichaReiser]

Isn't chars.as_str always guaranteed to be empty if chars.next() is None?

[IDrokin117]

@danparizher
I've been looking into the same problem while working on the issue #19792
Please examine the potential problems this PR could cause

1. Consider the example below. ruff 0.12.8 doesn't fix it due to missing format keyword snowman (playground). However, with the proposed changes, the format will be replaced by f-string. I suppose it is wrong behavior.

# ruff 0.12.8 
print("\\N{snowman} {}".format(1)) # doesn't apply UP032

# With the fix, UP032 will be applied
print("\\N{snowman} {}".format(1)) # -> print(f"\\N{snowman} {1}")

2. While the fix will resolve the first (1) case, it still has a problem with the second (2) case.

# ruff 0.12.8 
print("\N{snowman} {snowman}".format(snowman=3)) # -> print(f"\N{snowman} {3}") # (1)

# With the fix, here first {snowman} must also be substituted with {1}.
print("\\N{snowman} {snowman}".format(snowman=1)) # -> print(f"\\N{snowman} {1}") # (2)

Here you can find my solution that correctly addresses the cases mentioned above. Feel free to reuse it in your PR if it makes sense.

[MichaReiser]

Closing due to inactivity. I can re-open this PR if you plan to work on this. If anyone else is interested to work in this, feel free to submit a new PR that takes the above feedback into account.