Skip to content

Implement Go To Implementations command #1430

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.

Sign up for GitHub

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

Merged
ahejlsberg merged 2 commits into from
Jul 22, 2025
Merged

Implement Go To Implementations command #1430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e7eb71f
Implement LSP textDocument/implementation request
ahejlsberg Jul 21, 2025
a4384d4
Fix issue in go-to-definition on this.foo and super.foo
ahejlsberg Jul 22, 2025
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
4 changes: 1 addition & 3 deletions internal/ast/ast.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,15 +605,13 @@ func (n *Node) Type() *Node {
return n.AsCommonJSExport().Type
case KindBinaryExpression:
return n.AsBinaryExpression().Type
case KindEnumMember, KindBindingElement:
return nil
default:
funcLike := n.FunctionLikeData()
if funcLike != nil {
return funcLike.Type
}
}
panic("Unhandled case in Node.Type: " + n.Kind.String())
return nil
Copy link
Member

@jakebailey jakebailey Jul 21, 2025

Choose a reason for hiding this comment

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

This seems like it's going to end up biting us at some point when we forget to add a case here; is this required?

Copy link
Member

@DanielRosenwasser DanielRosenwasser Jul 21, 2025

Choose a reason for hiding this comment

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

Alternatively, Type() (panics) and TryGetType()/TypeOrNil() (returns nil)

Copy link
Member

@jakebailey jakebailey Jul 21, 2025

Choose a reason for hiding this comment

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

Well, other than that some nil returning is actually intentional IIRC

Copy link
Member Author

@ahejlsberg ahejlsberg Jul 21, 2025

Choose a reason for hiding this comment

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

Type() is already optional and we already have logic that always returns nil for some node kinds. Seems fine to just allow it for any node kind. Name() is similar already.

}

func (n *Node) Initializer() *Node {
Expand Down
2 changes: 1 addition & 1 deletion internal/ls/definition.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ func getDeclarationNameForKeyword(node *ast.Node) *ast.Node {
if decl := core.FirstOrNil(node.Parent.AsVariableDeclarationList().Declarations.Nodes); decl != nil && decl.Name() != nil {
return decl.Name()
}
} else if node.Parent.Name() != nil && node.Pos() < node.Parent.Name().Pos() {
} else if node.Parent.DeclarationData() != nil && node.Parent.Name() != nil && node.Pos() < node.Parent.Name().Pos() {
return node.Parent.Name()
}
}
Expand Down
63 changes: 45 additions & 18 deletions internal/ls/findallreferences.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,14 +128,10 @@ func newNodeEntryWithKind(node *ast.Node, kind entryKind) *referenceEntry {

func newNodeEntry(node *ast.Node) *referenceEntry {
// creates nodeEntry with `kind == entryKindNode`
n := node
if node != nil && node.Name() != nil {
n = node.Name()
}
return &referenceEntry{
kind: entryKindNode,
node: node,
context: getContextNodeForNodeEntry(n),
node: core.OrElse(node.Name(), node),
context: getContextNodeForNodeEntry(node),
}
}

Expand Down Expand Up @@ -410,22 +406,53 @@ func (l *LanguageService) ProvideReferences(params *lsproto.ReferenceParams) []*

symbolsAndEntries := l.getReferencedSymbolsForNode(position, node, program, program.GetSourceFiles(), options, nil)

return core.FlatMap(symbolsAndEntries, l.convertSymbolAndEntryToLocation)
return core.FlatMap(symbolsAndEntries, l.convertSymbolAndEntriesToLocations)
}

func (l *LanguageService) ProvideImplementations(params *lsproto.ImplementationParams) []*lsproto.Location {
program, sourceFile := l.getProgramAndFile(params.TextDocument.Uri)
position := int(l.converters.LineAndCharacterToPosition(sourceFile, params.Position))
node := astnav.GetTouchingPropertyName(sourceFile, position)

var seenNodes collections.Set[*ast.Node]
var entries []*referenceEntry
queue := l.getImplementationReferenceEntries(program, node, position)
Copy link
Member

@DanielRosenwasser DanielRosenwasser Jul 21, 2025

Choose a reason for hiding this comment

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

The original had something like the following check before processing as a queue

    if (
        node.parent.kind === SyntaxKind.PropertyAccessExpression
        || node.parent.kind === SyntaxKind.BindingElement
        || node.parent.kind === SyntaxKind.ElementAccessExpression
        || node.kind === SyntaxKind.SuperKeyword
    ) {
        referenceEntries = entries && [...entries];
    }

and idea what that was intended for? I think there was some special handling for not "cascading" beyond the direct superclass

Copy link
Member Author

@ahejlsberg ahejlsberg Jul 21, 2025

Choose a reason for hiding this comment

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

Yeah, I think it is so that Go To Implementation on say a super keyword only goes to that type and not other types that implement it. Which seems a little odd. Not really sure why you'd want to Go To Implementation on super, as opposed to Go To Definition or Go To Type Definition.

Copy link
Member

@jakebailey jakebailey Jul 22, 2025

Choose a reason for hiding this comment

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

I feel like that makes sense, though, since super is basically "do the parent class thing" and so shouldn't jump anywhere else?

Copy link
Member Author

@ahejlsberg ahejlsberg Jul 22, 2025

Choose a reason for hiding this comment

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

Well, but what does Go To Implementation on something that isn't a type really mean to begin with? I would think of it as go-to-implementation-of-the-type-of-the-thing-i-am-on. And if the type of super is Base, then implementations of Base also includes Derived.

Copy link
Member

@jakebailey jakebailey Jul 22, 2025

Choose a reason for hiding this comment

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

My mental model of it is "the implementation of super is code in a parent class's constructor".

Copy link
Member Author

@ahejlsberg ahejlsberg Jul 22, 2025

Choose a reason for hiding this comment

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

In Strada, Go To Definition on a super() call goes to the constructor code. But Go To Implementation goes to the base class declaration.

Copy link
Member

@DanielRosenwasser DanielRosenwasser Jul 22, 2025

Choose a reason for hiding this comment

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

Yeah I looked at the test cases and was surprised. Both go-to-definition and go-to-implementation should probably go to the constructor, but instead we go to the base class for go-to-implementation which feels like it is the less-intuitive thing.

Copy link
Member

@jakebailey jakebailey Jul 22, 2025

Choose a reason for hiding this comment

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

weirdddd

Copy link
Member Author

@ahejlsberg ahejlsberg Jul 22, 2025

Choose a reason for hiding this comment

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

I suppose the meaning we ascribe to "implementation" in go-to-implementations is something that implements that type. As opposed to the implementation of a method or constructor.

for len(queue) != 0 {
entry := queue[0]
queue = queue[1:]
if !seenNodes.Has(entry.node) {
seenNodes.Add(entry.node)
entries = append(entries, entry)
queue = append(queue, l.getImplementationReferenceEntries(program, entry.node, entry.node.Pos())...)
}
}

return l.convertEntriesToLocations(entries)
}

