Skip to content

Commit

Permalink
Locate targets under mutex lock
Browse files Browse the repository at this point in the history
  • Loading branch information
vinistock committed Dec 16, 2024
1 parent fface59 commit 6980656
Show file tree
Hide file tree
Showing 14 changed files with 86 additions and 46 deletions.
13 changes: 6 additions & 7 deletions lib/ruby_lsp/base_server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,21 @@ def initialize(**options)
@incoming_queue = T.let(Thread::Queue.new, Thread::Queue)
@outgoing_queue = T.let(Thread::Queue.new, Thread::Queue)
@cancelled_requests = T.let([], T::Array[Integer])
@mutex = T.let(Mutex.new, Mutex)
@worker = T.let(new_worker, Thread)
@current_request_id = T.let(1, Integer)
@store = T.let(Store.new, Store)
@global_state = T.let(GlobalState.new, GlobalState)
@outgoing_dispatcher = T.let(
Thread.new do
unless @test_mode
while (message = @outgoing_queue.pop)
@mutex.synchronize { @writer.write(message.to_hash) }
@global_state.synchronize { @writer.write(message.to_hash) }
end
end
end,
Thread,
)

@global_state = T.let(GlobalState.new, GlobalState)
Thread.main.priority = 1

# We read the initialize request in `exe/ruby-lsp` to be able to determine the workspace URI where Bundler should
Expand All @@ -51,7 +50,7 @@ def start
# 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
@mutex.synchronize do
@global_state.synchronize do
uri = message.dig(:params, :textDocument, :uri)

if uri
Expand Down Expand Up @@ -95,14 +94,14 @@ def start
"$/cancelRequest"
process_message(message)
when "shutdown"
@mutex.synchronize do
@global_state.synchronize do
send_log_message("Shutting down Ruby LSP...")
shutdown
run_shutdown
@writer.write(Result.new(id: message[:id], response: nil).to_hash)
end
when "exit"
@mutex.synchronize do
@global_state.synchronize do
status = @incoming_queue.closed? ? 0 : 1
send_log_message("Shutdown complete with status #{status}")
exit(status)
Expand Down Expand Up @@ -157,7 +156,7 @@ def new_worker
id = message[:id]

