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.

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!

DanielRosenwasser · 2025-05-16T23:32:35Z

internal/checker/checker.go

@@ -25962,16 +25962,23 @@ func (c *Checker) markPropertyAsReferenced(prop *ast.Symbol, nodeForCheckWriteOn
 	c.symbolReferenceLinks.Get(target).referenceKinds |= ast.SymbolFlagsAll
 }

-func (c *Checker) GetExpandedParameters(signature *Signature /* !!! skipUnionExpanding */) []*ast.Symbol {
+func (c *Checker) getExpandedParameters(signature *Signature, skipUnionExpanding *bool) [][]*ast.Symbol {


I would avoid making this a *bool. It implies one of two things to me:

You intend to mutate the state after the fact (which is why it's a pointer)

You need nil to be treated distinctly from false.

The only reason the old codebase had to account for undefined was so it could support an optional parameter which Go doesn't have.

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?

DanielRosenwasser · 2025-05-16T23:53:05Z

internal/ls/utilities.go

@@ -245,13 +245,99 @@ type PossibleTypeArgumentInfo struct {
 	nTypeArguments int
 }

-// !!! signature help
+// Get info for an expression like `f <` that may be the start of type arguments.
 func getPossibleTypeArgumentsInfo(tokenIn *ast.Node, sourceFile *ast.SourceFile) *PossibleTypeArgumentInfo {


Minor preference for this to just return an bool, PossibleTypeArgumentInfo - it doesn't need to allocate. Not sure what others think.

I think completions uses the detailed return info, so we'll probably need to return a struct here.

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.

gabritto · 2025-05-16T22:45:07Z

internal/ast/utilities.go

+	switch node.Kind {
+	case KindParameter, KindMethodDeclaration, KindMethodSignature, KindShorthandPropertyAssignment,
+		KindPropertyAssignment, KindPropertyDeclaration, KindPropertySignature:
+		return node.AsParameterDeclaration().QuestionToken != nil


That doesn't look right: if the node kind is, say, method declaration, node.AsParameterDeclaration() will fail. So I'd suggest either adding a separate cast for each node kind here, or maybe creating a QuestionToken() method in ast.go, in the style of the other getters we have there already.

gabritto · 2025-05-16T22:47:39Z

internal/checker/checker.go

@@ -29422,7 +29429,7 @@ func (c *Checker) GetSymbolAtLocation(node *ast.Node) *ast.Symbol {
 // be used only by the language service and external tools. The semantics of the function are deliberately "fuzzy"
 // and aim to just return *some* symbol for the node. To obtain the symbol associated with a node for type checking
 // purposes, use appropriate function for the context, e.g. `getResolvedSymbol` for an expression identifier,
-// `getSymbolOfDeclaration` for a declaration, etc.


These are probably leftover from the old rename.

gabritto · 2025-05-16T22:51:35Z

internal/checker/exports.go

+	return c.getReturnTypeOfSignature(sig)
+}
+
+func IsCallOrNewExpression(node *ast.Node) bool {


I would just move those node predicates to ast/utilities.go, like this one, GetInvokedExpression, IsJsxOpeningLikeElement, etc.

gabritto · 2025-05-16T22:54:52Z

internal/checker/inference.go

@@ -1235,7 +1235,7 @@ func (c *Checker) inferFromIntraExpressionSites(n *InferenceContext) {
 		if ast.IsMethodDeclaration(site.node) {
 			contextualType = c.getContextualTypeForObjectLiteralMethod(site.node, ContextFlagsNoConstraints)
 		} else {
-			contextualType = c.getContextualType(site.node, ContextFlagsNoConstraints)


Also looks like leftover from a rename?

gabritto · 2025-05-16T22:57:27Z

internal/checker/checker.go

@@ -25962,16 +25962,23 @@ func (c *Checker) markPropertyAsReferenced(prop *ast.Symbol, nodeForCheckWriteOn
 	c.symbolReferenceLinks.Get(target).referenceKinds |= ast.SymbolFlagsAll
 }

-func (c *Checker) GetExpandedParameters(signature *Signature /* !!! skipUnionExpanding */) []*ast.Symbol {
+func (c *Checker) getExpandedParameters(signature *Signature, skipUnionExpanding *bool) [][]*ast.Symbol {


skipUnionExpanding should just be bool, since we can't avoid passing arguments in Go like we can in TS with optional parameters.

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?

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

gabritto · 2025-05-17T00:35:50Z

internal/ls/utilities.go

@@ -245,13 +245,99 @@ type PossibleTypeArgumentInfo struct {
 	nTypeArguments int
 }

-// !!! signature help
+// Get info for an expression like `f <` that may be the start of type arguments.
 func getPossibleTypeArgumentsInfo(tokenIn *ast.Node, sourceFile *ast.SourceFile) *PossibleTypeArgumentInfo {


I think completions uses the detailed return info, so we'll probably need to return a struct here.

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.

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

internal/ls/signaturehelp.go

+		return containsNode(invocationChildren, startingToken)
+	case ast.KindCommaToken:
+		return containsNode(invocationChildren, startingToken)
+		// const containingList = findContainingList(startingToken);


Should this have a // !!!?

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Signature help implementation #861

Signature help implementation #861

navya9singh commented May 12, 2025

Copilot AI left a comment

navya9singh May 12, 2025

navya9singh May 12, 2025

jakebailey May 15, 2025

jakebailey May 15, 2025

DanielRosenwasser left a comment

DanielRosenwasser May 16, 2025

DanielRosenwasser May 16, 2025

DanielRosenwasser May 16, 2025

DanielRosenwasser May 16, 2025

DanielRosenwasser May 16, 2025

DanielRosenwasser May 16, 2025

DanielRosenwasser May 16, 2025

DanielRosenwasser May 16, 2025

gabritto May 17, 2025

DanielRosenwasser May 17, 2025

gabritto left a comment

gabritto May 16, 2025

gabritto May 16, 2025

gabritto May 16, 2025

gabritto May 16, 2025

gabritto May 16, 2025

gabritto May 17, 2025

gabritto May 17, 2025

gabritto May 17, 2025

gabritto May 17, 2025

gabritto May 17, 2025

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

Signature help implementation #861

Are you sure you want to change the base?

Signature help implementation #861

Conversation

navya9singh commented May 12, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielRosenwasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabritto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment