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

Index files as they are modified #2342

Closed
Closed
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
21 changes: 14 additions & 7 deletions lib/ruby_indexer/lib/ruby_indexer/index.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

@st0012 st0012 Jul 23, 2024

Choose a reason for hiding this comment

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

Are there cases where the uri would be different than indexable.to_uri? If there are, can we capture them in test cases?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also perhaps we can swap the arguments order, and have uri default to indexable.uri?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think we need to change IndexablePath to be simply an Indexable and then include the URI there. I was just trying to avoid parsing the same URI twice, but the code is ugly.

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
Expand Down
6 changes: 3 additions & 3 deletions lib/ruby_indexer/test/index_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
29 changes: 25 additions & 4 deletions lib/ruby_lsp/server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 }
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
7 changes: 7 additions & 0 deletions lib/ruby_lsp/store.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
85 changes: 85 additions & 0 deletions test/server_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
Loading