From 516cff3510f104878fc20fd0d1150c8eeb2c9a6d Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Tue, 7 Jan 2025 09:07:52 -0500 Subject: [PATCH] Move RuboCop file watching to the server (#2982) ### 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 https://github.com/microsoft/vscode-languageserver-node/issues/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. --- lib/ruby_lsp/global_state.rb | 3 ++ lib/ruby_lsp/server.rb | 86 +++++++++++++++++++----------------- lib/ruby_lsp/utils.rb | 43 ++++++++++++++++++ vscode/src/workspace.ts | 7 +-- 4 files changed, 93 insertions(+), 46 deletions(-) diff --git a/lib/ruby_lsp/global_state.rb b/lib/ruby_lsp/global_state.rb index 2c6687eb0..dfe2b0114 100644 --- a/lib/ruby_lsp/global_state.rb +++ b/lib/ruby_lsp/global_state.rb @@ -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) diff --git a/lib/ruby_lsp/server.rb b/lib/ruby_lsp/server.rb index 407116b0f..362cf6f0c 100644 --- a/lib/ruby_lsp/server.rb +++ b/lib/ruby_lsp/server.rb @@ -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)) @@ -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 } @@ -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( diff --git a/lib/ruby_lsp/utils.rb b/lib/ruby_lsp/utils.rb index 325f28211..4513bd8ef 100644 --- a/lib/ruby_lsp/utils.rb +++ b/lib/ruby_lsp/utils.rb @@ -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 @@ -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 diff --git a/vscode/src/workspace.ts b/vscode/src/workspace.ts index 5fcf55220..de77843ce 100644 --- a/vscode/src/workspace.ts +++ b/vscode/src/workspace.ts @@ -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;