Fix f-string middle panic when parsing t-strings#23232
Conversation
|
Summary -- This PR fixes #23198 by copying Dylan's changes from #19183 to the other `InterpolatedStringElementsKind` variant, in line with Micha's prophetic comment (https://github.com/astral-sh/ruff/pull/19183/changes#r2217325217). Test Plan -- Two new tests derived from the issue
ef64124 to
ba00d52
Compare
|
Not really sure what happened with the benchmarks, but it cleared up after rebasing on main. There are some ~2% regressions but also some improvements, so I think it's pretty neutral overall, as I would expect. |
truly a red flag that I casually asserted Micha was wrong. we should add a CI check for that. |
dylwil3
left a comment
There was a problem hiding this comment.
This looks great to me! I'm glad it wasn't too painful.
| const F_STRING_ELEMENTS_IN_FORMAT_SPEC = 1 << 27; | ||
| const T_STRING_ELEMENTS_IN_FORMAT_SPEC = 1 << 31; |
There was a problem hiding this comment.
weird that we skipped 27 (did I do that??)
should we just renumber them sequentially or does that break something?
There was a problem hiding this comment.
It was weird, but it wasn't you! It was like that before your PR. I'm assuming one of them got removed at some point?
I vaguely remember trying to reorder a different set of bitflags previously and someone suggested leaving them alone, but I'm happy to do it if you prefer. Maybe @dhruvmanila will have an opinion too.
There was a problem hiding this comment.
I think it's fine to change the values, I'm assuming that we should avoid doing that mainly to make the reviewing easier but I guess you could do it before merging. Anyways, this doesn't look like a big change 🤷♂️
|
It wasn't painful at all! You had already done the hard work for me 😄 |
|
Thanks so much for this. |
Summary
This PR fixes #23198 by copying Dylan's changes from #19183 to the other
InterpolatedStringElementsKindvariant, in line with Micha's prophetic comment (https://github.com/astral-sh/ruff/pull/19183/changes#r2217325217).Test Plan
Two new tests derived from the issue