Do not index documents excluded from indexing when they are opened - #4200
Open
rience wants to merge 1 commit into
Open
Do not index documents excluded from indexing when they are opened#4200rience wants to merge 1 commit into
rience wants to merge 1 commit into
Conversation
`excludedPatterns` is only applied while collecting `indexable_uris`. When the editor opens a document, the combined requests path indexes it unconditionally, so an excluded file still ends up in the index as soon as someone looks at it. This is visible in multi-root workspaces where one folder contains another, which is common in monorepos. Both the parent and the nested folder spawn a server, both match the document selector for the same file, and both index it on open. Every request that reads the index then returns the same result twice: `Definitions (2)`, listing one identical location per server. Extract the exclusion check out of `indexable_uris` into `Configuration#excluded?` and consult it before indexing an opened document. The absolute exclusion patterns are memoized, since exclusion is now checked once per indexable file, and invalidated when either the patterns or the workspace path change.
Author
|
I have signed the CLA! |
This file contains hidden or 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
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
excludedPatternsis applied only while collectingindexable_uris. When the editor opens a document, the combined requests path indexes it unconditionally —RubyDocument#should_index?returnstruefor any freshly opened document and never consults the index configuration — so an excluded file lands in the index as soon as somebody looks at it.That is surprising on its own, but it is most visible in multi-root workspaces where one folder contains another, which is the normal shape of a monorepo. Both the parent folder and the nested folder spawn a language server, both match the document selector for the same file (
${fsPath}/**/*), and both index it on open. Every request that reads the index then answers twice, and the editor merges the responses, soGo to Definitionopens a peek view titledDefinitions (2)listing the same location twice.Concretely, with a workspace whose folders are the repo root plus every app and component nested under it, opening
apps/billing/app/models/application_record.rblogs this, 5ms apart:and
Go to Definitionon any constant in that file offers two identical locations. Configuring the root workspace withcorrectly shrinks the root server's initial index (7,300 files down to 2,083 in the workspace I reproduced this on), but has no effect on documents once they are opened, so the duplicates come back for exactly the files you are working in.
This is related to #3639, but distinct: that issue is about one index gaining duplicate entries, whereas this is two servers each holding one entry for the same declaration.
Implementation
indexable_urisinto a publicConfiguration#excluded?(path), so there is one definition of "excluded" rather than a check inlined in the collection loop.indexable_uriscall, and invalidate that memo when either the patterns (apply_config) or the workspace path change.@bundle_pathmoves to the initializer for the same reason.excluded?deliberately only accounts for the exclusion patterns. A file that is merely not covered by the inclusion patterns is not reported as excluded, which keeps the semantics of the didOpen check the same as the semantics ofindexable_uris.Automated Tests
Configuration#excluded?: relative patterns, absolute patterns,nilpaths, and memo invalidation whenworkspace_pathchanges.Server: opening a document that matchesexcludedPatternsand then firing the document symbol request the editor sends on open leaves the index empty. Verified that this test fails without theserver.rbchange and passes with it.bundle exec rake test(18,542 runs, 0 failures, 0 errors),bundle exec rake test:indexer(322 runs, 0 failures),bundle exec rubocopandbundle exec srb tcare all clean locally.Manual verification
Reproduced against a fixture with the same shape as the workspace this came from — a folder rooted at the repo root with
excludedPatterns: ["apps/**/*"], containingapps/billing/app/models/{application_record,invoice}.rb— driving the server throughdidOpen, thedocumentSymbolrequest the editor sends on open, and thentextDocument/definitionon the superclass:ApplicationRecordmainThe location the root server returned on
mainis identical to the one the nestedbillingserver returns, which is what the editor renders as two entries. Note that reproducing this requires the file declaring the constant to be open, since that is what gets force-indexed — opening only the referencing file is not enough.I would still be happy to be redirected if you would rather fix this on the extension side by narrowing each client's document selector so that the closest workspace folder owns a file. That would also cover multi-root users who have not configured
excludedPatternsat all, which this change does not.