Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only reindex documents if declarations are being modified #2942

Open
wants to merge 1 commit into
base: 11-29-index_documents_on_modification
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This event was duplicated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we change Prism detect this and emit an error/warning?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather make our cop smarter if we can avoid paying the price on every single listener instantiation.

:on_constant_write_node_enter,
:on_constant_or_write_node_enter,
:on_constant_and_write_node_enter,
Expand Down
36 changes: 36 additions & 0 deletions lib/ruby_lsp/document.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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

Expand Down Expand Up @@ -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
Expand All @@ -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

Expand Down
68 changes: 68 additions & 0 deletions lib/ruby_lsp/ruby_document.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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
14 changes: 9 additions & 5 deletions lib/ruby_lsp/server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
67 changes: 67 additions & 0 deletions test/ruby_document_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
55 changes: 55 additions & 0 deletions test/server_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -919,6 +919,61 @@ class Foo
assert_equal(["Foo::<Class:Foo>"], index.linearized_ancestors_of("Foo::<Class: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)
Expand Down
Loading