Add code actions on save #6486
Add code actions on save #6486
Conversation
jpttrssn
force-pushed
the
code-actions-on-save
branch
from
f4c9f40
to
7d642ee
Compare
|
Nice! Does this work with #2507? As far as I can see, that was the only feature that was still not working there |
|
This would need some work to be compatible. I can work on adding my changes on top of that PR. |
|
That would be amazing. That PR should be merged soon. |
There was a problem hiding this comment.
Thanks for taking this on! I left some minor comments. I think we will likely land #2507 before this one since that has been waiting around for a longtime and while sometimes merging bugfixes before that PR makes sense ideally I think we should holdoff adding new LSP related features until after that lands.
helix-view/src/document.rs
Outdated
| #[inline] | ||
| pub fn full_range(&self) -> Range { | ||
| Range::new(0, self.text.len_chars()) | ||
| } | ||
|
|
There was a problem hiding this comment.
I dont think this needs a dedicated helper method. We have far more common operations that don't have helper methods. Adding too many helper methods clutters the (already very large) document impls even more
helix-view/src/document.rs
Outdated
| if code_actions.len() < code_actions_on_save.len() { | ||
| code_actions_on_save.iter().for_each(|configured_action| { | ||
| if !code_actions.iter().any(|action| match action { | ||
| helix_lsp::lsp::CodeActionOrCommand::CodeAction(x) => match &x.kind { | ||
| Some(kind) => kind.as_str() == configured_action, | ||
| None => false, | ||
| }, | ||
| _ => false, | ||
| }) { | ||
| log::error!( | ||
| "Configured code action on save is invalid {:?}", | ||
| configured_action | ||
| ); | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
I don't think these code actions are necessarily returned on every import so this would potentially generate a lot of errors. This should atlest be an infolog but this alsoO(NM)` so I am not sure we really need this anyway?
Also, I think a for loop is basically always more idiomatic than for_each
There was a problem hiding this comment.
I agree this isn't great. I struggled to find a good solution for this. Ideally this would be done at the same time as the filtering with the code_actions.retain() above. Perhaps a for loop there instead? I can play around with it some more and see if I can come up with something. Otherwise I'm fine with leaving this out. It's a nice to have, but like you said perhaps not needed.
helix-view/src/document.rs
Outdated
| matches!( | ||
| action, | ||
| helix_lsp::lsp::CodeActionOrCommand::CodeAction(x) if x.disabled.is_none() && | ||
| code_actions_on_save.iter().any(|a| match &x.kind { |
There was a problem hiding this comment.
probably a good idea to make this a HashSet so we avoid being O(MN) here even if M and N is usually small frserializing to a HashSet just takes replacing Vec with HashSet
helix-term/src/commands/lsp.rs
Outdated
| trigger_kind: Some(CodeActionTriggerKind::INVOKED), | ||
| }, | ||
| ) { | ||
| let future = match doc.code_actions(selection_range) { |
There was a problem hiding this comment.
I don't think we should move this to a function on the document. This doesn't need to be moved into helix-view for this PR to work. Just moving this to a separate function inside commands/lsp.rs works just as well. We generally try to keep the LSP-related stuff separate from the core document primitives. Same for the other function you added to the document (which should also just be a standalone function in this file)
There was a problem hiding this comment.
Alright, cool. That makes sense.
pascalkuthe
added
A-language-server
jpttrssn
force-pushed
the
code-actions-on-save
branch
from
7d642ee
to
0365774
Compare
pascalkuthe
added
S-waiting-on-review
|
#2507 has been merged now, which should enable this PR |
|
Great! I'll update this PR as soon as I've got my changes working with master. |
|
@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 |
|
No that's out of scope for this PR. This PR should only implement support for the relevant part of the LSP spec |
|
Any luck getting it rebased on latest master? Would love to test it again |
|
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
force-pushed
the
code-actions-on-save
branch
2 times, most recently
from
1f6a002
to
3b51a53
Compare
|
Finally got this rebased with master. Manually tested with the following language config: |
|
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 |
helix-core/src/syntax.rs
Outdated
| @@ -105,6 +105,8 @@ pub struct LanguageConfiguration { | |||
| pub comment_token: Option<String>, | |||
| pub text_width: Option<usize>, | |||
| pub soft_wrap: Option<SoftWrap>, | |||
| #[serde(default)] | |||
| pub code_actions_on_save: HashSet<String>, // List of LSP code actions to be run in order upon saving | |||
There was a problem hiding this comment.
A HashSet is ordered based on the item's hash value though. Let's just use a Vec here since I can also imagine usecases where you would want to execute action A, B then A again
There was a problem hiding this comment.
Yeah, this should be a Vec to keep the ordering.
helix-term/src/commands/lsp.rs
Outdated
| Some( | ||
| code_actions_for_range(doc, full_range) | ||
| .into_iter() | ||
| .map(|(request, ls_id)| { | ||
| let code_actions_on_save = code_actions_on_save_cfg.clone(); |
There was a problem hiding this comment.
This assumes that we can trigger multiple code actions in parallel, which works in some cases, but one action might impact another action (e.g. if A removes a function, and B formats the file). Maybe to simplify we should limit to a single code action on save (since that's the majority usecase anyway)
There was a problem hiding this comment.
If I'm understanding the LSP spec as long as the requests are in the correct order, the lsp should determine how best to execute those requests.
There was a problem hiding this comment.
Right, but if you send actions A and B in parallel, the server is computing the edits based on that initial state. Server only considers the change applied when we send back a didChange event. Helix does use OT internally to transform edits (so initial state -> A -> B can be applied) but it's not necessarily going to produce the same result as if we requested A, applied, then requested B and applied B.
There was a problem hiding this comment.
But over multiple language servers we can't guarantee the order. That's what you were getting at, I assume. Simplifying to a single code action would definitely make this problem go away, though then we are not feature parity with VSCode, if that is important.
There was a problem hiding this comment.
Right, but if you send actions A and B in parallel, the server is computing the edits based on that initial state. Server only considers the change applied when we send back a
didChangeevent. Helix does use OT internally to transform edits (so initial state -> A -> B can be applied) but it's not necessarily going to produce the same result as if we requested A, applied, then requested B and applied B.
Ok, I see. Thanks! Let me know which direction we want to take and I can make the changes.
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 There is definitely room for adding more functionality, but perhaps out of scope for this PR? |
|
This is great, I'm using it with 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 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 |
jpttrssn
force-pushed
the
code-actions-on-save
branch
2 times, most recently
from
ebb8dfc
to
e40d16e
Compare
|
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:
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 |
|
I don't think we should ever make |
jpttrssn
force-pushed
the
code-actions-on-save
branch
from
e40d16e
to
0d75992
Compare
|
Hi, [[language]]
name = "typescript"
code-actions-on-save = ["source.sortImports"]logs: I tried golang and it works as expected. Am I doing something wrong? |
|
@mariansimecek Looks like for the typescript-language-server the code action should be Do you get the expected code actions from within the editor when selecting over the entire file; |
Unfortunately, neither. |
|
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. |
@ds-281 Have you tried this? [[language]]
name = "go"
formatter = { command = "goimports"} |
|
@quantonganh thank you! I thought |
|
Any news on this? |
|
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. |
|
That would be much appreciated, thank you. It's nice that the |
Addresses issue #1565