Add code actions on save

[jpttrssn]

Addresses issue #1565

• Add a per language config option for code-actions-on-save that takes a list of LSP actions to run in order during save
• Verify configured code actions against available source code actions for the entire document being saved
• Apply actions on write and write_all before auto formatting
• Refactor to reduce duplicate code
• Update languages configuration documentation

[cd-a]

Nice!

Does this work with https://github.com/helix-editor/helix/pull/2507?

As far as I can see, that was the only feature that was still not working there

[jpttrssn]

This would need some work to be compatible. I can work on adding my changes on top of that PR.

[cd-a]

That would be amazing. That PR should be merged soon.

[cd-a]

https://github.com/helix-editor/helix/pull/2507 has been merged now, which should enable this PR

[jpttrssn]

Great! I'll update this PR as soon as I've got my changes working with master.

[ravsii]

@jpttrssn this only adds pre-defined LSP actions on save, but no support for third-party commands, like vscode's run on save?

just asking

[pascalkuthe]

No that's out of scope for this PR. This PR should only implement support for the relevant part of the LSP spec

[cd-a]

Any luck getting it rebased on latest master? Would love to test it again

[jpttrssn]

I've made some progress, but haven't had much time to work on it. I'll try to make some time, though. I also want to get this done.

[jpttrssn]

Finally got this rebased with master. Manually tested with the following language config:

[[language]]
name = "go"
code-actions-on-save = ["source.organizeImports", "source.invalid"]

[cd-a]

Great news, thanks @jpttrssn !

Any idea how to test this with TS/TSX?

I tried this under the language config but it doesn't have any effect

code-actions-on-save = ["source.fixAll.eslint"]

[jpttrssn]

Any idea how to test this with TS/TSX?

I tried this under the language config but it doesn't have any effect

code-actions-on-save = ["source.fixAll.eslint"]

source.fixAll.eslint appears to be specific to the VSCode Eslint extention. See: https://github.com/microsoft/vscode-eslint#settings-options

Addtionally, this PR only allows executing code actions that are returned from the LSP selecting over the range of the entire document. It would be the same as if you did %-space-a in the editor. I based this off of the PR to add code actions on save to VSCode: https://github.com/microsoft/vscode/pull/48086

There is definitely room for adding more functionality, but perhaps out of scope for this PR?

[rcorre]

This is great, I'm using it with source.organizeImports in Go and it seems to work well.

Out of curiosity, how do you know what the name of an "action" is, or what actions a particular LSP supports? I just copied "source.organizeImports" from the docs in the PR, but I have no idea how I'd figure out what else I could put in there, or if it would be the same for other language servers.

[jpttrssn]

Out of curiosity, how do you know what the name of an "action" is, or what actions a particular LSP supports? I just copied "source.organizeImports" from the docs in the PR, but I have no idea how I'd figure out what else I could put in there, or if it would be the same for other language servers.

There are only two source actions defined in the LSP spec (source.organizeImports and source.fixAll). However, the spec leaves the option for a language server or extension to provide additional source actions. I have myself yet to find a LSP I can use to test this PR with a source.fixAll.

As a side note, I'm in the process of re-working this PR to apply the configured code actions in order (applied individually) and enabling the source.fixAll option.

[jpttrssn]

This version will now run code actions, formatting and saving in order. It was a bit tricky to figure out a way to do this. I had to solve around the following constraints:

1. Current job implementation does not allow for running jobs in order
2. Each code action requires a async function call with a callback using the non-thread safe Editor
3. Formatting requires knowledge of the current document version after any code action changes

In order to solve this I ended up needing to run everything in the main thread. A downside to this is that there is no concurrency when saving multiple files. One plus is that there is now only one code path to save a file.

Perhaps there is a better solution I'm not seeing? Making the Editor thread safe, perhaps, but that seems like a big change. @archseer @pascalkuthe

[pascalkuthe]

I don't think we should ever make Editor thread save, that would require lots of locking and is not really necessary IMO (keeping the main editor state single threaded make it simpler to impose a chronological order on events too)

[mariansimecek]

Hi, can´t get it working with typescript LSP. I have this in languages.toml

 [[language]]
 name = "typescript"
 code-actions-on-save = ["source.sortImports"]

logs:

2023-11-09T20:39:17.277 helix_term::commands [DEBUG] Attempting code action on save "source.sortImports"
2023-11-09T20:39:17.277 helix_lsp::transport [INFO] typescript-language-server -> {"jsonrpc":"2.0","method":"textDocument/codeAction","params":{"context":{"diagnostics":[{"code":6133,"message":"'NodeTracing' is declared but its value is never read.","range":{"end":{"character":40,"line":0},"start":{"character":0,"line":0}},"severity":4,"source":"typescript"},{"code":6133,"message":"'Mod' is declared but its value is never read.","range":{"end":{"character":27,"line":2},"start":{"character":24,"line":2}},"severity":4,"source":"typescript"},{"code":6133,"message":"'something' is declared but its value is never read.","range":{"end":{"character":37,"line":3},"start":{"character":0,"line":3}},"severity":4,"source":"typescript"},{"code":6133,"message":"'Agent' is declared but its value is never read.","range":{"end":{"character":29,"line":4},"start":{"character":0,"line":4}},"severity":4,"source":"typescript"},{"code":6133,"message":"'aa' is declared but its value is never read.","range":{"end":{"character":6,"line":8},"start":{"character":4,"line":8}},"severity":4,"source":"typescript"},{"code":6133,"message":"'aa2' is declared but its value is never read.","range":{"end":{"character":7,"line":9},"start":{"character":4,"line":9}},"severity":4,"source":"typescript"}],"triggerKind":1},"range":{"end":{"character":0,"line":10},"start":{"character":0,"line":0}},"textDocument":{"uri":"file:///home/marian-simecek/dev/private/base-ts-proj/index.ts"}},"id":1}
2023-11-09T20:39:17.346 helix_lsp::transport [INFO] typescript-language-server <- {"jsonrpc":"2.0","id":1,"result":[{"title":"Remove import from 'inspector'","command":{"title":"Remove import from 'inspector'","command":"_typescript.applyWorkspaceEdit","arguments":[{"documentChanges":[{"textDocument":{"uri":"file:///home/marian-simecek/dev/private/base-ts-proj/index.ts","version":0},"edits":[{"range":{"start":{"line":0,"character":0},"end":{"line":1,"character":0}},"newText":""}]}]}]},"kind":"quickfix"},{"title":"Convert default export to named export","kind":"refactor","disabled":{"reason":"This file already has a default export"}},{"title":"Convert named export to default export","kind":"refactor","disabled":{"reason":"This file already has a default export"}},{"title":"Extract to typedef","kind":"refactor","disabled":{"reason":"Selection is not a valid type node"}},{"title":"Extract to type alias","kind":"refactor","disabled":{"reason":"Selection is not a valid type node"}},{"title":"Extract to interface","kind":"refactor","disabled":{"reason":"Selection is not a valid type node"}},{"title":"Move to a new file","kind":"refactor.move","command":{"title":"Move to a new file","command":"_typescript.applyRefactoring","arguments":[{"file":"/home/marian-simecek/dev/private/base-ts-proj/index.ts","startLine":1,"startOffset":1,"endLine":11,"endOffset":1,"refactor":"Move to a new file","action":"Move to a new file"}]}},{"title":"Add braces to arrow function","kind":"refactor","disabled":{"reason":"Could not find a containing arrow function"}},{"title":"Remove braces from arrow function","kind":"refactor","disabled":{"reason":"Could not find a containing arrow function"}},{"title":"Convert to template string","kind":"refactor","disabled":{"reason":"Can only convert string concatenation"}},{"title":"Convert to optional chain expression","kind":"refactor","disabled":{"reason":"Could not find convertible access expression"}},{"title":"Extract function","kind":"refactor","disabled":{"reason":"Cannot extract import statement."}},{"title":"Extract constant","kind":"refactor","disabled":{"reason":"Cannot extract import statement."}},{"title":"Generate 'get' and 'set' accessors","kind":"refactor","disabled":{"reason":"Could not find property for which to generate accessor"}},{"title":"Infer function return type","kind":"refactor","disabled":{"reason":"Return type must be inferred from a function"}}]}
2023-11-09T20:39:17.346 helix_lsp::transport [INFO] typescript-language-server <- [{"command":{"arguments":[{"documentChanges":[{"edits":[{"newText":"","range":{"end":{"character":0,"line":1},"start":{"character":0,"line":0}}}],"textDocument":{"uri":"file:///home/marian-simecek/dev/private/base-ts-proj/index.ts","version":0}}]}],"command":"_typescript.applyWorkspaceEdit","title":"Remove import from 'inspector'"},"kind":"quickfix","title":"Remove import from 'inspector'"},{"disabled":{"reason":"This file already has a default export"},"kind":"refactor","title":"Convert default export to named export"},{"disabled":{"reason":"This file already has a default export"},"kind":"refactor","title":"Convert named export to default export"},{"disabled":{"reason":"Selection is not a valid type node"},"kind":"refactor","title":"Extract to typedef"},{"disabled":{"reason":"Selection is not a valid type node"},"kind":"refactor","title":"Extract to type alias"},{"disabled":{"reason":"Selection is not a valid type node"},"kind":"refactor","title":"Extract to interface"},{"command":{"arguments":[{"action":"Move to a new file","endLine":11,"endOffset":1,"file":"/home/marian-simecek/dev/private/base-ts-proj/index.ts","refactor":"Move to a new file","startLine":1,"startOffset":1}],"command":"_typescript.applyRefactoring","title":"Move to a new file"},"kind":"refactor.move","title":"Move to a new file"},{"disabled":{"reason":"Could not find a containing arrow function"},"kind":"refactor","title":"Add braces to arrow function"},{"disabled":{"reason":"Could not find a containing arrow function"},"kind":"refactor","title":"Remove braces from arrow function"},{"disabled":{"reason":"Can only convert string concatenation"},"kind":"refactor","title":"Convert to template string"},{"disabled":{"reason":"Could not find convertible access expression"},"kind":"refactor","title":"Convert to optional chain expression"},{"disabled":{"reason":"Cannot extract import statement."},"kind":"refactor","title":"Extract function"},{"disabled":{"reason":"Cannot extract import statement."},"kind":"refactor","title":"Extract constant"},{"disabled":{"reason":"Could not find property for which to generate accessor"},"kind":"refactor","title":"Generate 'get' and 'set' accessors"},{"disabled":{"reason":"Return type must be inferred from a function"},"kind":"refactor","title":"Infer function return type"}]
2023-11-09T20:39:17.346 helix_term::commands [DEBUG] Code action on save not found "source.sortImports"
2023-11-09T20:39:17.346 helix_view::editor [ERROR] editor error: Code Action not found: "source.sortImports"