func (l *LanguageService) getImplementationReferenceEntries(program *compiler.Program, node *ast.Node, position int) []*referenceEntry {
options := refOptions{use: referenceUseReferences, implementations: true}
symbolsAndEntries := l.getReferencedSymbolsForNode(position, node, program, program.GetSourceFiles(), options, nil)
return core.FlatMap(symbolsAndEntries, func(s *SymbolAndEntries) []*referenceEntry { return s.references })
}

// == functions for conversions ==
func (l *LanguageService) convertSymbolAndEntryToLocation(s *SymbolAndEntries) []*lsproto.Location {
var locations []*lsproto.Location
for _, ref := range s.references {
if ref.textRange == nil {
sourceFile := ast.GetSourceFileOfNode(ref.node)
ref.textRange = l.getRangeOfNode(ref.node, sourceFile, nil /*endNode*/)
ref.fileName = sourceFile.FileName()
func (l *LanguageService) convertSymbolAndEntriesToLocations(s *SymbolAndEntries) []*lsproto.Location {
return l.convertEntriesToLocations(s.references)
}

func (l *LanguageService) convertEntriesToLocations(entries []*referenceEntry) []*lsproto.Location {
locations := make([]*lsproto.Location, len(entries))
for i, entry := range entries {
if entry.textRange == nil {
sourceFile := ast.GetSourceFileOfNode(entry.node)
entry.textRange = l.getRangeOfNode(entry.node, sourceFile, nil /*endNode*/)
entry.fileName = sourceFile.FileName()
}
locations[i] = &lsproto.Location{
Uri: FileNameToDocumentURI(entry.fileName),
Range: *entry.textRange,
}
locations = append(locations, &lsproto.Location{
Uri: FileNameToDocumentURI(ref.fileName),
Range: *ref.textRange,
})
}
return locations
}
Expand Down
16 changes: 16 additions & 0 deletions internal/lsp/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,8 @@ func (s *Server) handleRequestOrNotification(ctx context.Context, req *lsproto.R
return s.handleCompletion(ctx, req)
case *lsproto.ReferenceParams:
return s.handleReferences(ctx, req)
case *lsproto.ImplementationParams:
return s.handleImplementations(ctx, req)
case *lsproto.SignatureHelpParams:
return s.handleSignatureHelp(ctx, req)
case *lsproto.DocumentFormattingParams:
Expand Down Expand Up @@ -560,6 +562,9 @@ func (s *Server) handleInitialize(req *lsproto.RequestMessage) {
ReferencesProvider: &lsproto.BooleanOrReferenceOptions{
Boolean: ptrTo(true),
},
ImplementationProvider: &lsproto.BooleanOrImplementationOptionsOrImplementationRegistrationOptions{
Boolean: ptrTo(true),
},
DiagnosticProvider: &lsproto.DiagnosticOptionsOrDiagnosticRegistrationOptions{
DiagnosticOptions: &lsproto.DiagnosticOptions{
InterFileDependencies: true,
Expand Down Expand Up @@ -725,6 +730,17 @@ func (s *Server) handleReferences(ctx context.Context, req *lsproto.RequestMessa
return nil
}

func (s *Server) handleImplementations(ctx context.Context, req *lsproto.RequestMessage) error {
// goToImplementation
params := req.Params.(*lsproto.ImplementationParams)
project := s.projectService.EnsureDefaultProjectForURI(params.TextDocument.Uri)
languageService, done := project.GetLanguageServiceForRequest(ctx)
defer done()
locations := languageService.ProvideImplementations(params)
s.sendResult(req.ID, locations)
return nil
}

func (s *Server) handleCompletion(ctx context.Context, req *lsproto.RequestMessage) error {
params := req.Params.(*lsproto.CompletionParams)
project := s.projectService.EnsureDefaultProjectForURI(params.TextDocument.Uri)
Expand Down
Loading