Skip to content

Fix signature help ranges to prevent display after closing parentheses #1420

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Fix signature help ranges to prevent display after closing parentheses #1420

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0d375f2
Initial plan
Copilot Jul 18, 2025
65fec80
Fix signature help range issue - prevent showing help after closing p…
Copilot Jul 18, 2025
a76f028
Add test cases for signature help range and nested call precedence is…
Copilot Jul 18, 2025
8991cdb
Implement proper isSyntacticOwner fix for signature help ranges
Copilot Jul 21, 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
15 changes: 15 additions & 0 deletions internal/ls/signaturehelp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,13 @@
return nil
}

// Check if position is actually within the applicable range
// Fix for issue where signature help is shown after closing parenthesis
argRange := argumentInfo.argumentsRange
if position < argRange.Pos() || position > argRange.End() {
return nil
}
Copy link
Member

Choose a reason for hiding this comment

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

I think the real fix is to implement/port the commented out code:

		// !!!
		// const containingList = findContainingList(startingToken);
		// return !!containingList && contains(invocationChildren, containingList);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've implemented the commented-out code as suggested. The fix includes:

  1. Proper findContainingList logic for comma tokens with containsNodeList helper function
  2. Extended isSyntacticOwner to handle CloseParenToken cases to prevent signature help after closing parentheses
  3. Added getASTChildren helper to work with AST structure instead of just tokens
  4. Applied syntactic owner checks for critical cases like CloseParenToken even when onlyUseSyntacticOwners is false

The range-based fix has been replaced with this more comprehensive TypeScript-compatible solution in commit 8991cdb.


// cancellationToken.throwIfCancellationRequested();

// Extra syntactic and semantic filtering of signature help
Expand Down Expand Up @@ -602,6 +609,14 @@
// If the node is not a subspan of its parent, this is a big problem.
// There have been crashes that might be caused by this violation.
// Debug.assert(rangeContainsRange(n.parent, n), "Not a subspan", () => `Child: ${Debug.formatSyntaxKind(n.kind)}, parent: ${Debug.formatSyntaxKind(n.parent.kind)}`);

// Special case: if we're at a close paren token and the position is after its end,
// don't provide signature help. This handles the case where the cursor is right
// after the closing parenthesis
if n.Kind == ast.KindCloseParenToken && position >= n.End() {
continue
}

argumentInfo := getImmediatelyContainingArgumentOrContextualParameterInfo(n, position, sourceFile, checker)
if argumentInfo != nil {
return argumentInfo
Expand Down Expand Up @@ -1080,7 +1095,7 @@
left = tokenEnd
}
}
return tokens

Check failure on line 1098 in internal/ls/signaturehelp.go

View workflow job for this annotation

GitHub Actions / copilot 

nodeList.AsNode undefined (type *ast.NodeList has no field or method AsNode)
}

func containsNode(nodes []*ast.Node, node *ast.Node) bool {
Expand Down
111 changes: 111 additions & 0 deletions internal/ls/signaturehelp_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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}},
})
}