# Check if the request was cancelled before trying to process it
@mutex.synchronize do
@global_state.synchronize do
if id && @cancelled_requests.include?(id)
send_message(Error.new(
id: id,
Expand Down
6 changes: 6 additions & 0 deletions lib/ruby_lsp/global_state.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,12 @@ def initialize
)
@client_capabilities = T.let(ClientCapabilities.new, ClientCapabilities)
@enabled_feature_flags = T.let({}, T::Hash[Symbol, T::Boolean])
@mutex = T.let(Mutex.new, Mutex)
end

sig { type_parameters(:T).params(block: T.proc.returns(T.type_parameter(:T))).returns(T.type_parameter(:T)) }
def synchronize(&block)
@mutex.synchronize(&block)
end

sig { params(addon_name: String).returns(T.nilable(T::Hash[Symbol, T.untyped])) }
Expand Down
48 changes: 29 additions & 19 deletions lib/ruby_lsp/requests/code_action_resolve.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,12 @@ def switch_block_style
source_range = @code_action.dig(:data, :range)
return Error::EmptySelection if source_range[:start] == source_range[:end]

target = @document.locate_first_within_range(
@code_action.dig(:data, :range),
node_types: [Prism::CallNode],
)
target = @global_state.synchronize do
@document.locate_first_within_range(
@code_action.dig(:data, :range),
node_types: [Prism::CallNode],
)
end

return Error::InvalidTargetRange unless target.is_a?(Prism::CallNode)

Expand Down Expand Up @@ -92,10 +94,13 @@ def refactor_variable
source_range = @code_action.dig(:data, :range)
return Error::EmptySelection if source_range[:start] == source_range[:end]

scanner = @document.create_scanner
start_index = scanner.find_char_position(source_range[:start])
end_index = scanner.find_char_position(source_range[:end])
extracted_source = T.must(@document.source[start_index...end_index])
start_index, extracted_source = @global_state.synchronize do
scanner = @document.create_scanner
start_index = scanner.find_char_position(source_range[:start])
end_index = scanner.find_char_position(source_range[:end])
extracted_source = T.must(@document.source[start_index...end_index])
[start_index, extracted_source]
end

# Find the closest statements node, so that we place the refactor in a valid position
node_context = RubyDocument
Expand Down Expand Up @@ -192,10 +197,13 @@ def refactor_method
source_range = @code_action.dig(:data, :range)
return Error::EmptySelection if source_range[:start] == source_range[:end]

scanner = @document.create_scanner
start_index = scanner.find_char_position(source_range[:start])
end_index = scanner.find_char_position(source_range[:end])
extracted_source = T.must(@document.source[start_index...end_index])
start_index, extracted_source = @global_state.synchronize do
scanner = @document.create_scanner
start_index = scanner.find_char_position(source_range[:start])
end_index = scanner.find_char_position(source_range[:end])
extracted_source = T.must(@document.source[start_index...end_index])
[start_index, extracted_source]
end

# Find the closest method declaration node, so that we place the refactor in a valid position
node_context = RubyDocument.locate(
Expand Down Expand Up @@ -305,13 +313,15 @@ def recursively_switch_nested_block_styles(node, indentation)
def switch_block_body(body, indentation)
# Check if there are any nested blocks inside of the current block
body_loc = body.location
nested_block = @document.locate_first_within_range(
{
start: { line: body_loc.start_line - 1, character: body_loc.start_column },
end: { line: body_loc.end_line - 1, character: body_loc.end_column },
},
node_types: [Prism::BlockNode],
)
nested_block = @global_state.synchronize do
@document.locate_first_within_range(
{
start: { line: body_loc.start_line - 1, character: body_loc.start_column },
end: { line: body_loc.end_line - 1, character: body_loc.end_column },
},
node_types: [Prism::BlockNode],
)
end

body_content = body.slice.dup

Expand Down
4 changes: 3 additions & 1 deletion lib/ruby_lsp/requests/completion.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,9 @@ def initialize(document, global_state, params, sorbet_level, dispatcher)
@dispatcher = dispatcher
# Completion always receives the position immediately after the character that was just typed. Here we adjust it
# back by 1, so that we find the right node
char_position = document.create_scanner.find_char_position(params[:position]) - 1
char_position = global_state.synchronize do
document.create_scanner.find_char_position(params[:position]) - 1
end
delegate_request_if_needed!(global_state, document, char_position)

node_context = RubyDocument.locate(
Expand Down
4 changes: 3 additions & 1 deletion lib/ruby_lsp/requests/definition.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ def initialize(document, global_state, position, dispatcher, sorbet_level)
)
@dispatcher = dispatcher

char_position = document.create_scanner.find_char_position(position)
char_position = global_state.synchronize do
document.create_scanner.find_char_position(position)
end
delegate_request_if_needed!(global_state, document, char_position)

node_context = RubyDocument.locate(
Expand Down
4 changes: 3 additions & 1 deletion lib/ruby_lsp/requests/document_highlight.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ class DocumentHighlight < Request
end
def initialize(global_state, document, position, dispatcher)
super()
char_position = document.create_scanner.find_char_position(position)
char_position = global_state.synchronize do
document.create_scanner.find_char_position(position)
end
delegate_request_if_needed!(global_state, document, char_position)

node_context = RubyDocument.locate(
Expand Down
4 changes: 3 additions & 1 deletion lib/ruby_lsp/requests/hover.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ def provider
def initialize(document, global_state, position, dispatcher, sorbet_level)
super()

char_position = document.create_scanner.find_char_position(position)
char_position = global_state.synchronize do
document.create_scanner.find_char_position(position)
end
delegate_request_if_needed!(global_state, document, char_position)

node_context = RubyDocument.locate(
Expand Down
8 changes: 6 additions & 2 deletions lib/ruby_lsp/requests/prepare_rename.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,23 @@ class PrepareRename < Request

sig do
params(
global_state: GlobalState,
document: RubyDocument,
position: T::Hash[Symbol, T.untyped],
).void
end
def initialize(document, position)
def initialize(global_state, document, position)
super()
@global_state = global_state
@document = document
@position = T.let(position, T::Hash[Symbol, Integer])
end

sig { override.returns(T.nilable(Interface::Range)) }
def perform
char_position = @document.create_scanner.find_char_position(@position)
char_position = @global_state.synchronize do
@document.create_scanner.find_char_position(@position)
end

node_context = RubyDocument.locate(
@document.parse_result.value,
Expand Down
4 changes: 3 additions & 1 deletion lib/ruby_lsp/requests/references.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ def initialize(global_state, store, document, params)
sig { override.returns(T::Array[Interface::Location]) }
def perform
position = @params[:position]
char_position = @document.create_scanner.find_char_position(position)
char_position = @global_state.synchronize do
@document.create_scanner.find_char_position(position)
end

node_context = RubyDocument.locate(
@document.parse_result.value,
Expand Down
4 changes: 3 additions & 1 deletion lib/ruby_lsp/requests/rename.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,9 @@ def initialize(global_state, store, document, params)

sig { override.returns(T.nilable(Interface::WorkspaceEdit)) }
def perform
char_position = @document.create_scanner.find_char_position(@position)
char_position = @global_state.synchronize do
@document.create_scanner.find_char_position(@position)
end

node_context = RubyDocument.locate(
@document.parse_result.value,
Expand Down
14 changes: 9 additions & 5 deletions lib/ruby_lsp/requests/show_syntax_tree.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,12 @@ module Requests
class ShowSyntaxTree < Request
extend T::Sig

sig { params(document: RubyDocument, range: T.nilable(T::Hash[Symbol, T.untyped])).void }
def initialize(document, range)
sig do
params(global_state: GlobalState, document: RubyDocument, range: T.nilable(T::Hash[Symbol, T.untyped])).void
end
def initialize(global_state, document, range)
super()
@global_state = global_state
@document = document
@range = range
@tree = T.let(document.parse_result.value, Prism::ProgramNode)
Expand All @@ -32,9 +35,10 @@ def perform
def ast_for_range
range = T.must(@range)

scanner = @document.create_scanner
start_char = scanner.find_char_position(range[:start])
end_char = scanner.find_char_position(range[:end])
start_char, end_char = @global_state.synchronize do
scanner = @document.create_scanner
[scanner.find_char_position(range[:start]), scanner.find_char_position(range[:end])]
end

queue = @tree.statements.body.dup
found_nodes = []
Expand Down
5 changes: 4 additions & 1 deletion lib/ruby_lsp/requests/signature_help.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,10 @@ def provider
def initialize(document, global_state, position, context, dispatcher, sorbet_level) # rubocop:disable Metrics/ParameterLists
super()

char_position = document.create_scanner.find_char_position(position)
char_position = global_state.synchronize do
document.create_scanner.find_char_position(position)
end

delegate_request_if_needed!(global_state, document, char_position)

node_context = RubyDocument.locate(
Expand Down
11 changes: 6 additions & 5 deletions lib/ruby_lsp/server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ def process_message(message)
),
)
when "$/cancelRequest"
@mutex.synchronize { @cancelled_requests << message[:params][:id] }
@global_state.synchronize { @cancelled_requests << message[:params][:id] }
when nil
process_response(message) if message[:result]
end
Expand Down Expand Up @@ -377,7 +377,7 @@ def run_initialized

sig { params(message: T::Hash[Symbol, T.untyped]).void }
def text_document_did_open(message)
@mutex.synchronize do
@global_state.synchronize do
text_document = message.dig(:params, :textDocument)
language_id = case text_document[:languageId]
when "erb", "eruby"
Expand Down Expand Up @@ -417,7 +417,7 @@ def text_document_did_open(message)

sig { params(message: T::Hash[Symbol, T.untyped]).void }
def text_document_did_close(message)
@mutex.synchronize do
@global_state.synchronize do
uri = message.dig(:params, :textDocument, :uri)
@store.delete(uri)

Expand All @@ -436,7 +436,7 @@ def text_document_did_change(message)
params = message[:params]
text_document = params[:textDocument]

@mutex.synchronize do
@global_state.synchronize do
@store.push_edits(uri: text_document[:uri], edits: params[:contentChanges], version: text_document[:version])
end
end
Expand Down Expand Up @@ -755,7 +755,7 @@ def text_document_prepare_rename(message)
send_message(
Result.new(
id: message[:id],
response: Requests::PrepareRename.new(document, params[:position]).perform,
response: Requests::PrepareRename.new(@global_state, document, params[:position]).perform,
),
)
end
Expand Down Expand Up @@ -1056,6 +1056,7 @@ def text_document_show_syntax_tree(message)

response = {
ast: Requests::ShowSyntaxTree.new(
@global_state,
document,
params[:range],
).perform,
Expand Down
3 changes: 2 additions & 1 deletion test/requests/prepare_rename_expectations_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ def run_expectations(source)
position = @__params&.any? ? @__params[:position] : default_position
uri = URI("file://#{@_path}")
document = RubyLsp::RubyDocument.new(source: source, version: 1, uri: uri)
RubyLsp::Requests::PrepareRename.new(document, position).perform
global_state = RubyLsp::GlobalState.new
RubyLsp::Requests::PrepareRename.new(global_state, document, position).perform
end

private
Expand Down

0 comments on commit 6980656

Please sign in to comment.