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

internal/checker/utilities.go

into signatureHelpImplementation

internal/ls/signaturehelp.go

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?

internal/checker/checker.go

internal/checker/printer.go

internal/checker/services.go

internal/ls/signaturehelp.go

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

internal/ls/signaturehelp.go

into signatureHelpImplementation

jakebailey · 2025-06-02T23:44:19Z

internal/checker/services.go

+	return runWithoutResolvedSignatureCaching(c, node, func() *Signature {
+		var res *Signature
+		res, candidatesOutArray = c.getResolvedSignatureWorker(node, CheckModeIsForSignatureHelp, argumentCount)
+		return res
+	}), candidatesOutArray


I think this technically works due to the evaluation order of multi-returns, but it kinda hurts my head. I'd personally rather this callback return a struct with the two results (which are then unpacked or returned as-is), or this call be split out into its own line before the return.

e.g.

func GetResolvedSignatureForSignatureHelp(node *ast.Node, argumentCount int, c *Checker) (*Signature, []*Signature) { type result struct { signature *signature candidates []*signature } result := runWithoutResolvedSignatureCaching(c, node, func() result { signature, candidates := c.getResolvedSignatureWorker(node, CheckModeIsForSignatureHelp, argumentCount) return result{signature, candidates} }) return result.signature, result.candidates }

outdated

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

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

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

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

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 requested a review from andrewbranch May 15, 2025 21:47

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 previously 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

navya9singh added 2 commits June 2, 2025 11:52

update with node builder

8fca88c

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

09d531a

into signatureHelpImplementation

navya9singh requested a review from gabritto June 2, 2025 22:58

andrewbranch approved these changes Jun 2, 2025

View reviewed changes

jakebailey reviewed Jun 2, 2025

View reviewed changes

	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

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!