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

[Feature Request] Incremental textDocument/didChange #497

Open
huynhtrankhanh opened this issue Jun 16, 2023 · 2 comments
Open

[Feature Request] Incremental textDocument/didChange #497

huynhtrankhanh opened this issue Jun 16, 2023 · 2 comments
Labels

Comments

@huynhtrankhanh
Copy link

This is mainly useful when coq-lsp and the client are separated by a network.

Currently, coq-lsp only supports whole document sync. This is fine when both the server and the client are on the same machine, as parsing is pretty fast.

However, when the client has to access the server over a network, the client would need to send the whole document, possibly on every keystroke. This can have a deleterious effect on latency.

I propose that we should implement incremental textDocument/didChange simply by applying the diff on the document, then continue as if we'd been given the whole document.

@Alizter
Copy link
Collaborator

Alizter commented Jun 16, 2023

We definitely could implement incremental didChange. From what I have read the important parts of the LSP protocol are here:

Rust-analyzer (the LSP server for rust) implements incremental changes in the following way:
https://github.com/rust-lang/rust-analyzer/blob/ba329913fa33c29d4ccabf46998d3a0cfac57b0c/crates/rust-analyzer/src/lsp_utils.rs#L131

So we can take some inspiration from that.

I think we would like to have such a feature configurable since it might work better for others. I would be interested to see if there are any performance issues when processing a lot of changes.

Applying a diff seems like a sensible way to do this, however we would have to understand what the server should do when given an invalid diff.

@Alizter Alizter added the kind: enhancement New feature or request label Jun 16, 2023
@ejgallego
Copy link
Owner

Indeed this would be nice to have, as I mentioned in Zulip the reason I didn't put time on it is:

  • gains are quite marginal, I'm actually quite surprised to hear that is a problem even in a "network" case (do you have more details folks)
  • the change algorithm itself is not so easy to implement, LSP did a pretty bad, windows-specific choice of encoding so often you'll have to convert text to UTF-16 then back to UTF-8 which Coq can understand. So that'd be a source of bugs I don't feel like debugging myself.

Note that both ocaml-lsp and vscoq2 have implemented this algorithm (tho I'm not sure if they handle UTF properly) so in principle anyone interested could just go ahead a submit a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants