From f299a65f16b66ce43e060b6095bc2f73149d39c7 Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Fri, 29 Nov 2024 16:24:48 -0500 Subject: [PATCH] Only reindex documents if declarations are being modified --- .../lib/ruby_indexer/declaration_listener.rb | 1 - lib/ruby_lsp/document.rb | 36 ++++++++++ lib/ruby_lsp/ruby_document.rb | 68 +++++++++++++++++++ lib/ruby_lsp/server.rb | 14 ++-- test/ruby_document_test.rb | 67 ++++++++++++++++++ test/server_test.rb | 55 +++++++++++++++ 6 files changed, 235 insertions(+), 6 deletions(-) diff --git a/lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb b/lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb index 0295e4cf2..271ee4455 100644 --- a/lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb +++ b/lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb @@ -64,7 +64,6 @@ def initialize(index, dispatcher, parse_result, uri, collect_comments: false) :on_constant_path_or_write_node_enter, :on_constant_path_operator_write_node_enter, :on_constant_path_and_write_node_enter, - :on_constant_or_write_node_enter, :on_constant_write_node_enter, :on_constant_or_write_node_enter, :on_constant_and_write_node_enter, diff --git a/lib/ruby_lsp/document.rb b/lib/ruby_lsp/document.rb index 62b35b516..bed863271 100644 --- a/lib/ruby_lsp/document.rb +++ b/lib/ruby_lsp/document.rb @@ -40,6 +40,9 @@ class LocationNotFoundError < StandardError; end sig { returns(Encoding) } attr_reader :encoding + sig { returns(T.nilable(Edit)) } + attr_reader :last_edit + sig { returns(T.any(Interface::SemanticTokens, Object)) } attr_accessor :semantic_tokens @@ -53,6 +56,7 @@ def initialize(source:, version:, uri:, encoding: Encoding::UTF_8) @uri = T.let(uri, URI::Generic) @needs_parsing = T.let(true, T::Boolean) @parse_result = T.let(T.unsafe(nil), ParseResultType) + @last_edit = T.let(nil, T.nilable(Edit)) parse! end @@ -106,6 +110,19 @@ def push_edits(edits, version:) @version = version @needs_parsing = true @cache.clear + + last_edit = edits.last + return unless last_edit + + last_edit_range = last_edit[:range] + + @last_edit = if last_edit_range[:start] == last_edit_range[:end] + Insert.new(last_edit_range) + elsif last_edit[:text].empty? + Delete.new(last_edit_range) + else + Replace.new(last_edit_range) + end end # Returns `true` if the document was parsed and `false` if nothing needed parsing @@ -125,6 +142,25 @@ def past_expensive_limit? @source.length > MAXIMUM_CHARACTERS_FOR_EXPENSIVE_FEATURES end + class Edit + extend T::Sig + extend T::Helpers + + abstract! + + sig { returns(T::Hash[Symbol, T.untyped]) } + attr_reader :range + + sig { params(range: T::Hash[Symbol, T.untyped]).void } + def initialize(range) + @range = range + end + end + + class Insert < Edit; end + class Replace < Edit; end + class Delete < Edit; end + class Scanner extend T::Sig diff --git a/lib/ruby_lsp/ruby_document.rb b/lib/ruby_lsp/ruby_document.rb index 4cde7e656..28b9c523e 100644 --- a/lib/ruby_lsp/ruby_document.rb +++ b/lib/ruby_lsp/ruby_document.rb @@ -8,6 +8,22 @@ class RubyDocument < Document ParseResultType = type_member { { fixed: Prism::ParseResult } } + METHODS_THAT_CHANGE_DECLARATIONS = [ + :private_constant, + :attr_reader, + :attr_writer, + :attr_accessor, + :alias_method, + :include, + :prepend, + :extend, + :public, + :protected, + :private, + :module_function, + :private_class_method, + ].freeze + class SorbetLevel < T::Enum enums do None = new("none") @@ -239,5 +255,57 @@ def locate_node(position, node_types: []) node_types: node_types, ) end + + sig { returns(T::Boolean) } + def last_edit_may_change_declarations? + # This method controls when we should index documents. If there's no recent edit and the document has just been + # opened, we need to index it + return true unless @last_edit + + case @last_edit + when Delete + # Not optimized yet. It's not trivial to identify that a declaration has been removed since the source is no + # longer there and we don't remember the deleted text + true + when Insert, Replace + position_may_impact_declarations?(@last_edit.range[:start]) + else + false + end + end + + private + + sig { params(position: T::Hash[Symbol, Integer]).returns(T::Boolean) } + def position_may_impact_declarations?(position) + node_context = locate_node(position) + node_at_edit = node_context.node + + # Adjust to the parent when editing the constant of a class/module declaration + if node_at_edit.is_a?(Prism::ConstantReadNode) || node_at_edit.is_a?(Prism::ConstantPathNode) + node_at_edit = node_context.parent + end + + case node_at_edit + when Prism::ClassNode, Prism::ModuleNode, Prism::SingletonClassNode, Prism::DefNode, + Prism::ConstantPathWriteNode, Prism::ConstantPathOrWriteNode, Prism::ConstantPathOperatorWriteNode, + Prism::ConstantPathAndWriteNode, Prism::ConstantOrWriteNode, Prism::ConstantWriteNode, + Prism::ConstantAndWriteNode, Prism::ConstantOperatorWriteNode, Prism::GlobalVariableAndWriteNode, + Prism::GlobalVariableOperatorWriteNode, Prism::GlobalVariableOrWriteNode, Prism::GlobalVariableTargetNode, + Prism::GlobalVariableWriteNode, Prism::InstanceVariableWriteNode, Prism::InstanceVariableAndWriteNode, + Prism::InstanceVariableOperatorWriteNode, Prism::InstanceVariableOrWriteNode, + Prism::InstanceVariableTargetNode, Prism::AliasMethodNode + true + when Prism::MultiWriteNode + [*node_at_edit.lefts, *node_at_edit.rest, *node_at_edit.rights].any? do |node| + node.is_a?(Prism::ConstantTargetNode) || node.is_a?(Prism::ConstantPathTargetNode) + end + when Prism::CallNode + receiver = node_at_edit.receiver + (!receiver || receiver.is_a?(Prism::SelfNode)) && METHODS_THAT_CHANGE_DECLARATIONS.include?(node_at_edit.name) + else + false + end + end end end diff --git a/lib/ruby_lsp/server.rb b/lib/ruby_lsp/server.rb index 7ac3be070..199099ab3 100644 --- a/lib/ruby_lsp/server.rb +++ b/lib/ruby_lsp/server.rb @@ -494,11 +494,15 @@ def run_combined_requests(message) code_lens = Requests::CodeLens.new(@global_state, uri, dispatcher) inlay_hint = Requests::InlayHints.new(document, T.must(@store.features_configuration.dig(:inlayHint)), dispatcher) - # 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) + if document.is_a?(RubyDocument) && document.last_edit_may_change_declarations? + # 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 + else dispatcher.dispatch(parse_result.value) end diff --git a/test/ruby_document_test.rb b/test/ruby_document_test.rb index edbb1e910..5a632957f 100644 --- a/test/ruby_document_test.rb +++ b/test/ruby_document_test.rb @@ -793,6 +793,73 @@ class Foo MESSAGE end + def test_document_tracks_latest_edit_context + document = RubyLsp::RubyDocument.new(source: +<<~RUBY, version: 1, uri: URI("file:///foo/bar.rb")) + class Foo + + end + RUBY + + # Insert + range = { start: { line: 1, character: 0 }, end: { line: 1, character: 0 } } + document.push_edits([{ range: range, text: "d" }], version: 2) + + last_edit = T.must(document.last_edit) + assert_instance_of(RubyLsp::Document::Insert, last_edit) + assert_equal(range, last_edit.range) + + # Replace + range = { start: { line: 1, character: 0 }, end: { line: 1, character: 1 } } + document.push_edits([{ range: range, text: "def" }], version: 3) + + last_edit = T.must(document.last_edit) + assert_instance_of(RubyLsp::Document::Replace, last_edit) + assert_equal(range, last_edit.range) + + # Delete + range = { start: { line: 1, character: 0 }, end: { line: 1, character: 3 } } + document.push_edits([{ range: range, text: "" }], version: 3) + + last_edit = T.must(document.last_edit) + assert_instance_of(RubyLsp::Document::Delete, last_edit) + assert_equal(range, last_edit.range) + + assert_equal(<<~RUBY, document.source) + class Foo + + end + RUBY + end + + def test_last_edit_may_change_declarations_for_inserts + document = RubyLsp::RubyDocument.new(source: +<<~RUBY, version: 1, uri: URI("file:///foo/bar.rb")) + class Foo + end + RUBY + assert_predicate(document, :last_edit_may_change_declarations?) + + range = { start: { line: 0, character: 9 }, end: { line: 0, character: 9 } } + document.push_edits([{ range: range, text: "t" }], version: 2) + + assert_instance_of(RubyLsp::Document::Insert, document.last_edit) + assert_predicate(document, :last_edit_may_change_declarations?) + end + + def test_last_edit_may_change_declarations_for_replaces + document = RubyLsp::RubyDocument.new(source: +<<~RUBY, version: 1, uri: URI("file:///foo/bar.rb")) + class Foo + end + RUBY + + assert_predicate(document, :last_edit_may_change_declarations?) + + range = { start: { line: 0, character: 6 }, end: { line: 0, character: 9 } } + document.push_edits([{ range: range, text: "Bar" }], version: 2) + + assert_instance_of(RubyLsp::Document::Replace, document.last_edit) + assert_predicate(document, :last_edit_may_change_declarations?) + end + private def assert_error_edit(actual, error_range) diff --git a/test/server_test.rb b/test/server_test.rb index 3d65cf3df..f1ec50c23 100644 --- a/test/server_test.rb +++ b/test/server_test.rb @@ -919,6 +919,61 @@ class Foo assert_equal(["Foo::"], index.linearized_ancestors_of("Foo::")) end + def test_edits_outside_of_declarations_do_not_trigger_indexing + 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\n\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: "d", 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 + index.expects(:delete).never + @server.process_message({ + id: 2, + method: "textDocument/documentSymbol", + params: { textDocument: { uri: uri } }, + }) + + entries = index["Foo"] + assert_equal(1, entries.length) + end + private def with_uninstalled_rubocop(&block)