Signature help implementation #861

navya9singh · 2025-05-12T21:09:45Z

This pr covers mostly everything that is needed for signature help except for signature help in .js files. Some tests that I haven't yet ported from Strada are tests for:

Tagged templates
Verify no signature help
Filtered triggers
Tests with JSDoc comments and other comments. It needs some changes in the test setup

into signatureHelpImplementation

Copilot

Pull Request Overview

This PR implements support for signature help in the language server and makes extensive naming refactors throughout the checker and utility modules. Key changes include adding a new handler for signature help in the server, updating configuration for the SignatureHelpProvider, and renaming many internal API functions (e.g. getSignaturesOfType → GetSignaturesOfType) to improve consistency and clarity.

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
internal/lsp/server.go	Adds new case handling for SignatureHelpParams and configures the signature help provider.
internal/ls/utilities.go	Updates comments and refactors type argument info functions without altering behavior.
internal/checker/utilities.go	Renames internal helper functions (e.g. CanHaveSymbol, IsCallOrNewExpression) for consistency.
internal/checker/types.go	Adds accessor methods on Signature and TupleType for better API exposure.
internal/checker/services.go	Consistently replaces lower-case API functions with their exported versions.
internal/checker/relater.go	Refactors signature comparison and tuple handling to use new exported functions.
internal/checker/printer.go	Adjusts printer logic to use the new exported APIs and updates variadic parameter expansion.
internal/checker/jsx.go	Updates JSX-related checks to use renamed functions.
internal/checker/inference.go	Updates inference utilities with new function names.
internal/checker/flow.go	Refactors flow analysis to call updated API names.
internal/checker/checker.go	Widespread updates to use capitalized exported API functions for type retrievals and checks.
internal/astnav/tokens.go	Adds a new helper function, HasQuestionToken, for token analysis.
internal/ast/utilities.go	Adds helpers for handling template literal tokens and string text nodes.
_submodules/TypeScript	Updates the submodule commit to a new revision.

_submodules/TypeScript

navya9singh · 2025-05-12T21:15:14Z

internal/checker/utilities.go

@@ -2073,3 +2084,211 @@ func (c *Checker) checkNotCanceled() {
 		panic("Checker was previously cancelled")
 	}
 }
+


These are some functions I added temporarily to make Signature help work. They will be removed once the node builder pr is merged. Also, these do not give the best result for generics, and returns object instead. This is why some of the tests have a comments //returns object because this part returns object.

into signatureHelpImplementation

navya9singh · 2025-05-12T22:28:03Z

internal/ls/signaturehelp.go

+	// Converting signatureHelpParameter to *lsproto.ParameterInformation
+	var parameters []*lsproto.ParameterInformation


Some of these functions are converting from local types to lsproto type because they needed some extra parameters like isRest, isOptional or isVariadic to be able to correctly calculate the parameter count and the parameter index that would be highlighted.

This slice can be allocated with the correct length/capacity

internal/ls/utilities.go

into signatureHelpImplementation

jakebailey · 2025-05-15T22:33:39Z

internal/astnav/tokens.go

@@ -602,3 +602,13 @@ func getNodeVisitor(
 		},
 	})
 }
