Get node's children with forEachChild + scanner in signature help
#1584
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in signature help functionality where tokens were being assigned incorrect parent nodes, causing crashes during signature help requests. The issue occurred because the original getTokensFromNode function was incorrectly assuming that all tokens within a node's range had that node as their parent.
Key changes:
- Replaced
getTokensFromNodewithgetChildrenFromNodethat properly usesForEachChildto traverse actual child nodes - Updated the token creation logic to scan between child nodes and assign correct parent relationships
- Added fourslash test infrastructure for signature help verification to prevent regressions
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/ls/signaturehelp.go | Core fix: replaced token scanning logic with proper child node traversal and correct parent assignment |
| internal/fourslash/tests/signatureHelpTokenCrash_test.go | New test case reproducing the crash scenario with nested parentheses in function calls |
| internal/fourslash/fourslash.go | Added VerifySignatureHelp infrastructure and refactored position prefix generation for better test support |
| token := scanner.Token() | ||
| tokenFullStart := scanner.TokenFullStart() | ||
| tokenEnd := scanner.TokenEnd() | ||
| children = append(children, sourceFile.GetOrCreateToken(token, tokenFullStart, tokenEnd, node)) | ||
| pos = tokenEnd | ||
| scanner.Scan() | ||
| } |
There was a problem hiding this comment.
This code seems to never be run; do we just not have an examples of trailing tokens at the moment? (The code seems correct, I think anyway)
There was a problem hiding this comment.
Yeah, I think it's just not tested right now, especially since it's only signature help using it and I don't know how good the signature help tests are in terms of coverage (even in Strada).
2 participants
Fixes #1298.
The issue had two different call stacks, but it turned out to be the same bug that was showing up in two different places.
I had to add a very minimal version of
fourslash.VerifySignatureHelpto be able to write a fourslash test for this.Context:
node.getChildren()has been replaced in Corsa by code that visits child nodes and uses the scanner to obtain child tokens. In this new setup, we usesourceFile.GetOrCreateTokento get a token object if one has been created before, or creating a new token object and caching it. That's done like that because we want the invariant that if you callsourceFile.GetOrCreateTokenwith the same token information (i.e. same kind, position, etc), you get the same Go object and comparing tokens by pointer equality will work out. A consequence of this is that you have to provide the same parent when getting or creating a token, i.e. the parent the token would have in Strada.getTokensFromNodein signature help was not respecting this: it was using the scanner to scan all tokens in the range of a given node, and assuming the given node would be the token's parent. That's wrong because a token in the given node's range could have e.g. a child of that node as its parent. For instance, if you have the call expression nodefoo(()), then the first(token has the call expression as parent, but the second(has as parent the parenthesized expression().