Skip to content

Support LocationLink in go-to-definition with default enablement #1490

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.

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 7 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
9 changes: 8 additions & 1 deletion internal/fourslash/fourslash.go
Original file line number Diff line number Diff line change
Expand Up @@ -891,7 +891,14 @@ func (f *FourslashTest) VerifyBaselineGoToDefinition(
} else if result.Location != nil {
resultAsLocations = []lsproto.Location{*result.Location}
} else if result.DefinitionLinks != nil {
t.Fatalf("Unexpected definition response type at marker '%s': %T", *f.lastKnownMarkerName, result.DefinitionLinks)
// Convert DefinitionLinks to Locations for baseline comparison
resultAsLocations = make([]lsproto.Location, len(*result.DefinitionLinks))
Copy link
Member

Choose a reason for hiding this comment

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

The right thing is not to normalize to Locations. Instead, what the original code did was roughly checking if there was a single result, and if so, set the appropriate additionalSpan for getBaselineContentForFile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the normalization logic to use additionalSpan for single DefinitionLink results instead of converting to Locations. Now properly sets the origin selection range as additionalSpan for baseline comparison. Commit 694ad3b.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, but switch this to use core.Map

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to use core.Map instead of manual loop. Commit 0abc02d.

for i, link := range *result.DefinitionLinks {
resultAsLocations[i] = lsproto.Location{
Uri: link.TargetUri,
Range: link.TargetSelectionRange,
}
}
}

f.baseline.addResult("goToDefinition", f.getBaselineForLocationsWithFileContents(resultAsLocations, baselineFourslashLocationsOptions{
Expand Down
45 changes: 45 additions & 0 deletions internal/fourslash/tests/gen/locationlink_endtoend_test.go
Copy link
Member

Choose a reason for hiding this comment

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

Get rid of this test, we have coverage elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the test as requested. Commit 694ad3b.

Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package fourslash_test

import (
"testing"

"github.com/microsoft/typescript-go/internal/fourslash"
"github.com/microsoft/typescript-go/internal/lsp/lsproto"
"github.com/microsoft/typescript-go/internal/testutil"
)

func TestLocationLinkEndToEnd(t *testing.T) {
t.Parallel()

defer testutil.RecoverAndFail(t, "Panic on fourslash test")
const content = `// @Filename: types.ts
export interface [|/*def*/Person|] {
name: string;
age: number;
}

export function createPerson(name: string, age: number): Person {
return { name, age };
}

// @Filename: main.ts
import { /*usage*/Person, createPerson } from "./types";

const john: Person = createPerson("John", 30);`

// Test with LinkSupport enabled
linkSupport := true
capabilities := &lsproto.ClientCapabilities{
TextDocument: &lsproto.TextDocumentClientCapabilities{
Definition: &lsproto.DefinitionClientCapabilities{
LinkSupport: &linkSupport,
},
},
}

f := fourslash.NewFourslash(t, capabilities, content)

// Verify that going to definition from "usage" marker works
// and returns LocationLink format when client supports it
f.VerifyBaselineGoToDefinition(t, "usage")
}
63 changes: 54 additions & 9 deletions internal/ls/definition.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
"github.com/microsoft/typescript-go/internal/scanner"
)

func (l *LanguageService) ProvideDefinition(ctx context.Context, documentURI lsproto.DocumentUri, position lsproto.Position) (lsproto.DefinitionResponse, error) {
func (l *LanguageService) ProvideDefinition(ctx context.Context, documentURI lsproto.DocumentUri, position lsproto.Position, clientCapabilities *lsproto.DefinitionClientCapabilities) (lsproto.DefinitionResponse, error) {
program, file := l.getProgramAndFile(documentURI)
node := astnav.GetTouchingPropertyName(file, int(l.converters.LineAndCharacterToPosition(file, position)))
if node.Kind == ast.KindSourceFile {
Expand All @@ -23,13 +23,13 @@ func (l *LanguageService) ProvideDefinition(ctx context.Context, documentURI lsp

if node.Kind == ast.KindOverrideKeyword {
if sym := getSymbolForOverriddenMember(c, node); sym != nil {
return l.createLocationsFromDeclarations(sym.Declarations), nil
return l.createDefinitionResponse(sym.Declarations, node, file, clientCapabilities), nil
}
}

if ast.IsJumpStatementTarget(node) {
if label := getTargetLabel(node.Parent, node.Text()); label != nil {
return l.createLocationsFromDeclarations([]*ast.Node{label}), nil
return l.createDefinitionResponse([]*ast.Node{label}, node, file, clientCapabilities), nil
}
}

Expand All @@ -42,16 +42,16 @@ func (l *LanguageService) ProvideDefinition(ctx context.Context, documentURI lsp

if node.Kind == ast.KindReturnKeyword || node.Kind == ast.KindYieldKeyword || node.Kind == ast.KindAwaitKeyword {
if fn := ast.FindAncestor(node, ast.IsFunctionLikeDeclaration); fn != nil {
return l.createLocationsFromDeclarations([]*ast.Node{fn}), nil
return l.createDefinitionResponse([]*ast.Node{fn}, node, file, clientCapabilities), nil
}
}

if calledDeclaration := tryGetSignatureDeclaration(c, node); calledDeclaration != nil {
return l.createLocationsFromDeclarations([]*ast.Node{calledDeclaration}), nil
return l.createDefinitionResponse([]*ast.Node{calledDeclaration}, node, file, clientCapabilities), nil
}

if ast.IsIdentifier(node) && ast.IsShorthandPropertyAssignment(node.Parent) {
return l.createLocationsFromDeclarations(c.GetResolvedSymbol(node).Declarations), nil
return l.createDefinitionResponse(c.GetResolvedSymbol(node).Declarations, node, file, clientCapabilities), nil
}

node = getDeclarationNameForKeyword(node)
Expand All @@ -70,15 +70,15 @@ func (l *LanguageService) ProvideDefinition(ctx context.Context, documentURI lsp
if symbol.Flags&(ast.SymbolFlagsProperty|ast.SymbolFlagsMethod|ast.SymbolFlagsAccessor) != 0 && symbol.Parent != nil && symbol.Parent.Flags&ast.SymbolFlagsObjectLiteral != 0 {
if objectLiteral := core.FirstOrNil(symbol.Parent.Declarations); objectLiteral != nil {
if declarations := c.GetContextualDeclarationsForObjectLiteralElement(objectLiteral, symbol.Name); len(declarations) != 0 {
return l.createLocationsFromDeclarations(declarations), nil
return l.createDefinitionResponse(declarations, node, file, clientCapabilities), nil
}
}
}
return l.createLocationsFromDeclarations(symbol.Declarations), nil
return l.createDefinitionResponse(symbol.Declarations, node, file, clientCapabilities), nil
}

if indexInfos := c.GetIndexSignaturesAtLocation(node); len(indexInfos) != 0 {
return l.createLocationsFromDeclarations(indexInfos), nil
return l.createDefinitionResponse(indexInfos, node, file, clientCapabilities), nil
}

return lsproto.LocationOrLocationsOrDefinitionLinksOrNull{}, nil
Expand Down Expand Up @@ -126,6 +126,51 @@ func getDeclarationNameForKeyword(node *ast.Node) *ast.Node {
return node
}

func (l *LanguageService) createDefinitionResponse(declarations []*ast.Node, originNode *ast.Node, originFile *ast.SourceFile, clientCapabilities *lsproto.DefinitionClientCapabilities) lsproto.DefinitionResponse {
// Check if client supports LocationLink
if clientCapabilities != nil && clientCapabilities.LinkSupport != nil && *clientCapabilities.LinkSupport {
return l.createLocationLinksFromDeclarations(declarations, originNode, originFile)
}
// Fall back to traditional Location response
return l.createLocationsFromDeclarations(declarations)
}

func (l *LanguageService) createLocationLinksFromDeclarations(declarations []*ast.Node, originNode *ast.Node, originFile *ast.SourceFile) lsproto.DefinitionResponse {
someHaveBody := core.Some(declarations, func(node *ast.Node) bool { return node.Body() != nil })
links := make([]*lsproto.LocationLink, 0, len(declarations))

// Calculate origin selection range (the "bound span")
originSelectionRange := l.createLspRangeFromNode(originNode, originFile)

for _, decl := range declarations {
if !someHaveBody || decl.Body() != nil {
file := ast.GetSourceFileOfNode(decl)
name := core.OrElse(ast.GetNameOfDeclaration(decl), decl)

// For targetRange, use the full declaration range
var targetRange *lsproto.Range
if decl.Body() != nil {
// For declarations with body, include the full declaration
targetRange = l.createLspRangeFromBounds(scanner.GetTokenPosOfNode(decl, file, false), decl.End(), file)
} else {
// For declarations without body, use the declaration itself
targetRange = l.createLspRangeFromNode(decl, file)
}

// For targetSelectionRange, use just the name/identifier part
targetSelectionRange := l.createLspRangeFromNode(name, file)

links = append(links, &lsproto.LocationLink{
OriginSelectionRange: originSelectionRange,
TargetUri: FileNameToDocumentURI(file.FileName()),
TargetRange: *targetRange,
TargetSelectionRange: *targetSelectionRange,
})
}
}
return lsproto.LocationOrLocationsOrDefinitionLinksOrNull{DefinitionLinks: &links}
}

func (l *LanguageService) createLocationsFromDeclarations(declarations []*ast.Node) lsproto.DefinitionResponse {
Copy link
Member

Choose a reason for hiding this comment

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

Who is using this function anymore? Can it be used in the new code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The createLocationsFromDeclarations function is still being used as the fallback when LinkSupport is false in the createDefinitionResponse method. It provides backwards compatibility for clients that don't support LocationLink.

someHaveBody := core.Some(declarations, func(node *ast.Node) bool { return node.Body() != nil })
locations := make([]lsproto.Location, 0, len(declarations))
Expand Down
38 changes: 38 additions & 0 deletions internal/ls/definition_link_support_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package ls

import (
"testing"

"github.com/microsoft/typescript-go/internal/lsp/lsproto"
"gotest.tools/v3/assert"
)

func TestLocationLinkSupport(t *testing.T) {
t.Parallel()

// Simple integration test to ensure LocationLink support works
// without causing import cycles

// Test that client capabilities are correctly used
linkSupport := true
capabilities := &lsproto.DefinitionClientCapabilities{
LinkSupport: &linkSupport,
}

// Test that the capability checking logic works
assert.Assert(t, capabilities != nil)
assert.Assert(t, capabilities.LinkSupport != nil)
assert.Assert(t, *capabilities.LinkSupport)

// Test with capabilities disabled
linkSupportFalse := false
capabilitiesDisabled := &lsproto.DefinitionClientCapabilities{
LinkSupport: &linkSupportFalse,
}
assert.Assert(t, capabilitiesDisabled.LinkSupport != nil)
assert.Assert(t, !*capabilitiesDisabled.LinkSupport)

// Test with nil capabilities
var nilCapabilities *lsproto.DefinitionClientCapabilities
assert.Assert(t, nilCapabilities == nil)
}
3 changes: 2 additions & 1 deletion internal/ls/definition_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,8 @@ func runDefinitionTest(t *testing.T, input string, expected map[string]lsproto.D
locations, err := languageService.ProvideDefinition(
ctx,
ls.FileNameToDocumentURI(file),
marker.LSPosition)
marker.LSPosition,
nil) // No client capabilities in test
assert.NilError(t, err)
assert.DeepEqual(t, locations, expectedResult)
}
Expand Down
2 changes: 1 addition & 1 deletion internal/ls/untitled_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ x++;`
// Also test definition using ProvideDefinition
uri := ls.FileNameToDocumentURI("/Untitled-2.ts")
lspPosition := lsproto.Position{Line: 2, Character: 0}
definition, err := service.ProvideDefinition(t.Context(), uri, lspPosition)
definition, err := service.ProvideDefinition(t.Context(), uri, lspPosition, nil)
assert.NilError(t, err)
if definition.Locations != nil {
t.Logf("Definition found: %d locations", len(*definition.Locations))
Expand Down
9 changes: 8 additions & 1 deletion internal/lsp/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -722,7 +722,7 @@ func (s *Server) handleDefinition(ctx context.Context, params *lsproto.Definitio
project := s.projectService.EnsureDefaultProjectForURI(params.TextDocument.Uri)
languageService, done := project.GetLanguageServiceForRequest(ctx)
defer done()
return languageService.ProvideDefinition(ctx, params.TextDocument.Uri, params.Position)
return languageService.ProvideDefinition(ctx, params.TextDocument.Uri, params.Position, getDefinitionClientCapabilities(s.initializeParams))
}

func (s *Server) handleTypeDefinition(ctx context.Context, params *lsproto.TypeDefinitionParams) (lsproto.TypeDefinitionResponse, error) {
Expand Down Expand Up @@ -878,3 +878,10 @@ func getCompletionClientCapabilities(params *lsproto.InitializeParams) *lsproto.
}
return params.Capabilities.TextDocument.Completion
}

func getDefinitionClientCapabilities(params *lsproto.InitializeParams) *lsproto.DefinitionClientCapabilities {
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 this should be in getCapabilitiesWithDefaults, but maybe it's good to be targeted. Don't change it for now, but wondering thoughts from others on the team (e.g. @jakebailey @gabritto)

if params == nil || params.Capabilities == nil || params.Capabilities.TextDocument == nil {
return nil
}
return params.Capabilities.TextDocument.Definition
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
//// [tests/cases/compiler/gotoDefinitionLocationLink.ts] ////

//// [file1.ts]
export interface Person {
name: string;
age: number;
}

export function createPerson(name: string, age: number): Person {
return { name, age };
}

//// [file2.ts]
import { Person, createPerson } from "./file1";

const person: Person = createPerson("John", 30);
console.log(person.name);

//// [file1.js]
export function createPerson(name, age) {
return { name, age };
}
//// [file2.js]
import { createPerson } from "./file1";
const person = createPerson("John", 30);
console.log(person.name);
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
//// [tests/cases/compiler/gotoDefinitionLocationLink.ts] ////

=== file1.ts ===
export interface Person {
>Person : Symbol(Person, Decl(file1.ts, 0, 0))

name: string;
>name : Symbol(name, Decl(file1.ts, 0, 25))

age: number;
>age : Symbol(age, Decl(file1.ts, 1, 17))
}

export function createPerson(name: string, age: number): Person {
>createPerson : Symbol(createPerson, Decl(file1.ts, 3, 1))
>name : Symbol(name, Decl(file1.ts, 5, 29))
>age : Symbol(age, Decl(file1.ts, 5, 42))
>Person : Symbol(Person, Decl(file1.ts, 0, 0))

return { name, age };
>name : Symbol(name, Decl(file1.ts, 6, 12))
>age : Symbol(age, Decl(file1.ts, 6, 18))
}

=== file2.ts ===
import { Person, createPerson } from "./file1";
>Person : Symbol(Person, Decl(file2.ts, 0, 8))
>createPerson : Symbol(createPerson, Decl(file2.ts, 0, 16))

const person: Person = createPerson("John", 30);
>person : Symbol(person, Decl(file2.ts, 2, 5))
>Person : Symbol(Person, Decl(file2.ts, 0, 8))
>createPerson : Symbol(createPerson, Decl(file2.ts, 0, 16))

console.log(person.name);
>console.log : Symbol(log, Decl(lib.dom.d.ts, --, --))
>console : Symbol(console, Decl(lib.dom.d.ts, --, --))
>log : Symbol(log, Decl(lib.dom.d.ts, --, --))
>person.name : Symbol(name, Decl(file1.ts, 0, 25))
>person : Symbol(person, Decl(file2.ts, 2, 5))
>name : Symbol(name, Decl(file1.ts, 0, 25))

Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
//// [tests/cases/compiler/gotoDefinitionLocationLink.ts] ////

=== file1.ts ===
export interface Person {
name: string;
>name : string

age: number;
>age : number
}

export function createPerson(name: string, age: number): Person {
>createPerson : (name: string, age: number) => Person
>name : string
>age : number

return { name, age };
>{ name, age } : { name: string; age: number; }
>name : string
>age : number
}

=== file2.ts ===
import { Person, createPerson } from "./file1";
>Person : any
>createPerson : (name: string, age: number) => Person

const person: Person = createPerson("John", 30);
>person : Person
>createPerson("John", 30) : Person
>createPerson : (name: string, age: number) => Person
>"John" : "John"
>30 : 30

console.log(person.name);
>console.log(person.name) : void
>console.log : (...data: any[]) => void
>console : Console
>log : (...data: any[]) => void
>person.name : string
>person : Person
>name : string

Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// === goToDefinition ===
// === /types.ts ===

// export interface [|Person|] {
// name: string;
// age: number;
// }
// // --- (line: 5) skipped ---


// === /main.ts ===

// import { /*GO TO DEFINITION*/Person, createPerson } from "./types";
//
// const john: Person = createPerson("John", 30);
Loading
Loading