From d03faf88248de65787498a8aabf0d1b8a452df35 Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Fri, 29 Nov 2024 15:37:45 -0500 Subject: [PATCH] Index documents on modification --- jekyll/index.markdown | 4 - lib/ruby_indexer/lib/ruby_indexer/index.rb | 31 +++-- lib/ruby_indexer/test/index_test.rb | 3 +- lib/ruby_lsp/server.rb | 9 +- test/server_test.rb | 130 +++++++++++++++++++++ 5 files changed, 161 insertions(+), 16 deletions(-) diff --git a/jekyll/index.markdown b/jekyll/index.markdown index 33157d793..f81379931 100644 --- a/jekyll/index.markdown +++ b/jekyll/index.markdown @@ -158,10 +158,6 @@ Sorry, your browser doesn't support embedded videos. This video demonstrates the ### Completion -{: .note } -There is currently a technical limitation that [code is only indexed when saved](https://github.com/Shopify/ruby-lsp/issues/1908). -This means if you add, for example, a new method to its called, its completion will not be available until you save that file. - The completion feature provides users with completion candidates when the text they type matches certain indexed components. This helps speed up coding by reducing the need to type out full method names or constants. It also allows developers to discover constants or methods that are available to them. diff --git a/lib/ruby_indexer/lib/ruby_indexer/index.rb b/lib/ruby_indexer/lib/ruby_indexer/index.rb index fbfa578c8..d7e850181 100644 --- a/lib/ruby_indexer/lib/ruby_indexer/index.rb +++ b/lib/ruby_indexer/lib/ruby_indexer/index.rb @@ -48,6 +48,8 @@ def initialize ) @configuration = T.let(RubyIndexer::Configuration.new, Configuration) + + @initial_indexing_completed = T.let(false, T::Boolean) end # Register an included `hook` that will be executed when `module_name` is included into any namespace @@ -56,8 +58,8 @@ def register_included_hook(module_name, &hook) (@included_hooks[module_name] ||= []) << hook end - sig { params(uri: URI::Generic).void } - def delete(uri) + sig { params(uri: URI::Generic, skip_require_paths_tree: T::Boolean).void } + def delete(uri, skip_require_paths_tree: false) key = uri.to_s # For each constant discovered in `path`, delete the associated entry from the index. If there are no entries # left, delete the constant from the index. @@ -80,6 +82,7 @@ def delete(uri) end @uris_to_entries.delete(key) + return if skip_require_paths_tree require_path = uri.require_path @require_paths_tree.delete(require_path) if require_path @@ -357,12 +360,13 @@ def index_all(uris: @configuration.indexable_uris, &block) # When troubleshooting an indexing issue, e.g. through irb, it's not obvious that `index_all` will augment the # existing index values, meaning it may contain 'stale' entries. This check ensures that the user is aware of this # behavior and can take appropriate action. - # binding.break - if @entries.any? + if @initial_indexing_completed raise IndexNotEmptyError, "The index is not empty. To prevent invalid entries, `index_all` can only be called once." end + @initial_indexing_completed = true + RBSIndexer.new(self).index_ruby_core # Calculate how many paths are worth 1% of progress progress_step = (uris.length / 100.0).ceil @@ -618,17 +622,24 @@ def class_variable_completion_candidates(name, owner_name) end # Synchronizes a change made to the given URI. This method will ensure that new declarations are indexed, removed - # declarations removed and that the ancestor linearization cache is cleared if necessary - sig { params(uri: URI::Generic, source: String).void } - def handle_change(uri, source) + # declarations removed and that the ancestor linearization cache is cleared if necessary. If a block is passed, the + # consumer of this API has to handle deleting and inserting/updating entries in the index instead of passing the + # document's source (used to handle unsaved changes to files) + sig do + params(uri: URI::Generic, source: T.nilable(String), block: T.nilable(T.proc.params(index: Index).void)).void + end + def handle_change(uri, source = nil, &block) key = uri.to_s original_entries = @uris_to_entries[key] - delete(uri) - index_single(uri, source) + if block + block.call(self) + else + delete(uri) + index_single(uri, T.must(source)) + end updated_entries = @uris_to_entries[key] - return unless original_entries && updated_entries # A change in one ancestor may impact several different others, which could be including that ancestor through diff --git a/lib/ruby_indexer/test/index_test.rb b/lib/ruby_indexer/test/index_test.rb index 80a5a8dc4..c79c6d2ec 100644 --- a/lib/ruby_indexer/test/index_test.rb +++ b/lib/ruby_indexer/test/index_test.rb @@ -2061,7 +2061,8 @@ def test_build_non_redundant_name end def test_prevents_multiple_calls_to_index_all - # For this test class, `index_all` is already called once in `setup`. + @index.index_all + assert_raises(Index::IndexNotEmptyError) do @index.index_all end diff --git a/lib/ruby_lsp/server.rb b/lib/ruby_lsp/server.rb index 362cf6f0c..d2143952e 100644 --- a/lib/ruby_lsp/server.rb +++ b/lib/ruby_lsp/server.rb @@ -463,7 +463,14 @@ def run_combined_requests(message) document_link = Requests::DocumentLink.new(uri, parse_result.comments, dispatcher) code_lens = Requests::CodeLens.new(@global_state, uri, dispatcher) inlay_hint = Requests::InlayHints.new(document, T.must(@store.features_configuration.dig(:inlayHint)), dispatcher) - dispatcher.dispatch(parse_result.value) + + # Re-index the file as it is modified. This mode of indexing updates entries only. Require path trees are only + # updated on save + @global_state.index.handle_change(uri) do |index| + index.delete(uri, skip_require_paths_tree: true) + RubyIndexer::DeclarationListener.new(index, dispatcher, parse_result, uri, collect_comments: true) + dispatcher.dispatch(parse_result.value) + end # Store all responses retrieve in this round of visits in the cache and then return the response for the request # we actually received diff --git a/test/server_test.rb b/test/server_test.rb index fae9bef5a..735ace549 100644 --- a/test/server_test.rb +++ b/test/server_test.rb @@ -796,6 +796,136 @@ def test_cancelling_requests_returns_expected_error_code assert_equal("Request 1 was cancelled", error.message) end + def test_unsaved_changes_are_indexed_when_computing_automatic_features + uri = URI("file:///foo.rb") + index = @server.global_state.index + + # Simulate opening a file. First, send the notification to open the file with a class inside + @server.process_message({ + method: "textDocument/didOpen", + params: { + textDocument: { + uri: uri, + text: +"class Foo\nend", + version: 1, + languageId: "ruby", + }, + }, + }) + # Fire the automatic features requests to trigger indexing + @server.process_message({ + id: 1, + method: "textDocument/documentSymbol", + params: { textDocument: { uri: uri } }, + }) + + entries = index["Foo"] + assert_equal(1, entries.length) + + # Modify the file without saving + @server.process_message({ + method: "textDocument/didChange", + params: { + textDocument: { uri: uri, version: 2 }, + contentChanges: [ + { text: " def bar\n end\n", range: { start: { line: 1, character: 0 }, end: { line: 1, character: 0 } } }, + ], + }, + }) + + # Parse the document after it was modified. This occurs automatically when we receive a text document request, to + # avoid parsing the document multiple times, but that depends on request coming in through the STDIN pipe, which + # isn't reproduced here. Parsing manually matches what happens normally + store = @server.instance_variable_get(:@store) + store.get(uri).parse! + + # Trigger the automatic features again + @server.process_message({ + id: 2, + method: "textDocument/documentSymbol", + params: { textDocument: { uri: uri } }, + }) + + # There should still only be one entry for each declaration, but we should have picked up the new ones + entries = index["Foo"] + assert_equal(1, entries.length) + + entries = index["bar"] + assert_equal(1, entries.length) + end + + def test_ancestors_are_recomputed_even_on_unsaved_changes + uri = URI("file:///foo.rb") + index = @server.global_state.index + source = +<<~RUBY + module Bar; end + + class Foo + extend Bar + end + RUBY + + # Simulate opening a file. First, send the notification to open the file with a class inside + @server.process_message({ + method: "textDocument/didOpen", + params: { + textDocument: { + uri: uri, + text: source, + version: 1, + languageId: "ruby", + }, + }, + }) + # Fire the automatic features requests to trigger indexing + @server.process_message({ + id: 1, + method: "textDocument/documentSymbol", + params: { textDocument: { uri: uri } }, + }) + + assert_equal(["Foo::", "Bar"], index.linearized_ancestors_of("Foo::")) + + # Delete the extend + @server.process_message({ + method: "textDocument/didChange", + params: { + textDocument: { uri: uri, version: 2 }, + contentChanges: [ + { text: "", range: { start: { line: 3, character: 0 }, end: { line: 3, character: 12 } } }, + ], + }, + }) + + # Parse the document after it was modified. This occurs automatically when we receive a text document request, to + # avoid parsing the document multiple times, but that depends on request coming in through the STDIN pipe, which + # isn't reproduced here. Parsing manually matches what happens normally + store = @server.instance_variable_get(:@store) + document = store.get(uri) + + assert_equal(<<~RUBY, document.source) + module Bar; end + + class Foo + + end + RUBY + + document.parse! + + # Trigger the automatic features again + @server.process_message({ + id: 2, + method: "textDocument/documentSymbol", + params: { textDocument: { uri: uri } }, + }) + + result = find_message(RubyLsp::Result, id: 2) + refute_nil(result) + + assert_equal(["Foo::"], index.linearized_ancestors_of("Foo::")) + end + private def with_uninstalled_rubocop(&block)