From d619b2c8b13183f0981dbac812c67c5ad14af145 Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Mon, 22 Jul 2024 16:37:32 -0400 Subject: [PATCH] Index files as they are modified --- lib/ruby_indexer/lib/ruby_indexer/index.rb | 21 ++++-- lib/ruby_indexer/test/index_test.rb | 6 +- lib/ruby_lsp/server.rb | 29 +++++++- lib/ruby_lsp/store.rb | 7 ++ test/server_test.rb | 85 ++++++++++++++++++++++ 5 files changed, 134 insertions(+), 14 deletions(-) diff --git a/lib/ruby_indexer/lib/ruby_indexer/index.rb b/lib/ruby_indexer/lib/ruby_indexer/index.rb index 0baa8587e..f7ae24f50 100644 --- a/lib/ruby_indexer/lib/ruby_indexer/index.rb +++ b/lib/ruby_indexer/lib/ruby_indexer/index.rb @@ -469,16 +469,23 @@ def instance_variable_completion_candidates(name, owner_name) # Synchronizes a change made to the given indexable path. This method will ensure that new declarations are indexed, # removed declarations removed and that the ancestor linearization cache is cleared if necessary - sig { params(indexable: IndexablePath).void } - def handle_change(indexable) - uri = indexable.to_uri.to_s - original_entries = @uris_to_entries[uri] + sig { params(uri: URI::Generic, indexable: IndexablePath).void } + def handle_change(uri, indexable) + handling_ancestor_change(uri) do + delete(indexable) + index_single(indexable) + end + end - delete(indexable) - index_single(indexable) + # Handle ancestor changes that may occur when invoking the given block. The goal of this method is to automatically + # synchronize ancestors when handling changes in the index + sig { params(indexable_or_uri: T.any(IndexablePath, URI::Generic), block: T.proc.void).void } + def handling_ancestor_change(indexable_or_uri, &block) + uri = (indexable_or_uri.is_a?(IndexablePath) ? indexable_or_uri.to_uri : indexable_or_uri).to_s + original_entries = @uris_to_entries[uri] + block.call updated_entries = @uris_to_entries[uri] - 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 c8e524a11..b65f11152 100644 --- a/lib/ruby_indexer/test/index_test.rb +++ b/lib/ruby_indexer/test/index_test.rb @@ -796,7 +796,7 @@ class Bar end RUBY - @index.handle_change(indexable_path) + @index.handle_change(indexable_path.to_uri, indexable_path) assert_empty(@index.instance_variable_get(:@ancestors)) assert_equal(["Bar", "Object", "Kernel", "BasicObject"], @index.linearized_ancestors_of("Bar")) end @@ -833,7 +833,7 @@ def baz; end end RUBY - @index.handle_change(indexable_path) + @index.handle_change(indexable_path.to_uri, indexable_path) refute_empty(@index.instance_variable_get(:@ancestors)) assert_equal(["Bar", "Foo", "Object", "Kernel", "BasicObject"], @index.linearized_ancestors_of("Bar")) end @@ -866,7 +866,7 @@ class Bar end RUBY - @index.handle_change(indexable_path) + @index.handle_change(indexable_path.to_uri, indexable_path) assert_empty(@index.instance_variable_get(:@ancestors)) assert_equal(["Bar", "Object", "Kernel", "BasicObject"], @index.linearized_ancestors_of("Bar")) end diff --git a/lib/ruby_lsp/server.rb b/lib/ruby_lsp/server.rb index 051fa9aff..ede0f62d0 100644 --- a/lib/ruby_lsp/server.rb +++ b/lib/ruby_lsp/server.rb @@ -297,8 +297,9 @@ def text_document_did_open(message) sig { params(message: T::Hash[Symbol, T.untyped]).void } def text_document_did_close(message) + uri = message.dig(:params, :textDocument, :uri) + @mutex.synchronize do - uri = message.dig(:params, :textDocument, :uri) @store.delete(uri) # Clear diagnostics for the closed file, so that they no longer appear in the problems tab @@ -309,6 +310,12 @@ def text_document_did_close(message) ), ) end + + # When we receive a didClose notification for an unsaved (which happens when the user saves the file or when it's + # simply closed), we need to remove declarations related to it from the index because immediately after we will + # receive a didChangeWatchedFiles notification related to the saved file creation. Otherwise, we end up with + # duplicate entries + @global_state.index.delete(uri) if uri.scheme == "untitled" end sig { params(message: T::Hash[Symbol, T.untyped]).void } @@ -362,9 +369,21 @@ def run_combined_requests(message) document_symbol = Requests::DocumentSymbol.new(uri, dispatcher) document_link = Requests::DocumentLink.new(uri, parse_result.comments, dispatcher) code_lens = Requests::CodeLens.new(@global_state, uri, dispatcher) - semantic_highlighting = Requests::SemanticHighlighting.new(@global_state, dispatcher) - dispatcher.dispatch(parse_result.value) + + # If we're running the combined requests for a Ruby document and that document's URI is either `file` or + # `untitled` (a save vs an unsaved Ruby file), then we run indexing at the same time to capture new declarations. + # The reason we have to check for the scheme is because, if we were to accidentally index a URI for a `git` scheme + # for example, we would end up with duplicate entries in the index + if document.is_a?(RubyDocument) && (uri.scheme == "file" || uri.scheme == "untitled") + @global_state.index.handling_ancestor_change(uri) do + @global_state.index.delete(uri) + RubyIndexer::DeclarationListener.new(@global_state.index, dispatcher, parse_result, uri) + dispatcher.dispatch(parse_result.value) + end + else + 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 @@ -668,7 +687,9 @@ def workspace_did_change_watched_files(message) when Constant::FileChangeType::CREATED index.index_single(indexable) when Constant::FileChangeType::CHANGED - index.handle_change(indexable) + # We only want to handle changes if the files being modified are not currently opened in the editor. For files + # that are open, the indexing happens when they are modified as part of running the combined requests + index.handle_change(uri, indexable) unless @store.has?(uri) when Constant::FileChangeType::DELETED index.delete(indexable) end diff --git a/lib/ruby_lsp/store.rb b/lib/ruby_lsp/store.rb index 8aebcdcc8..67ce5eab2 100644 --- a/lib/ruby_lsp/store.rb +++ b/lib/ruby_lsp/store.rb @@ -95,6 +95,13 @@ def delete(uri) @state.delete(uri.to_s) end + # Returns `true` if the store contains the document with the given URI, which means that document is being managed + # by the client + sig { params(uri: URI::Generic).returns(T::Boolean) } + def has?(uri) + @state.key?(uri.to_s) + end + sig do type_parameters(:T) .params( diff --git a/test/server_test.rb b/test/server_test.rb index 1c64fff32..6ae9f3ce9 100644 --- a/test/server_test.rb +++ b/test/server_test.rb @@ -428,6 +428,23 @@ def test_changed_file_only_indexes_ruby }) end + def test_handles_file_modifications_for_documents_not_managed_by_client + @server.global_state.index.expects(:index_single).once.with do |indexable| + indexable.full_path == "/foo.rb" + end + @server.process_message({ + method: "workspace/didChangeWatchedFiles", + params: { + changes: [ + { + uri: URI("file:///foo.rb"), + type: RubyLsp::Constant::FileChangeType::CHANGED, + }, + ], + }, + }) + end + def test_workspace_addons create_test_addons @server.load_addons @@ -544,6 +561,74 @@ def test_closing_document_before_computing_features_does_not_error end end + def test_indexing_on_file_changes + uri = URI("untitled:Untitled-1") + index = @server.global_state.index + + capture_io do + @server.process_message({ + method: "textDocument/didOpen", + params: { + textDocument: { + uri: uri, + text: "class Foo\nend", + version: 1, + languageId: "ruby", + }, + }, + }) + + # Initial indexing for an unsaved file + @server.process_message({ + id: 1, + method: "textDocument/documentSymbol", + params: { + textDocument: { + uri: uri, + }, + }, + }) + + assert_equal(1, index["Foo"].length) + + # Changing the file should re-index, but not create duplicates + @server.process_message({ + id: 1, + method: "textDocument/didChange", + params: { + textDocument: { + uri: uri, + version: 2, + }, + contentChanges: [{ range: { start: { line: 0, character: 9 }, end: { line: 0, character: 9 } }, text: "\n" }], + }, + }) + + @server.process_message({ + id: 1, + method: "textDocument/documentSymbol", + params: { + textDocument: { + uri: uri, + }, + }, + }) + + assert_equal(1, index["Foo"].length) + + # Closing the file should clear the related entries + @server.process_message({ + method: "textDocument/didClose", + params: { + textDocument: { + uri: uri, + }, + }, + }) + assert_nil(index["Foo"]) + end + end + private def with_uninstalled_rubocop(&block)