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

Async formatting #53

Closed
hovsater opened this issue Jun 2, 2021 · 10 comments
Closed

Async formatting #53

hovsater opened this issue Jun 2, 2021 · 10 comments
Labels
A-language-server Area: Language server client

Comments

@hovsater
Copy link
Contributor

hovsater commented Jun 2, 2021

Whenever I save a file (:write) with LSP enabled (e.g., solargraph for Ruby or rust-analyzer for Rust) the editor halts for 1-2 seconds until finally finishing. This behaviour isn't present when LSP is disabled.

@hovsater hovsater changed the title Noticable delay when saving a Ruby file with LSP enabled Noticable delay when saving a file with LSP enabled Jun 2, 2021
@archseer
Copy link
Member

archseer commented Jun 2, 2021

This is because we try to :format via the LSP before saving. I was afraid this would be an issue on a slower language server (you can also notice it before rust-analyzer fully boots up). Need to handle that better, probably by making it opt-in per language.

@hovsater
Copy link
Contributor Author

hovsater commented Jun 3, 2021

Would it be possible to format the code asynchronously? It's kind of unexpected that it's a synchronous operation blocking the UI.

If not, I think it makes sense for you to be able to opt out. 👍

@archseer
Copy link
Member

archseer commented Jun 4, 2021

I think you would likely run into issues if formatting took a while to run async while you made further text edits.

I'll add a config option to enable it only for certain languages.

@hovsater
Copy link
Contributor Author

hovsater commented Jun 4, 2021

Yeah, I guess it depends. I've seen it done both ways. If done asynchronously, one could discard the formatting if the buffer changed in between saving the file and the action actually completing.

@jneem
Copy link
Contributor

jneem commented Jun 8, 2021

I think it does make sense to discard the formatting in that case, but one annoying thing is that you want the formatting to be saved. I guess you'd need to:

  1. make a clone of the document synchronously (cheap, because Ropes clone cheaply)
  2. spawn a "format" task that computes the formatting changes, then spawns a "save" task and returns the formatting changes
  3. the "save" task applies the formatting changes to the cloned document and then saves it
  4. the main task gets the formatting changes from the format task and applies them as long as the document hasn't changed.

I could try implementing that if it sounds reasonable.

@pickfire
Copy link
Contributor

pickfire commented Jun 8, 2021

Same noticeable delay when I press gd waiting for lsp. It doesn't automatically do the task even though lsp starts up. Should have sort of queuing mechanism.

@jneem
Copy link
Contributor

jneem commented Jun 12, 2021

Is there no interest in the async formatting version? I like auto-format on save, but I'd also like it not to block the editor...

@archseer
Copy link
Member

Sorry about that, I missed the comment! I think some of this overlaps with #177 (comment)

I'd be interested in this, but the async aspects of helix-term still need some work. It's hard to keep a reference in a spawned task without blocking, you'd probably need some sort of a callback mechanism similar to how the LspCallbacks hack works (you keep a DocumentId, the callback gives you a &mut Editor that can be used to look up the doc again).

@archseer archseer reopened this Jun 12, 2021
@archseer archseer changed the title Noticable delay when saving a file with LSP enabled Async formatting Jun 12, 2021
@archseer
Copy link
Member

Same noticeable delay when I press gd waiting for lsp. It doesn't automatically do the task even though lsp starts up. Should have sort of queuing mechanism.

This is because rust-analyzer takes some time to index before it starts responding to queries. workDone events from LSP will give us a way to cancel these types of queries, at the moment we time out after 2s and then discard the data once received later.

@archseer
Copy link
Member

#285 now solves this, thanks to @jneem for the implementation!

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
Projects
None yet
Development

No branches or pull requests

4 participants