feat(rome_lsp): add support for incremental document sync in the language server #3504
Conversation
✅ Deploy Preview for rometools canceled.
|
| @@ -6,13 +6,15 @@ use crate::line_index::LineIndex; | |||
| #[derive(Clone)] | |||
| pub(crate) struct Document { | |||
| pub(crate) version: i32, | |||
| pub(crate) content: String, | |||
There was a problem hiding this comment.
Wondering the memory impact of this in large workspaces. We should be able to restore this from the CST, right?
Unless we also don't store the CST. :D
Should we store this and line indices in a temp file?
There was a problem hiding this comment.
VS code sends close commands when you close a tab. It's, therefore, more of a concern for people who have many tabs open.
There was a problem hiding this comment.
If we are going to have this only for open tabs, I think this is not a problem.
There was a problem hiding this comment.
We do store the CST but it's not accessible in the LSP crate, since despite its name the language server is actually mostly language agnostic. I think in the long term it would be nice to be able to incrementally apply these updates directly on the syntax tree instead of having to materialize the entire string and re-parse the whole file again, but incremental parsing is a hard problem especially with how context-specific the syntax of JavaScript is ...
| @@ -6,13 +6,15 @@ use crate::line_index::LineIndex; | |||
| #[derive(Clone)] | |||
| pub(crate) struct Document { | |||
| pub(crate) version: i32, | |||
| pub(crate) content: String, | |||
There was a problem hiding this comment.
VS code sends close commands when you close a tab. It's, therefore, more of a concern for people who have many tabs open.
| match change.range { | ||
| Some(range) => { | ||
| let range = utils::text_range(&doc.line_index, range)?; | ||
| content.replace_range(Range::<usize>::from(range), &change.text); |
There was a problem hiding this comment.
Is my assumption correct that replace_range returns a String. Could Document use that string directly instead of copying into its own string in new?
There was a problem hiding this comment.
Well replace_range actually mutates the existing string in place, but the end result is the same. The reason why the content needs to be cloned into Document::new instead of taking ownership of it is because we still need to pass it to workspace.change_file afterward
Summary
Historically our LSP implementation has been using full document sync, where the editor sends the entire content of the document on every change notification, since it's easier to get right. However sending the whole buffer is a heavy operation, so VSCode will debounce change notifications or defer them until other language server requests are sent. This has caused issues in the past with requests being deferred too much causing the document state to go out of sync between the editor and language server.
This PR implements support for the incremental document sync mode where the editor sends in individual changes as they happen, meaning updates can be processed more frequently and could generally cause the editor experience to feel more reactive.
One downside to this approach is that it may introduce new synchronization issues since
tower_lspmay schedule requests to be executed in parallel, I haven't seen it happen while testing on a local build of the server but it's hard to be absolutely sure about this kind of concurrency issues.Test Plan
I've added a test for a complex update in the tests for the LSP crate, it relies on the
get_syntax_treemethod to check the changes are correctly applied since there are no easy way to peek at the state of the active workspace documents over the language server protocol.