+
+// !!!
+func HasQuestionToken(node *ast.Node) bool {


Should this go into the ast package?

jakebailey · 2025-05-15T22:36:08Z

internal/checker/checker.go

@@ -17392,7 +17392,7 @@ func (c *Checker) getTypeOfPropertyOfType(t *Type, name string) *Type {
 	return nil
 }

-func (c *Checker) getSignaturesOfType(t *Type, kind SignatureKind) []*Signature {
+func (c *Checker) GetSignaturesOfType(t *Type, kind SignatureKind) []*Signature {


Don't rename this method; just declare GetSignaturesOfType in exports.go and then forward to this unexported method.

I think the same should go for all of the other unexported functions; we're trying to consolidate these into a clearer API and also avoid huge diffs due to renames (which this PR does at the moment).

into signatureHelpImplementation

DanielRosenwasser

Mostly stylistic feedback. I haven't fully reviewed but I haven't seen anything glaringly different from the original so far!

internal/checker/checker.go

DanielRosenwasser · 2025-05-16T23:33:43Z

internal/checker/services.go

+	ancestorNode := ast.FindAncestor(node, func(n *ast.Node) bool {
+		return ast.IsCallLikeOrFunctionLikeExpression(n)
+	})


Suggested change

ancestorNode := ast.FindAncestor(node, func(n *ast.Node) bool {

return ast.IsCallLikeOrFunctionLikeExpression(n)

})

ancestorNode := ast.FindAncestor(node, ast.IsCallLikeOrFunctionLikeExpression)

DanielRosenwasser · 2025-05-16T23:42:34Z

internal/checker/services.go

+		return ast.IsCallLikeOrFunctionLikeExpression(n)
+	})
+	if ancestorNode != nil {
+		cachedResolvedSignatures := make(map[*SignatureLinks]*Signature)


I might just do two slices here instead of a map, but this might be cleaner.

Either way, I would just add a comment like

Suggested change

cachedResolvedSignatures := make(map[*SignatureLinks]*Signature)

// We're going to temporarily reset resolved signatures for all surrounding calls.

cachedResolvedSignatures := make(map[*SignatureLinks]*Signature)

DanielRosenwasser · 2025-05-16T23:43:01Z

internal/checker/services.go

+		result := fn()
+		for signatureLinks, resolvedSignature := range cachedResolvedSignatures {


Suggested change

result := fn()

for signatureLinks, resolvedSignature := range cachedResolvedSignatures {

result := fn()

// Once our work is over, we'll restore any signatures we previously resolved.

for signatureLinks, resolvedSignature := range cachedResolvedSignatures {

DanielRosenwasser · 2025-05-16T23:44:24Z

internal/ls/utilities.go

+	// if tokenIn != nil {
+	// 	searchPosition = scanner.GetTokenPosOfNode(tokenIn, sourceFile, false /*includeJSDoc*/)
+	// }
+	if strings.LastIndex(sourceFile.Text(), "<") == -1 {


Suggested change

if strings.LastIndex(sourceFile.Text(), "<") == -1 {

if strings.LastIndexByte(sourceFile.Text(), '<') == -1 {

DanielRosenwasser · 2025-05-16T23:48:21Z

internal/ls/utilities.go

+	// This function determines if the node could be type argument position
+	// Since during editing, when type argument list is not complete,
+	// the tree could be of any shape depending on the tokens parsed before current node,
+	// scanning of the previous identifier followed by "<" before current node would give us better result
+	// Note that we also balance out the already provided type arguments, arrays, object literals while doing so


Suggested change

// This function determines if the node could be type argument position

// Since during editing, when type argument list is not complete,

// the tree could be of any shape depending on the tokens parsed before current node,

// scanning of the previous identifier followed by "<" before current node would give us better result

// Note that we also balance out the already provided type arguments, arrays, object literals while doing so

// This function determines if the node could be a type argument position

// When editing, it is common to have an incomplete type argument list (e.g. missing ">"),

// so the tree can have any shape depending on the tokens before the current node.

// Instead, scanning for an identifier followed by a "<" before current node

// will typically give us better results than inspecting the tree.

// Note that we also balance out the already provided type arguments, arrays, object literals while doing so.

DanielRosenwasser · 2025-05-16T23:51:01Z

internal/ls/utilities.go

+			if token == nil || !ast.IsIdentifier(token) {
+				return nil
+			}
+			if remainingLessThanTokens <= 0 {


No need to check less-than, right?

Yeah, it can instead be changed to check if remainingLessThanTokens == 0 (in Strada it had (!remainingLessThanTokens)) because this part is trying to check if we have found the start of the type argument.

internal/ls/utilities.go

DanielRosenwasser · 2025-05-17T00:02:05Z

internal/ls/utilities.go

+	}
+	tokenKind := token.Kind
+	remainingMatchingTokens := 0
+	for true {


I think the test of the codebases uses

Suggested change

for true {

for {

gabritto

Some minor comments, but in general I think this is looking good.

internal/ast/utilities.go

internal/checker/checker.go

internal/checker/exports.go

internal/checker/inference.go

internal/checker/checker.go

gabritto · 2025-05-17T00:22:39Z

internal/ls/utilities.go

-}
+	// This is a rare case, but one that saves on a _lot_ of work if true - if the source file has _no_ `<` character,
+	// then there obviously can't be any type arguments - no expensive brace-matching backwards scanning required
+	// searchPosition := len(sourceFile.Text())


Is this missing a // !!!, or are we getting rid of this code?

Yes, getting rid of it.

gabritto · 2025-05-17T00:32:44Z

internal/ls/signaturehelp.go

+	}
+
+	// Only need to be careful if the user typed a character and signature help wasn't showing.
+	onlyUseSyntacticOwners := context.TriggerKind == 2


Suggested change

onlyUseSyntacticOwners := context.TriggerKind == 2

onlyUseSyntacticOwners := context.TriggerKind == lsproto.SignatureHelpTriggerKindTriggerCharacter

internal/ls/utilities.go

gabritto · 2025-05-17T00:37:07Z

internal/ls/signaturehelp.go

+	printer := printer.NewPrinter(printer.PrinterOptions{NewLine: core.NewLineKindLF}, printer.PrintHandlers{}, nil)
+
+	var getTypeParameters []signatureHelpParameter = []signatureHelpParameter{}
+	if candidateSignature.TypeParameters() != nil && len(candidateSignature.TypeParameters()) != 0 {


This here is what I mentioned elsewhere that could become just len(candidateSignature.TypeParameters()) != 0.

internal/ls/signaturehelp.go

into signatureHelpImplementation

internal/ast/utilities.go

andrewbranch

This is looking pretty good overall. Beyond the comments I left, I suggest you look at every declaration of a slice and ask these questions:

is this a displayParts slice that should just be a string or a strings.Builder?
if not, can I allocate it with the correct capacity up front?

andrewbranch · 2025-05-20T20:24:24Z

internal/checker/checker.go

+			result := [][]*ast.Symbol{}
+			for _, t := range restType.AsUnionType().Types() {
+				result = append(result, c.expandSignatureParametersWithTupleMembers(signature, t.AsTypeReference(), restIndex, restSymbol))


You can allocate this array with the right size:

Suggested change

result := [][]*ast.Symbol{}

for _, t := range restType.AsUnionType().Types() {

result = append(result, c.expandSignatureParametersWithTupleMembers(signature, t.AsTypeReference(), restIndex, restSymbol))

types := restType.AsUnionType().Types()

result := make([][]*ast.Symbol, len(types))

for i, t := range types {

result[i] = c.expandSignatureParametersWithTupleMembers(signature, t.AsTypeReference(), restIndex, restSymbol)

andrewbranch · 2025-05-20T20:25:20Z

internal/checker/printer.go

@@ -585,7 +585,7 @@ func (p *Printer) printSignature(sig *Signature, returnSeparator string) {
 		p.printType(p.c.getTypeOfSymbol(sig.thisParameter))
 		tail = true
 	}
-	expandedParameters := p.c.GetExpandedParameters(sig)
+	expandedParameters := p.c.getExpandedParameters(sig, true)[0]


Suggested change

expandedParameters := p.c.getExpandedParameters(sig, true)[0]

expandedParameters := p.c.getExpandedParameters(sig, true /*skipUnionExpanding*/)[0]

andrewbranch · 2025-05-20T20:26:16Z

internal/checker/services.go

+func (c *Checker) getResolvedSignatureWorker(node *ast.Node, candidatesOutArray *[]*Signature, checkMode CheckMode, argumentCount int) *Signature {
+	parsedNode := printer.NewEmitContext().ParseNode(node)
+	c.apparentArgumentCount = &argumentCount
+	var res *Signature = nil


Suggested change

var res *Signature = nil

var res *Signature

andrewbranch · 2025-05-20T20:28:51Z

internal/checker/services.go

+
+func (c *Checker) getResolvedSignatureWorker(node *ast.Node, candidatesOutArray *[]*Signature, checkMode CheckMode, argumentCount int) *Signature {
+	parsedNode := printer.NewEmitContext().ParseNode(node)
+	c.apparentArgumentCount = &argumentCount


You forgot to unset this at the end of the function.

andrewbranch · 2025-05-20T20:49:23Z

internal/ls/signaturehelp.go

+	// in the UI but can be omitted.
+	Documentation *string
+	// The Parameters of this signature.
+	Parameters *[]signatureHelpParameter


Don't use a pointer to a slice if you can avoid it. You range over this without checking for nil, so it seems like you can avoid it.

andrewbranch · 2025-05-20T20:53:06Z

internal/ls/signaturehelp.go

+	signatureInformation := []*lsproto.SignatureInformation{}
+	signatureInformation = append(signatureInformation, &lsproto.SignatureInformation{
+		Label:         item.Label,
+		Documentation: nil,
+		Parameters:    &parameters,
+	})


This can be a single initialization

andrewbranch · 2025-05-20T21:00:49Z

internal/ls/signaturehelp.go

+	}
+
+	// Creating display label
+	typeParameterDisplayParts := []string{}


I would use a strings.Builder here

andrewbranch · 2025-05-20T21:29:34Z

internal/ls/signaturehelp.go

+	callTargetDisplayParts := []string{}
+	if callTargetSymbol != nil {
+		callTargetDisplayParts = append(callTargetDisplayParts, c.SymbolToString(callTargetSymbol))
+	}
+	var items [][]signatureInformation
+	for _, candidateSignature := range *candidates {
+		items = append(items, getSignatureHelpItem(candidateSignature, argumentInfo.isTypeParameterList, callTargetDisplayParts, enclosingDeclaration, sourceFile, c))


This is the only call to getSignatureHelpItem, and callTargetDisplayParts is always length 0 or 1. It could just be a string. We know we're not going to have display parts anymore, so it doesn't seem like there's a need to keep the slices around. A lot of things can probably just become strings.

andrewbranch · 2025-05-20T21:29:49Z

internal/ls/signaturehelp.go

+	if callTargetSymbol != nil {
+		callTargetDisplayParts = append(callTargetDisplayParts, c.SymbolToString(callTargetSymbol))
+	}
+	var items [][]signatureInformation


This can be allocated with the correct length/capacity

andrewbranch · 2025-05-20T21:37:50Z

internal/ls/signaturehelp.go

+	selectedItemIndex := 0
+	itemSeen := 0
+	for i := range items {
+		item := items[i]
+		if (*candidates)[i] == resolvedSignature {
+			selectedItemIndex = itemSeen
+			if len(item) > 1 {
+				count := 0
+				for _, j := range item {
+					if j.IsVariadic || len(*j.Parameters) >= argumentInfo.argumentCount {
+						selectedItemIndex = itemSeen + count
+						break
+					}
+					count++
+				}
+			}
+		}
+		itemSeen = itemSeen + len(item)
+	}


This looks terrifying but I see it's ported exactly 😅

into signatureHelpImplementation

navya9singh added 18 commits April 1, 2025 15:29

initial implementation

f7b2484

Merge branch 'main' of https://github.com/navya9singh/typescript-go-repo

202e970

into signatureHelpImplementation

adding test setup

fff762c

Merge branch 'main' of https://github.com/navya9singh/typescript-go-repo

cbdb32b

into signatureHelpImplementation

cleanup

5d8476c

Merge branch 'main' of https://github.com/navya9singh/typescript-go-repo

dcd021f

into signatureHelpImplementation

adding more tests and fixes

473fbed

Merge branch 'main' of https://github.com/navya9singh/typescript-go-repo

0d73105

into signatureHelpImplementation

adding more tests and fixes

d68643b

Merge branch 'main' of https://github.com/navya9singh/typescript-go-repo

ed043a8

into signatureHelpImplementation

Merge branch 'main' of https://github.com/navya9singh/typescript-go-repo

e8c5dae

into signatureHelpImplementation

adding more tests

0c01e98

cleanup

7acd18e

Merge branch 'main' of https://github.com/navya9singh/typescript-go-repo

4a359ec

into signatureHelpImplementation

removing exported function

0ccbd20

removing getSymbolOfDeclaration export

151f03a

cleanup

85cdb01

removing deleted code

a22ab20

Copilot AI review requested due to automatic review settings May 12, 2025 21:09

Copilot AI reviewed May 12, 2025

View reviewed changes

jakebailey reviewed May 12, 2025

View reviewed changes

_submodules/TypeScript Outdated Show resolved Hide resolved

navya9singh commented May 12, 2025

View reviewed changes

navya9singh added 2 commits May 12, 2025 14:17

Merge branch 'main' of https://github.com/navya9singh/typescript-go-repo

e57cf78

into signatureHelpImplementation

removing submodule updates

50a3bbe

navya9singh commented May 12, 2025

View reviewed changes

gabritto reviewed May 13, 2025

View reviewed changes

internal/ls/utilities.go Outdated Show resolved Hide resolved

internal/ls/utilities.go Outdated Show resolved Hide resolved

navya9singh added 4 commits May 15, 2025 12:14

fixing tests

4bc1228

fixing formatting and lint errors

f415ca2

Merge branch 'main' of https://github.com/navya9singh/typescript-go-repo

ace6765

into signatureHelpImplementation

small fix

e836eb1

navya9singh added 2 commits May 15, 2025 13:28

Merge branch 'main' of https://github.com/navya9singh/typescript-go-repo

31bfa2a

into signatureHelpImplementation

adding noembed

aafc78f

navya9singh requested review from gabritto, jakebailey and andrewbranch May 15, 2025 21:46

jakebailey reviewed May 15, 2025

View reviewed changes

navya9singh added 2 commits May 16, 2025 13:46

adding exported functions to export.go

1bf462c

Merge branch 'main' of https://github.com/navya9singh/typescript-go-repo

8f34a4f

into signatureHelpImplementation

DanielRosenwasser requested changes May 17, 2025

View reviewed changes

gabritto reviewed May 17, 2025

View reviewed changes

addressing pr comments

7a4c633

navya9singh requested review from gabritto and DanielRosenwasser May 19, 2025 19:42

navya9singh added 3 commits May 20, 2025 10:22

addressing comments

d8db5cc

Merge branch 'main' of https://github.com/navya9singh/typescript-go-repo

9c81431

into signatureHelpImplementation

cleanup

628f4b4

gabritto reviewed May 20, 2025

View reviewed changes

internal/ast/utilities.go Show resolved Hide resolved

andrewbranch requested changes May 20, 2025

View reviewed changes

navya9singh added 9 commits May 23, 2025 17:24

addressing comments

0ff32ac

Merge branch 'main' of https://github.com/navya9singh/typescript-go-repo

7469666

into signatureHelpImplementation

fixing merge conflicts

e621d71

Merge branch 'main' of https://github.com/navya9singh/typescript-go-repo

7792a6e

into signatureHelpImplementation

renames

21b43cd

removing pointer to slice

c61cf81

Merge branch 'main' of https://github.com/navya9singh/typescript-go-repo

5b1f838

into signatureHelpImplementation

fixing lint errors

98de4c1

Merge branch 'main' of https://github.com/navya9singh/typescript-go-repo

093ff05

into signatureHelpImplementation

navya9singh requested a review from andrewbranch May 29, 2025 17:47

		// Converting signatureHelpParameter to *lsproto.ParameterInformation
		var parameters []*lsproto.ParameterInformation

	cachedResolvedSignatures := make(map[SignatureLinks]Signature)
	// We're going to temporarily reset resolved signatures for all surrounding calls.
	cachedResolvedSignatures := make(map[SignatureLinks]Signature)

		result := fn()
		for signatureLinks, resolvedSignature := range cachedResolvedSignatures {

	if strings.LastIndex(sourceFile.Text(), "<") == -1 {
	if strings.LastIndexByte(sourceFile.Text(), '<') == -1 {

-	// This function determines if the node could be type argument position
-	// Since during editing, when type argument list is not complete,
-	// the tree could be of any shape depending on the tokens parsed before current node,
-	// scanning of the previous identifier followed by "<" before current node would give us better result
-	// Note that we also balance out the already provided type arguments, arrays, object literals while doing so
+	// This function determines if the node could be a type argument position
+	// When editing, it is common to have an incomplete type argument list (e.g. missing ">"),
+	// so the tree can have any shape depending on the tokens before the current node.
+	// Instead, scanning for an identifier followed by a "<" before current node
+	// will typically give us better results than inspecting the tree.
+	// Note that we also balance out the already provided type arguments, arrays, object literals while doing so.

	onlyUseSyntacticOwners := context.TriggerKind == 2
	onlyUseSyntacticOwners := context.TriggerKind == lsproto.SignatureHelpTriggerKindTriggerCharacter

-			result := [][]*ast.Symbol{}
-			for _, t := range restType.AsUnionType().Types() {
-				result = append(result, c.expandSignatureParametersWithTupleMembers(signature, t.AsTypeReference(), restIndex, restSymbol))
+			types := restType.AsUnionType().Types()
+			result := make([][]*ast.Symbol, len(types))
+			for i, t := range types {
+				result[i] = c.expandSignatureParametersWithTupleMembers(signature, t.AsTypeReference(), restIndex, restSymbol)

	expandedParameters := p.c.getExpandedParameters(sig, true)[0]
	expandedParameters := p.c.getExpandedParameters(sig, true /skipUnionExpanding/)[0]

Signature help implementation #861

Are you sure you want to change the base?

Signature help implementation #861

Uh oh!

Conversation

navya9singh commented May 12, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabritto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

andrewbranch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!