Skip to content

Commit

Permalink
Move RuboCop file watching to the server (#2982)
Browse files Browse the repository at this point in the history
### Motivation

Closes #1457

This was originally attempted in #1921 and #2510. This PR moves watching `.rubocop.yml` to the server, so that we can avoid a full server restart and instead just create a new instance of the RuboCop runner.

This PR still suffers from the same issue reported in both previous attempts: trying to clear existing diagnostics after changing `.rubocop.yml` simply does not work. I believe this must be some sort of bug in the client or some assumption we're not satisfying. Clearing diagnostics works normally when closing files on `textDocument/didClose` notifications, so I see no reason why it wouldn't work while processing a did change watched files notification.

Despite the bug, since we're trying to improve stability of the LSP, I think it's still worth moving forward. Launching the server is still the most critical part in our entire life cycle and reducing the amount of times we restart helps prevent unnecessary pain for developers. Additionally, simply editing the file with the stale diagnostics already makes them go away immediately, so the bug is not that terrible.

I created an issue to discuss the bug with the maintainers of the protocol microsoft/vscode-languageserver-node#1594.

### Implementation

Stopped restarting the server on `.rubocop.yml` and `.rubocop` modifications. Instead, we watch the files in the server and simply re-create the instance of the runner.

Note that this has the added benefit of working on any editor.
  • Loading branch information
vinistock authored Jan 7, 2025
1 parent 413eaea commit 516cff3
Show file tree
Hide file tree
Showing 4 changed files with 93 additions and 46 deletions.
3 changes: 3 additions & 0 deletions lib/ruby_lsp/global_state.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ class GlobalState
sig { returns(ClientCapabilities) }
attr_reader :client_capabilities

sig { returns(URI::Generic) }
attr_reader :workspace_uri

sig { void }
def initialize
@workspace_uri = T.let(URI::Generic.from_path(path: Dir.pwd), URI::Generic)
Expand Down
86 changes: 46 additions & 40 deletions lib/ruby_lsp/server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -298,29 +298,11 @@ def run_initialize(message)

# Not every client supports dynamic registration or file watching
if @global_state.client_capabilities.supports_watching_files
send_message(
Request.new(
id: @current_request_id,
method: "client/registerCapability",
params: Interface::RegistrationParams.new(
registrations: [
# Register watching Ruby files
Interface::Registration.new(
id: "workspace/didChangeWatchedFiles",
method: "workspace/didChangeWatchedFiles",
register_options: Interface::DidChangeWatchedFilesRegistrationOptions.new(
watchers: [
Interface::FileSystemWatcher.new(
glob_pattern: "**/*.rb",
kind: Constant::WatchKind::CREATE | Constant::WatchKind::CHANGE | Constant::WatchKind::DELETE,
),
],
),
),
],
),
),
)
send_message(Request.register_watched_files(@current_request_id, "**/*.rb"))
send_message(Request.register_watched_files(
@current_request_id,
Interface::RelativePattern.new(base_uri: @global_state.workspace_uri.to_s, pattern: ".rubocop.yml"),
))
end

process_indexing_configuration(options.dig(:initializationOptions, :indexing))
Expand Down Expand Up @@ -418,12 +400,7 @@ def text_document_did_close(message)
@store.delete(uri)

# Clear diagnostics for the closed file, so that they no longer appear in the problems tab
send_message(
Notification.new(
method: "textDocument/publishDiagnostics",
params: Interface::PublishDiagnosticsParams.new(uri: uri.to_s, diagnostics: []),
),
)
send_message(Notification.publish_diagnostics(uri.to_s, []))
end

sig { params(message: T::Hash[Symbol, T.untyped]).void }
Expand Down Expand Up @@ -1004,26 +981,55 @@ def workspace_did_change_watched_files(message)
uri = URI(change[:uri])
file_path = uri.to_standardized_path
next if file_path.nil? || File.directory?(file_path)
next unless file_path.end_with?(".rb")

load_path_entry = $LOAD_PATH.find { |load_path| file_path.start_with?(load_path) }
uri.add_require_path_from_load_entry(load_path_entry) if load_path_entry
if file_path.end_with?(".rb")
handle_ruby_file_change(index, file_path, change[:type])
next
end

content = File.read(file_path)
file_name = File.basename(file_path)

case change[:type]
when Constant::FileChangeType::CREATED
index.index_single(uri, content)
when Constant::FileChangeType::CHANGED
index.handle_change(uri, content)
when Constant::FileChangeType::DELETED
index.delete(uri)
if file_name == ".rubocop.yml" || file_name == ".rubocop"
handle_rubocop_config_change(uri)
end
end

Addon.file_watcher_addons.each { |addon| T.unsafe(addon).workspace_did_change_watched_files(changes) }
end

sig { params(index: RubyIndexer::Index, file_path: String, change_type: Integer).void }
def handle_ruby_file_change(index, file_path, change_type)
load_path_entry = $LOAD_PATH.find { |load_path| file_path.start_with?(load_path) }
uri = URI::Generic.from_path(load_path_entry: load_path_entry, path: file_path)

content = File.read(file_path)

case change_type
when Constant::FileChangeType::CREATED
index.index_single(uri, content)
when Constant::FileChangeType::CHANGED
index.handle_change(uri, content)
when Constant::FileChangeType::DELETED
index.delete(uri)
end
end

sig { params(uri: URI::Generic).void }
def handle_rubocop_config_change(uri)
return unless defined?(Requests::Support::RuboCopFormatter)

send_log_message("Reloading RuboCop since #{uri} changed")
@global_state.register_formatter("rubocop", Requests::Support::RuboCopFormatter.new)

# Clear all existing diagnostics since the config changed. This has to happen under a mutex because the `state`
# hash cannot be mutated during iteration or that will throw an error
@global_state.synchronize do
@store.each do |uri, _document|
send_message(Notification.publish_diagnostics(uri.to_s, []))
end
end
end

sig { params(message: T::Hash[Symbol, T.untyped]).void }
def workspace_symbol(message)
send_message(
Expand Down
43 changes: 43 additions & 0 deletions lib/ruby_lsp/utils.rb
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,20 @@ def progress_end(id)
),
)
end

