diff --git a/internal/ls/signaturehelp.go b/internal/ls/signaturehelp.go index 8ad8567e21..60ec9ae124 100644 --- a/internal/ls/signaturehelp.go +++ b/internal/ls/signaturehelp.go @@ -93,11 +93,12 @@ func (l *LanguageService) GetSignatureHelpItems( candidateInfo := getCandidateOrTypeInfo(argumentInfo, typeChecker, sourceFile, startingToken, onlyUseSyntacticOwners) // cancellationToken.throwIfCancellationRequested(); - // if (!candidateInfo) { !!! - // // We didn't have any sig help items produced by the TS compiler. If this is a JS - // // file, then see if we can figure out anything better. - // return isSourceFileJS(sourceFile) ? createJSSignatureHelpItems(argumentInfo, program, cancellationToken) : undefined; - // } + if candidateInfo == nil { + // We didn't have any sig help items produced by the TS compiler. If this is a JS + // file, then see if we can figure out anything better. + // return isSourceFileJS(sourceFile) ? createJSSignatureHelpItems(argumentInfo, program, cancellationToken) : undefined; + return nil + } // return typeChecker.runWithCancellationToken(cancellationToken, typeChecker => if candidateInfo.candidateInfo != nil { @@ -513,7 +514,9 @@ type CandidateOrTypeInfo struct { func getCandidateOrTypeInfo(info *argumentListInfo, c *checker.Checker, sourceFile *ast.SourceFile, startingToken *ast.Node, onlyUseSyntacticOwners bool) *CandidateOrTypeInfo { if info.invocation.callInvocation != nil { - if onlyUseSyntacticOwners && !isSyntacticOwner(startingToken, info.invocation.callInvocation.node, sourceFile) { + // Apply syntactic owner check when explicitly requested, or for critical cases like after closing tokens + shouldCheckSyntacticOwner := onlyUseSyntacticOwners || startingToken.Kind == ast.KindCloseParenToken + if shouldCheckSyntacticOwner && !isSyntacticOwner(startingToken, info.invocation.callInvocation.node, sourceFile) { return nil } resolvedSignature, candidates := checker.GetResolvedSignatureForSignatureHelp(info.invocation.callInvocation.node, info.argumentCount, c) @@ -562,19 +565,23 @@ func isSyntacticOwner(startingToken *ast.Node, node *ast.Node, sourceFile *ast.S if !ast.IsCallOrNewExpression(node) { return false } - invocationChildren := getTokensFromNode(node, sourceFile) + invocationChildren := getASTChildren(node) switch startingToken.Kind { case ast.KindOpenParenToken: return containsNode(invocationChildren, startingToken) case ast.KindCommaToken: - return containsNode(invocationChildren, startingToken) - // !!! - // const containingList = findContainingList(startingToken); - // return !!containingList && contains(invocationChildren, containingList); + containingList := findContainingList(startingToken, sourceFile) + return containingList != nil && containsNodeList(invocationChildren, containingList) case ast.KindLessThanToken: return containsPrecedingToken(startingToken, sourceFile, node.AsCallExpression().Expression) + case ast.KindCloseParenToken: + // If we're at a close paren token, only provide signature help if we're inside the token, + // not after it. This prevents signature help from showing after the function call ends. + return containsNode(invocationChildren, startingToken) default: - return false + // For other token types (like string literals, identifiers, etc.), check if they are + // contained within the invocation node (i.e., they are arguments or part of arguments) + return containsNode(invocationChildren, startingToken) } } @@ -1040,6 +1047,18 @@ func countBinaryExpressionParameters(b *ast.BinaryExpression) int { return 2 } +func getASTChildren(node *ast.Node) []*ast.Node { + if node == nil { + return nil + } + children := []*ast.Node{} + node.VisitEachChild(ast.NewNodeVisitor(func(n *ast.Node) *ast.Node { + children = append(children, n) + return n + }, nil, ast.NodeVisitorHooks{})) + return children +} + func getTokensFromNode(node *ast.Node, sourceFile *ast.SourceFile) []*ast.Node { if node == nil { return nil @@ -1092,6 +1111,19 @@ func containsNode(nodes []*ast.Node, node *ast.Node) bool { return false } +func containsNodeList(nodes []*ast.Node, nodeList *ast.NodeList) bool { + if nodeList == nil { + return false + } + // Check if any of the nodes has the same range as the NodeList + for i := range nodes { + if nodes[i].Pos() == nodeList.Pos() && nodes[i].End() == nodeList.End() { + return true + } + } + return false +} + func getArgumentListInfoForTemplate(tagExpression *ast.TaggedTemplateExpression, argumentIndex *int, sourceFile *ast.SourceFile) *argumentListInfo { // argumentCount is either 1 or (numSpans + 1) to account for the template strings array argument. argumentCount := 1 diff --git a/internal/ls/signaturehelp_test.go b/internal/ls/signaturehelp_test.go index 36be7655de..c726971a02 100644 --- a/internal/ls/signaturehelp_test.go +++ b/internal/ls/signaturehelp_test.go @@ -1040,12 +1040,16 @@ func runSignatureHelpTest(t *testing.T, input string, expected map[string]verify }, } preferences := &ls.UserPreferences{} + for markerName, expectedResult := range expected { marker, ok := markerPositions[markerName] if !ok { t.Fatalf("No marker found for '%s'", markerName) } result := languageService.ProvideSignatureHelp(ctx, ls.FileNameToDocumentURI(file), marker.LSPosition, context, capabilities, preferences) + if result == nil { + t.Fatalf("Expected signature help for marker '%s' but got nil", markerName) + } assert.Equal(t, expectedResult.text, result.Signatures[*result.ActiveSignature].Label) assert.Equal(t, expectedResult.parameterCount, len(*result.Signatures[*result.ActiveSignature].Parameters)) assert.DeepEqual(t, expectedResult.activeParameter, result.ActiveParameter) @@ -1055,3 +1059,110 @@ func runSignatureHelpTest(t *testing.T, input string, expected map[string]verify } } } + +func runSignatureHelpTestWithNil(t *testing.T, input string, expected map[string]verifySignatureHelpOptions, expectedNil []string) { + testData := fourslash.ParseTestData(t, input, "/mainFile.ts") + file := testData.Files[0].FileName() + markerPositions := testData.MarkerPositions + ctx := projecttestutil.WithRequestID(t.Context()) + languageService, done := createLanguageService(ctx, file, map[string]string{ + file: testData.Files[0].Content, + }) + defer done() + context := &lsproto.SignatureHelpContext{ + TriggerKind: lsproto.SignatureHelpTriggerKindInvoked, + TriggerCharacter: nil, + } + ptrTrue := ptrTo(true) + capabilities := &lsproto.SignatureHelpClientCapabilities{ + SignatureInformation: &lsproto.ClientSignatureInformationOptions{ + ActiveParameterSupport: ptrTrue, + NoActiveParameterSupport: ptrTrue, + ParameterInformation: &lsproto.ClientSignatureParameterInformationOptions{ + LabelOffsetSupport: ptrTrue, + }, + }, + } + preferences := &ls.UserPreferences{} + + // Check expected non-nil results + for markerName, expectedResult := range expected { + marker, ok := markerPositions[markerName] + if !ok { + t.Fatalf("No marker found for '%s'", markerName) + } + result := languageService.ProvideSignatureHelp(ctx, ls.FileNameToDocumentURI(file), marker.LSPosition, context, capabilities, preferences) + if result == nil { + t.Fatalf("Expected signature help for marker '%s' but got nil", markerName) + } + assert.Equal(t, expectedResult.text, result.Signatures[*result.ActiveSignature].Label) + assert.Equal(t, expectedResult.parameterCount, len(*result.Signatures[*result.ActiveSignature].Parameters)) + assert.DeepEqual(t, expectedResult.activeParameter, result.ActiveParameter) + // Checking the parameter span that will be highlighted in the editor + if expectedResult.activeParameter != nil && int(expectedResult.activeParameter.Value) < expectedResult.parameterCount { + assert.Equal(t, expectedResult.parameterSpan, *(*result.Signatures[*result.ActiveSignature].Parameters)[int(result.ActiveParameter.Value)].Label.String) + } + } + + // Check expected nil results + for _, markerName := range expectedNil { + marker, ok := markerPositions[markerName] + if !ok { + t.Fatalf("No marker found for '%s'", markerName) + } + result := languageService.ProvideSignatureHelp(ctx, ls.FileNameToDocumentURI(file), marker.LSPosition, context, capabilities, preferences) + if result != nil { + t.Fatalf("Expected no signature help for marker '%s' but got: %s", markerName, result.Signatures[*result.ActiveSignature].Label) + } + } +} + +func TestSignatureHelpRangeIssue(t *testing.T) { + t.Parallel() + if !bundled.Embedded { + t.Skip("bundled files are not embedded") + } + + input := `let obj = { + foo(s: string): string { + return s; + } +}; + +let s = obj.foo("Hello, world!"/*insideCall*/)/*afterCall*/;` + + runSignatureHelpTestWithNil(t, input, + map[string]verifySignatureHelpOptions{ + "insideCall": {text: "foo(s: string): string", parameterCount: 1, parameterSpan: "s: string", activeParameter: &lsproto.Nullable[uint32]{Value: 0}}, + }, + []string{"afterCall"}) +} + +func TestSignatureHelpNestedCallPrecedence(t *testing.T) { + t.Parallel() + if !bundled.Embedded { + t.Skip("bundled files are not embedded") + } + + input := `function foo(x: any) { + return x; +} + +function bar(x: any) { + return x; +} + +foo(/*outerA*/ /*outerB*/bar/*callTarget*/(/*innerParam*/)/*outerC*/ /*outerD*/)` + + runSignatureHelpTest(t, input, map[string]verifySignatureHelpOptions{ + // Outer call should take precedence for these positions + "outerA": {text: "foo(x: any): any", parameterCount: 1, parameterSpan: "x: any", activeParameter: &lsproto.Nullable[uint32]{Value: 0}}, + "outerB": {text: "foo(x: any): any", parameterCount: 1, parameterSpan: "x: any", activeParameter: &lsproto.Nullable[uint32]{Value: 0}}, + "callTarget": {text: "foo(x: any): any", parameterCount: 1, parameterSpan: "x: any", activeParameter: &lsproto.Nullable[uint32]{Value: 0}}, + "outerC": {text: "foo(x: any): any", parameterCount: 1, parameterSpan: "x: any", activeParameter: &lsproto.Nullable[uint32]{Value: 0}}, + "outerD": {text: "foo(x: any): any", parameterCount: 1, parameterSpan: "x: any", activeParameter: &lsproto.Nullable[uint32]{Value: 0}}, + + // Inner call should only take precedence for this position + "innerParam": {text: "bar(x: any): any", parameterCount: 1, parameterSpan: "x: any", activeParameter: &lsproto.Nullable[uint32]{Value: 0}}, + }) +}