Fix signature help ranges to prevent display after closing parentheses

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

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}},
+	})
+}