From c314acb304857a8fe5032a133b7d13996cdd62f0 Mon Sep 17 00:00:00 2001 From: Jonatan Pettersson Date: Fri, 6 Sep 2024 20:56:48 +0200 Subject: [PATCH] Allow textDocument/didChange notification to be synchronous --- helix-lsp/src/client.rs | 68 +++++++++++++++-- helix-term/src/commands/lsp.rs | 125 ++++++++++++++++++++------------ helix-term/src/ui/completion.rs | 10 ++- helix-term/src/ui/editor.rs | 8 +- helix-view/src/document.rs | 67 ++++++++++++----- helix-view/src/handlers/lsp.rs | 44 +++++++++-- 6 files changed, 240 insertions(+), 82 deletions(-) diff --git a/helix-lsp/src/client.rs b/helix-lsp/src/client.rs index cc1c4ce8f..07a0b1e48 100644 --- a/helix-lsp/src/client.rs +++ b/helix-lsp/src/client.rs @@ -12,6 +12,7 @@ use crate::lsp::{ }; use helix_core::{find_workspace, syntax::LanguageServerFeature, ChangeSet, Rope}; use helix_loader::VERSION_AND_GIT_HASH; +use helix_lsp_types::TextDocumentContentChangeEvent; use helix_stdx::path; use parking_lot::Mutex; use serde::Deserialize; @@ -482,6 +483,28 @@ impl Client { } } + /// Send a RPC notification to the language server synchronously. + pub fn notify_sync(&self, params: R::Params) -> Result<()> + where + R::Params: serde::Serialize, + { + let server_tx = self.server_tx.clone(); + + let params = serde_json::to_value(params)?; + + let notification = jsonrpc::Notification { + jsonrpc: Some(jsonrpc::Version::V2), + method: R::METHOD.to_string(), + params: Self::value_into_params(params), + }; + + server_tx + .send(Payload::Notification(notification)) + .map_err(|e| Error::Other(e.into()))?; + + Ok(()) + } + /// Reply to a language server RPC call. pub fn reply( &self, @@ -930,6 +953,44 @@ impl Client { new_text: &Rope, changes: &ChangeSet, ) -> Option>> { + if let Some(changes) = self.text_document_did_change_impl(old_text, new_text, changes) { + return Some(self.notify::( + lsp::DidChangeTextDocumentParams { + text_document, + content_changes: changes, + }, + )); + }; + None + } + + /// Will send textDocument/didChange notificaiton synchronously + pub fn text_document_did_change_sync( + &self, + text_document: lsp::VersionedTextDocumentIdentifier, + old_text: &Rope, + new_text: &Rope, + changes: &ChangeSet, + ) -> Option> { + if let Some(changes) = self.text_document_did_change_impl(old_text, new_text, changes) { + return Some( + self.notify_sync::( + lsp::DidChangeTextDocumentParams { + text_document, + content_changes: changes, + }, + ), + ); + }; + None + } + + pub fn text_document_did_change_impl( + &self, + old_text: &Rope, + new_text: &Rope, + changes: &ChangeSet, + ) -> Option> { let capabilities = self.capabilities.get().unwrap(); // Return early if the server does not support document sync. @@ -961,12 +1022,7 @@ impl Client { kind => unimplemented!("{:?}", kind), }; - Some(self.notify::( - lsp::DidChangeTextDocumentParams { - text_document, - content_changes: changes, - }, - )) + Some(changes) } pub fn text_document_did_close( diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs index 9455d17b2..69172c96e 100644 --- a/helix-term/src/commands/lsp.rs +++ b/helix-term/src/commands/lsp.rs @@ -551,6 +551,11 @@ struct CodeActionOrCommandItem { language_server_id: LanguageServerId, } +struct CodeActionItem { + lsp_item: lsp::CodeAction, + language_server_id: LanguageServerId, +} + impl ui::menu::Item for CodeActionOrCommandItem { type Data = (); fn format(&self, _data: &Self::Data) -> Row { @@ -712,8 +717,36 @@ pub fn code_action(cx: &mut Context) { // always present here let action = action.unwrap(); + let Some(language_server) = editor.language_server_by_id(action.language_server_id) + else { + editor.set_error("Language Server disappeared"); + return; + }; + let offset_encoding = language_server.offset_encoding(); + + match &action.lsp_item { + lsp::CodeActionOrCommand::Command(command) => { + log::debug!("code action command: {:?}", command); + execute_lsp_command(editor, action.language_server_id, command.clone()); + } + lsp::CodeActionOrCommand::CodeAction(code_action) => { + log::debug!("code action: {:?}", code_action); + let resolved_code_action = + resolve_code_action(code_action, language_server); + + if let Some(ref workspace_edit) = + resolved_code_action.as_ref().unwrap_or(code_action).edit + { + let _ = editor.apply_workspace_edit(offset_encoding, workspace_edit); + } - apply_code_action(editor, action); + // if code action provides both edit and command first the edit + // should be applied and then the command + if let Some(command) = &code_action.command { + execute_lsp_command(editor, action.language_server_id, command.clone()); + } + } + } }); picker.move_down(); // pre-select the first item @@ -780,7 +813,7 @@ pub fn code_actions_on_save(cx: &mut compositor::Context, doc_id: &DocumentId) { log::debug!("Attempting code action on save {:?}", code_action_kind); let doc = doc!(cx.editor, doc_id); let full_range = Range::new(0, doc.text().len_chars()); - let code_actions: Vec = + let code_actions: Vec = code_actions_for_range(doc, full_range, Some(vec![code_action_kind.clone()])) .into_iter() .filter_map(|(future, language_server_id)| { @@ -808,10 +841,15 @@ pub fn code_actions_on_save(cx: &mut compositor::Context, doc_id: &DocumentId) { // Use the first matching code action if let Some(lsp_item) = actions.first() { - return Some(CodeActionOrCommandItem { - lsp_item: lsp_item.clone(), - language_server_id, - }); + return match lsp_item { + CodeActionOrCommand::CodeAction(code_action) => { + Some(CodeActionItem { + lsp_item: code_action.clone(), + language_server_id, + }) + } + _ => None, + }; } } } @@ -831,55 +869,52 @@ pub fn code_actions_on_save(cx: &mut compositor::Context, doc_id: &DocumentId) { code_action.lsp_item, code_action.language_server_id ); - apply_code_action(cx.editor, &code_action); + let Some(language_server) = cx + .editor + .language_server_by_id(code_action.language_server_id) + else { + log::error!( + "Language server disappeared {:?}", + code_action.language_server_id + ); + continue; + }; - // TODO: Find a better way to handle this - // Sleep to avoid race condition between next code action/auto-format - // and the textDocument/didChange notification - std::thread::sleep(std::time::Duration::from_millis(10)); + let offset_encoding = language_server.offset_encoding(); + + let resolved_code_action = + resolve_code_action(&code_action.lsp_item, language_server); + + if let Some(ref workspace_edit) = resolved_code_action + .as_ref() + .unwrap_or(&code_action.lsp_item) + .edit + { + let _ = cx + .editor + .apply_workspace_edit_sync(offset_encoding, workspace_edit); + } } } } } -fn apply_code_action(editor: &mut Editor, action: &CodeActionOrCommandItem) { - let Some(language_server) = editor.language_server_by_id(action.language_server_id) else { - editor.set_error("Language Server disappeared"); - return; - }; - let offset_encoding = language_server.offset_encoding(); - - match &action.lsp_item { - lsp::CodeActionOrCommand::Command(command) => { - log::debug!("code action command: {:?}", command); - execute_lsp_command(editor, action.language_server_id, command.clone()); - } - lsp::CodeActionOrCommand::CodeAction(code_action) => { - log::debug!("code action: {:?}", code_action); - // we support lsp "codeAction/resolve" for `edit` and `command` fields - let mut resolved_code_action = None; - if code_action.edit.is_none() || code_action.command.is_none() { - if let Some(future) = language_server.resolve_code_action(code_action.clone()) { - if let Ok(response) = helix_lsp::block_on(future) { - if let Ok(code_action) = serde_json::from_value::(response) { - resolved_code_action = Some(code_action); - } - } +pub fn resolve_code_action( + code_action: &CodeAction, + language_server: &Client, +) -> Option { + // we support lsp "codeAction/resolve" for `edit` and `command` fields + let mut resolved_code_action = None; + if code_action.edit.is_none() || code_action.command.is_none() { + if let Some(future) = language_server.resolve_code_action(code_action.clone()) { + if let Ok(response) = helix_lsp::block_on(future) { + if let Ok(code_action) = serde_json::from_value::(response) { + resolved_code_action = Some(code_action); } } - let resolved_code_action = resolved_code_action.as_ref().unwrap_or(code_action); - - if let Some(ref workspace_edit) = resolved_code_action.edit { - let _ = editor.apply_workspace_edit(offset_encoding, workspace_edit); - } - - // if code action provides both edit and command first the edit - // should be applied and then the command - if let Some(command) = &code_action.command { - execute_lsp_command(editor, action.language_server_id, command.clone()); - } } } + resolved_code_action } pub fn execute_lsp_command( diff --git a/helix-term/src/ui/completion.rs b/helix-term/src/ui/completion.rs index 14397bb5c..c34b12fb2 100644 --- a/helix-term/src/ui/completion.rs +++ b/helix-term/src/ui/completion.rs @@ -253,7 +253,7 @@ impl Completion { }) } // if more text was entered, remove it - doc.restore(view, &savepoint, false); + doc.restore(view, &savepoint, None); // always present here let item = item.unwrap(); @@ -273,7 +273,7 @@ impl Completion { if let Some(CompleteAction::Selected { savepoint }) = editor.last_completion.take() { - doc.restore(view, &savepoint, false); + doc.restore(view, &savepoint, None); } // always present here let mut item = item.unwrap().clone(); @@ -289,7 +289,11 @@ impl Completion { } }; // if more text was entered, remove it - doc.restore(view, &savepoint, true); + doc.restore( + view, + &savepoint, + Some(helix_view::document::EmitLspNotification::Async), + ); // save an undo checkpoint before the completion doc.append_changes_to_history(view); let transaction = item_to_transaction( diff --git a/helix-term/src/ui/editor.rs b/helix-term/src/ui/editor.rs index f7541fe25..a8b1b4693 100644 --- a/helix-term/src/ui/editor.rs +++ b/helix-term/src/ui/editor.rs @@ -974,7 +974,11 @@ impl EditorView { let (view, doc) = current!(cxt.editor); if let Some(last_savepoint) = last_savepoint.as_deref() { - doc.restore(view, last_savepoint, true); + doc.restore( + view, + last_savepoint, + Some(helix_view::document::EmitLspNotification::Async), + ); } let text = doc.text().slice(..); @@ -1063,7 +1067,7 @@ impl EditorView { }), CompleteAction::Selected { savepoint } => { let (view, doc) = current!(editor); - doc.restore(view, &savepoint, false); + doc.restore(view, &savepoint, None); } } } diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 91ec27874..d491206da 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -130,6 +130,12 @@ pub enum DocumentOpenError { IoError(#[from] io::Error), } +#[derive(Debug)] +pub enum EmitLspNotification { + Async, + Sync, +} + pub struct Document { pub(crate) id: DocumentId, text: Rope, @@ -1269,7 +1275,7 @@ impl Document { &mut self, transaction: &Transaction, view_id: ViewId, - emit_lsp_notification: bool, + emit_lsp_notification: Option, ) -> bool { use helix_core::Assoc; @@ -1426,19 +1432,31 @@ impl Document { }); } - if emit_lsp_notification { - // TODO: move to hook - // emit lsp notification + // emit lsp notification + if let Some(emit_lsp_notification) = emit_lsp_notification { for language_server in self.language_servers() { - let notify = language_server.text_document_did_change( - self.versioned_identifier(), - &old_doc, - self.text(), - changes, - ); - - if let Some(notify) = notify { - tokio::spawn(notify); + match emit_lsp_notification { + EmitLspNotification::Async => { + // TODO: move to hook + let notify = language_server.text_document_did_change( + self.versioned_identifier(), + &old_doc, + self.text(), + changes, + ); + + if let Some(notify) = notify { + tokio::spawn(notify); + } + } + EmitLspNotification::Sync => { + let _ = language_server.text_document_did_change_sync( + self.versioned_identifier(), + &old_doc, + self.text(), + changes, + ); + } } } } @@ -1450,7 +1468,7 @@ impl Document { &mut self, transaction: &Transaction, view_id: ViewId, - emit_lsp_notification: bool, + emit_lsp_notification: Option, ) -> bool { // store the state just before any changes are made. This allows us to undo to the // state just before a transaction was applied. @@ -1473,14 +1491,20 @@ impl Document { } /// Apply a [`Transaction`] to the [`Document`] to change its text. pub fn apply(&mut self, transaction: &Transaction, view_id: ViewId) -> bool { - self.apply_inner(transaction, view_id, true) + self.apply_inner(transaction, view_id, Some(EmitLspNotification::Async)) + } + + /// Apply a [`Transaction`] to the [`Document`] to change its text and + /// emit the lsp notifcation synchronously + pub fn apply_sync_notification(&mut self, transaction: &Transaction, view_id: ViewId) -> bool { + self.apply_inner(transaction, view_id, Some(EmitLspNotification::Sync)) } /// Apply a [`Transaction`] to the [`Document`] to change its text /// without notifying the language servers. This is useful for temporary transactions /// that must not influence the server. pub fn apply_temporary(&mut self, transaction: &Transaction, view_id: ViewId) -> bool { - self.apply_inner(transaction, view_id, false) + self.apply_inner(transaction, view_id, None) } fn undo_redo_impl(&mut self, view: &mut View, undo: bool) -> bool { @@ -1492,7 +1516,7 @@ impl Document { let mut history = self.history.take(); let txn = if undo { history.undo() } else { history.redo() }; let success = if let Some(txn) = txn { - self.apply_impl(txn, view.id, true) + self.apply_impl(txn, view.id, Some(EmitLspNotification::Async)) } else { false }; @@ -1548,7 +1572,12 @@ impl Document { savepoint } - pub fn restore(&mut self, view: &mut View, savepoint: &SavePoint, emit_lsp_notification: bool) { + pub fn restore( + &mut self, + view: &mut View, + savepoint: &SavePoint, + emit_lsp_notification: Option, + ) { assert_eq!( savepoint.view, view.id, "Savepoint must not be used with a different view!" @@ -1581,7 +1610,7 @@ impl Document { }; let mut success = false; for txn in txns { - if self.apply_impl(&txn, view.id, true) { + if self.apply_impl(&txn, view.id, Some(EmitLspNotification::Async)) { success = true; } } diff --git a/helix-view/src/handlers/lsp.rs b/helix-view/src/handlers/lsp.rs index 6aff2e50c..42e68ee95 100644 --- a/helix-view/src/handlers/lsp.rs +++ b/helix-view/src/handlers/lsp.rs @@ -93,6 +93,7 @@ impl Editor { version: Option, text_edits: Vec, offset_encoding: OffsetEncoding, + sync_lsp_notification: bool, ) -> Result<(), ApplyEditErrorKind> { let uri = match Uri::try_from(url) { Ok(uri) => uri, @@ -133,16 +134,37 @@ impl Editor { let transaction = generate_transaction_from_edits(doc.text(), text_edits, offset_encoding); let view = view_mut!(self, view_id); - doc.apply(&transaction, view.id); + if sync_lsp_notification { + doc.apply_sync_notification(&transaction, view.id); + } else { + doc.apply(&transaction, view.id); + } doc.append_changes_to_history(view); Ok(()) } - // TODO make this transactional (and set failureMode to transactional) pub fn apply_workspace_edit( &mut self, offset_encoding: OffsetEncoding, workspace_edit: &lsp::WorkspaceEdit, + ) -> Result<(), ApplyEditError> { + self.apply_workspace_edit_impl(offset_encoding, workspace_edit, false) + } + + pub fn apply_workspace_edit_sync( + &mut self, + offset_encoding: OffsetEncoding, + workspace_edit: &lsp::WorkspaceEdit, + ) -> Result<(), ApplyEditError> { + self.apply_workspace_edit_impl(offset_encoding, workspace_edit, true) + } + + // TODO make this transactional (and set failureMode to transactional) + fn apply_workspace_edit_impl( + &mut self, + offset_encoding: OffsetEncoding, + workspace_edit: &lsp::WorkspaceEdit, + sync_lsp_notificaiton: bool, ) -> Result<(), ApplyEditError> { if let Some(ref document_changes) = workspace_edit.document_changes { match document_changes { @@ -164,6 +186,7 @@ impl Editor { document_edit.text_document.version, edits, offset_encoding, + sync_lsp_notificaiton, ) .map_err(|kind| ApplyEditError { kind, @@ -201,6 +224,7 @@ impl Editor { document_edit.text_document.version, edits, offset_encoding, + sync_lsp_notificaiton, ) .map_err(|kind| { ApplyEditError { @@ -221,11 +245,17 @@ impl Editor { log::debug!("workspace changes: {:?}", changes); for (i, (uri, text_edits)) in changes.iter().enumerate() { let text_edits = text_edits.to_vec(); - self.apply_text_edits(uri, None, text_edits, offset_encoding) - .map_err(|kind| ApplyEditError { - kind, - failed_change_idx: i, - })?; + self.apply_text_edits( + uri, + None, + text_edits, + offset_encoding, + sync_lsp_notificaiton, + ) + .map_err(|kind| ApplyEditError { + kind, + failed_change_idx: i, + })?; } }