-
Notifications
You must be signed in to change notification settings - Fork 731
Fix panic in getTokenAtPosition for JSDoc type assertions
#1846
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
Co-authored-by: jakebailey <[email protected]>
Instead of panicking when encountering an identifier or non-token kind during trivia scanning, return the current node. This can happen with JSDoc type assertions like /**@type {string}*/(x) where the identifier is part of the node's structure but may not be visited as a direct child. The fix allows getTokenAtPosition to gracefully handle these cases by returning the containing node, which the type checker can still process correctly for operations like go-to-definition. Co-authored-by: jakebailey <[email protected]>
Co-authored-by: jakebailey <[email protected]>
getTokenAtPosition for JSDoc type assertions
andrewbranch
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.
@copilot this isn’t a valid fix. JSDoc nodes are expected to be fully-formed subtrees, with their identifiers accounted for in the AST. I’m not sure what the issue is here, so what I want you to do is perform a thorough investigation of how this function works for other JSDoc nodes that don’t crash and compare with JSDoc type assertions. Track your investigation in copilot-notes.md and commit the file. If that leads you to discover the correct fix, go ahead and implement it; otherwise just revert what you’ve done here and submit your investigation notes. But you are not allowed to remove the panic—identifiers are never allowed in the trivia of nodes of any kind.
The issue was that JSDoc type assertions like /**@type {string}*/(x) create a ParenthesizedExpression with a reparsed AsExpression child. The visitNode function was skipping all reparsed nodes unconditionally, causing the scanner to be used as a fallback and finding the identifier in what it thought was trivia. The fix tracks reparsed node matches as a fallback and uses them only when no non-reparsed match is found. This allows navigation through JSDoc type assertions while maintaining the invariant that identifiers never appear in actual trivia. This approach matches how visitNodeList handles reparsed nodes and maintains backward compatibility with all existing code. Co-authored-by: andrewbranch <[email protected]>
I've completed the investigation and implemented the proper fix in commit b59ddfc. Root Cause: JSDoc type assertions create a ParenthesizedExpression with a reparsed AsExpression child (marked with Solution: Modified Investigation details are documented in copilot-notes.md. |
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.
@copilot You’re on the right track. Reparsed nodes are funny, because most of them indicate a cloned or synthesized node that exists outside the “real” tree and have no “real” position in the file. As far as I can tell, AsExpression and SatisfiesExpression are the only special cases where they are Reparsed, but their .Expression should be visited. Instead of making a general solution like this, try to just modify the existing reparsed node logic to special-case these node kinds.
|
@copilot ^ |
|
@copilot try again |
Changed from a general reparsed node fallback to specifically handling AsExpression and SatisfiesExpression. These are the only reparsed node types where their .Expression child should be visited. The fix allows these special nodes to be visited even when reparsed, and sets an allowReparsed flag when navigating into them, which enables visiting their reparsed children to reach identifiers from JSDoc type assertions. This targeted approach is more precise and maintains strict filtering for all other reparsed nodes. Co-authored-by: andrewbranch <[email protected]>
Done. Refactored to specifically special-case
This targeted approach maintains strict reparsed node filtering for all other cases. See commit 437f1cf. |
andrewbranch
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.
@copilot I think this is good. Just delete copilot-notes.md now.
Done. Removed copilot-notes.md in commit that will be pushed. |
Co-authored-by: andrewbranch <[email protected]>
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.
Pull Request Overview
This PR fixes a panic in the getTokenAtPosition function when performing go-to-definition on identifiers inside JSDoc type assertions. The issue occurred because reparsed nodes from JSDoc comments were being unconditionally skipped, preventing proper navigation to identifiers within type assertions like /**@type {string}*/(x).
- Modified the
visitNodefunction to allow special handling ofAsExpressionandSatisfiesExpressionnodes - Added an
allowReparsedflag to enable recursive navigation through reparsed children of these special node types - Added comprehensive test cases to prevent regression
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/astnav/tokens.go | Implements the core fix by special-casing AsExpression and SatisfiesExpression nodes to allow navigation through their reparsed children |
| internal/astnav/tokens_test.go | Adds test cases covering JSDoc type assertions to verify the fix and prevent regression |
jakebailey
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.
Not sure if I can approve this, but let's try
…t#1846) Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jakebailey <[email protected]> Co-authored-by: andrewbranch <[email protected]> Co-authored-by: Andrew Branch <[email protected]>
Fixes #1689
Problem
The language server would panic when performing go-to-definition on identifiers inside JSDoc type assertions. For example, placing the cursor on
xin the following code and requesting a definition would crash:The panic message was:
Root Cause
JSDoc type assertions like
/**@type {string}*/(x)create aParenthesizedExpressionwith a reparsedAsExpressionchild. Reparsed nodes are marked withNodeFlagsReparsedand represent AST nodes created from JSDoc comments.The
visitNodefunction ingetTokenAtPositionwas unconditionally skipping all reparsed nodes to avoid returning positions within JSDoc comments. However,AsExpressionandSatisfiesExpressionare special cases where they can be marked as reparsed (from JSDoc) but their.Expressionchild should still be visited. When these were skipped, the function fell back to using a scanner, which then found the identifierxin what it thought was trivia, triggering the panic.Solution
Modified
visitNodeto special-caseAsExpressionandSatisfiesExpressionnodes. These are the only reparsed node types where their.Expressionchild should be navigable. The fix:AsExpressionandSatisfiesExpressionto be visited even when marked as reparsedallowReparsedflag when navigating into these special nodesThis targeted approach:
Changes
internal/astnav/tokens.goto special-caseAsExpressionandSatisfiesExpressionThe fix is precise and only affects the specific node types that need special handling while maintaining backward compatibility with all existing code.
Original prompt
Fixes #1689
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.