From 523018eb419f564120dfba53871a2211b7d21195 Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Mon, 8 Apr 2024 02:46:32 +0200 Subject: [PATCH 1/2] use newtype parttern for langauge server id --- Cargo.lock | 1 + Cargo.toml | 1 + helix-core/Cargo.toml | 3 +- helix-core/src/diagnostic.rs | 21 +++- helix-lsp/Cargo.toml | 1 + helix-lsp/src/client.rs | 14 ++- helix-lsp/src/file_event.rs | 16 +-- helix-lsp/src/lib.rs | 141 +++++++++++++++----------- helix-lsp/src/transport.rs | 14 +-- helix-term/src/application.rs | 11 +- helix-term/src/commands/lsp.rs | 14 ++- helix-term/src/handlers/completion.rs | 2 +- helix-term/src/ui/completion.rs | 5 +- helix-term/src/ui/spinner.rs | 8 +- helix-view/src/document.rs | 28 ++--- helix-view/src/editor.rs | 22 ++-- helix-view/src/gutter.rs | 2 +- 17 files changed, 176 insertions(+), 128 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6af72fc422b9..017a248a8e41 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1393,6 +1393,7 @@ dependencies = [ "parking_lot", "serde", "serde_json", + "slotmap", "thiserror", "tokio", "tokio-stream", diff --git a/Cargo.toml b/Cargo.toml index 3bfc4b8e18a3..6206281b7fb6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -39,6 +39,7 @@ package.helix-term.opt-level = 2 [workspace.dependencies] tree-sitter = { version = "0.22" } nucleo = "0.2.0" +slotmap = "1.0.7" [workspace.package] version = "24.3.0" diff --git a/helix-core/Cargo.toml b/helix-core/Cargo.toml index 7482262eb59e..16d6b457f497 100644 --- a/helix-core/Cargo.toml +++ b/helix-core/Cargo.toml @@ -25,8 +25,7 @@ smartstring = "1.0.1" unicode-segmentation = "1.11" unicode-width = "0.1" unicode-general-category = "0.6" -# slab = "0.4.2" -slotmap = "1.0" +slotmap.workspace = true tree-sitter.workspace = true once_cell = "1.19" arc-swap = "1" diff --git a/helix-core/src/diagnostic.rs b/helix-core/src/diagnostic.rs index 52d77a9aace5..d41119d38e54 100644 --- a/helix-core/src/diagnostic.rs +++ b/helix-core/src/diagnostic.rs @@ -1,4 +1,6 @@ //! LSP diagnostic utility types. +use std::fmt; + use serde::{Deserialize, Serialize}; /// Describes the severity level of a [`Diagnostic`]. @@ -47,8 +49,25 @@ pub struct Diagnostic { pub message: String, pub severity: Option, pub code: Option, - pub language_server_id: usize, + pub provider: DiagnosticProvider, pub tags: Vec, pub source: Option, pub data: Option, } + +// TODO turn this into an enum + feature flag when lsp becomes optional +pub type DiagnosticProvider = LanguageServerId; + +// while I would prefer having this in helix-lsp that necessitates a bunch of +// conversions I would rather not add. I think its fine since this just a very +// trivial newtype wrapper and we would need something similar once we define +// completions in core +slotmap::new_key_type! { + pub struct LanguageServerId; +} + +impl fmt::Display for LanguageServerId { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{:?}", self.0) + } +} diff --git a/helix-lsp/Cargo.toml b/helix-lsp/Cargo.toml index 0b502f768add..f87f15f219bf 100644 --- a/helix-lsp/Cargo.toml +++ b/helix-lsp/Cargo.toml @@ -31,3 +31,4 @@ tokio = { version = "1.37", features = ["rt", "rt-multi-thread", "io-util", "io- tokio-stream = "0.1.15" parking_lot = "0.12.1" arc-swap = "1" +slotmap.workspace = true diff --git a/helix-lsp/src/client.rs b/helix-lsp/src/client.rs index b2290e0316a8..79d8adb31c95 100644 --- a/helix-lsp/src/client.rs +++ b/helix-lsp/src/client.rs @@ -2,7 +2,7 @@ use crate::{ file_operations::FileOperationsInterest, find_lsp_workspace, jsonrpc, transport::{Payload, Transport}, - Call, Error, OffsetEncoding, Result, + Call, Error, LanguageServerId, OffsetEncoding, Result, }; use helix_core::{find_workspace, syntax::LanguageServerFeature, ChangeSet, Rope}; @@ -46,7 +46,7 @@ fn workspace_for_uri(uri: lsp::Url) -> WorkspaceFolder { #[derive(Debug)] pub struct Client { - id: usize, + id: LanguageServerId, name: String, _process: Child, server_tx: UnboundedSender, @@ -179,10 +179,14 @@ impl Client { server_environment: HashMap, root_path: PathBuf, root_uri: Option, - id: usize, + id: LanguageServerId, name: String, req_timeout: u64, - ) -> Result<(Self, UnboundedReceiver<(usize, Call)>, Arc)> { + ) -> Result<( + Self, + UnboundedReceiver<(LanguageServerId, Call)>, + Arc, + )> { // Resolve path to the binary let cmd = helix_stdx::env::which(cmd)?; @@ -234,7 +238,7 @@ impl Client { &self.name } - pub fn id(&self) -> usize { + pub fn id(&self) -> LanguageServerId { self.id } diff --git a/helix-lsp/src/file_event.rs b/helix-lsp/src/file_event.rs index 93457fa55b41..8f4c167566df 100644 --- a/helix-lsp/src/file_event.rs +++ b/helix-lsp/src/file_event.rs @@ -3,24 +3,24 @@ use std::{collections::HashMap, path::PathBuf, sync::Weak}; use globset::{GlobBuilder, GlobSetBuilder}; use tokio::sync::mpsc; -use crate::{lsp, Client}; +use crate::{lsp, Client, LanguageServerId}; enum Event { FileChanged { path: PathBuf, }, Register { - client_id: usize, + client_id: LanguageServerId, client: Weak, registration_id: String, options: lsp::DidChangeWatchedFilesRegistrationOptions, }, Unregister { - client_id: usize, + client_id: LanguageServerId, registration_id: String, }, RemoveClient { - client_id: usize, + client_id: LanguageServerId, }, } @@ -59,7 +59,7 @@ impl Handler { pub fn register( &self, - client_id: usize, + client_id: LanguageServerId, client: Weak, registration_id: String, options: lsp::DidChangeWatchedFilesRegistrationOptions, @@ -72,7 +72,7 @@ impl Handler { }); } - pub fn unregister(&self, client_id: usize, registration_id: String) { + pub fn unregister(&self, client_id: LanguageServerId, registration_id: String) { let _ = self.tx.send(Event::Unregister { client_id, registration_id, @@ -83,12 +83,12 @@ impl Handler { let _ = self.tx.send(Event::FileChanged { path }); } - pub fn remove_client(&self, client_id: usize) { + pub fn remove_client(&self, client_id: LanguageServerId) { let _ = self.tx.send(Event::RemoveClient { client_id }); } async fn run(mut rx: mpsc::UnboundedReceiver) { - let mut state: HashMap = HashMap::new(); + let mut state: HashMap = HashMap::new(); while let Some(event) = rx.recv().await { match event { Event::FileChanged { path } => { diff --git a/helix-lsp/src/lib.rs b/helix-lsp/src/lib.rs index 262bc1b9447a..04f018bc423d 100644 --- a/helix-lsp/src/lib.rs +++ b/helix-lsp/src/lib.rs @@ -17,6 +17,7 @@ use helix_core::syntax::{ LanguageConfiguration, LanguageServerConfiguration, LanguageServerFeatures, }; use helix_stdx::path; +use slotmap::SlotMap; use tokio::sync::mpsc::UnboundedReceiver; use std::{ @@ -28,8 +29,9 @@ use std::{ use thiserror::Error; use tokio_stream::wrappers::UnboundedReceiverStream; -pub type Result = core::result::Result; +pub type Result = core::result::Result; pub type LanguageServerName = String; +pub use helix_core::diagnostic::LanguageServerId; #[derive(Error, Debug)] pub enum Error { @@ -651,38 +653,42 @@ impl Notification { #[derive(Debug)] pub struct Registry { - inner: HashMap>>, + inner: SlotMap>, + inner_by_name: HashMap>>, syn_loader: Arc>, - counter: usize, - pub incoming: SelectAll>, + pub incoming: SelectAll>, pub file_event_handler: file_event::Handler, } impl Registry { pub fn new(syn_loader: Arc>) -> Self { Self { - inner: HashMap::new(), + inner: SlotMap::with_key(), + inner_by_name: HashMap::new(), syn_loader, - counter: 0, incoming: SelectAll::new(), file_event_handler: file_event::Handler::new(), } } - pub fn get_by_id(&self, id: usize) -> Option<&Client> { - self.inner - .values() - .flatten() - .find(|client| client.id() == id) - .map(|client| &**client) + pub fn get_by_id(&self, id: LanguageServerId) -> Option<&Arc> { + self.inner.get(id) } - pub fn remove_by_id(&mut self, id: usize) { + pub fn remove_by_id(&mut self, id: LanguageServerId) { + let Some(client) = self.inner.remove(id) else { + log::error!("client was already removed"); + return + }; self.file_event_handler.remove_client(id); - self.inner.retain(|_, language_servers| { - language_servers.retain(|ls| id != ls.id()); - !language_servers.is_empty() - }); + let instances = self + .inner_by_name + .get_mut(client.name()) + .expect("inner and inner_by_name must be synced"); + instances.retain(|ls| id != ls.id()); + if instances.is_empty() { + self.inner_by_name.remove(client.name()); + } } fn start_client( @@ -692,28 +698,28 @@ impl Registry { doc_path: Option<&std::path::PathBuf>, root_dirs: &[PathBuf], enable_snippets: bool, - ) -> Result>> { + ) -> Result, StartupError> { let syn_loader = self.syn_loader.load(); let config = syn_loader .language_server_configs() .get(&name) .ok_or_else(|| anyhow::anyhow!("Language server '{name}' not defined"))?; - let id = self.counter; - self.counter += 1; - if let Some(NewClient(client, incoming)) = start_client( - id, - name, - ls_config, - config, - doc_path, - root_dirs, - enable_snippets, - )? { - self.incoming.push(UnboundedReceiverStream::new(incoming)); - Ok(Some(client)) - } else { - Ok(None) - } + let id = self.inner.try_insert_with_key(|id| { + start_client( + id, + name, + ls_config, + config, + doc_path, + root_dirs, + enable_snippets, + ) + .map(|client| { + self.incoming.push(UnboundedReceiverStream::new(client.1)); + client.0 + }) + })?; + Ok(self.inner[id].clone()) } /// If this method is called, all documents that have a reference to language servers used by the language config have to refresh their language servers, @@ -730,7 +736,7 @@ impl Registry { .language_servers .iter() .filter_map(|LanguageServerFeatures { name, .. }| { - if self.inner.contains_key(name) { + if self.inner_by_name.contains_key(name) { let client = match self.start_client( name.clone(), language_config, @@ -738,16 +744,18 @@ impl Registry { root_dirs, enable_snippets, ) { - Ok(client) => client?, - Err(error) => return Some(Err(error)), + Ok(client) => client, + Err(StartupError::NoRequiredRootFound) => return None, + Err(StartupError::Error(err)) => return Some(Err(err)), }; let old_clients = self - .inner + .inner_by_name .insert(name.clone(), vec![client.clone()]) .unwrap(); for old_client in old_clients { self.file_event_handler.remove_client(old_client.id()); + self.inner.remove(client.id()); tokio::spawn(async move { let _ = old_client.force_shutdown().await; }); @@ -762,9 +770,10 @@ impl Registry { } pub fn stop(&mut self, name: &str) { - if let Some(clients) = self.inner.remove(name) { + if let Some(clients) = self.inner_by_name.remove(name) { for client in clients { self.file_event_handler.remove_client(client.id()); + self.inner.remove(client.id()); tokio::spawn(async move { let _ = client.force_shutdown().await; }); @@ -781,7 +790,7 @@ impl Registry { ) -> impl Iterator>)> + 'a { language_config.language_servers.iter().filter_map( move |LanguageServerFeatures { name, .. }| { - if let Some(clients) = self.inner.get(name) { + if let Some(clients) = self.inner_by_name.get(name) { if let Some((_, client)) = clients.iter().enumerate().find(|(i, client)| { client.try_add_doc(&language_config.roots, root_dirs, doc_path, *i == 0) }) { @@ -796,21 +805,21 @@ impl Registry { enable_snippets, ) { Ok(client) => { - let client = client?; - self.inner + self.inner_by_name .entry(name.to_owned()) .or_default() .push(client.clone()); Some((name.clone(), Ok(client))) } - Err(err) => Some((name.to_owned(), Err(err))), + Err(StartupError::NoRequiredRootFound) => None, + Err(StartupError::Error(err)) => Some((name.to_owned(), Err(err))), } }, ) } pub fn iter_clients(&self) -> impl Iterator> { - self.inner.values().flatten() + self.inner.values() } } @@ -833,7 +842,7 @@ impl ProgressStatus { /// Acts as a container for progress reported by language servers. Each server /// has a unique id assigned at creation through [`Registry`]. This id is then used /// to store the progress in this map. -pub struct LspProgressMap(HashMap>); +pub struct LspProgressMap(HashMap>); impl LspProgressMap { pub fn new() -> Self { @@ -841,28 +850,35 @@ impl LspProgressMap { } /// Returns a map of all tokens corresponding to the language server with `id`. - pub fn progress_map(&self, id: usize) -> Option<&HashMap> { + pub fn progress_map( + &self, + id: LanguageServerId, + ) -> Option<&HashMap> { self.0.get(&id) } - pub fn is_progressing(&self, id: usize) -> bool { + pub fn is_progressing(&self, id: LanguageServerId) -> bool { self.0.get(&id).map(|it| !it.is_empty()).unwrap_or_default() } /// Returns last progress status for a given server with `id` and `token`. - pub fn progress(&self, id: usize, token: &lsp::ProgressToken) -> Option<&ProgressStatus> { + pub fn progress( + &self, + id: LanguageServerId, + token: &lsp::ProgressToken, + ) -> Option<&ProgressStatus> { self.0.get(&id).and_then(|values| values.get(token)) } /// Checks if progress `token` for server with `id` is created. - pub fn is_created(&mut self, id: usize, token: &lsp::ProgressToken) -> bool { + pub fn is_created(&mut self, id: LanguageServerId, token: &lsp::ProgressToken) -> bool { self.0 .get(&id) .map(|values| values.get(token).is_some()) .unwrap_or_default() } - pub fn create(&mut self, id: usize, token: lsp::ProgressToken) { + pub fn create(&mut self, id: LanguageServerId, token: lsp::ProgressToken) { self.0 .entry(id) .or_default() @@ -872,7 +888,7 @@ impl LspProgressMap { /// Ends the progress by removing the `token` from server with `id`, if removed returns the value. pub fn end_progress( &mut self, - id: usize, + id: LanguageServerId, token: &lsp::ProgressToken, ) -> Option { self.0.get_mut(&id).and_then(|vals| vals.remove(token)) @@ -881,7 +897,7 @@ impl LspProgressMap { /// Updates the progress of `token` for server with `id` to `status`, returns the value replaced or `None`. pub fn update( &mut self, - id: usize, + id: LanguageServerId, token: lsp::ProgressToken, status: lsp::WorkDoneProgress, ) -> Option { @@ -892,19 +908,30 @@ impl LspProgressMap { } } -struct NewClient(Arc, UnboundedReceiver<(usize, Call)>); +struct NewClient(Arc, UnboundedReceiver<(LanguageServerId, Call)>); + +enum StartupError { + NoRequiredRootFound, + Error(Error), +} + +impl> From for StartupError { + fn from(value: T) -> Self { + StartupError::Error(value.into()) + } +} /// start_client takes both a LanguageConfiguration and a LanguageServerConfiguration to ensure that /// it is only called when it makes sense. fn start_client( - id: usize, + id: LanguageServerId, name: String, config: &LanguageConfiguration, ls_config: &LanguageServerConfiguration, doc_path: Option<&std::path::PathBuf>, root_dirs: &[PathBuf], enable_snippets: bool, -) -> Result> { +) -> Result { let (workspace, workspace_is_cwd) = helix_loader::find_workspace(); let workspace = path::normalize(workspace); let root = find_lsp_workspace( @@ -929,7 +956,7 @@ fn start_client( .map(|entry| entry.file_name()) .any(|entry| globset.is_match(entry)) { - return Ok(None); + return Err(StartupError::NoRequiredRootFound); } } @@ -981,7 +1008,7 @@ fn start_client( initialize_notify.notify_one(); }); - Ok(Some(NewClient(client, incoming))) + Ok(NewClient(client, incoming)) } /// Find an LSP workspace of a file using the following mechanism: diff --git a/helix-lsp/src/transport.rs b/helix-lsp/src/transport.rs index f2f35d6abf4b..bd671abe1e6f 100644 --- a/helix-lsp/src/transport.rs +++ b/helix-lsp/src/transport.rs @@ -1,4 +1,4 @@ -use crate::{jsonrpc, Error, Result}; +use crate::{jsonrpc, Error, LanguageServerId, Result}; use anyhow::Context; use log::{error, info}; use serde::{Deserialize, Serialize}; @@ -37,7 +37,7 @@ enum ServerMessage { #[derive(Debug)] pub struct Transport { - id: usize, + id: LanguageServerId, name: String, pending_requests: Mutex>>>, } @@ -47,10 +47,10 @@ impl Transport { server_stdout: BufReader, server_stdin: BufWriter, server_stderr: BufReader, - id: usize, + id: LanguageServerId, name: String, ) -> ( - UnboundedReceiver<(usize, jsonrpc::Call)>, + UnboundedReceiver<(LanguageServerId, jsonrpc::Call)>, UnboundedSender, Arc, ) { @@ -194,7 +194,7 @@ impl Transport { async fn process_server_message( &self, - client_tx: &UnboundedSender<(usize, jsonrpc::Call)>, + client_tx: &UnboundedSender<(LanguageServerId, jsonrpc::Call)>, msg: ServerMessage, language_server_name: &str, ) -> Result<()> { @@ -251,7 +251,7 @@ impl Transport { async fn recv( transport: Arc, mut server_stdout: BufReader, - client_tx: UnboundedSender<(usize, jsonrpc::Call)>, + client_tx: UnboundedSender<(LanguageServerId, jsonrpc::Call)>, ) { let mut recv_buffer = String::new(); loop { @@ -329,7 +329,7 @@ impl Transport { async fn send( transport: Arc, mut server_stdin: BufWriter, - client_tx: UnboundedSender<(usize, jsonrpc::Call)>, + client_tx: UnboundedSender<(LanguageServerId, jsonrpc::Call)>, mut client_rx: UnboundedReceiver, initialize_notify: Arc, ) { diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index 809393c7fd7b..6bdf60bca91e 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -4,7 +4,7 @@ use helix_core::{diagnostic::Severity, pos_at_coords, syntax, Selection}; use helix_lsp::{ lsp::{self, notification::Notification}, util::lsp_range_to_range, - LspProgressMap, + LanguageServerId, LspProgressMap, }; use helix_stdx::path::get_relative_path; use helix_view::{ @@ -655,7 +655,7 @@ impl Application { pub async fn handle_language_server_message( &mut self, call: helix_lsp::Call, - server_id: usize, + server_id: LanguageServerId, ) { use helix_lsp::{Call, MethodCall, Notification}; @@ -1030,12 +1030,7 @@ impl Application { Ok(json!(result)) } Ok(MethodCall::RegisterCapability(params)) => { - if let Some(client) = self - .editor - .language_servers - .iter_clients() - .find(|client| client.id() == server_id) - { + if let Some(client) = self.editor.language_servers.get_by_id(server_id) { for reg in params.registrations { match reg.method.as_str() { lsp::notification::DidChangeWatchedFiles::METHOD => { diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs index 6a5ceae6f354..88bd07919c4a 100644 --- a/helix-term/src/commands/lsp.rs +++ b/helix-term/src/commands/lsp.rs @@ -6,7 +6,7 @@ use helix_lsp::{ NumberOrString, }, util::{diagnostic_to_lsp_diagnostic, lsp_range_to_range, range_to_lsp_range}, - Client, OffsetEncoding, + Client, LanguageServerId, OffsetEncoding, }; use tokio_stream::StreamExt; use tui::{ @@ -266,7 +266,7 @@ enum DiagnosticsFormat { fn diag_picker( cx: &Context, - diagnostics: BTreeMap>, + diagnostics: BTreeMap>, format: DiagnosticsFormat, ) -> Picker { // TODO: drop current_path comparison and instead use workspace: bool flag? @@ -497,7 +497,7 @@ pub fn workspace_diagnostics_picker(cx: &mut Context) { struct CodeActionOrCommandItem { lsp_item: lsp::CodeActionOrCommand, - language_server_id: usize, + language_server_id: LanguageServerId, } impl ui::menu::Item for CodeActionOrCommandItem { @@ -757,7 +757,11 @@ impl ui::menu::Item for lsp::Command { } } -pub fn execute_lsp_command(editor: &mut Editor, language_server_id: usize, cmd: lsp::Command) { +pub fn execute_lsp_command( + editor: &mut Editor, + language_server_id: LanguageServerId, + cmd: lsp::Command, +) { // the command is executed on the server and communicated back // to the client asynchronously using workspace edits let future = match editor @@ -1034,7 +1038,7 @@ pub fn rename_symbol(cx: &mut Context) { fn create_rename_prompt( editor: &Editor, prefill: String, - language_server_id: Option, + language_server_id: Option, ) -> Box { let prompt = ui::Prompt::new( "rename-to:".into(), diff --git a/helix-term/src/handlers/completion.rs b/helix-term/src/handlers/completion.rs index 491ca56389c3..8bd85ef6d66d 100644 --- a/helix-term/src/handlers/completion.rs +++ b/helix-term/src/handlers/completion.rs @@ -251,7 +251,7 @@ fn request_completion( .into_iter() .map(|item| CompletionItem { item, - language_server_id, + provider: language_server_id, resolved: false, }) .collect(); diff --git a/helix-term/src/ui/completion.rs b/helix-term/src/ui/completion.rs index 1fcb8e547117..2b3437e11850 100644 --- a/helix-term/src/ui/completion.rs +++ b/helix-term/src/ui/completion.rs @@ -94,7 +94,7 @@ impl menu::Item for CompletionItem { #[derive(Debug, PartialEq, Default, Clone)] pub struct CompletionItem { pub item: lsp::CompletionItem, - pub language_server_id: usize, + pub provider: LanguageServerId, pub resolved: bool, } @@ -224,7 +224,7 @@ impl Completion { ($item:expr) => { match editor .language_servers - .get_by_id($item.language_server_id) + .get_by_id($item.provider) { Some(ls) => ls, None => { @@ -285,7 +285,6 @@ impl Completion { let language_server = language_server!(item); let offset_encoding = language_server.offset_encoding(); - // resolve item if not yet resolved if !item.resolved { if let Some(resolved) = Self::resolve_completion_item(language_server, item.item.clone()) diff --git a/helix-term/src/ui/spinner.rs b/helix-term/src/ui/spinner.rs index 379c4489f318..9ce610555935 100644 --- a/helix-term/src/ui/spinner.rs +++ b/helix-term/src/ui/spinner.rs @@ -1,16 +1,18 @@ use std::{collections::HashMap, time::Instant}; +use helix_lsp::LanguageServerId; + #[derive(Default, Debug)] pub struct ProgressSpinners { - inner: HashMap, + inner: HashMap, } impl ProgressSpinners { - pub fn get(&self, id: usize) -> Option<&Spinner> { + pub fn get(&self, id: LanguageServerId) -> Option<&Spinner> { self.inner.get(&id) } - pub fn get_or_create(&mut self, id: usize) -> &mut Spinner { + pub fn get_or_create(&mut self, id: LanguageServerId) -> &mut Spinner { self.inner.entry(id).or_default() } } diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index d44b4240c7f2..3393fbed7633 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -624,7 +624,7 @@ where *mut_ref = f(mem::take(mut_ref)); } -use helix_lsp::{lsp, Client, LanguageServerName}; +use helix_lsp::{lsp, Client, LanguageServerId, LanguageServerName}; use url::Url; impl Document { @@ -1296,11 +1296,7 @@ impl Document { }); self.diagnostics.sort_unstable_by_key(|diagnostic| { - ( - diagnostic.range, - diagnostic.severity, - diagnostic.language_server_id, - ) + (diagnostic.range, diagnostic.severity, diagnostic.provider) }); // Update the inlay hint annotations' positions, helping ensure they are displayed in the proper place @@ -1644,7 +1640,7 @@ impl Document { }) } - pub fn supports_language_server(&self, id: usize) -> bool { + pub fn supports_language_server(&self, id: LanguageServerId) -> bool { self.language_servers().any(|l| l.id() == id) } @@ -1767,7 +1763,7 @@ impl Document { text: &Rope, language_config: Option<&LanguageConfiguration>, diagnostic: &helix_lsp::lsp::Diagnostic, - language_server_id: usize, + language_server_id: LanguageServerId, offset_encoding: helix_lsp::OffsetEncoding, ) -> Option { use helix_core::diagnostic::{Range, Severity::*}; @@ -1844,7 +1840,7 @@ impl Document { tags, source: diagnostic.source.clone(), data: diagnostic.data.clone(), - language_server_id, + provider: language_server_id, }) } @@ -1857,13 +1853,13 @@ impl Document { &mut self, diagnostics: impl IntoIterator, unchanged_sources: &[String], - language_server_id: Option, + language_server_id: Option, ) { if unchanged_sources.is_empty() { self.clear_diagnostics(language_server_id); } else { self.diagnostics.retain(|d| { - if language_server_id.map_or(false, |id| id != d.language_server_id) { + if language_server_id.map_or(false, |id| id != d.provider) { return true; } @@ -1876,18 +1872,14 @@ impl Document { } self.diagnostics.extend(diagnostics); self.diagnostics.sort_unstable_by_key(|diagnostic| { - ( - diagnostic.range, - diagnostic.severity, - diagnostic.language_server_id, - ) + (diagnostic.range, diagnostic.severity, diagnostic.provider) }); } /// clears diagnostics for a given language server id if set, otherwise all diagnostics are cleared - pub fn clear_diagnostics(&mut self, language_server_id: Option) { + pub fn clear_diagnostics(&mut self, language_server_id: Option) { if let Some(id) = language_server_id { - self.diagnostics.retain(|d| d.language_server_id != id); + self.diagnostics.retain(|d| d.provider != id); } else { self.diagnostics.clear(); } diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs index d7058d3ef022..2e04037a8998 100644 --- a/helix-view/src/editor.rs +++ b/helix-view/src/editor.rs @@ -16,7 +16,7 @@ use helix_vcs::DiffProviderRegistry; use futures_util::stream::select_all::SelectAll; use futures_util::{future, StreamExt}; -use helix_lsp::Call; +use helix_lsp::{Call, LanguageServerId}; use tokio_stream::wrappers::UnboundedReceiverStream; use std::{ @@ -960,7 +960,7 @@ pub struct Editor { pub macro_recording: Option<(char, Vec)>, pub macro_replaying: Vec, pub language_servers: helix_lsp::Registry, - pub diagnostics: BTreeMap>, + pub diagnostics: BTreeMap>, pub diff_providers: DiffProviderRegistry, pub debugger: Option, @@ -1020,7 +1020,7 @@ pub type Motion = Box; pub enum EditorEvent { DocumentSaved(DocumentSavedEventResult), ConfigEvent(ConfigEvent), - LanguageServerMessage((usize, Call)), + LanguageServerMessage((LanguageServerId, Call)), DebuggerEvent(dap::Payload), IdleTimer, Redraw, @@ -1260,8 +1260,13 @@ impl Editor { } #[inline] - pub fn language_server_by_id(&self, language_server_id: usize) -> Option<&helix_lsp::Client> { - self.language_servers.get_by_id(language_server_id) + pub fn language_server_by_id( + &self, + language_server_id: LanguageServerId, + ) -> Option<&helix_lsp::Client> { + self.language_servers + .get_by_id(language_server_id) + .map(|client| &**client) } /// Refreshes the language server for a given document @@ -1861,7 +1866,7 @@ impl Editor { /// Returns all supported diagnostics for the document pub fn doc_diagnostics<'a>( language_servers: &'a helix_lsp::Registry, - diagnostics: &'a BTreeMap>, + diagnostics: &'a BTreeMap>, document: &Document, ) -> impl Iterator + 'a { Editor::doc_diagnostics_with_filter(language_servers, diagnostics, document, |_, _| true) @@ -1871,10 +1876,9 @@ impl Editor { /// filtered by `filter` which is invocated with the raw `lsp::Diagnostic` and the language server id it came from pub fn doc_diagnostics_with_filter<'a>( language_servers: &'a helix_lsp::Registry, - diagnostics: &'a BTreeMap>, - + diagnostics: &'a BTreeMap>, document: &Document, - filter: impl Fn(&lsp::Diagnostic, usize) -> bool + 'a, + filter: impl Fn(&lsp::Diagnostic, LanguageServerId) -> bool + 'a, ) -> impl Iterator + 'a { let text = document.text().clone(); let language_config = document.language.clone(); diff --git a/helix-view/src/gutter.rs b/helix-view/src/gutter.rs index ebdac9e2371a..36f719f795a8 100644 --- a/helix-view/src/gutter.rs +++ b/helix-view/src/gutter.rs @@ -71,7 +71,7 @@ pub fn diagnostic<'doc>( d.line == line && doc .language_servers_with_feature(LanguageServerFeature::Diagnostics) - .any(|ls| ls.id() == d.language_server_id) + .any(|ls| ls.id() == d.provider) }); diagnostics_on_line.max_by_key(|d| d.severity).map(|d| { write!(out, "●").ok(); From 5f070f8b5a634262ec0aa451c268cac11aacc74c Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Sun, 21 Apr 2024 16:14:39 +0200 Subject: [PATCH 2/2] don't overload LS with completion resolve requests While moving completion resolve to the event system in #9668 we introduced what is essentially a "DOS attack" on slow LSPs. Completion resolve requests were made in the render loop and debounced with a timeout. Once the timeout expired the resolve request was made. The problem is the next frame would immediately request a new completion resolve request (and mark the old one as obsolete but because LSP has no notion of cancelation the server would still process it). So we were in essence sending one completion request to the server every 150ms and only stopped if the server managed to respond before we rendered a new frame. This caused overload on slower machines/with slower LS. In this PR I revamped the resolve handler so that a request is only ever resolved once. Both by checking if a request is already in-flight and by marking failed resolve requests as resolved. --- helix-lsp/src/client.rs | 38 +++-- helix-term/src/handlers.rs | 2 +- helix-term/src/handlers/completion.rs | 2 + helix-term/src/handlers/completion/resolve.rs | 153 ++++++++++++++++++ helix-term/src/ui/completion.rs | 115 +++---------- helix-term/src/ui/menu.rs | 4 +- 6 files changed, 194 insertions(+), 120 deletions(-) create mode 100644 helix-term/src/handlers/completion/resolve.rs diff --git a/helix-lsp/src/client.rs b/helix-lsp/src/client.rs index 79d8adb31c95..254625a3a025 100644 --- a/helix-lsp/src/client.rs +++ b/helix-lsp/src/client.rs @@ -397,6 +397,16 @@ impl Client { &self, params: R::Params, ) -> impl Future> + where + R::Params: serde::Serialize, + { + self.call_with_ref::(¶ms) + } + + fn call_with_ref( + &self, + params: &R::Params, + ) -> impl Future> where R::Params: serde::Serialize, { @@ -405,7 +415,7 @@ impl Client { fn call_with_timeout( &self, - params: R::Params, + params: &R::Params, timeout_secs: u64, ) -> impl Future> where @@ -414,17 +424,16 @@ impl Client { let server_tx = self.server_tx.clone(); let id = self.next_request_id(); + let params = serde_json::to_value(params); async move { use std::time::Duration; use tokio::time::timeout; - let params = serde_json::to_value(params)?; - let request = jsonrpc::MethodCall { jsonrpc: Some(jsonrpc::Version::V2), id: id.clone(), method: R::METHOD.to_string(), - params: Self::value_into_params(params), + params: Self::value_into_params(params?), }; let (tx, mut rx) = channel::>(1); @@ -741,7 +750,7 @@ impl Client { new_uri: url_from_path(new_path)?, }]; let request = self.call_with_timeout::( - lsp::RenameFilesParams { files }, + &lsp::RenameFilesParams { files }, 5, ); @@ -1026,21 +1035,10 @@ impl Client { pub fn resolve_completion_item( &self, - completion_item: lsp::CompletionItem, - ) -> Option>> { - let capabilities = self.capabilities.get().unwrap(); - - // Return early if the server does not support resolving completion items. - match capabilities.completion_provider { - Some(lsp::CompletionOptions { - resolve_provider: Some(true), - .. - }) => (), - _ => return None, - } - - let res = self.call::(completion_item); - Some(async move { Ok(serde_json::from_value(res.await?)?) }) + completion_item: &lsp::CompletionItem, + ) -> impl Future> { + let res = self.call_with_ref::(completion_item); + async move { Ok(serde_json::from_value(res.await?)?) } } pub fn resolve_code_action( diff --git a/helix-term/src/handlers.rs b/helix-term/src/handlers.rs index 1b7d9b8c0b92..d45809d36a94 100644 --- a/helix-term/src/handlers.rs +++ b/helix-term/src/handlers.rs @@ -11,7 +11,7 @@ use crate::handlers::signature_help::SignatureHelpHandler; pub use completion::trigger_auto_completion; pub use helix_view::handlers::Handlers; -mod completion; +pub mod completion; mod signature_help; pub fn setup(config: Arc>) -> Handlers { diff --git a/helix-term/src/handlers/completion.rs b/helix-term/src/handlers/completion.rs index 8bd85ef6d66d..68956c85f504 100644 --- a/helix-term/src/handlers/completion.rs +++ b/helix-term/src/handlers/completion.rs @@ -30,6 +30,8 @@ use crate::ui::lsp::SignatureHelp; use crate::ui::{self, CompletionItem, Popup}; use super::Handlers; +pub use resolve::ResolveHandler; +mod resolve; #[derive(Debug, PartialEq, Eq, Clone, Copy)] enum TriggerKind { diff --git a/helix-term/src/handlers/completion/resolve.rs b/helix-term/src/handlers/completion/resolve.rs new file mode 100644 index 000000000000..fb5179e13f01 --- /dev/null +++ b/helix-term/src/handlers/completion/resolve.rs @@ -0,0 +1,153 @@ +use std::sync::Arc; + +use helix_lsp::lsp; +use tokio::sync::mpsc::Sender; +use tokio::time::{Duration, Instant}; + +use helix_event::{send_blocking, AsyncHook, CancelRx}; +use helix_view::Editor; + +use crate::handlers::completion::CompletionItem; +use crate::job; + +/// A hook for resolving incomplete completion items. +/// +/// From the [LSP spec](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_completion): +/// +/// > If computing full completion items is expensive, servers can additionally provide a +/// > handler for the completion item resolve request. ... +/// > A typical use case is for example: the `textDocument/completion` request doesn't fill +/// > in the `documentation` property for returned completion items since it is expensive +/// > to compute. When the item is selected in the user interface then a +/// > 'completionItem/resolve' request is sent with the selected completion item as a parameter. +/// > The returned completion item should have the documentation property filled in. +pub struct ResolveHandler { + last_request: Option>, + resolver: Sender, +} + +impl ResolveHandler { + pub fn new() -> ResolveHandler { + ResolveHandler { + last_request: None, + resolver: ResolveTimeout { + next_request: None, + in_flight: None, + } + .spawn(), + } + } + + pub fn ensure_item_resolved(&mut self, editor: &mut Editor, item: &mut CompletionItem) { + if item.resolved { + return; + } + let needs_resolve = item.item.documentation.is_none() + || item.item.detail.is_none() + || item.item.additional_text_edits.is_none(); + if !needs_resolve { + item.resolved = true; + return; + } + if self.last_request.as_deref().is_some_and(|it| it == item) { + return; + } + let Some(ls) = editor.language_servers.get_by_id(item.provider).cloned() else { + item.resolved = true; + return; + }; + if matches!( + ls.capabilities().completion_provider, + Some(lsp::CompletionOptions { + resolve_provider: Some(true), + .. + }) + ) { + let item = Arc::new(item.clone()); + self.last_request = Some(item.clone()); + send_blocking(&self.resolver, ResolveRequest { item, ls }) + } else { + item.resolved = true; + } + } +} + +struct ResolveRequest { + item: Arc, + ls: Arc, +} + +#[derive(Default)] +struct ResolveTimeout { + next_request: Option, + in_flight: Option<(helix_event::CancelTx, Arc)>, +} + +impl AsyncHook for ResolveTimeout { + type Event = ResolveRequest; + + fn handle_event( + &mut self, + request: Self::Event, + timeout: Option, + ) -> Option { + if self + .next_request + .as_ref() + .is_some_and(|old_request| old_request.item == request.item) + { + timeout + } else if self + .in_flight + .as_ref() + .is_some_and(|(_, old_request)| old_request.item == request.item.item) + { + self.next_request = None; + None + } else { + self.next_request = Some(request); + Some(Instant::now() + Duration::from_millis(150)) + } + } + + fn finish_debounce(&mut self) { + let Some(request) = self.next_request.take() else { return }; + let (tx, rx) = helix_event::cancelation(); + self.in_flight = Some((tx, request.item.clone())); + tokio::spawn(request.execute(rx)); + } +} + +impl ResolveRequest { + async fn execute(self, cancel: CancelRx) { + let future = self.ls.resolve_completion_item(&self.item.item); + let Some(resolved_item) = helix_event::cancelable_future(future, cancel).await else { + return; + }; + job::dispatch(move |_, compositor| { + if let Some(completion) = &mut compositor + .find::() + .unwrap() + .completion + { + let resolved_item = match resolved_item { + Ok(item) => CompletionItem { + item, + resolved: true, + ..*self.item + }, + Err(err) => { + log::error!("completion resolve request failed: {err}"); + // set item to resolved so we don't request it again + // we could also remove it but that oculd be odd ui + let mut item = (*self.item).clone(); + item.resolved = true; + item + } + }; + completion.replace_item(&self.item, resolved_item); + }; + }) + .await + } +} diff --git a/helix-term/src/ui/completion.rs b/helix-term/src/ui/completion.rs index 2b3437e11850..7a08c90ce373 100644 --- a/helix-term/src/ui/completion.rs +++ b/helix-term/src/ui/completion.rs @@ -1,9 +1,7 @@ use crate::{ compositor::{Component, Context, Event, EventResult}, - handlers::trigger_auto_completion, - job, + handlers::{completion::ResolveHandler, trigger_auto_completion}, }; -use helix_event::AsyncHook; use helix_view::{ document::SavePoint, editor::CompleteAction, @@ -12,17 +10,16 @@ use helix_view::{ theme::{Modifier, Style}, ViewId, }; -use tokio::time::Instant; use tui::{buffer::Buffer as Surface, text::Span}; -use std::{borrow::Cow, sync::Arc, time::Duration}; +use std::{borrow::Cow, sync::Arc}; use helix_core::{chars, Change, Transaction}; use helix_view::{graphics::Rect, Document, Editor}; use crate::ui::{menu, Markdown, Menu, Popup, PromptEvent}; -use helix_lsp::{lsp, util, OffsetEncoding}; +use helix_lsp::{lsp, util, LanguageServerId, OffsetEncoding}; impl menu::Item for CompletionItem { type Data = (); @@ -104,7 +101,7 @@ pub struct Completion { #[allow(dead_code)] trigger_offset: usize, filter: String, - resolve_handler: tokio::sync::mpsc::Sender, + resolve_handler: ResolveHandler, } impl Completion { @@ -365,7 +362,7 @@ impl Completion { // TODO: expand nucleo api to allow moving straight to a Utf32String here // and avoid allocation during matching filter: String::from(fragment), - resolve_handler: ResolveHandler::default().spawn(), + resolve_handler: ResolveHandler::new(), }; // need to recompute immediately in case start_offset != trigger_offset @@ -383,7 +380,16 @@ impl Completion { language_server: &helix_lsp::Client, completion_item: lsp::CompletionItem, ) -> Option { - let future = language_server.resolve_completion_item(completion_item)?; + if !matches!( + language_server.capabilities().completion_provider, + Some(lsp::CompletionOptions { + resolve_provider: Some(true), + .. + }) + ) { + return None; + } + let future = language_server.resolve_completion_item(&completion_item); let response = helix_lsp::block_on(future); match response { Ok(item) => Some(item), @@ -416,7 +422,7 @@ impl Completion { self.popup.contents().is_empty() } - fn replace_item(&mut self, old_item: CompletionItem, new_item: CompletionItem) { + pub fn replace_item(&mut self, old_item: &CompletionItem, new_item: CompletionItem) { self.popup.contents_mut().replace_option(old_item, new_item); } @@ -438,12 +444,12 @@ impl Component for Completion { self.popup.render(area, surface, cx); // if we have a selection, render a markdown popup on top/below with info - let option = match self.popup.contents().selection() { + let option = match self.popup.contents_mut().selection_mut() { Some(option) => option, None => return, }; if !option.resolved { - helix_event::send_blocking(&self.resolve_handler, option.clone()); + self.resolve_handler.ensure_item_resolved(cx.editor, option); } // need to render: // option.detail @@ -541,88 +547,3 @@ impl Component for Completion { markdown_doc.render(doc_area, surface, cx); } } - -/// A hook for resolving incomplete completion items. -/// -/// From the [LSP spec](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_completion): -/// -/// > If computing full completion items is expensive, servers can additionally provide a -/// > handler for the completion item resolve request. ... -/// > A typical use case is for example: the `textDocument/completion` request doesn't fill -/// > in the `documentation` property for returned completion items since it is expensive -/// > to compute. When the item is selected in the user interface then a -/// > 'completionItem/resolve' request is sent with the selected completion item as a parameter. -/// > The returned completion item should have the documentation property filled in. -#[derive(Debug, Default)] -struct ResolveHandler { - trigger: Option, - request: Option, -} - -impl AsyncHook for ResolveHandler { - type Event = CompletionItem; - - fn handle_event( - &mut self, - item: Self::Event, - timeout: Option, - ) -> Option { - if self - .trigger - .as_ref() - .is_some_and(|trigger| trigger == &item) - { - timeout - } else { - self.trigger = Some(item); - self.request = None; - Some(Instant::now() + Duration::from_millis(150)) - } - } - - fn finish_debounce(&mut self) { - let Some(item) = self.trigger.take() else { return }; - let (tx, rx) = helix_event::cancelation(); - self.request = Some(tx); - job::dispatch_blocking(move |editor, _| resolve_completion_item(editor, item, rx)) - } -} - -fn resolve_completion_item( - editor: &mut Editor, - item: CompletionItem, - cancel: helix_event::CancelRx, -) { - let Some(language_server) = editor.language_server_by_id(item.language_server_id) else { - return; - }; - - let Some(future) = language_server.resolve_completion_item(item.item.clone()) else { - return; - }; - - tokio::spawn(async move { - match helix_event::cancelable_future(future, cancel).await { - Some(Ok(resolved_item)) => { - job::dispatch(move |_, compositor| { - if let Some(completion) = &mut compositor - .find::() - .unwrap() - .completion - { - let resolved_item = CompletionItem { - item: resolved_item, - language_server_id: item.language_server_id, - resolved: true, - }; - - completion.replace_item(item, resolved_item); - }; - }) - .await - } - Some(Err(err)) => log::error!("completion resolve request failed: {err}"), - None => (), - } - }); -} diff --git a/helix-term/src/ui/menu.rs b/helix-term/src/ui/menu.rs index c0e60b33e344..f9f038e7ab34 100644 --- a/helix-term/src/ui/menu.rs +++ b/helix-term/src/ui/menu.rs @@ -241,9 +241,9 @@ impl Menu { } impl Menu { - pub fn replace_option(&mut self, old_option: T, new_option: T) { + pub fn replace_option(&mut self, old_option: &T, new_option: T) { for option in &mut self.options { - if old_option == *option { + if old_option == option { *option = new_option; break; }