how does r-a handle incrementally updating files?

[azdavis]

i lifted (with minor tweaks) apply_document_changes from the r-a codebase and put it into my own language server for standard ml, millet.

recently there was an issue on millet involving frequent crashes, azdavis/millet#53. i investigated and was able to reproduce.

the proximate cause is that String::replace_range panics in apply_document_changes. the reason why appears to be that we get a DidChangeWatchedFiles in between some DidChangeTextDocument notifications.

specifically, the file contents that we read from disk as a result of the DidChangeWatchedFiles notif seem out of date with what's in the editor, so any following DidChangeTextDocument notifications cause us to crash.

i was wondering how r-a handles this? i know vaguely there is a Vfs used across r-a; my own lsp does not have this.

[lnicola]

I think

rust-analyzer/crates/rust-analyzer/src/handlers/notification.rs

Line 245 in b9b0d29

pub(crate) fn handle_did_change_watched_files(

calls into vfs-notify, which reads the file and tells vfs about it, then in

rust-analyzer/crates/rust-analyzer/src/global_state.rs

Line 236 in b9b0d29

pub(crate) fn process_changes(&mut self) -> bool {

we pull in those changes.

But I didn't understand this part in your linked issue:

3. the current contents on the filesystem are not up-to-date somehow??
4. we get another DidChangeTextDocument based on the more up-to-date file contents
5. trying to apply those changes to the out-of-date file contents from the fs fails

If you're re-reading the files from disk, why do you see at step 5 the file contents from step 3 or earlier?

[azdavis]

what happens is at step 3 we read the file from disk, but that doesn't have the most up to date unsaved changes from the editor. so then we set the files contents to the old contents from the disk. but then in steps 4 and 5 we're getting more changed notifications from the client, which when we try to apply to the old version of the file (which we just read from the disk) that fails.

[azdavis]

maybe i should ignore did changed watches files if the file is opened and we're getting did change notifications about it? i noticed in the docs for DidOpenTextDocument:

The document's truth is now managed by the client and the server must not try to read the document's truth using the document's uri

but i don't register for DidOpenTextDocument notifications. maybe i should, and when a file is opened mark it as ignored for the purposes of responding to DidChangeWatchedFiles?

[lnicola]

Oh, I'm less familiar with that part, but I think we have a separate set of "edited" files, so the VFS knows about what's on disk, but we overlay those on top. Not sure how it interacts with salsa though (because we do push the reloaded files into salsa AFAIK).

[Veykril]

but i don't register for DidOpenTextDocument notifications. maybe i should, and when a file is opened mark it as ignored for the purposes of responding to DidChangeWatchedFiles?

Yes you should, you can't reasonably handle DidChangeTextDocument notifications without keeping track which files the client has opened. The spec even requires the client to only send DidChangeTextDocument notifications for documents that are considedered opened at the time by DidOpenTextDocument

[Veykril]

We explicitly ignore our file watcher for documents that are considered open,

rust-analyzer/crates/rust-analyzer/src/main_loop.rs

Lines 611 to 617 in b9b0d29

	// if the file is in mem docs, it's managed by the client via notifications
	// so only set it if its not in there
	if !self.mem_docs.contains(&path)
	&& (is_changed || vfs.file_id(&path).is_none())
	{
	vfs.set_file_contents(path, contents);
	}