Skip to content

Fix signature help crash when nested call has trailing comma #2315

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.

Sign up for GitHub

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

Merged
DanielRosenwasser merged 3 commits into from
Dec 11, 2025
Merged

Fix signature help crash when nested call has trailing comma #2315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c62d5e2
Initial plan
Copilot Dec 10, 2025
a453037
Fix signature help crash on call target when nested call has trailing…
Copilot Dec 10, 2025
0d98c09
Fix signature help crash with trailing comma and update test with min…
Copilot Dec 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions internal/checker/checker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9648,6 +9648,17 @@ func (c *Checker) getArgumentArityError(node *ast.Node, signatures []*Signature,
}
return diagnostic
default:
// Guard against out-of-bounds access when maxCount >= len(args).
// This can happen when we reach this fallback error path but the argument
// count actually matches the parameter count (e.g., due to trailing commas
// causing signature resolution to fail for other reasons).
if maxCount >= len(args) {
diagnostic := NewDiagnosticForNode(errorNode, message, parameterRange, len(args))
if headMessage != nil {
diagnostic = ast.NewDiagnosticChain(diagnostic, headMessage)
}
return diagnostic
}
Copy link
Member

@DanielRosenwasser DanielRosenwasser Dec 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent a lot of time being convinced this was wrong, but this is actually an acceptable fix - in signature help, we resolve the signature with a higher argument count if there's a trailing comma.

In Strada, we used to slice instead of indexing by maxCount.

const errorSpan = factory.createNodeArray(args.slice(max));
const pos = first(errorSpan).pos;
let end = last(errorSpan).end;

This never failed because you can never have a trailing comma on zero elements - you'll always have an omitted expression. (This isn't true! See below)

We could go back to the same approach here.

It felt weird that we even try to report errors - but I think signature help does rely on actually resolving the signature in the error path. We have a call to

	c.isSignatureApplicable(s.node, s.args, last, c.assignableRelation, CheckModeNormal, true /*reportErrors*/, nil /*inferenceContext*/, &diags)

where the result is just unchecked, so I assume that's just for side-effects.

So I couldn't just adjust reportErrors like so:

- 	reportErrors := !c.isInferencePartiallyBlocked && candidatesOutArray == nil
+ 	reportErrors := !c.isInferencePartiallyBlocked && candidatesOutArray == nil || checkMode&CheckModeIsForSignatureHelp != 0

because tests would start failing.

Copy link
Member

@DanielRosenwasser DanielRosenwasser Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think Strada has the same bug!

microsoft/TypeScript#55732

Copy link
Member

@jakebailey jakebailey Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens to the diag? Does it just keep accumulating them?

Copy link
Member

@DanielRosenwasser DanielRosenwasser Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, just like every other similar operation in the LS.

Copy link
Member

@DanielRosenwasser DanielRosenwasser Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(and keep in mind, it was always doing that - it just doesn't crash now in this one case)

sourceFile := ast.GetSourceFileOfNode(node)
pos := scanner.SkipTrivia(sourceFile.Text(), args[maxCount].Pos())
end := args[len(args)-1].End()
Expand Down
28 changes: 28 additions & 0 deletions internal/fourslash/tests/signatureHelpNestedCallTrailingComma_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package fourslash_test

import (
"testing"

"github.com/microsoft/typescript-go/internal/fourslash"
"github.com/microsoft/typescript-go/internal/lsp/lsproto"
"github.com/microsoft/typescript-go/internal/testutil"
)

func TestSignatureHelpNestedCallTrailingComma(t *testing.T) {
t.Parallel()
defer testutil.RecoverAndFail(t, "Panic on fourslash test")
// Regression test for crash when requesting signature help on a call target
// where the nested call has a trailing comma.
// Both outer and inner calls must have trailing commas, and outer must be generic.
const content = `declare function outer<T>(range: T): T;
declare function inner(a: any): any;
outer(inner/*1*/(undefined,),);`
Copy link
Member

@DanielRosenwasser DanielRosenwasser Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Also, worth noting that this crash happens at several locations in the identifier inner in the test case)

f, done := fourslash.NewFourslash(t, nil /*capabilities*/, content)
defer done()
f.GoToMarker(t, "1")
f.VerifySignatureHelpPresent(t, &lsproto.SignatureHelpContext{
IsRetrigger: false,
TriggerKind: lsproto.SignatureHelpTriggerKindInvoked,
})
}