From f280652feb1aae7de606840007a3295db59bdb36 Mon Sep 17 00:00:00 2001 From: Gabriela Araujo Britto Date: Thu, 14 Aug 2025 11:03:07 -0700 Subject: [PATCH 1/3] fix signature help's use of token reading --- internal/fourslash/fourslash.go | 73 ++++++++++++++++++- .../tests/signatureHelpTokenCrash_test.go | 44 +++++++++++ internal/ls/signaturehelp.go | 45 +++++++----- 3 files changed, 142 insertions(+), 20 deletions(-) create mode 100644 internal/fourslash/tests/signatureHelpTokenCrash_test.go diff --git a/internal/fourslash/fourslash.go b/internal/fourslash/fourslash.go index 06427dbbc4..ddd8b48d53 100644 --- a/internal/fourslash/fourslash.go +++ b/internal/fourslash/fourslash.go @@ -564,12 +564,11 @@ func (f *FourslashTest) verifyCompletionsWorker(t *testing.T, expected *Completi if !resultOk { t.Fatalf(prefix+"Unexpected response type for completion request: %T", resMsg.AsResponse().Result) } - f.verifyCompletionsResult(t, f.currentCaretPosition, result.List, expected, prefix) + f.verifyCompletionsResult(t, result.List, expected, prefix) } func (f *FourslashTest) verifyCompletionsResult( t *testing.T, - position lsproto.Position, actual *lsproto.CompletionList, expected *CompletionsExpectedList, prefix string, @@ -1400,3 +1399,73 @@ func (f *FourslashTest) VerifyQuickInfoIs(t *testing.T, expectedText string, exp } f.verifyHoverContent(t, hover.Contents, expectedText, expectedDocumentation, prefix) } + +type SignatureHelpCase struct { + Context *lsproto.SignatureHelpContext + MarkerInput MarkerInput + Expected *lsproto.SignatureHelp +} + +func (f *FourslashTest) VerifySignatureHelp(t *testing.T, signatureHelpCases ...*SignatureHelpCase) { + for _, option := range signatureHelpCases { + switch marker := option.MarkerInput.(type) { + case string: + f.GoToMarker(t, marker) + f.verifySignatureHelp(t, option.Context, option.Expected) + case *Marker: + f.goToMarker(t, marker) + f.verifySignatureHelp(t, option.Context, option.Expected) + case []string: + for _, markerName := range marker { + f.GoToMarker(t, markerName) + f.verifySignatureHelp(t, option.Context, option.Expected) + } + case []*Marker: + for _, marker := range marker { + f.goToMarker(t, marker) + f.verifySignatureHelp(t, option.Context, option.Expected) + } + case nil: + f.verifySignatureHelp(t, option.Context, option.Expected) + default: + t.Fatalf("Invalid marker input type: %T. Expected string, *Marker, []string, or []*Marker.", option.MarkerInput) + } + } +} + +func (f *FourslashTest) verifySignatureHelp( + t *testing.T, + context *lsproto.SignatureHelpContext, + expected *lsproto.SignatureHelp, +) { + var prefix string + if f.lastKnownMarkerName != nil { + prefix = fmt.Sprintf("At marker '%s': ", *f.lastKnownMarkerName) + } else { + prefix = fmt.Sprintf("At position (Ln %d, Col %d): ", f.currentCaretPosition.Line, f.currentCaretPosition.Character) + } + params := &lsproto.SignatureHelpParams{ + TextDocument: lsproto.TextDocumentIdentifier{ + Uri: ls.FileNameToDocumentURI(f.activeFilename), + }, + Position: f.currentCaretPosition, + Context: context, + } + resMsg, result, resultOk := sendRequest(t, f, lsproto.TextDocumentSignatureHelpInfo, params) + if resMsg == nil { + t.Fatalf(prefix+"Nil response received for signature help request", f.lastKnownMarkerName) + } + if !resultOk { + t.Fatalf(prefix+"Unexpected response type for signature help request: %T", resMsg.AsResponse().Result) + } + f.verifySignatureHelpResult(t, result.SignatureHelp, expected, prefix) +} + +func (f *FourslashTest) verifySignatureHelpResult( + t *testing.T, + actual *lsproto.SignatureHelp, + expected *lsproto.SignatureHelp, + prefix string, +) { + assertDeepEqual(t, actual, expected, prefix+" SignatureHelp mismatch") +} diff --git a/internal/fourslash/tests/signatureHelpTokenCrash_test.go b/internal/fourslash/tests/signatureHelpTokenCrash_test.go new file mode 100644 index 0000000000..84d1d98772 --- /dev/null +++ b/internal/fourslash/tests/signatureHelpTokenCrash_test.go @@ -0,0 +1,44 @@ +package fourslash_test + +import ( + "testing" + + "github.com/microsoft/typescript-go/internal/fourslash" + . "github.com/microsoft/typescript-go/internal/fourslash/tests/util" + "github.com/microsoft/typescript-go/internal/lsp/lsproto" + "github.com/microsoft/typescript-go/internal/testutil" +) + +func TestSignatureHelpTokenCrash(t *testing.T) { + t.Parallel() + defer testutil.RecoverAndFail(t, "Panic on fourslash test") + const content = ` +function foo(a: any, b: any) { + +} + +foo((/*1*/ + +/** This is a JSDoc comment */ +foo/** More comments*/((/*2*/ +` + f := fourslash.NewFourslash(t, nil /*capabilities*/, content) + f.VerifySignatureHelp(t, &fourslash.SignatureHelpCase{ + MarkerInput: "1", + Expected: nil, + Context: &lsproto.SignatureHelpContext{ + IsRetrigger: false, + TriggerCharacter: PtrTo("("), + TriggerKind: lsproto.SignatureHelpTriggerKindTriggerCharacter, + }, + }) + f.VerifySignatureHelp(t, &fourslash.SignatureHelpCase{ + MarkerInput: "2", + Expected: nil, + Context: &lsproto.SignatureHelpContext{ + IsRetrigger: false, + TriggerCharacter: PtrTo("("), + TriggerKind: lsproto.SignatureHelpTriggerKindTriggerCharacter, + }, + }) +} diff --git a/internal/ls/signaturehelp.go b/internal/ls/signaturehelp.go index 94984a9910..4c7ec93e66 100644 --- a/internal/ls/signaturehelp.go +++ b/internal/ls/signaturehelp.go @@ -597,19 +597,14 @@ func getCandidateOrTypeInfo(info *argumentListInfo, c *checker.Checker, sourceFi return nil // return Debug.assertNever(invocation); } -func isSyntacticOwner(startingToken *ast.Node, node *ast.Node, sourceFile *ast.SourceFile) bool { // !!! not tested +func isSyntacticOwner(startingToken *ast.Node, node *ast.CallLikeExpression, sourceFile *ast.SourceFile) bool { // !!! not tested if !ast.IsCallOrNewExpression(node) { return false } - invocationChildren := getTokensFromNode(node, sourceFile) + invocationChildren := getChildrenFromNode(node, sourceFile) switch startingToken.Kind { - case ast.KindOpenParenToken: - return containsNode(invocationChildren, startingToken) - case ast.KindCommaToken: + case ast.KindOpenParenToken, ast.KindCommaToken: return containsNode(invocationChildren, startingToken) - // !!! - // const containingList = findContainingList(startingToken); - // return !!containingList && contains(invocationChildren, containingList); case ast.KindLessThanToken: return containsPrecedingToken(startingToken, sourceFile, node.AsCallExpression().Expression) default: @@ -1079,20 +1074,34 @@ func countBinaryExpressionParameters(b *ast.BinaryExpression) int { return 2 } -func getTokensFromNode(node *ast.Node, sourceFile *ast.SourceFile) []*ast.Node { - if node == nil { - return nil - } +func getChildrenFromNode(node *ast.CallLikeExpression, sourceFile *ast.SourceFile) []*ast.Node { + var childNodes []*ast.Node + node.ForEachChild(func(child *ast.Node) bool { + childNodes = append(childNodes, child) + return false + }) var children []*ast.Node - current := node - left := node.Pos() - scanner := scanner.GetScannerForSourceFile(sourceFile, left) - for left < current.End() { + pos := node.Pos() + for _, child := range childNodes { + scanner := scanner.GetScannerForSourceFile(sourceFile, pos) + for pos < child.Pos() { + token := scanner.Token() + tokenFullStart := scanner.TokenFullStart() + tokenEnd := scanner.TokenEnd() + children = append(children, sourceFile.GetOrCreateToken(token, tokenFullStart, tokenEnd, node)) + pos = tokenEnd + scanner.Scan() + } + children = append(children, child) + pos = child.End() + } + scanner := scanner.GetScannerForSourceFile(sourceFile, pos) + for pos < node.End() { token := scanner.Token() tokenFullStart := scanner.TokenFullStart() tokenEnd := scanner.TokenEnd() - children = append(children, sourceFile.GetOrCreateToken(token, tokenFullStart, tokenEnd, current)) - left = tokenEnd + children = append(children, sourceFile.GetOrCreateToken(token, tokenFullStart, tokenEnd, node)) + pos = tokenEnd scanner.Scan() } return children From d65240eb66d26ef1c49126410cd62bdd7a4771c7 Mon Sep 17 00:00:00 2001 From: Gabriela Araujo Britto Date: Thu, 14 Aug 2025 11:47:31 -0700 Subject: [PATCH 2/3] refactor position prefix into a method in fourslash --- internal/fourslash/fourslash.go | 52 ++++++++++----------------------- 1 file changed, 15 insertions(+), 37 deletions(-) diff --git a/internal/fourslash/fourslash.go b/internal/fourslash/fourslash.go index ddd8b48d53..8b4526610f 100644 --- a/internal/fourslash/fourslash.go +++ b/internal/fourslash/fourslash.go @@ -544,12 +544,7 @@ func (f *FourslashTest) VerifyCompletions(t *testing.T, markerInput MarkerInput, } func (f *FourslashTest) verifyCompletionsWorker(t *testing.T, expected *CompletionsExpectedList) { - var prefix string - if f.lastKnownMarkerName != nil { - prefix = fmt.Sprintf("At marker '%s': ", *f.lastKnownMarkerName) - } else { - prefix = fmt.Sprintf("At position (Ln %d, Col %d): ", f.currentCaretPosition.Line, f.currentCaretPosition.Character) - } + prefix := f.getCurrentPositionPrefix() params := &lsproto.CompletionParams{ TextDocument: lsproto.TextDocumentIdentifier{ Uri: ls.FileNameToDocumentURI(f.activeFilename), @@ -931,17 +926,11 @@ func (f *FourslashTest) VerifyBaselineHover(t *testing.T) { } resMsg, result, resultOk := sendRequest(t, f, lsproto.TextDocumentHoverInfo, params) - var prefix string - if f.lastKnownMarkerName != nil { - prefix = fmt.Sprintf("At marker '%s': ", *f.lastKnownMarkerName) - } else { - prefix = fmt.Sprintf("At position (Ln %d, Col %d): ", f.currentCaretPosition.Line, f.currentCaretPosition.Character) - } if resMsg == nil { - t.Fatalf(prefix+"Nil response received for quick info request", f.lastKnownMarkerName) + t.Fatalf(f.getCurrentPositionPrefix()+"Nil response received for quick info request", f.lastKnownMarkerName) } if !resultOk { - t.Fatalf(prefix+"Unexpected response type for quick info request: %T", resMsg.AsResponse().Result) + t.Fatalf(f.getCurrentPositionPrefix()+"Unexpected response type for quick info request: %T", resMsg.AsResponse().Result) } return markerAndItem[*lsproto.Hover]{Marker: marker, Item: result.Hover}, true @@ -1024,17 +1013,11 @@ func (f *FourslashTest) VerifyBaselineSignatureHelp(t *testing.T) { } resMsg, result, resultOk := sendRequest(t, f, lsproto.TextDocumentSignatureHelpInfo, params) - var prefix string - if f.lastKnownMarkerName != nil { - prefix = fmt.Sprintf("At marker '%s': ", *f.lastKnownMarkerName) - } else { - prefix = fmt.Sprintf("At position (Ln %d, Col %d): ", f.currentCaretPosition.Line, f.currentCaretPosition.Character) - } if resMsg == nil { - t.Fatalf(prefix+"Nil response received for signature help request", f.lastKnownMarkerName) + t.Fatalf(f.getCurrentPositionPrefix()+"Nil response received for signature help request", f.lastKnownMarkerName) } if !resultOk { - t.Fatalf(prefix+"Unexpected response type for signature help request: %T", resMsg.AsResponse().Result) + t.Fatalf(f.getCurrentPositionPrefix()+"Unexpected response type for signature help request: %T", resMsg.AsResponse().Result) } return markerAndItem[*lsproto.SignatureHelp]{Marker: marker, Item: result.SignatureHelp}, true @@ -1318,8 +1301,7 @@ func (f *FourslashTest) getScriptInfo(fileName string) *scriptInfo { func (f *FourslashTest) VerifyQuickInfoAt(t *testing.T, marker string, expectedText string, expectedDocumentation string) { f.GoToMarker(t, marker) hover := f.getQuickInfoAtCurrentPosition(t) - - f.verifyHoverContent(t, hover.Contents, expectedText, expectedDocumentation, fmt.Sprintf("At marker '%s': ", marker)) + f.verifyHoverContent(t, hover.Contents, expectedText, expectedDocumentation, f.getCurrentPositionPrefix()) } func (f *FourslashTest) getQuickInfoAtCurrentPosition(t *testing.T) *lsproto.Hover { @@ -1391,13 +1373,7 @@ func (f *FourslashTest) quickInfoIsEmpty(t *testing.T) (bool, *lsproto.Hover) { func (f *FourslashTest) VerifyQuickInfoIs(t *testing.T, expectedText string, expectedDocumentation string) { hover := f.getQuickInfoAtCurrentPosition(t) - var prefix string - if f.lastKnownMarkerName != nil { - prefix = fmt.Sprintf("At marker '%s': ", *f.lastKnownMarkerName) - } else { - prefix = fmt.Sprintf("At position (Ln %d, Col %d): ", f.currentCaretPosition.Line, f.currentCaretPosition.Character) - } - f.verifyHoverContent(t, hover.Contents, expectedText, expectedDocumentation, prefix) + f.verifyHoverContent(t, hover.Contents, expectedText, expectedDocumentation, f.getCurrentPositionPrefix()) } type SignatureHelpCase struct { @@ -1438,12 +1414,7 @@ func (f *FourslashTest) verifySignatureHelp( context *lsproto.SignatureHelpContext, expected *lsproto.SignatureHelp, ) { - var prefix string - if f.lastKnownMarkerName != nil { - prefix = fmt.Sprintf("At marker '%s': ", *f.lastKnownMarkerName) - } else { - prefix = fmt.Sprintf("At position (Ln %d, Col %d): ", f.currentCaretPosition.Line, f.currentCaretPosition.Character) - } + prefix := f.getCurrentPositionPrefix() params := &lsproto.SignatureHelpParams{ TextDocument: lsproto.TextDocumentIdentifier{ Uri: ls.FileNameToDocumentURI(f.activeFilename), @@ -1469,3 +1440,10 @@ func (f *FourslashTest) verifySignatureHelpResult( ) { assertDeepEqual(t, actual, expected, prefix+" SignatureHelp mismatch") } + +func (f *FourslashTest) getCurrentPositionPrefix() string { + if f.lastKnownMarkerName != nil { + return fmt.Sprintf("At marker '%s': ", *f.lastKnownMarkerName) + } + return fmt.Sprintf("At position (Ln %d, Col %d): ", f.currentCaretPosition.Line, f.currentCaretPosition.Character) +} From 4d032d392ef994010b0e2528227fbba405f6f22b Mon Sep 17 00:00:00 2001 From: Gabriela Araujo Britto Date: Thu, 14 Aug 2025 13:49:24 -0700 Subject: [PATCH 3/3] rename and move getChildren function --- internal/ls/signaturehelp.go | 35 +---------------------------------- internal/ls/utilities.go | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 34 deletions(-) diff --git a/internal/ls/signaturehelp.go b/internal/ls/signaturehelp.go index 4c7ec93e66..e7ff15b03b 100644 --- a/internal/ls/signaturehelp.go +++ b/internal/ls/signaturehelp.go @@ -601,7 +601,7 @@ func isSyntacticOwner(startingToken *ast.Node, node *ast.CallLikeExpression, sou if !ast.IsCallOrNewExpression(node) { return false } - invocationChildren := getChildrenFromNode(node, sourceFile) + invocationChildren := getChildrenFromNonJSDocNode(node, sourceFile) switch startingToken.Kind { case ast.KindOpenParenToken, ast.KindCommaToken: return containsNode(invocationChildren, startingToken) @@ -1074,39 +1074,6 @@ func countBinaryExpressionParameters(b *ast.BinaryExpression) int { return 2 } -func getChildrenFromNode(node *ast.CallLikeExpression, sourceFile *ast.SourceFile) []*ast.Node { - var childNodes []*ast.Node - node.ForEachChild(func(child *ast.Node) bool { - childNodes = append(childNodes, child) - return false - }) - var children []*ast.Node - pos := node.Pos() - for _, child := range childNodes { - scanner := scanner.GetScannerForSourceFile(sourceFile, pos) - for pos < child.Pos() { - token := scanner.Token() - tokenFullStart := scanner.TokenFullStart() - tokenEnd := scanner.TokenEnd() - children = append(children, sourceFile.GetOrCreateToken(token, tokenFullStart, tokenEnd, node)) - pos = tokenEnd - scanner.Scan() - } - children = append(children, child) - pos = child.End() - } - scanner := scanner.GetScannerForSourceFile(sourceFile, pos) - for pos < node.End() { - token := scanner.Token() - tokenFullStart := scanner.TokenFullStart() - tokenEnd := scanner.TokenEnd() - children = append(children, sourceFile.GetOrCreateToken(token, tokenFullStart, tokenEnd, node)) - pos = tokenEnd - scanner.Scan() - } - return children -} - func getTokenFromNodeList(nodeList *ast.NodeList, nodeListParent *ast.Node, sourceFile *ast.SourceFile) []*ast.Node { if nodeList == nil || nodeListParent == nil { return nil diff --git a/internal/ls/utilities.go b/internal/ls/utilities.go index ab58122018..aa8a9f0be6 100644 --- a/internal/ls/utilities.go +++ b/internal/ls/utilities.go @@ -1627,3 +1627,37 @@ func getLeadingCommentRangesOfNode(node *ast.Node, file *ast.SourceFile) iter.Se } return scanner.GetLeadingCommentRanges(&ast.NodeFactory{}, file.Text(), node.Pos()) } + +// Equivalent to Strada's `node.getChildren()` for non-JSDoc nodes. +func getChildrenFromNonJSDocNode(node *ast.Node, sourceFile *ast.SourceFile) []*ast.Node { + var childNodes []*ast.Node + node.ForEachChild(func(child *ast.Node) bool { + childNodes = append(childNodes, child) + return false + }) + var children []*ast.Node + pos := node.Pos() + for _, child := range childNodes { + scanner := scanner.GetScannerForSourceFile(sourceFile, pos) + for pos < child.Pos() { + token := scanner.Token() + tokenFullStart := scanner.TokenFullStart() + tokenEnd := scanner.TokenEnd() + children = append(children, sourceFile.GetOrCreateToken(token, tokenFullStart, tokenEnd, node)) + pos = tokenEnd + scanner.Scan() + } + children = append(children, child) + pos = child.End() + } + scanner := scanner.GetScannerForSourceFile(sourceFile, pos) + for pos < node.End() { + token := scanner.Token() + tokenFullStart := scanner.TokenFullStart() + tokenEnd := scanner.TokenEnd() + children = append(children, sourceFile.GetOrCreateToken(token, tokenFullStart, tokenEnd, node)) + pos = tokenEnd + scanner.Scan() + } + return children +}