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

LSP: add support for finding module fn references #3147

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

Conversation

nicklimmm
Copy link
Contributor

@nicklimmm nicklimmm commented May 17, 2024

Closes #1541

Using AST traversal to collect references. Extending to other nodes, e.g. local vars, should be simple enough.

This only collects references in the project's modules.

With the references feature, we can reuse the implementation for renaming, as renaming needs to find all references and perform updates on those.

Considerations

Unpreciseness of definition_location()

Currently, the definition_location() method doesn't give the precise location, for example:

foo.bar()
//   v
// `definition_location()` on the `.bar` (of type `TypedExpr::ModuleSelect`) 
// will return the span of `pub fn bar()` in the `foo` module
//
// Ideally, it should return only the span of the function name `bar`, not the whole `pub fn bar()`

With the current behaviour, the output of finding references is still reasonable enough, even though it's not pointing to the exact location.

To fix the above, I am proposing a solution: add additional span information for preciseness in the corresponding AST nodes, e.g. add the span of the function name. This would also be very helpful in renaming, as we don't need to use error-prone string matchings (which we need to take care of comments) to perform the correct edits.

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Seeing as we already traverse to build a call graph and the final solution to this is to use the call graph would it not make more sense to use that here? Rather than add a tree walker and then remove it later. It would also mean we're in a position to use the graph for other features too.

@nicklimmm
Copy link
Contributor Author

nicklimmm commented May 28, 2024

Seeing as we already traverse to build a call graph and the final solution to this is to use the call graph would it not make more sense to use that here? Rather than add a tree walker and then remove it later.

From what I know, the current call graph implementation in call_graph.rs only deals with a single module, so we can't resolve the usage of functions from other modules. I expect a pretty significant change (and friction) when we try to capture relationships between modules.

A plus with this AST approach is it is quite extensible and separated from other components too.

It would also mean we're in a position to use the graph for other features too.

What are those features that will use the call graph too?

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.

LSP: Show references to functions
2 participants