diff --git a/lib/ruby_lsp/base_server.rb b/lib/ruby_lsp/base_server.rb index 2a5a1c971..14e630d71 100644 --- a/lib/ruby_lsp/base_server.rb +++ b/lib/ruby_lsp/base_server.rb @@ -39,46 +39,13 @@ def start @reader.each_message do |message| method = message[:method] - # We must parse the document under a mutex lock or else we might switch threads and accept text edits in the - # source. Altering the source reference during parsing will put the parser in an invalid internal state, since - # it started parsing with one source but then it changed in the middle. We don't want to do this for text - # synchronization notifications - @global_state.synchronize do - uri = message.dig(:params, :textDocument, :uri) - - if uri - begin - parsed_uri = URI(uri) - message[:params][:textDocument][:uri] = parsed_uri - - # We don't want to try to parse documents on text synchronization notifications - unless method.start_with?("textDocument/did") - document = @store.get(parsed_uri) - - # If the client supports request delegation and we're working with an ERB document and there was - # something to parse, then we have to maintain the client updated about the virtual state of the host - # language source - if document.parse! && @global_state.client_capabilities.supports_request_delegation && - document.is_a?(ERBDocument) - - send_message( - Notification.new( - method: "delegate/textDocument/virtualState", - params: { - textDocument: { - uri: uri, - text: document.host_language_source, - }, - }, - ), - ) - end - end - rescue URI::Error - # A client can send a document URI with a scheme Ruby's URI parser rejects, so we don't want to fail - rescue Store::NonExistingDocumentError - # If we receive a request for a file that no longer exists, we don't want to fail - end + uri = message.dig(:params, :textDocument, :uri) + + if uri + begin + message[:params][:textDocument][:uri] = URI(uri) + rescue URI::Error + # A client can send a document URI with a scheme Ruby's URI parser rejects, so we don't want to fail end end @@ -178,11 +145,48 @@ def handle_incoming_message(message) end end + parse_document(message) process_message(message) # Ensure we remove the request if it got cancelled while it was being processed @cancelled_requests.delete(id) end + # Parse the document targeted by a request right before processing it. This must happen in the worker thread, after + # any text synchronization notifications that were queued ahead of the request have been applied. Parsing when the + # message is read would use the source from before those edits, and the request would then compute (and cache) its + # response from an outdated parse result + #: (Hash[Symbol, untyped] message) -> void + def parse_document(message) + method = message[:method] + uri = message.dig(:params, :textDocument, :uri) + return if method.nil? || method.start_with?("textDocument/did") || !uri.is_a?(URI::Generic) + + @global_state.synchronize do + document = @store.get(uri) + + # If the client supports request delegation and we're working with an ERB document and there was + # something to parse, then we have to maintain the client updated about the virtual state of the host + # language source + if document.parse! && @global_state.client_capabilities.supports_request_delegation && + document.is_a?(ERBDocument) + + send_message( + Notification.new( + method: "delegate/textDocument/virtualState", + params: { + textDocument: { + uri: uri.to_s, + text: document.host_language_source, + }, + }, + ), + ) + end + rescue Store::NonExistingDocumentError + # If we receive a request for a file that no longer exists, we don't want to fail + end + end + #: ((Result | Error | Notification | Request) message) -> void def send_message(message) # When we're shutting down the server, there's a small race condition between closing the thread queues and diff --git a/test/server_test.rb b/test/server_test.rb index 1301c8b59..fe5191636 100644 --- a/test/server_test.rb +++ b/test/server_test.rb @@ -1779,6 +1779,43 @@ def test_does_not_crash_when_a_document_uri_cannot_be_parsed server&.run_shutdown end + def test_requests_parse_the_document_after_queued_edits_are_applied + # Text synchronization notifications are processed in the worker thread. A request read right after an edit must + # not be computed (and cached) from a parse result that predates that edit + uri = URI::Generic.from_path(path: File.join(Dir.pwd, "fake.rb")).to_s + messages = [ + { + method: "textDocument/didOpen", + params: { textDocument: { uri: uri, text: "def foo", version: 1, languageId: "ruby" } }, + }, + { + method: "textDocument/didChange", + params: { + textDocument: { uri: uri, version: 2 }, + contentChanges: [ + { range: { start: { line: 0, character: 7 }, end: { line: 0, character: 7 } }, text: "\nend" }, + ], + }, + }, + { id: 1, method: "textDocument/diagnostic", params: { textDocument: { uri: uri } } }, + ] + input = messages.map do |message| + body = message.to_json + "Content-Length: #{body.bytesize}\r\n\r\n#{body}" + end.join + + @server.run_shutdown + @server = RubyLsp::Server.new(test_mode: true, reader: StringIO.new(input), writer: StringIO.new) + @server.global_state.stubs(:active_linters).returns([]) + + capture_subprocess_io { @server.start } + + result = find_message(RubyLsp::Result, id: 1) + items = result.response #: as RubyLsp::Interface::FullDocumentDiagnosticReport + .items + assert_empty(items.map(&:message)) + end + private def run_initialize_server_with_setup_error(error)