-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Fix unreachable panic in parser
#19183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
|
@MichaReiser okay after a more thorough investigation I have a better understanding of what is going on here and have gone with a different fix. I've tried to summarize my reasoning in the updated PR summary. Let me know whether you agree, or whether you'd like me to solve the issue in a different way. |
MichaReiser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Thank you
* main: (25 commits) [ty] Sync vendored typeshed stubs (#19461) [ty] Extend tuple `__len__` and `__bool__` special casing to also cover tuple subclasses (#19289) [ty] bump docstring-adder pin (#19458) [ty] Disallow assignment to `Final` class attributes (#19457) Update dependency ruff to v0.12.4 (#19442) Update pre-commit hook astral-sh/ruff-pre-commit to v0.12.4 (#19443) Update rui314/setup-mold digest to 702b190 (#19441) Update taiki-e/install-action action to v2.56.19 (#19448) Update Rust crate strum_macros to v0.27.2 (#19447) Update Rust crate strum to v0.27.2 (#19446) Update Rust crate rand to v0.9.2 (#19444) Update Rust crate serde_json to v1.0.141 (#19445) Fix `unreachable` panic in parser (#19183) [`ruff`] Support byte strings (`RUF055`) (#18926) [ty] Avoid second lookup for `infer_maybe_standalone_expression` (#19439) [ty] Implemented "go to definition" support for import statements (#19428) [ty] Avoid secondary tree traversal to get call expression for keyword arguments (#19429) [ty] Add goto definition to playground (#19425) [ty] Add support for `@warnings.deprecated` (#19376) [ty] make `del x` force local resolution of `x` in the current scope (#19389) ...
Parsing the (invalid) expression
f"{\t"i}"caused a panic because theTStringMiddlecharacter was "unreachable" due the way the parser recovered from the line continuation (it ate the t-string start).The cause of the issue is as follows:
The parser begins parsing the f-string and expects to see a list of objects, essentially alternating between interpolated elements and ordinary strings. It is happy to see the first left brace, but then there is a lexical error caused by the line-continuation character. So instead of the parser seeing a list of elements with just one member, it sees a list that starts like this:
NameTStringStartandTStringMiddleWhen it sees the
TStringStarterror recovery says "that's a list element I don't know what to do with, let's skip it". When it seesTStringMiddleit says "oh, that looks like the middle of some interpolated string so let's try to parse it as one of the literal elements of myFString". Unfortunately, the function being used to parse individual list elements thinks (arguably correctly) that it's not possible to have aTStringMiddlesitting in yourFString, and hitsunreachable.Two potential ways (among many) to solve this issue are:
TStringMiddleas a valid "literal" part of an f-string during parsing (with the hope/understanding that this would only occur in an invalid context)TStringMiddleas an "unexpected/invalid list item" in the same way that we skippedTStringStart.I have opted for the second approach since it seems somehow more morally correct, even though it loses more information. To implement this, the recovery context needs to know whether we are in an f-string or t-string - hence the changes to that enum. As a bonus we get slightly more specific error messages in some cases.
Closes #18860