diff --git a/lib/ruby_indexer/lib/ruby_indexer/index.rb b/lib/ruby_indexer/lib/ruby_indexer/index.rb index 9ce885555..875041cf3 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_string - 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_string : 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_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)