Make signature help arg index handling more like Strada #1586
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the signature help implementation to remove nullable argument index handling, making it more like "Strada" (likely referring to another TypeScript implementation or reference). The changes simplify the code by treating argument index as a non-nullable integer value instead of an optional pointer.
- Removes complex null-checking logic for argument indices
- Changes
argumentIndexfrom*inttointthroughout the codebase - Simplifies the argument index calculation and handling logic
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/ls/signaturehelp.go | Main refactoring removing nullable argument index logic and simplifying related functions |
| internal/ls/completions.go | Updates completion logic to work with non-nullable argument index |
| internal/fourslash/tests/signatureHelpCrash_test.go | Adds regression test for signature help crash scenario |
| return 0 | ||
| } | ||
| return *argumentCount | ||
| return getArgumentIndexOrCount(getTokenFromNodeList(arguments, node.Parent, sourceFile), nil, c) |
There was a problem hiding this comment.
After #1584 I'm pretty sure we could drop getTokenFromNodeList, but potentially not because you can't pass a NodeList into getChildrenFromNonJSDocNode... (Or I'm missing the obvious transform that makes getChildrenFromNonJSDocNode work)
There was a problem hiding this comment.
this turns out to be complicated, for another day
There was a problem hiding this comment.
Yeah, I think you'd have to make getChildrenFromNonJSDocNode take in an optional slice of nodes (instead of it obtaining one by node.ForEachChild() and also a range... Or you could just extract the part of getChildrenFromNonJSDocNode that collects nodes + tokens into a separate one and reuse that for node lists.
There was a problem hiding this comment.
That's a good idea, better than what I was trying.
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
Fixes #1497