I tried golang and it works as expected.

Am I doing something wrong?

[jpttrssn]

@mariansimecek Looks like for the typescript-language-server the code action should be source.sortImports.ts. But regardless, none of the Typescript specific source.*.ts code actions are being returned as options from the typescript-language-server, whether during on-save or within the editor itself. I was able to reproduce this. There might be some issue when the LSP announces these actions. I can dig in a little here, but it doesn't look like this is an issue specific to this PR.

Do you get the expected code actions from within the editor when selecting over the entire file; %-space-a?

[mariansimecek]

Do you get the expected code actions from within the editor when selecting over the entire file; %-space-a?

Unfortunately, neither.

[ds-281]

Hey, just wanted to drop a message to encourage this to be reviewed, and if given the green light, merged.

This makes working with helix on golang code a much better experience (manually doing imports stuff is a pain). I'm no rust expert, so best I can do is build (rebased on top of master) and try the changes out, and to the limited extent of my testing, it's doing what I need.

[quantonganh]

This makes working with helix on golang code a much better experience (manually doing imports stuff is a pain)

@ds-281 Have you tried this?

[[language]]
name = "go"
formatter = { command = "goimports"}

[rcorre]

@quantonganh thank you! I thought goimports needed a filename, as the docs don't suggest it can use stdin/stdout, but it in fact works fine. This would still be a nice feature to have, so you could take advantage of language server features without needing a separate binary, but that's a good work around for now.

[brielov]

Any news on this?

[jpttrssn]

This probably needs a revisit at this point. From the sound of it this PR might benefit from the new "event system" that came out in the latest release (24.03). I'll see if I can find some time to see what's possible there.

[ds-281]

That would be much appreciated, thank you.

It's nice that the goimports trick works, but relying on it adds an extra dependency to the system that feels unnecessary and, to my understanding, relies on the fact that only one action is needed (as opposed to this PR, which takes a list of LSP actions to run upon save).