Improve parser error recovery for invalid UnaryOp nodes inside unpacking assignments#24697
Draft
AlexWaygood wants to merge 1 commit intomainfrom
Draft
Improve parser error recovery for invalid UnaryOp nodes inside unpacking assignments#24697AlexWaygood wants to merge 1 commit intomainfrom
UnaryOp nodes inside unpacking assignments#24697AlexWaygood wants to merge 1 commit intomainfrom
Conversation
…lid` Root-cause fix. `helpers::set_expr_ctx` was propagating `Store` (or `Del`) through `UnaryOp`'s operand when fixing up the ctx on a parsed assignment target. But `UnaryOp` is never a valid assignment or deletion target in Python, so propagating the target ctx into its operand told a lie: an empty or otherwise-unrelated `Name` inside would then claim to be a binding site. That lie was the root of the LSP panic reported in astral-sh/ty#3283: semantic indexing trusted the ctx and created a phantom binding; the unpacker ignored `UnaryOp` in its `_ =>` arm and never recorded a type; `binding_type` on the phantom binding then hit `expression_type()` with no entry and panicked. Marking the operand `Invalid` aligns the two walkers: semantic indexing's existing `(Invalid, _) => (false, false)` arm skips binding creation, so there is no phantom binding for the unpacker to fail to cover. Parser snapshots for three already-failing invalid-syntax fixtures are updated to reflect the ctx change on the (rejected) recovery AST. Three LSP e2e tests — two existing and one new completion-triggering test for `a, not x = ...` — cover the regression.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root-cause fix.
helpers::set_expr_ctxwas propagatingStore(orDel)through
UnaryOp's operand when fixing up the ctx on a parsed assignmenttarget. But
UnaryOpis never a valid assignment or deletion target inPython, so propagating the target ctx into its operand told a lie: an
empty or otherwise-unrelated
Nameinside would then claim to be abinding site.
That lie was the root of the LSP panic reported in astral-sh/ty#3283:
semantic indexing trusted the ctx and created a phantom binding; the
unpacker ignored
UnaryOpin its_ =>arm and never recorded a type;binding_typeon the phantom binding then hitexpression_type()withno entry and panicked.
Marking the operand
Invalidaligns the two walkers: semantic indexing'sexisting
(Invalid, _) => (false, false)arm skips binding creation, sothere is no phantom binding for the unpacker to fail to cover.
Parser snapshots for three already-failing invalid-syntax fixtures are
updated to reflect the ctx change on the (rejected) recovery AST. Three
LSP e2e tests — two existing and one new completion-triggering test for
a, not x = ...— cover the regression.Summary
Test Plan