Skip to content
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

Add go to definition support for autoloaded constants #1893

Open
vinistock opened this issue Apr 5, 2024 · 11 comments
Open

Add go to definition support for autoloaded constants #1893

vinistock opened this issue Apr 5, 2024 · 11 comments
Assignees
Labels
enhancement New feature or request good-first-issue Good for newcomers help-wanted Extra attention is needed pinned This issue or pull request is pinned and won't be marked as stale

Comments

@vinistock
Copy link
Member

Basically, the same feature as go to definition for require, but in this case for autoloads.

module Foo
  autoload :Bar, "bar" # <- jumping to bar should take you to the right file
end

We can probably add the same for the constant too, since we know that it must resolve to Foo::Bar, which makes it easy to search in the index.

@vinistock vinistock added enhancement New feature or request good-first-issue Good for newcomers help-wanted Extra attention is needed pinned This issue or pull request is pinned and won't be marked as stale labels Apr 5, 2024
@Super-Xray
Copy link

Hello @vinistock , I'm interested in this issue, can I handle it?

@vinistock
Copy link
Member Author

Sure! Feel free to put up a PR.

@Super-Xray
Copy link

Super-Xray commented Apr 15, 2024

Excuse me, I have finished jump-to-file logic but I notice that I don't have the permission to push my own branch so that I can't put up my PR, @vinistock, could you please give me the permission?

@st0012
Copy link
Member

st0012 commented Apr 15, 2024

@Super-Xray Usually, devs contribute to an OSS project by forking it, pushed the new branch to the fork, and then open a PR from the fork against the target repo.

@Super-Xray
Copy link

Super-Xray commented Apr 16, 2024

Thank you very much! Since this is my first OSS contribution, I didn't know this before.

@Super-Xray
Copy link

Sorry, I havn't fully understand this completely, "We can probably add the same for the constant too, since we know that it must resolve to Foo::Bar, which makes it easy to search in the index.", do you mean from ':Bar' in this autoload statement jump to the its class definition line?

@vinistock
Copy link
Member Author

That's what I originally meant, but thinking more about it, I think we can simplify this. Since the definition listener will only handle method call events (and not symbol or string events), it'll be hard to differentiate between the arguments. And the most specific jump you can make is to the constant, so the file path doesn't provide much value.

We can change the definition listener to do something like this

class Definition
  def on_call_node_enter(node)
    message = node.name
    
    case message
    when :require, :require_relative
      handle_require_definition(node)
    when :autoload
      handle_autoload_definition(node)
    else
      handle_method_definition(node)
    end
  end

  private

  def handle_autoload_definition(node)
    # Get the name of the constant from the first argument of the call node
    # Invoke find_in_index with the name of the constant
  end
end

@Super-Xray
Copy link

haha! That's what I think about too! using find_in_index to jump to the correct line!

@Super-Xray
Copy link

Super-Xray commented Apr 22, 2024

However,I have found that the structure of autoload statement is
image
there aren't any ConstantNode here so I think it's difficult to use find_in_index straightly (becuase constant_name method requires ConstantPathNode/ConstantReadNode/ConstantTargetNode input).
In order to find the correct line, I think maybe we can refer to the mechanism of searching class or method in a file (input '@' + class_name/method name) because we can get the class's name from SymbolNode.
For example, if we want to jump to the position where RubyLsp::Requests::Request is defined, we first jump to the file(it was been implemented before), and then search for Request by using @Request.
Does this idea work?And I want to know where is this "@"-searching mechanism?Thanks for your help!

@vinistock
Copy link
Member Author

The find_in_index method accepts the name of the constant you're looking for as a string and already takes care of the namespacing for you.

For example

module Foo
  autoload :Bar, "some/file"
end

In this case, you need to invoke find_in_index with the name Bar and the existing functionality will automatically discover that the referenced constant is actually Foo::Bar. Basically, what you need to do is extract the name from the autoload arguments and pass that name to find_in_index.

This will already make the LSP jump to the exact location where that constant is defined. You won't need the @ based search (document symbol), to land on the right place in the file.

@Super-Xray
Copy link

Super-Xray commented Apr 23, 2024

Thank you very much! I found this way didn't work before probably because the 'ruby-lsp' project is an sorbet project so that autoload :Request, "ruby_lsp/requests/request" can't activate the function(next if @typechecker_enabled && not_in_dependencies?(file_path) in find_in_index). I create an 'foo.rb' and 'bar.rb' and it does work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good-first-issue Good for newcomers help-wanted Extra attention is needed pinned This issue or pull request is pinned and won't be marked as stale
Projects
None yet
Development

No branches or pull requests

3 participants