Fix signature help crash when nested call has trailing comma #2315
Conversation
… comma Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
DanielRosenwasser
left a comment
DanielRosenwasser
left a comment
There was a problem hiding this comment.
Try again @copilot.
| outer( | ||
| inner(/*1*/ | ||
| undefined, | ||
| undefined, | ||
| undefined, | ||
| undefined, | ||
| undefined, | ||
| ), | ||
| );` |
There was a problem hiding this comment.
You should use the more minimal example from the repro.
There was a problem hiding this comment.
I get that the example wasn't failing when you tried, but really, this is what I saw. The minimal repro was:
declare function outer<T>(range: T): T;
declare function inner(a: any): any;
outer(inner(undefined,),);
The LSP request was
{"method":"textDocument/signatureHelp","params":{"textDocument":{"uri":"file:///example.ts"},"position":{"line":3,"character":11},"context":{"triggerKind":1,"isRetrigger":false}}}The crash message for that example was
panic handling request textDocument/signatureHelp: runtime error: index out of range [1] with length 1
Make sure there is no TriggerCharacter, and also, this was an invoked trigger (so use SignatureHelpTriggerKindInvoked).
There was a problem hiding this comment.
Updated the test to use the minimal repro with the marker at inner/*1*/(undefined,) and SignatureHelpTriggerKindInvoked. The test now correctly reproduces the crash without the fix. (0d98c09)
internal/checker/checker.go
Outdated
| // Handle edge case where maxCount >= len(args) | ||
| // This can happen when signature resolution fails for reasons other than argument count, | ||
| // such as when trailing commas create OmittedExpressions that affect type inference. | ||
| if maxCount >= len(args) { | ||
| diagnostic := NewDiagnosticForNode(errorNode, message, parameterRange, len(args)) | ||
| if headMessage != nil { | ||
| diagnostic = ast.NewDiagnosticChain(diagnostic, headMessage) | ||
| } | ||
| return diagnostic | ||
| } |
There was a problem hiding this comment.
This feels wrong - the error is because there is a trailing comma. I don't think there is an omitted expression anywhere, so the arities seem right.
There was a problem hiding this comment.
Updated the comment to clarify that the issue is due to trailing commas causing signature resolution to fail, not OmittedExpressions. The bounds check guards against maxCount >= len(args) which occurs when we reach this fallback error path but the argument count actually matches the parameter count. (0d98c09)
…imal repro Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
getArgumentArityErrorwhenmaxCount >= len(args)args[maxCount]without bounds check in the default branchchecker.go- add bounds check before accessingargs[maxCount]Original prompt
This section details on the original issue you should resolve
<issue_title>Signature help crash on call target when nested call has trailing comma</issue_title>
<issue_description>Had a hard time coming up with a minimal repro but here's the actual code it was found in.
https://github.com/microsoft/TypeScript/blob/1da8266179589bbc977ccbd8712614ed5ddd3004/src/services/navigationBar.ts#L681-L690
If you request signature help at the marker very quickly, you can trigger the following crash. This is tricky because it doesn't always happen. One way to often accomplish this in VS Code is to
,and delete it)