Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add code actions on save #6486

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jpttrssn
Copy link
Contributor

@jpttrssn jpttrssn commented Mar 30, 2023

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
Copy link
Contributor

cd-a commented Mar 31, 2023

Nice!

Does this work with #2507?

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

@jpttrssn
Copy link
Contributor Author

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

@cd-a
Copy link
Contributor

cd-a commented Mar 31, 2023

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

Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 1417 to 1826
#[inline]
pub fn full_range(&self) -> Range {
Range::new(0, self.text.len_chars())
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 671 to 686
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
);
}
})
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

matches!(
action,
helix_lsp::lsp::CodeActionOrCommand::CodeAction(x) if x.disabled.is_none() &&
code_actions_on_save.iter().any(|a| match &x.kind {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

trigger_kind: Some(CodeActionTriggerKind::INVOKED),
},
) {
let future = match doc.code_actions(selection_range) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, cool. That makes sense.

@pascalkuthe pascalkuthe added A-language-server Area: Language server client E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 14, 2023
@pascalkuthe pascalkuthe added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 18, 2023
@cd-a
Copy link
Contributor

cd-a commented May 19, 2023

#2507 has been merged now, which should enable this PR

@jpttrssn
Copy link
Contributor Author

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

@ravsii
Copy link

ravsii commented May 19, 2023

@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
Copy link
Member

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
Copy link
Contributor

cd-a commented Jun 12, 2023

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

@jpttrssn
Copy link
Contributor Author

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 jpttrssn force-pushed the code-actions-on-save branch 2 times, most recently from 1f6a002 to 3b51a53 Compare June 17, 2023 15:08
@jpttrssn
Copy link
Contributor Author

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
Copy link
Contributor

cd-a commented Jun 17, 2023

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

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this should be a Vec to keep the ordering.

Comment on lines 780 to 784
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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#messageOrdering

Copy link
Member

@archseer archseer Jun 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Ok, I see. Thanks! Let me know which direction we want to take and I can make the changes.

@jpttrssn
Copy link
Contributor Author

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: microsoft/vscode#48086

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

@rcorre
Copy link
Contributor

rcorre commented Jun 30, 2023

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
Copy link
Contributor Author

jpttrssn commented Jul 1, 2023

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 jpttrssn force-pushed the code-actions-on-save branch 2 times, most recently from ebb8dfc to e40d16e Compare September 16, 2023 19:54
@brielov
Copy link

brielov commented Apr 27, 2024

Any news on this?

@jpttrssn
Copy link
Contributor Author

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
Copy link

ds-281 commented May 2, 2024

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

@jpttrssn jpttrssn marked this pull request as draft August 23, 2024 14:10
@jpttrssn jpttrssn force-pushed the code-actions-on-save branch 8 times, most recently from 46b50b7 to 22e849b Compare September 2, 2024 08:44
* 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
@Philipp-M
Copy link
Contributor

(Just a suggestion: I'd use additional commits instead of rebasing/ammending, so that it's easier to track what has changed since the last push)

@jpttrssn
Copy link
Contributor Author

jpttrssn commented Sep 3, 2024

(Just a suggestion: I'd use additional commits instead of rebasing/ammending, so that it's easier to track what has changed since the last push)

OK! Didn't think anyone was paying attention. I'm currently working through issues getting my tests to go through on windows and didn't think anyone would be interested in seeing those commits. Switched to doing this on my fork instead.

@ds-281
Copy link

ds-281 commented Sep 3, 2024

Indeed, I'm also following this issue, and since I'm not overly familiar with Rust (and this project's structure) I would have had an easier time to compare the changes incrementally.

Of course, once the work is done and ready for review, squash-rebasing everything in a perfect set of commits without mistakes makes sense if that's what you prefer.

@jpttrssn
Copy link
Contributor Author

jpttrssn commented Sep 3, 2024

@archseer @pascalkuthe

Some notable changes in this latest revision are the addition of integration tests, ensuring that write_all_quit works and running the code actions directly in the typed::write_* functions (instead of a job).

Note that there is a race condition between the textDocument/didChange notification and running the next code action and/or auto-format which I've "solved" with a sleep. Obviously not an ideal solution. This also causes the lsp integration tests on the Windows runner to fail unless the tests are not run in parallel. It looks like there is a TODO to make applying work space edits transactional, which sounds like it might help here. Other ideas and/or guidance welcome.

Also, I experimented with using a job to handle the code actions, but this causes a panic with write_quit/write_all_quit (i.e., block_try_flush_writes) because of the need to use helix_lsp::block_on in the callback in order to both call async lsp functions and have access to the editor for applying workspace edits. If running this in a job is preferred, one workaround could be to run multiple jobs with a single code action (per language server) in a sequence (which I have experimented some with), but would be more complex and then there's still the race condition mentioned above.

Long story short this felt like the most straight forward approach and a good starting point with room for future improvements. Let me know what you think!

@jpttrssn jpttrssn marked this pull request as ready for review September 3, 2024 19:40
@the-mikedavis
Copy link
Member

I'm very wary of adding integration tests against a language server. As you have in the code already we need to install the language server in CI and require that you install it if you want to run the tests manually, the tests need to sleep pretty pervasively to wait for the language server to respond, and we need to work around language-server-specific oddities like the LF line-ending setting here. If we want to test different features we need to test against many servers: if we want to test pull diagnostics (#11315) for example we need to depend on a version of language servers that uses that feature. Plus I don't think that the code paths involved with this feature will change often enough that the tests would catch a breakage that woludn't be caught in review. On balance I don't think LSP integration tests are worth their added weight and I would prefer we test LSP features manually.

What do you think @archseer & @pascalkuthe?

@jpttrssn
Copy link
Contributor Author

jpttrssn commented Sep 8, 2024

In c314acb I resolved the need to sleep to fix the race condition by allowing the textDocument/didChange notification to be sent synchronously (only during on save). Tried to have this affect as little other code as I could.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-language-server Area: Language server client E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.