Skip to content

LSP: Jump to the function name #792

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

shinichy
Copy link

@shinichy shinichy commented Apr 13, 2025

Improved the "Go to Definition" command to jump to the function name instead of the beginning of the function keyword.

Example:

function foo() {
   ...
}

foo(); // "Go to Definition" jumps to `foo` instead of `function`.

@@ -15,25 +16,70 @@ func (l *LanguageService) ProvideDefinitions(fileName string, position int) []Lo
}

checker := program.GetTypeChecker()
calledDeclaration := tryGetSignatureDeclaration(checker, node)
Copy link
Author

Choose a reason for hiding this comment

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

I tried to port the logic from src/services/goToDefinition.ts in the TypeScript repository.

@jakebailey jakebailey requested a review from Copilot April 16, 2025 23:43
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves the “Go to Definition” command so that it now jumps directly to a function’s name instead of its leading keyword. Key changes include:

  • Updating the definition lookup in internal/ls/definition.go to focus on the function name.
  • Renaming several helper functions to exported versions (e.g., GetResolvedSignature, IsCallLikeExpression) for consistency.
  • Adding new utility functions in internal/ast/utilities.go to support the enhanced navigation.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
internal/ls/definition.go Refactored definition location logic and added helper functions.
internal/checker/utilities.go Renamed and added exported helper functions for signature checks.
internal/checker/jsx.go Updated signature resolution to use exported helper functions.
internal/checker/flow.go Updated calls to use the exported GetResolvedSignature function.
internal/checker/checker.go Replaced internal function calls with their exported counterparts.
internal/ast/utilities.go Added a utility to determine property access context.
Comments suppressed due to low confidence (4)

internal/ls/definition.go:19

  • [nitpick] Consider renaming 'tryGetSignatureDeclaration' to better convey its intent (e.g., 'getSignatureDeclaration') if it always returns a declaration or nil.
calledDeclaration := tryGetSignatureDeclaration(checker, node)

internal/checker/utilities.go:1297

  • Ensure that the change to an exported function name is accompanied by updated documentation comments to clarify its usage.
func IsCallLikeExpression(node *ast.Node) bool {

internal/checker/checker.go:7904

  • [nitpick] Verify that all calls to the renamed 'GetResolvedSignature' are updated consistently and that its behavior is properly documented.
func (c *Checker) GetResolvedSignature(node *ast.Node, candidatesOutArray *[]*Signature, checkMode CheckMode) *Signature {

internal/ast/utilities.go:2654

  • [nitpick] Add a documentation comment for 'IsRightSideOfPropertyAccess' to clearly describe its purpose and usage in the codebase.
func IsRightSideOfPropertyAccess(node *Node) bool {

@sandersn
Copy link
Member

@iisaduan is working on find-all-refs, so she may have already looked at this code.

Should some tests change? I'm not sure if fourslash tests are available yet.

@iisaduan
Copy link
Member

It's pretty separate from the find all refs work I've been doing, so there's no duplication. But the PR probably should update tests, so we have some sort of reference for what has been ported already/what we expect to work

@iisaduan
Copy link
Member

I could probably test this with my find all refs testing, but it's not quite ready yet

@shinichy
Copy link
Author

shinichy commented May 1, 2025

It's good to know that there's no duplication, as that's something I was concerned about since your team seems to be actively working on the ls package. I'll also try to add a test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants