Skip to content
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
11 changes: 10 additions & 1 deletion lib/ruby_lsp/base_server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ def initialize(**options)
@install_error = options[:install_error] #: StandardError?
@incoming_queue = Thread::Queue.new #: Thread::Queue
@outgoing_queue = Thread::Queue.new #: Thread::Queue
@sent_requests = {}
@cancelled_requests = [] #: Array[Integer]
@worker = new_worker #: Thread
@current_request_id = 1 #: Integer
Expand All @@ -21,7 +22,15 @@ def initialize(**options)
@outgoing_dispatcher = Thread.new do
unless @test_mode
while (message = @outgoing_queue.pop)
@global_state.synchronize { @writer.write(message.to_hash) }
@global_state.synchronize do
Copy link
Member

Choose a reason for hiding this comment

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

We already process one response for server->client requests here.

Are we able to reuse that and avoid changing the base server?

Copy link
Author

Choose a reason for hiding this comment

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

This implementation relies on the response having method inside the payload. In this case it does not, which is why I needed to match it to the request by it's ID to know what method it is. Here are relevant pieces from my session:

[jsonrpc] e[17:49:30.206] <-- workspace/configuration[2] {"id":2,"method":"workspace/configuration","params":{"items":[{"section":"rubyLsp"}]},"jsonrpc":"2.0"}
[jsonrpc] e[17:49:30.207] --> workspace/configuration[2] {"jsonrpc":"2.0","id":2,"result":[{"formatter":"standard","linters":["standard"],"enabledFeatures":{"codeActions":true,"diagnostics":true,"formatting":true}}]}

Perhaps there's a better way to do the reply-to-request matching I did not find?

# If the message is a request from server to client, we save it because we might
# need it later to understand the response.
if message.is_a?(Request)
id = message.to_hash[:id]
@sent_requests[id] = message
end
@writer.write(message.to_hash)
end
end
end
end #: Thread
Expand Down
30 changes: 29 additions & 1 deletion lib/ruby_lsp/server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,9 @@ def process_message(message)
type_hierarchy_supertypes(message)
when "typeHierarchy/subtypes"
type_hierarchy_subtypes(message)
when "workspace/didChangeConfiguration"
send_log_message("Re-applying Ruby LSP configuration after workspace configuration change")
workspace_configuration_did_change(message)
when "workspace/didChangeWatchedFiles"
workspace_did_change_watched_files(message)
when "workspace/symbol"
Expand Down Expand Up @@ -157,9 +160,16 @@ def process_message(message)
# Process responses to requests that were sent to the client
#: (Hash[Symbol, untyped] message) -> void
def process_response(message)
case message.dig(:result, :method)
# Some replies have method in their payload, but some do not and we need to match
# the request by id to find what the method is.
method = (message[:result].is_a?(Hash) && message.dig(:result, :method)) || @sent_requests[message[:id]]&.to_hash&.fetch(:method)

case method
when "window/showMessageRequest"
window_show_message_request(message)
when "workspace/configuration"
send_log_message("Received workspace configuration from client: #{message}")
workspace_configuration_received(message)
end
end

Expand Down Expand Up @@ -196,6 +206,21 @@ def load_addons(include_project_addons: true)

private

#: (Hash[Symbol, untyped] message) -> void
def workspace_configuration_did_change(message)
Copy link
Member

Choose a reason for hiding this comment

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

My understanding based on the spec is that the parameters of workspace/didChangeConfiguration already include which settings are supposed to be changed.

Why do we need to make a request to the client? Could we use what we receive as parameters instead?

Copy link
Author

Choose a reason for hiding this comment

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

The way I understand the spec is that workspace/didChangeConfiguration can include the payload, but it's soft-deprecated behaviour, and I chose not to implement it.

The workspace/configuration request is sent from the server to the client to fetch configuration settings from the client. [...] This pull model replaces the old push model were the client signaled configuration change via an event.

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_configuration

I think it's a strategic decision for the maintaining team to choose which approach to support and I will be happy to adjust this PR to whatever you decide (push, pull or both).

# This assumes that the workspace configuration is under "rubyLsp" key, which seems
# to be the standard naming convention.
send_message(Request.workspace_configuration(
@current_request_id, section: "rubyLsp"
))
end

def workspace_configuration_received(message)
options = { initializationOptions: message[:result]&.first }
messages_to_send = @global_state.apply_options(options)
Comment on lines +219 to +220
Copy link
Author

@katafrakt katafrakt Nov 3, 2025

Choose a reason for hiding this comment

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

This is a hack to reuse apply_options, which expects initializationOptions, but perhaps should be properly extracted to a method just accepting the options to apply.

Also, this just takes first hash from the reply's result, which works here, because we only asked for one configuration item, but technically is not exactly in line with the spec.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this method is coupling information that we get during initialization (like the workspace folder) with other settings. It would indeed be nice to split into two methods:

  1. One that handles information required by the spec (workspace folders, capabilities and so on)
  2. Another that handles only the Ruby LSP's specific settings (what gets passed as initializationOptions)

Beyond just improving the design, invoking this twice may have weird consequences, like accidentally changing the negotiated encoding, so I don't think we can reuse it as is.

For example, if the editor and server negotiated UTF-8 initially, invoking this method without passing the capabilities -> general -> positionEncodings data will result in the server choosing UTF-16 (respecting the spec's default encoding), which would mean that editor and server would be trying to communicate using different encodings - leading to crashes, documents being in an invalid state and so on.

messages_to_send.each { |notification| send_message(notification) }
end

#: (Hash[Symbol, untyped] message) -> void
def run_initialize(message)
options = message[:params]
Expand Down Expand Up @@ -273,6 +298,9 @@ def run_initialize(message)
rename_provider: rename_provider,
references_provider: !@global_state.has_type_checker,
document_range_formatting_provider: true,
workspace: {
configuration: true,
},
experimental: {
addon_detection: true,
compose_bundle: true,
Expand Down
11 changes: 11 additions & 0 deletions lib/ruby_lsp/utils.rb
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,17 @@ def register_watched_files(
),
)
end

def workspace_configuration(
id,
section:
)
new(id: id, method: "workspace/configuration", params: Interface::ConfigurationParams.new(
items: [
Interface::ConfigurationItem.new(section: section),
],
))
end
end

#: (id: (Integer | String), method: String, params: Object) -> void
Expand Down
25 changes: 25 additions & 0 deletions test/server_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,31 @@ def test_backtrace_is_printed_to_stderr_on_exceptions
end
end

def test_reply_to_workspace_configuration_modifies_global_state
@server.instance_variable_set(:@sent_requests, {
1 => RubyLsp::Request.workspace_configuration(
1, section: "rubyLsp"
),
})

@server.process_message({
id: 1,
result: [{ formatter: "standard" }],
})

assert_equal("standard", @server.global_state.formatter)
end

def test_did_change_configuration_sends_workspace_configuration_request
@server.process_message({
id: 1,
method: "workspace/didChangeConfiguration",
params: {},
})

find_message(RubyLsp::Request, "workspace/configuration")
end

def test_changed_file_only_indexes_ruby
path = File.join(Dir.pwd, "lib", "foo.rb")
File.write(path, "class Foo\nend")
Expand Down