From af5a16f6e3c2c95141b60395ca39d0c8de9edd1b Mon Sep 17 00:00:00 2001 From: Jonatan Pettersson Date: Thu, 8 Aug 2024 19:59:52 +0200 Subject: [PATCH 1/7] Add code actions on save * Add code-actions-on-save config * Match VS Code config to allow future flexibility * Refactor lsp commands to allow for code reuse * Attempt code actions for all language servers for the document * Add lsp specific integration tests * Update documentation in book * Canonicalize path argument when retrieving documents by path * Resolves issue when running lsp integration tests in windows --- .cargo/config.toml | 1 + .github/workflows/build.yml | 11 + book/src/languages.md | 1 + docs/CONTRIBUTING.md | 8 + helix-core/src/syntax.rs | 9 + helix-term/Cargo.toml | 1 + helix-term/src/commands/lsp.rs | 247 ++++++++++++------ helix-term/src/commands/typed.rs | 29 +- helix-term/tests/integration-lsp.rs | 18 ++ .../tests/test/lsp/code_actions_on_save.rs | 223 ++++++++++++++++ helix-term/tests/test/lsp/mod.rs | 3 + helix-view/src/editor.rs | 6 +- 12 files changed, 468 insertions(+), 89 deletions(-) create mode 100644 helix-term/tests/integration-lsp.rs create mode 100644 helix-term/tests/test/lsp/code_actions_on_save.rs create mode 100644 helix-term/tests/test/lsp/mod.rs diff --git a/.cargo/config.toml b/.cargo/config.toml index af4312dc8790..962e844d519d 100644 --- a/.cargo/config.toml +++ b/.cargo/config.toml @@ -14,4 +14,5 @@ rustflags = ["--cfg", "tokio_unstable", "-C", "target-feature=-crt-static"] [alias] xtask = "run --package xtask --" integration-test = "test --features integration --profile integration --workspace --test integration" +integration-test-lsp = "test --features integration-lsp --profile integration --workspace --test integration-lsp -- --test-threads=1" diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 93fcb98164df..8714d3a1bb1a 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -51,12 +51,23 @@ jobs: key: ${{ runner.os }}-stable-v${{ env.CACHE_VERSION }}-tree-sitter-grammars-${{ hashFiles('languages.toml') }} restore-keys: ${{ runner.os }}-stable-v${{ env.CACHE_VERSION }}-tree-sitter-grammars- + - name: Install go lsp integration tests + uses: actions/setup-go@v5 + with: + go-version: '^1.22.0' + + - name: Install gopls for lsp integration tests + run: go install golang.org/x/tools/gopls@latest + - name: Run cargo test run: cargo test --workspace - name: Run cargo integration-test run: cargo integration-test + - name: Run cargo integration-test-lsp + run: cargo integration-test-lsp + strategy: matrix: os: [ubuntu-latest, macos-latest, windows-latest] diff --git a/book/src/languages.md b/book/src/languages.md index fe105cced820..ac3d4a42a79f 100644 --- a/book/src/languages.md +++ b/book/src/languages.md @@ -71,6 +71,7 @@ These configuration keys are available: | `text-width` | Maximum line length. Used for the `:reflow` command and soft-wrapping if `soft-wrap.wrap-at-text-width` is set, defaults to `editor.text-width` | | `workspace-lsp-roots` | Directories relative to the workspace root that are treated as LSP roots. Should only be set in `.helix/config.toml`. Overwrites the setting of the same name in `config.toml` if set. | | `persistent-diagnostic-sources` | An array of LSP diagnostic sources assumed unchanged when the language server resends the same set of diagnostics. Helix can track the position for these diagnostics internally instead. Useful for diagnostics that are recomputed on save. +| `code-actions-on-save` | List of LSP code actions to be run in order on save, for example `[{ code-action = "source.organizeImports", enabled = true }]` | ### File-type detection and the `file-types` key diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index 86caff717a63..ed621fb24532 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -57,6 +57,14 @@ Contributors using MacOS might encounter `Too many open files (os error 24)` failures while running integration tests. This can be resolved by increasing the default value (e.g. to `10240` from `256`) by running `ulimit -n 10240`. +### Language Server tests + +There are integration tests specific for language server integration that can be +run with `cargo integration-test-lsp` and have additional dependencies. + +* [go](https://go.dev) +* [gopls](https://pkg.go.dev/golang.org/x/tools/gopls) + ## Minimum Stable Rust Version (MSRV) Policy Helix follows the MSRV of Firefox. diff --git a/helix-core/src/syntax.rs b/helix-core/src/syntax.rs index 7be512f52e2c..d5be494da9de 100644 --- a/helix-core/src/syntax.rs +++ b/helix-core/src/syntax.rs @@ -118,6 +118,8 @@ pub struct LanguageConfiguration { pub block_comment_tokens: Option>, pub text_width: Option, pub soft_wrap: Option, + #[serde(default)] + pub code_actions_on_save: Option>, // List of LSP code actions to be run in order upon saving #[serde(default)] pub auto_format: bool, @@ -490,6 +492,13 @@ pub struct AdvancedCompletion { pub default: Option, } +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(rename_all = "kebab-case")] +pub struct CodeActionsOnSave { + pub code_action: String, + pub enabled: bool, +} + #[derive(Debug, PartialEq, Eq, Clone, Deserialize, Serialize)] #[serde(rename_all = "kebab-case", untagged)] pub enum DebugConfigCompletion { diff --git a/helix-term/Cargo.toml b/helix-term/Cargo.toml index 5f691d44a20e..ec7b2dadd844 100644 --- a/helix-term/Cargo.toml +++ b/helix-term/Cargo.toml @@ -16,6 +16,7 @@ homepage.workspace = true default = ["git"] unicode-lines = ["helix-core/unicode-lines", "helix-view/unicode-lines"] integration = ["helix-event/integration_test"] +integration-lsp = ["integration"] git = ["helix-vcs/git"] [[bin]] diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs index 93ac2a849f79..e7e0e37428b4 100644 --- a/helix-term/src/commands/lsp.rs +++ b/helix-term/src/commands/lsp.rs @@ -1,20 +1,24 @@ -use futures_util::{stream::FuturesOrdered, FutureExt}; +use futures_util::{ + stream::{FuturesOrdered, FuturesUnordered}, + FutureExt, +}; use helix_lsp::{ block_on, lsp::{ - self, CodeAction, CodeActionOrCommand, CodeActionTriggerKind, DiagnosticSeverity, - NumberOrString, + self, CodeAction, CodeActionKind, CodeActionOrCommand, CodeActionTriggerKind, + DiagnosticSeverity, NumberOrString, }, util::{diagnostic_to_lsp_diagnostic, lsp_range_to_range, range_to_lsp_range}, Client, LanguageServerId, OffsetEncoding, }; +use serde_json::Value; use tokio_stream::StreamExt; use tui::{text::Span, widgets::Row}; use super::{align_view, push_jump, Align, Context, Editor}; use helix_core::{ - syntax::LanguageServerFeature, text_annotations::InlineAnnotation, Selection, Uri, + syntax::LanguageServerFeature, text_annotations::InlineAnnotation, Range, Selection, Uri, }; use helix_stdx::path; use helix_view::{ @@ -22,7 +26,7 @@ use helix_view::{ editor::Action, handlers::lsp::SignatureHelpInvoked, theme::Style, - Document, View, + Document, DocumentId, View, }; use crate::{ @@ -542,9 +546,9 @@ pub fn workspace_diagnostics_picker(cx: &mut Context) { cx.push_layer(Box::new(overlaid(picker))); } -struct CodeActionOrCommandItem { - lsp_item: lsp::CodeActionOrCommand, - language_server_id: LanguageServerId, +pub struct CodeActionOrCommandItem { + pub lsp_item: lsp::CodeActionOrCommand, + pub language_server_id: LanguageServerId, } impl ui::menu::Item for CodeActionOrCommandItem { @@ -619,34 +623,8 @@ pub fn code_action(cx: &mut Context) { let selection_range = doc.selection(view.id).primary(); - let mut seen_language_servers = HashSet::new(); - - let mut futures: FuturesOrdered<_> = doc - .language_servers_with_feature(LanguageServerFeature::CodeAction) - .filter(|ls| seen_language_servers.insert(ls.id())) - // TODO this should probably already been filtered in something like "language_servers_with_feature" - .filter_map(|language_server| { - let offset_encoding = language_server.offset_encoding(); - let language_server_id = language_server.id(); - let range = range_to_lsp_range(doc.text(), selection_range, offset_encoding); - // Filter and convert overlapping diagnostics - let code_action_context = lsp::CodeActionContext { - diagnostics: doc - .diagnostics() - .iter() - .filter(|&diag| { - selection_range - .overlaps(&helix_core::Range::new(diag.range.start, diag.range.end)) - }) - .map(|diag| diagnostic_to_lsp_diagnostic(doc.text(), diag, offset_encoding)) - .collect(), - only: None, - trigger_kind: Some(CodeActionTriggerKind::INVOKED), - }; - let code_action_request = - language_server.code_actions(doc.identifier(), range, code_action_context)?; - Some((code_action_request, language_server_id)) - }) + let mut futures: FuturesUnordered<_> = code_actions_for_range(doc, selection_range, None) + .into_iter() .map(|(request, ls_id)| async move { let json = request.await?; let response: Option = serde_json::from_value(json)?; @@ -734,59 +712,174 @@ 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); - // 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()) + apply_code_action(editor, action); + }); + picker.move_down(); // pre-select the first item + + let popup = Popup::new("code-action", picker).with_scrollbar(false); + + compositor.replace_or_push("code-action", popup); + }; + + Ok(Callback::EditorCompositor(Box::new(call))) + }); +} + +pub fn code_actions_for_range( + doc: &Document, + range: helix_core::Range, + only: Option>, +) -> Vec<( + impl Future>, + LanguageServerId, +)> { + let mut seen_language_servers = HashSet::new(); + + doc.language_servers_with_feature(LanguageServerFeature::CodeAction) + .filter(|ls| seen_language_servers.insert(ls.id())) + // TODO this should probably already been filtered in something like "language_servers_with_feature" + .filter_map(|language_server| { + let offset_encoding = language_server.offset_encoding(); + let language_server_id = language_server.id(); + let lsp_range = range_to_lsp_range(doc.text(), range, offset_encoding); + // Filter and convert overlapping diagnostics + let code_action_context = lsp::CodeActionContext { + diagnostics: doc + .diagnostics() + .iter() + .filter(|&diag| { + range.overlaps(&helix_core::Range::new(diag.range.start, diag.range.end)) + }) + .map(|diag| diagnostic_to_lsp_diagnostic(doc.text(), diag, offset_encoding)) + .collect(), + only: only.clone(), + trigger_kind: Some(CodeActionTriggerKind::INVOKED), + }; + let code_action_request = + language_server.code_actions(doc.identifier(), lsp_range, code_action_context)?; + Some((code_action_request, language_server_id)) + }) + .collect::>() +} + +/// Will apply the code actions on save from the language config for each language server +pub fn code_actions_on_save(cx: &mut compositor::Context, doc_id: &DocumentId) { + let doc = doc!(cx.editor, doc_id); + + let code_actions_on_save_cfg = doc + .language_config() + .and_then(|c| c.code_actions_on_save.clone()); + + if let Some(code_actions_on_save_cfg) = code_actions_on_save_cfg { + for code_action_kind in code_actions_on_save_cfg.into_iter().filter_map(|action| { + action + .enabled + .then_some(CodeActionKind::from(action.code_action)) + }) { + 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 = + code_actions_for_range(doc, full_range, Some(vec![code_action_kind.clone()])) + .into_iter() + .filter_map(|(future, language_server_id)| { + if let Ok(json) = helix_lsp::block_on(future) { + if let Ok(Some(mut actions)) = serde_json::from_value::< + Option, + >(json) { - 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); - } + // Retain only enabled code actions that do not have commands. + // + // Commands are deprecated and are not supported because they apply + // workspace edits asynchronously and there is currently no mechanism + // to handle waiting for the workspace edits to be applied before moving + // on to the next code action (or auto-format). + actions.retain(|action| { + matches!( + action, + CodeActionOrCommand::CodeAction(CodeAction { + disabled: None, + command: None, + .. + }) + ) + }); + + // Use the first matching code action + if let Some(lsp_item) = actions.first() { + return Some(CodeActionOrCommandItem { + lsp_item: lsp_item.clone(), + language_server_id, + }); } } } - let resolved_code_action = - resolved_code_action.as_ref().unwrap_or(code_action); + None + }) + .collect(); - if let Some(ref workspace_edit) = resolved_code_action.edit { - let _ = editor.apply_workspace_edit(offset_encoding, workspace_edit); - } + if code_actions.is_empty() { + log::debug!("Code action on save not found {:?}", code_action_kind); + cx.editor + .set_error(format!("Code Action not found: {:?}", code_action_kind)); + } - // 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()); + for code_action in code_actions { + log::debug!( + "Applying code action on save {:?} for language server {:?}", + code_action.lsp_item, + code_action.language_server_id + ); + apply_code_action(cx.editor, &code_action); + + // 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(1000)); + } + } + } +} + +pub 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); } } } - }); - picker.move_down(); // pre-select the first item - - let popup = Popup::new("code-action", picker).with_scrollbar(false); + } + let resolved_code_action = resolved_code_action.as_ref().unwrap_or(code_action); - compositor.replace_or_push("code-action", popup); - }; + if let Some(ref workspace_edit) = resolved_code_action.edit { + let _ = editor.apply_workspace_edit(offset_encoding, workspace_edit); + } - Ok(Callback::EditorCompositor(Box::new(call))) - }); + // 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()); + } + } + } } pub fn execute_lsp_command( diff --git a/helix-term/src/commands/typed.rs b/helix-term/src/commands/typed.rs index 7ad0369fc1bd..98a307d867e4 100644 --- a/helix-term/src/commands/typed.rs +++ b/helix-term/src/commands/typed.rs @@ -12,6 +12,7 @@ use helix_core::{line_ending, shellwords::Shellwords}; use helix_view::document::{read_to_string, DEFAULT_LANGUAGE_NAME}; use helix_view::editor::{CloseError, ConfigEvent}; use serde_json::Value; + use ui::completers::{self, Completer}; #[derive(Clone)] @@ -334,36 +335,41 @@ fn write_impl( force: bool, ) -> anyhow::Result<()> { let config = cx.editor.config(); - let jobs = &mut cx.jobs; - let (view, doc) = current!(cx.editor); let path = path.map(AsRef::as_ref); + let (view, doc) = current!(cx.editor); + let doc_id = doc.id(); + let view_id = view.id; + if config.insert_final_newline { - insert_final_newline(doc, view.id); + insert_final_newline(doc, view_id); } // Save an undo checkpoint for any outstanding changes. doc.append_changes_to_history(view); + code_actions_on_save(cx, &doc_id); + let fmt = if config.auto_format { + let doc = doc!(cx.editor, &doc_id); doc.auto_format().map(|fmt| { let callback = make_format_callback( doc.id(), doc.version(), - view.id, + view_id, fmt, Some((path.map(Into::into), force)), ); - jobs.add(Job::with_callback(callback).wait_before_exiting()); + cx.jobs + .add(Job::with_callback(callback).wait_before_exiting()); }) } else { None }; if fmt.is_none() { - let id = doc.id(); - cx.editor.save(id, path, force)?; + cx.editor.save(doc_id, path, force)?; } Ok(()) @@ -677,7 +683,6 @@ pub fn write_all_impl( ) -> anyhow::Result<()> { let mut errors: Vec<&'static str> = Vec::new(); let config = cx.editor.config(); - let jobs = &mut cx.jobs; let saves: Vec<_> = cx .editor .documents @@ -708,13 +713,16 @@ pub fn write_all_impl( let view = view_mut!(cx.editor, target_view); if config.insert_final_newline { - insert_final_newline(doc, target_view); + insert_final_newline(doc, view.id); } // Save an undo checkpoint for any outstanding changes. doc.append_changes_to_history(view); + code_actions_on_save(cx, &doc_id); + let fmt = if config.auto_format { + let doc = doc!(cx.editor, &doc_id); doc.auto_format().map(|fmt| { let callback = make_format_callback( doc_id, @@ -723,7 +731,8 @@ pub fn write_all_impl( fmt, Some((None, force)), ); - jobs.add(Job::with_callback(callback).wait_before_exiting()); + cx.jobs + .add(Job::with_callback(callback).wait_before_exiting()); }) } else { None diff --git a/helix-term/tests/integration-lsp.rs b/helix-term/tests/integration-lsp.rs new file mode 100644 index 000000000000..bd032368b31c --- /dev/null +++ b/helix-term/tests/integration-lsp.rs @@ -0,0 +1,18 @@ +#[cfg(feature = "integration-lsp")] +mod test { + mod helpers; + + use helix_term::config::Config; + + use indoc::indoc; + + use self::helpers::*; + + #[tokio::test(flavor = "multi_thread")] + async fn hello_world() -> anyhow::Result<()> { + test(("#[\n|]#", "ihello world", "hello world#[|\n]#")).await?; + Ok(()) + } + + mod lsp; +} diff --git a/helix-term/tests/test/lsp/code_actions_on_save.rs b/helix-term/tests/test/lsp/code_actions_on_save.rs new file mode 100644 index 000000000000..83936261f5da --- /dev/null +++ b/helix-term/tests/test/lsp/code_actions_on_save.rs @@ -0,0 +1,223 @@ +use helix_term::application::Application; +use std::{ + io::Read, + path::{Path, PathBuf}, +}; + +use super::*; + +// Check that we have gopls available while also allowing +// for gopls to initialize +fn assert_gopls(app: &Application, path: &Path) { + let doc = app.editor.document_by_path(path); + let mut ls = None; + let mut initialized = false; + if let Some(doc) = doc { + for _ in 0..10 { + ls = doc.language_servers().find(|s| s.name() == "gopls"); + + if let Some(gopls) = ls { + if gopls.is_initialized() { + initialized = true; + // TODO: Make this deterministic + // Sleep to give time to send textDocument/didOpen notification + std::thread::sleep(std::time::Duration::from_millis(1000)); + break; + } + } + + std::thread::sleep(std::time::Duration::from_millis(100)); + } + } + assert!( + doc.is_some(), + "doc not found {:?} in {:?}", + path, + app.editor + .documents + .iter() + .filter_map(|(_, d)| d.path()) + .collect::>() + ); + assert!(ls.is_some(), "gopls language server not found"); + assert!(initialized, "gopls language server not initialized in time"); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_organize_imports_go() -> anyhow::Result<()> { + let lang_conf = indoc! {r#" + [[language]] + name = "go" + code-actions-on-save = [{ code-action = "source.organizeImports", enabled = true }] + indent = { tab-width = 4, unit = " " } + "#}; + + let text = indoc! {r#" + #[p|]#ackage main + + import "fmt" + + import "path" + + func main() { + fmt.Println("a") + path.Join("b") + } + "#}; + + let dir = tempfile::Builder::new().tempdir()?; + let mut file = tempfile::Builder::new().suffix(".go").tempfile_in(&dir)?; + let mut app = helpers::AppBuilder::new() + .with_config(Config::default()) + .with_lang_loader(helpers::test_syntax_loader(Some(lang_conf.into()))) + .with_file(file.path(), None) + .with_input_text(text) + .build()?; + + test_key_sequences( + &mut app, + vec![ + ( + None, + Some(&|app| { + assert_gopls(app, file.path()); + }), + ), + (Some(":w"), None), + ], + false, + ) + .await?; + + assert_file_has_content( + &mut file, + "package main\n\nimport (\n\t\"fmt\"\n\t\"path\"\n)\n\nfunc main() {\n\tfmt.Println(\"a\")\n\tpath.Join(\"b\")\n}\n" + )?; + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_organize_imports_go_write_all_quit() -> anyhow::Result<()> { + let lang_conf = indoc! {r#" + [[language]] + name = "go" + code-actions-on-save = [{ code-action = "source.organizeImports", enabled = true }] + "#}; + + let text = indoc! {r#" + #[p|]#ackage main + + import "path" + import "fmt" + + func main() { + fmt.Println("a") + path.Join("b") + } + "#}; + + let dir = tempfile::Builder::new().tempdir()?; + let mut file1 = tempfile::Builder::new().suffix(".go").tempfile_in(&dir)?; + let mut file2 = tempfile::Builder::new().suffix(".go").tempfile_in(&dir)?; + let mut app = helpers::AppBuilder::new() + .with_config(Config::default()) + .with_lang_loader(helpers::test_syntax_loader(Some(lang_conf.into()))) + .with_file(file1.path(), None) + .with_input_text(text) + .build()?; + + test_key_sequences( + &mut app, + vec![ + ( + Some(&format!( + ":o {}ipackage mainimport \"fmt\"func test()", + file2.path().to_string_lossy(), + )), + None, + ), + ( + None, + Some(&|app| { + assert_gopls(app, file1.path()); + assert_gopls(app, file2.path()); + }), + ), + (Some(":wqa"), None), + ], + true, + ) + .await?; + + assert_file_has_content( + &mut file1, + "package main\n\nimport (\n\t\"fmt\"\n\t\"path\"\n)\n\nfunc main() {\n\tfmt.Println(\"a\")\n\tpath.Join(\"b\")\n}\n", + )?; + + assert_file_has_content(&mut file2, "package main\n\nfunc test()\n")?; + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_invalid_code_action_go() -> anyhow::Result<()> { + let lang_conf = indoc! {r#" + [[language]] + name = "go" + code-actions-on-save = [{ code-action = "source.invalid", enabled = true }] + "#}; + + let text = indoc! {r#" + #[p|]#ackage main + + import "fmt" + + import "path" + + func main() { + fmt.Println("a") + path.Join("b") + } + "#}; + + let dir = tempfile::Builder::new().tempdir()?; + let mut file = tempfile::Builder::new().suffix(".go").tempfile_in(&dir)?; + let mut app = helpers::AppBuilder::new() + .with_config(Config::default()) + .with_lang_loader(helpers::test_syntax_loader(Some(lang_conf.into()))) + .with_file(file.path(), None) + .with_input_text(text) + .build()?; + + test_key_sequences( + &mut app, + vec![ + ( + None, + Some(&|app| { + assert_gopls(app, file.path()); + }), + ), + ( + Some(":w"), + Some(&|app| { + assert!(!app.editor.is_err(), "error: {:?}", app.editor.get_status()); + }), + ), + ], + false, + ) + .await?; + + reload_file(&mut file).unwrap(); + let mut file_content = String::new(); + file.as_file_mut().read_to_string(&mut file_content)?; + + assert_file_has_content( + &mut file, + "package main\n\nimport \"fmt\"\n\nimport \"path\"\n\nfunc main() {\n\tfmt.Println(\"a\")\n\tpath.Join(\"b\")\n}\n", + )?; + + Ok(()) +} diff --git a/helix-term/tests/test/lsp/mod.rs b/helix-term/tests/test/lsp/mod.rs new file mode 100644 index 000000000000..4f90acb884fa --- /dev/null +++ b/helix-term/tests/test/lsp/mod.rs @@ -0,0 +1,3 @@ +use super::*; + +mod code_actions_on_save; diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs index 1708b3b4e053..c8eff4997146 100644 --- a/helix-view/src/editor.rs +++ b/helix-view/src/editor.rs @@ -1929,13 +1929,15 @@ impl Editor { } pub fn document_by_path>(&self, path: P) -> Option<&Document> { + let path = helix_stdx::path::canonicalize(path); self.documents() - .find(|doc| doc.path().map(|p| p == path.as_ref()).unwrap_or(false)) + .find(|doc| doc.path().map(|p| p == &path).unwrap_or(false)) } pub fn document_by_path_mut>(&mut self, path: P) -> Option<&mut Document> { + let path = helix_stdx::path::canonicalize(path); self.documents_mut() - .find(|doc| doc.path().map(|p| p == path.as_ref()).unwrap_or(false)) + .find(|doc| doc.path().map(|p| p == &path).unwrap_or(false)) } /// Returns all supported diagnostics for the document From b3bf1c61ad07c1b8ac6710701001c61536b97100 Mon Sep 17 00:00:00 2001 From: Jonatan Pettersson Date: Tue, 3 Sep 2024 09:54:34 +0200 Subject: [PATCH 2/7] Use custom idle timeout in tests --- helix-term/src/commands/lsp.rs | 2 +- .../tests/test/lsp/code_actions_on_save.rs | 34 ++++++++++++++++--- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs index e7e0e37428b4..caa698b1a24d 100644 --- a/helix-term/src/commands/lsp.rs +++ b/helix-term/src/commands/lsp.rs @@ -836,7 +836,7 @@ pub fn code_actions_on_save(cx: &mut compositor::Context, doc_id: &DocumentId) { // 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(1000)); + std::thread::sleep(std::time::Duration::from_millis(100)); } } } diff --git a/helix-term/tests/test/lsp/code_actions_on_save.rs b/helix-term/tests/test/lsp/code_actions_on_save.rs index 83936261f5da..b5cf5a43f648 100644 --- a/helix-term/tests/test/lsp/code_actions_on_save.rs +++ b/helix-term/tests/test/lsp/code_actions_on_save.rs @@ -6,6 +6,9 @@ use std::{ use super::*; +// Give time to send textDocument/didOpen notification +const IDLE_TIMEOUT: std::time::Duration = std::time::Duration::from_millis(500); + // Check that we have gopls available while also allowing // for gopls to initialize fn assert_gopls(app: &Application, path: &Path) { @@ -21,7 +24,7 @@ fn assert_gopls(app: &Application, path: &Path) { initialized = true; // TODO: Make this deterministic // Sleep to give time to send textDocument/didOpen notification - std::thread::sleep(std::time::Duration::from_millis(1000)); + // std::thread::sleep(std::time::Duration::from_millis(IDLE_TIMEOUT)); break; } } @@ -68,7 +71,13 @@ async fn test_organize_imports_go() -> anyhow::Result<()> { let dir = tempfile::Builder::new().tempdir()?; let mut file = tempfile::Builder::new().suffix(".go").tempfile_in(&dir)?; let mut app = helpers::AppBuilder::new() - .with_config(Config::default()) + .with_config(Config { + editor: helix_view::editor::Config { + idle_timeout: IDLE_TIMEOUT, + ..Default::default() + }, + ..Default::default() + }) .with_lang_loader(helpers::test_syntax_loader(Some(lang_conf.into()))) .with_file(file.path(), None) .with_input_text(text) @@ -121,7 +130,13 @@ async fn test_organize_imports_go_write_all_quit() -> anyhow::Result<()> { let mut file1 = tempfile::Builder::new().suffix(".go").tempfile_in(&dir)?; let mut file2 = tempfile::Builder::new().suffix(".go").tempfile_in(&dir)?; let mut app = helpers::AppBuilder::new() - .with_config(Config::default()) + .with_config(Config { + editor: helix_view::editor::Config { + idle_timeout: IDLE_TIMEOUT, + ..Default::default() + }, + ..Default::default() + }) .with_lang_loader(helpers::test_syntax_loader(Some(lang_conf.into()))) .with_file(file1.path(), None) .with_input_text(text) @@ -155,7 +170,10 @@ async fn test_organize_imports_go_write_all_quit() -> anyhow::Result<()> { "package main\n\nimport (\n\t\"fmt\"\n\t\"path\"\n)\n\nfunc main() {\n\tfmt.Println(\"a\")\n\tpath.Join(\"b\")\n}\n", )?; - assert_file_has_content(&mut file2, "package main\n\nfunc test()\n")?; + assert_file_has_content( + &mut file2, + &LineFeedHandling::Native.apply("package main\n\nfunc test()\n"), + )?; Ok(()) } @@ -184,7 +202,13 @@ async fn test_invalid_code_action_go() -> anyhow::Result<()> { let dir = tempfile::Builder::new().tempdir()?; let mut file = tempfile::Builder::new().suffix(".go").tempfile_in(&dir)?; let mut app = helpers::AppBuilder::new() - .with_config(Config::default()) + .with_config(Config { + editor: helix_view::editor::Config { + idle_timeout: IDLE_TIMEOUT, + ..Default::default() + }, + ..Default::default() + }) .with_lang_loader(helpers::test_syntax_loader(Some(lang_conf.into()))) .with_file(file.path(), None) .with_input_text(text) From 6dc9644d1e793243aa8377e08a94a172aeb7f149 Mon Sep 17 00:00:00 2001 From: Jonatan Pettersson Date: Tue, 3 Sep 2024 12:41:57 +0200 Subject: [PATCH 3/7] Set default line ending to LF in tests --- helix-term/tests/test/lsp/code_actions_on_save.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/helix-term/tests/test/lsp/code_actions_on_save.rs b/helix-term/tests/test/lsp/code_actions_on_save.rs index b5cf5a43f648..51e3dbad136f 100644 --- a/helix-term/tests/test/lsp/code_actions_on_save.rs +++ b/helix-term/tests/test/lsp/code_actions_on_save.rs @@ -74,6 +74,7 @@ async fn test_organize_imports_go() -> anyhow::Result<()> { .with_config(Config { editor: helix_view::editor::Config { idle_timeout: IDLE_TIMEOUT, + default_line_ending: helix_view::editor::LineEndingConfig::LF, ..Default::default() }, ..Default::default() @@ -133,6 +134,7 @@ async fn test_organize_imports_go_write_all_quit() -> anyhow::Result<()> { .with_config(Config { editor: helix_view::editor::Config { idle_timeout: IDLE_TIMEOUT, + default_line_ending: helix_view::editor::LineEndingConfig::LF, ..Default::default() }, ..Default::default() @@ -170,10 +172,7 @@ async fn test_organize_imports_go_write_all_quit() -> anyhow::Result<()> { "package main\n\nimport (\n\t\"fmt\"\n\t\"path\"\n)\n\nfunc main() {\n\tfmt.Println(\"a\")\n\tpath.Join(\"b\")\n}\n", )?; - assert_file_has_content( - &mut file2, - &LineFeedHandling::Native.apply("package main\n\nfunc test()\n"), - )?; + assert_file_has_content(&mut file2, "package main\n\nfunc test()\n")?; Ok(()) } @@ -205,6 +204,7 @@ async fn test_invalid_code_action_go() -> anyhow::Result<()> { .with_config(Config { editor: helix_view::editor::Config { idle_timeout: IDLE_TIMEOUT, + default_line_ending: helix_view::editor::LineEndingConfig::LF, ..Default::default() }, ..Default::default() From 67eaf5cd55aec07d8e61466bb81a37552fa5198e Mon Sep 17 00:00:00 2001 From: Jonatan Pettersson Date: Tue, 3 Sep 2024 13:27:53 +0200 Subject: [PATCH 4/7] Reduce sleep to 10ms --- helix-term/src/commands/lsp.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs index caa698b1a24d..0d6d4799287b 100644 --- a/helix-term/src/commands/lsp.rs +++ b/helix-term/src/commands/lsp.rs @@ -836,7 +836,7 @@ pub fn code_actions_on_save(cx: &mut compositor::Context, doc_id: &DocumentId) { // 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(100)); + std::thread::sleep(std::time::Duration::from_millis(10)); } } } From ae5844af8c6ce4d6043cbf36b9e4bdee470cced9 Mon Sep 17 00:00:00 2001 From: Jonatan Pettersson Date: Tue, 3 Sep 2024 19:24:52 +0200 Subject: [PATCH 5/7] Code cleanup --- helix-term/src/commands/lsp.rs | 8 ++++---- helix-term/src/commands/typed.rs | 1 - helix-term/tests/test/lsp/code_actions_on_save.rs | 14 ++++++++------ 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs index 0d6d4799287b..9455d17b2817 100644 --- a/helix-term/src/commands/lsp.rs +++ b/helix-term/src/commands/lsp.rs @@ -546,9 +546,9 @@ pub fn workspace_diagnostics_picker(cx: &mut Context) { cx.push_layer(Box::new(overlaid(picker))); } -pub struct CodeActionOrCommandItem { - pub lsp_item: lsp::CodeActionOrCommand, - pub language_server_id: LanguageServerId, +struct CodeActionOrCommandItem { + lsp_item: lsp::CodeActionOrCommand, + language_server_id: LanguageServerId, } impl ui::menu::Item for CodeActionOrCommandItem { @@ -842,7 +842,7 @@ pub fn code_actions_on_save(cx: &mut compositor::Context, doc_id: &DocumentId) { } } -pub fn apply_code_action(editor: &mut Editor, action: &CodeActionOrCommandItem) { +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; diff --git a/helix-term/src/commands/typed.rs b/helix-term/src/commands/typed.rs index 98a307d867e4..586882106a1a 100644 --- a/helix-term/src/commands/typed.rs +++ b/helix-term/src/commands/typed.rs @@ -12,7 +12,6 @@ use helix_core::{line_ending, shellwords::Shellwords}; use helix_view::document::{read_to_string, DEFAULT_LANGUAGE_NAME}; use helix_view::editor::{CloseError, ConfigEvent}; use serde_json::Value; - use ui::completers::{self, Completer}; #[derive(Clone)] diff --git a/helix-term/tests/test/lsp/code_actions_on_save.rs b/helix-term/tests/test/lsp/code_actions_on_save.rs index 51e3dbad136f..fb8a0ebe0000 100644 --- a/helix-term/tests/test/lsp/code_actions_on_save.rs +++ b/helix-term/tests/test/lsp/code_actions_on_save.rs @@ -9,6 +9,11 @@ use super::*; // Give time to send textDocument/didOpen notification const IDLE_TIMEOUT: std::time::Duration = std::time::Duration::from_millis(500); +// Gopls does not use native line endings so set default line ending +// to LF to avoid issues in Windows tests. +const DEFAULT_LINE_ENDING: helix_view::editor::LineEndingConfig = + helix_view::editor::LineEndingConfig::LF; + // Check that we have gopls available while also allowing // for gopls to initialize fn assert_gopls(app: &Application, path: &Path) { @@ -22,9 +27,6 @@ fn assert_gopls(app: &Application, path: &Path) { if let Some(gopls) = ls { if gopls.is_initialized() { initialized = true; - // TODO: Make this deterministic - // Sleep to give time to send textDocument/didOpen notification - // std::thread::sleep(std::time::Duration::from_millis(IDLE_TIMEOUT)); break; } } @@ -74,7 +76,7 @@ async fn test_organize_imports_go() -> anyhow::Result<()> { .with_config(Config { editor: helix_view::editor::Config { idle_timeout: IDLE_TIMEOUT, - default_line_ending: helix_view::editor::LineEndingConfig::LF, + default_line_ending: DEFAULT_LINE_ENDING, ..Default::default() }, ..Default::default() @@ -134,7 +136,7 @@ async fn test_organize_imports_go_write_all_quit() -> anyhow::Result<()> { .with_config(Config { editor: helix_view::editor::Config { idle_timeout: IDLE_TIMEOUT, - default_line_ending: helix_view::editor::LineEndingConfig::LF, + default_line_ending: DEFAULT_LINE_ENDING, ..Default::default() }, ..Default::default() @@ -204,7 +206,7 @@ async fn test_invalid_code_action_go() -> anyhow::Result<()> { .with_config(Config { editor: helix_view::editor::Config { idle_timeout: IDLE_TIMEOUT, - default_line_ending: helix_view::editor::LineEndingConfig::LF, + default_line_ending: DEFAULT_LINE_ENDING, ..Default::default() }, ..Default::default() From c314acb304857a8fe5032a133b7d13996cdd62f0 Mon Sep 17 00:00:00 2001 From: Jonatan Pettersson Date: Fri, 6 Sep 2024 20:56:48 +0200 Subject: [PATCH 6/7] 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 cc1c4ce8fe67..07a0b1e48de0 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 9455d17b2817..69172c96ecc1 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 14397bb5c4c7..c34b12fb22dd 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 f7541fe25750..a8b1b469367b 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 91ec27874853..d491206dac9c 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 6aff2e50c58d..42e68ee954e3 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, + })?; } } From edca92063823b46924696cf58053d38f2b84a5fc Mon Sep 17 00:00:00 2001 From: Jonatan Pettersson Date: Sat, 7 Sep 2024 21:58:48 +0200 Subject: [PATCH 7/7] Use FuturesOrdered; Fix from rebase conflict --- helix-term/src/commands/lsp.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs index 69172c96ecc1..24a780ddefd5 100644 --- a/helix-term/src/commands/lsp.rs +++ b/helix-term/src/commands/lsp.rs @@ -1,7 +1,4 @@ -use futures_util::{ - stream::{FuturesOrdered, FuturesUnordered}, - FutureExt, -}; +use futures_util::{stream::FuturesOrdered, FutureExt}; use helix_lsp::{ block_on, lsp::{ @@ -628,7 +625,7 @@ pub fn code_action(cx: &mut Context) { let selection_range = doc.selection(view.id).primary(); - let mut futures: FuturesUnordered<_> = code_actions_for_range(doc, selection_range, None) + let mut futures: FuturesOrdered<_> = code_actions_for_range(doc, selection_range, None) .into_iter() .map(|(request, ls_id)| async move { let json = request.await?;