sig do
params(
uri: String,
diagnostics: T::Array[Interface::Diagnostic],
version: T.nilable(Integer),
).returns(Notification)
end
def publish_diagnostics(uri, diagnostics, version: nil)
new(
method: "textDocument/publishDiagnostics",
params: Interface::PublishDiagnosticsParams.new(uri: uri, diagnostics: diagnostics, version: version),
)
end
end

extend T::Sig
Expand All @@ -155,6 +169,35 @@ def to_hash
class Request < Message
extend T::Sig

class << self
extend T::Sig

sig { params(id: Integer, pattern: T.any(Interface::RelativePattern, String), kind: Integer).returns(Request) }
def register_watched_files(
id,
pattern,
kind: Constant::WatchKind::CREATE | Constant::WatchKind::CHANGE | Constant::WatchKind::DELETE
)
new(
id: id,
method: "client/registerCapability",
params: Interface::RegistrationParams.new(
registrations: [
Interface::Registration.new(
id: "workspace/didChangeWatchedFiles",
method: "workspace/didChangeWatchedFiles",
register_options: Interface::DidChangeWatchedFilesRegistrationOptions.new(
watchers: [
Interface::FileSystemWatcher.new(glob_pattern: pattern, kind: kind),
],
),
),
],
),
)
end
end

sig { params(id: T.any(Integer, String), method: String, params: Object).void }
def initialize(id:, method:, params:)
@id = id
Expand Down
7 changes: 1 addition & 6 deletions vscode/src/workspace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,7 @@ import {
} from "./common";
import { WorkspaceChannel } from "./workspaceChannel";

const WATCHED_FILES = [
"Gemfile.lock",
"gems.locked",
".rubocop.yml",
".rubocop",
];
const WATCHED_FILES = ["Gemfile.lock", "gems.locked"];

export class Workspace implements WorkspaceInterface {
public lspClient?: Client;
Expand Down

0 comments on commit 516cff3

Please sign in to comment.