Replace RubyIndexer::Collector with RubyIndexer::DeclarationListener #2008
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
st0012
added
chore
st0012
force-pushed
the
rewrite-index-collector
branch
2 times, most recently
from
6739fe9
to
9584ebd
Compare
st0012
commented
May 3, 2024
vinistock
approved these changes
May 3, 2024
| @@ -185,9 +185,11 @@ def index_all(indexable_paths: RubyIndexer.configuration.indexables, &block) | |||
| sig { params(indexable_path: IndexablePath, source: T.nilable(String)).void } | |||
| def index_single(indexable_path, source = nil) | |||
| content = source || File.read(indexable_path.full_path) | |||
| dispatcher = Prism::Dispatcher.new | |||
There was a problem hiding this comment.
If we want to be able to index at the same time that we run semantic highlighting, document symbol and others, then we'll need to receiver the dispatcher as an argument. Maybe we can make it optional with the default of Prism::Dispatcher.new?
There was a problem hiding this comment.
While I agree with the idea, I prefer leaving this change to the PR where we're actually doing so as it will make the change linked its dependencies. If we do it in this PR, it'd be hard to understand the intention without checking the PR discussions.
This rewrite will give us a few benefits: - We can use the same pattern as we write request listeners, which will make maintenance easier. - It will make some future indexing features easier to implement. With collector, we don't have access to a node's leave events, which makes features like capturing visibility hard to implement. - Since `Prism::Dispatcher` visits all nodes in the AST, we can now collect nodes that are defined unconventionally, like behind conditionals or begin blocks, just by doing the switching. Unfortunately, this change will make indexing slower. In Core, it increases the indexing time from ~32 seconds to ~41 seconds (~28%). But on the other hand, if in the future `Prism::Dispatcher` receives performance improvements, the indexer will benefit automatically from them too.
st0012
force-pushed
the
rewrite-index-collector
branch
from
9584ebd
to
dd4b44f
Compare
st0012
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
This rewrite will give us a few benefits:
Prism::Dispatchervisits all nodes in the AST, we can nowcollect nodes that are defined unconventionally, like behind conditionals
or begin blocks, just by doing the switching.
Unfortunately, this change will make indexing slower. In Core, it increases the indexing time from ~32 seconds to ~41 seconds (~28%). But on the other hand, if in the future
Prism::Dispatcherreceives performance improvements, the indexer will benefit automatically from them too.Implementation
RubyIndexer::DeclarationListenerclassPrism::Dispatcherand register the relevant events,DeclarationListenercan reuse most methods fromCollectorwith minor tweaks and renaming.Collectorreferences withDeclarationListener.Automated Tests
I added a few simple tests for the newly supported cases.
Manual Tests