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
pull/6486/head
Jonatan Pettersson 4 months ago
parent 1b5295a3f3
commit af5a16f6e3

@ -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"

@ -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]

@ -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

@ -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.

@ -118,6 +118,8 @@ pub struct LanguageConfiguration {
pub block_comment_tokens: Option<Vec<BlockCommentToken>>,
pub text_width: Option<usize>,
pub soft_wrap: Option<SoftWrap>,
#[serde(default)]
pub code_actions_on_save: Option<Vec<CodeActionsOnSave>>, // 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<String>,
}
#[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 {

@ -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]]

@ -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<lsp::CodeActionResponse> = serde_json::from_value(json)?;
@ -734,8 +712,138 @@ 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 {
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<CodeActionKind>>,
) -> Vec<(
impl Future<Output = Result<Value, helix_lsp::Error>>,
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::<Vec<_>>()
}
/// 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<CodeActionOrCommandItem> =
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<helix_lsp::lsp::CodeActionResponse>,
>(json)
{
// 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,
});
}
}
}
None
})
.collect();
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));
}
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;
};
@ -751,20 +859,15 @@ pub fn code_action(cx: &mut Context) {
// 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 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::<CodeAction>(response)
{
if let Ok(code_action) = serde_json::from_value::<CodeAction>(response) {
resolved_code_action = Some(code_action);
}
}
}
}
let resolved_code_action =
resolved_code_action.as_ref().unwrap_or(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);
@ -777,16 +880,6 @@ pub fn code_action(cx: &mut Context) {
}
}
}
});
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 execute_lsp_command(

@ -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

@ -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<esc>", "hello world#[|\n]#")).await?;
Ok(())
}
mod lsp;
}

@ -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::<Vec<&PathBuf>>()
);
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<ret>"), 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 {}<ret>ipackage main<ret>import \"fmt\"<ret>func test()<ret><esc>",
file2.path().to_string_lossy(),
)),
None,
),
(
None,
Some(&|app| {
assert_gopls(app, file1.path());
assert_gopls(app, file2.path());
}),
),
(Some(":wqa<ret>"), 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<ret>"),
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(())
}

@ -0,0 +1,3 @@
use super::*;
mod code_actions_on_save;

@ -1929,13 +1929,15 @@ impl Editor {
}
pub fn document_by_path<P: AsRef<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<P: AsRef<Path>>(&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

Loading…
Cancel
Save