Skip to content

fix: Fix process-changes not deduplicating changes correctly #14025

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

Merged
merged 1 commit into from
Jan 25, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/proc-macro-api/src/lib.rs
Original file line number Diff line number Diff line change
@@ -121,7 +121,7 @@ impl ProcMacroServer {
}

pub fn load_dylib(&self, dylib: MacroDylib) -> Result<Vec<ProcMacro>, ServerError> {
let _p = profile::span("ProcMacroClient::by_dylib_path");
let _p = profile::span("ProcMacroClient::load_dylib");
let macros =
self.process.lock().unwrap_or_else(|e| e.into_inner()).find_proc_macros(&dylib.path)?;

36 changes: 25 additions & 11 deletions crates/rust-analyzer/src/global_state.rs
Original file line number Diff line number Diff line change
@@ -3,7 +3,7 @@
//!
//! Each tick provides an immutable snapshot of the state as `WorldSnapshot`.
use std::{sync::Arc, time::Instant};
use std::{mem, sync::Arc, time::Instant};

use crossbeam_channel::{unbounded, Receiver, Sender};
use flycheck::FlycheckHandle;
@@ -197,37 +197,51 @@ impl GlobalState {
// We need to fix up the changed events a bit, if we have a create or modify for a file
// id that is followed by a delete we actually no longer observe the file text from the
// create or modify which may cause problems later on
let mut collapsed_create_delete = false;
changed_files.dedup_by(|a, b| {
use vfs::ChangeKind::*;

let has_collapsed_create_delete = mem::replace(&mut collapsed_create_delete, false);

if a.file_id != b.file_id {
return false;
}

match (a.change_kind, b.change_kind) {
// true => delete the second element (a), we swap them here as they are inverted by dedup_by
match (b.change_kind, a.change_kind) {
// duplicate can be merged
(Create, Create) | (Modify, Modify) | (Delete, Delete) => true,
// just leave the create, modify is irrelevant
(Create, Modify) => {
std::mem::swap(a, b);
(Create, Modify) => true,
// modify becomes irrelevant if the file is deleted
(Modify, Delete) => {
mem::swap(a, b);
true
}
// Remove the create message, and in the following loop, also remove the delete
(Create, Delete) => {
collapsed_create_delete = true;
b.change_kind = Delete;
true
}
// trailing delete from earlier
(Delete, Create | Modify) if has_collapsed_create_delete => {
b.change_kind = Create;
true
}
// modify becomes irrelevant if the file is deleted
(Modify, Delete) => true,
// we should fully remove this occurrence,
// but leaving just a delete works as well
(Create, Delete) => true,
// this is equivalent to a modify
(Delete, Create) => {
a.change_kind = Modify;
b.change_kind = Modify;
true
}
// can't really occur
(Modify, Create) => false,
(Delete, Modify) => false,
}
});

if collapsed_create_delete {
changed_files.pop();
Copy link
Member

@lnicola lnicola Jan 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've only skimmed the changes, but does this work for [Create(f1), Delete(f1), Create(f2), Delete(f2)]?

And generally, I don't think dedup_by guarantees anything about the order of calls. Maybe we should "just" keep a map from FileId to ChangeKind, and go over the input once and update the map, without sorting it first? It might even be (asymptotically) faster. We only keep one ChangeKind per file, except for the "can't really occur" cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should work, because the flag is only true at this point if it was set to true during the last call to the closure (the beginning of the closure resets it to false).

I agree that the code is hard to follow though, and dedup_by is probably not helping with that.

Copy link
Member

@lnicola lnicola Jan 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But on my example, this results in [Delete(f1)] instead of [], which is probably not desirable. collapsed_create_delete will only cause the last Delete to be dropped.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A simple map might be a better choice here indeed

}
for file in &changed_files {
if let Some(path) = vfs.file_path(file.file_id).as_path() {
let path = path.to_path_buf();
4 changes: 2 additions & 2 deletions crates/rust-analyzer/src/reload.rs
Original file line number Diff line number Diff line change
@@ -362,7 +362,7 @@ impl GlobalState {
let loader = &mut self.loader;
let mem_docs = &self.mem_docs;
let mut load = move |path: &AbsPath| {
let _p = profile::span("GlobalState::load");
let _p = profile::span("switch_workspaces::load");
let vfs_path = vfs::VfsPath::from(path.to_path_buf());
if !mem_docs.contains(&vfs_path) {
let contents = loader.handle.load_sync(path);
@@ -584,10 +584,10 @@ pub(crate) fn load_proc_macro(
path: &AbsPath,
dummy_replace: &[Box<str>],
) -> ProcMacroLoadResult {
let server = server.map_err(ToOwned::to_owned)?;
let res: Result<Vec<_>, String> = (|| {
let dylib = MacroDylib::new(path.to_path_buf())
.map_err(|io| format!("Proc-macro dylib loading failed: {io}"))?;
let server = server.map_err(ToOwned::to_owned)?;
let vec = server.load_dylib(dylib).map_err(|e| format!("{e}"))?;
if vec.is_empty() {
return Err("proc macro library returned no proc macros".to_string());
3 changes: 2 additions & 1 deletion crates/vfs/src/lib.rs
Original file line number Diff line number Diff line change
@@ -75,6 +75,7 @@ pub struct Vfs {
}

/// Changed file in the [`Vfs`].
#[derive(Debug)]
pub struct ChangedFile {
/// Id of the changed file
pub file_id: FileId,
@@ -161,9 +162,9 @@ impl Vfs {
let file_id = self.alloc_file_id(path);
let change_kind = match (&self.get(file_id), &contents) {
(None, None) => return false,
(Some(old), Some(new)) if old == new => return false,
(None, Some(_)) => ChangeKind::Create,
(Some(_), None) => ChangeKind::Delete,
(Some(old), Some(new)) if old == new => return false,
(Some(_), Some(_)) => ChangeKind::Modify,
};