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 find_references to index #1044

Closed
wants to merge 1 commit into from
Closed

Conversation

vinistock
Copy link
Member

Motivation

First step towards #202. At least for classes, modules and constants.

This PR adds a method to find references to a fully qualified name in the indexer, so that we can provide the references request.

Implementation

The idea is that we go through all indexed file paths, using a new visitor to find references that match the given fully qualified name. We collect everything and return.

I find this implementation really interesting because it surfaces references in gems too. So, if you want to know how ActiveRecord::Base is used inside Rails, you can just ask for references on the class and it'll bring you results.

Automated Tests

Added tests.

@vinistock vinistock added the enhancement New feature or request label Sep 28, 2023
@vinistock vinistock added this to the 2023-Q3 milestone Sep 28, 2023
@vinistock vinistock self-assigned this Sep 28, 2023
@vinistock vinistock requested a review from a team as a code owner September 28, 2023 13:42
@@ -164,6 +164,16 @@ def index_single(indexable_path, source = nil)
# If `path` is a directory, just ignore it and continue indexing
end

# Finds references to a fully qualified in all indexed files
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this going to run for every request to find the references of a constant? How fast is it on a big project with a bunch of dependencies?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's the idea. I'll collect some data so that we know roughly what to expect.

Given that the operation of asking for references is user initiated and not on the critical performance path, I think it should be fine. There's also the option to show a loading dialogue while we search.

@index = index
@path = path
@fully_qualified_name = fully_qualified_name
@nesting = T.let([], T::Array[String])
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be a common behavior for a bunch of visitors, I wonder if we could provide this feature through a super class?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's fair, we can have a parent class for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I tried this refactor and it makes things a little weird for our indexing visitor, since we need to stuff to happen between the push and pop of the stack. I think this might be a bit premature.

Let's wait a bit until we have a clearer picture and then we can refactor with more confidence.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the superclass can introduce a visit_class_body and visit_module_body methods for example that are to be redefined in the subclasses.

So the subclasses do not need to touch visit_class_node and get the nesting behavior for free.

@vinistock vinistock force-pushed the vs/add_find_references_to_index branch from c9e1410 to a480134 Compare October 2, 2023 14:45
@vinistock
Copy link
Member Author

I'm going to put this on hold for the moment. Finding references like this takes around 40 seconds, which also slows down Sorbet's response because VS Code keeps waiting for all LSPs to return before showing results.

I did extensive profiling of our indexing mechanism and the biggest gains we can have come from making Prism instantiate Prism::Location and Prism::Source lazily (we spend more than 40% of our time creating instances and on GC).

When we can make those instantiations lazily, indexing will be significantly sped up. Then we can make a better decision and maybe even collect references in the first indexing pass.

@vinistock vinistock closed this Oct 25, 2023
@greatscotty greatscotty deleted the vs/add_find_references_to_index branch March 12, 2024 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants