Add support for incremental text synchronization

[lnicola]

Fixes #3762.

This still needs a ra_vfs PR, but I want to know I'm on the right track. I tested the change and it didn't crash horribly, but YMMV.

[kjeremy]

As an aside I'm surprised that this isn't something negotiated by the LSP using client/server caps. It seems weird to me that a client must support all three modes: None, Full and Incremental.

[kjeremy]

@lnicola does this improve responsiveness? I would expect the over the wire performance to be negligible but it looks like we avoid a 200ms delay in the vscode client in incremental mode: https://github.com/microsoft/vscode-languageserver-node/blob/e5d7ad881b1a51b486e3f0e4aa0fbc25dad2be58/client/src/client.ts#L1004

[lnicola]

I can't really tell, I only checked that it works. The type hints seemed pretty snappy, though. But when looking into the Builder failure I tried to sniff the client-server communication and couldn't notice the delay while typing.

I hope this won't make RA burn more CPU than before.

[kjeremy]

I just did a quick smoke test of this and it seems to work. I didn't spot any regressions.

[lnicola]

Does it feel faster?

[kjeremy]

The highlighting felt snappier but I was eyeballing it.

[matklad]

This seems reasonable, though I am not sure if it works correctly with our \r\n translation.

Let's add some unit-tests here, which create an empty VFS and apply some hard-coded JSONs to it?

[matklad]

(the vfs PR is published)

[lnicola]

I'm not sure how to structure it, is there another test working with JSON requests like that? Maybe we should add a test to ra_vfs instead, since there's not much happening here (most of the code is about looking up the file path and line index, and conversions).

[matklad]

I mean, the tricky bit here is correctly interpreting DidChangeTextDocumentParams json (specifically, that we correctly transition positions, and we correclty handle \r\n)

I think just extracting this bit into a function and unit-testing it should be possible. The funciton would take two arguments: VFS and a line index, and it would apply changes to VFS.

The test would take initial file contents, the DidChangeTextDocumentParams, and the expected file context. it'll crate a VFS with a single file, calculate line index for it, apply the extracted function and compare results.

I think thre required DidChangeTextDocumentParams can be copy-pasted from the lsp log messages? using multiple cursor and doing multi-line edits should cover enough interesting cases.

[lnicola]

I'm so confused -- I can't really figure out how to use Vfs. I can create one, but do I need to provide a real (disk) path, or can I just call Vfs::load? And then I must call its handle_task method with the event it returns? Finally, how do I read a file? VfsFileData is private.

[matklad]

Hm, indeed, I think because the scanning happens in background it's not really possible to test this correctly with VFS :-(

Here's as far as I got:

#[test]
fn smoked_vfs() {
    struct All;
    impl ra_vfs::Filter for All {
        fn include_dir(&self, dir_path: &relative_path::RelativePath) -> bool {
            true
        }
        fn include_file(&self, file_path: &relative_path::RelativePath) -> bool {
            true
        }
    }
    let root = RootEntry::new("/dummy".into(), Box::new(All));
    let (mut vfs, vfs_roots) =
        Vfs::new(vec![root], Box::new(|task| drop(task) /* :-( */), Watch(false));

    let file_id = vfs.add_file_overlay(Path::new("/dummy/foo.rs"), "fn main() {}".into()).unwrap();
    let changes = vfs.commit_changes();
    match changes.as_slice() {
        [VfsChange::AddFile { text, .. }] => text.as_str() == "fn main() {}",
        changes => panic!("{:?}", changes),
    };

    vfs.change_file_overlay(Path::new("/dummy/foo.rs"), "fn bar() {}".into());
    vfs.commit_changes();
    match changes.as_slice() {
        [VfsChange::ChangeFile { text, .. }] => text.as_str() == "fn main() {}",
        changes => panic!("{:?}", changes),
    };
}

But perhaps we don't even need VFS at all? it seems like

fn apply_change(chage: lsp_types::TextDocumentChange, original_text: &mut String, line_endings: LineEndings)

Should actually be enough there?

What I am trying to test here is that we correctly interpret TextDocumentChange. Hm, actually, I think we might not be doing that right now? IIRC, it has somewhat insane API in that each atomic change should be applied in order. That is, the "coordinate space" of each atomic change depends on preceeding changes. But that means that we might actually need to recompute/adjust LineIndex appropriately?

[lnicola]

I've been worried about this from the start -- I'm not sure how we're supposed to handle multiple changed, maybe we should take a look at other language servers.

One thing I noticed is that Code tries to sort the ranges in reverse, so they don't conflict. I didn't test inserting multiple newlines, but it might work.

[lnicola]

microsoft/language-server-protocol#289 applying the changes in order seems to be correct.

[matklad]

applying the changes in order seems to be correct.

Yes, they need to be applied in order, but the question is, will the offsets shift? The wont', if the changes are indeed reveres-sorted, but that's not a part of the protocol :-(

[kjeremy]

I think that's a question for @dbaeumer.

…

On Thu, Apr 30, 2020, 6:22 AM Aleksey Kladov ***@***.***> wrote: applying the changes in order seems to be correct. Yes, they need to be applied in order, but the question is, will the offsets shift? The wont', if the changes are indeed reveres-sorted, but that's not a part of the protocol :-( — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4153 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBACREFMOXRQ63XL4L3CXDRPFGQDANCNFSM4MRD5VJA> .

[lnicola]

Just to make sure I got it right: if a file changes, we invalidate the line index at the turn of the main loop, so we're worried about it becoming stale while we're applying the updates? And if the ranges are sorted backwards, it's not an issue.

[matklad]

Right. Specifically, let's say we start with an empty file, and the first edit added adds three \n characters, and the second edit adds "hello" at the end. For the second edit, the line will be 3. But the line index we have is empty, so we'll panic when resolving that line three to numerical offset.

The correct (albeit inneficient) algorithm would be:

let mut text = initial_text;
for change in changes {
    let line_index = LineIndex::new(&text);
    let text_edit = change.conv_with(&line_index);
    text = text_edit.apply(text);
}

[lnicola]

I checked a couple of implementations:

• Go doesn't use a line index: https://github.com/sourcegraph/go-langserver/blob/master/langserver/ast.go#L15
• TypeScript doesn't support incremental synchronization: https://github.com/sourcegraph/javascript-typescript-langserver/blob/master/src/typescript-service.ts#L1614
• clangd doesn't use an index: https://reviews.llvm.org/diffusion/L/browse/clang-tools-extra/trunk/clangd/SourceCode.cpp$149
• the Dart analysis server rebuilds the index on each change: https://github.com/dart-lang/sdk/blob/master/pkg/analysis_server/lib/src/lsp/source_edits.dart#L11

[matklad]

bors d+

We can add a temporary flag to config, to allow users to opt-out if this misbehaves (and I think this can misbehave both due to our bugs, and due to client bugs). OTOH, I am also ok with merging this as is, so, at your discretion.

[bors]

✌️ lnicola can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

[lnicola]

The capabilities seem to be sent right from the start, so I'm not sure how we can disable incremental sync by configuration.

Let's merge it and we can disable it before Monday if anyone complains.

bors r=matklad

[bors]

Build succeeded:

• Rust (macos-latest)
• Rust (ubuntu-latest)
• Rust (windows-latest)
• TypeScript

[kjeremy]

The capabilities thing is a bug in lsp-server (see rust-analyzer/lsp-server#14) so I think we could look at the initial params or experimental client caps and change the mode if we need to.

[dbaeumer]

Do you still need an answer on this: #4153 (comment)

	/**
	 * The actual content changes. The content changes describe single state changes
	 * to the document. So if there are two content changes c1 (at array index 0) and
	 * c2 (at array index 1) for a document in state S then c1 moves the document from
	 * S to S' and c2 from S' to S''. So c1 is computed on the state S and c2 is computed
	 * on the state S'.
	 *
	 * To mirror the content of a document using change events use the following approach:
	 * - start with the same initial content
	 * - apply the 'textDocument/didChange' notifications in the order you recevie them.
	 * - apply the `TextDocumentContentChangeEvent`s in a single notification in the order
	 *   you receive them.
	 */

Should explain everything.