-
Notifications
You must be signed in to change notification settings - Fork 681
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
3775124
34b1eb8
5ec5a0b
694ad3b
0abc02d
df65882
a8ba72c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
|
@@ -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 | ||
} | ||
} | ||
|
||
|
@@ -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) | ||
|
@@ -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 | ||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
someHaveBody := core.Some(declarations, func(node *ast.Node) bool { return node.Body() != nil }) | ||
locations := make([]lsproto.Location, 0, len(declarations)) | ||
|
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) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
|
@@ -878,3 +878,10 @@ func getCompletionClientCapabilities(params *lsproto.InitializeParams) *lsproto. | |
} | ||
return params.Capabilities.TextDocument.Completion | ||
} | ||
|
||
func getDefinitionClientCapabilities(params *lsproto.InitializeParams) *lsproto.DefinitionClientCapabilities { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be in |
||
if params == nil || params.Capabilities == nil || params.Capabilities.TextDocument == nil { | ||
return nil | ||
} | ||
return params.Capabilities.TextDocument.Definition | ||
} |
There was a problem hiding this comment.
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
Location
s. Instead, what the original code did was roughly checking if there was a single result, and if so, set the appropriateadditionalSpan
forgetBaselineContentForFile
.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.