From 89392cd23d5f2573a8087c8ae219e7cd4cdb05fe Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Tue, 2 Nov 2021 14:33:55 -0700 Subject: [PATCH] Avoid using worktree handle in File's path methods This avoids a circular model update that was happening when trying to retrieve the absolute path from a buffer's file while applying remote operations. --- crates/language/src/lib.rs | 16 +++--- crates/project/src/worktree.rs | 100 +++++++++++++++++++-------------- crates/workspace/src/items.rs | 6 +- 3 files changed, 68 insertions(+), 54 deletions(-) diff --git a/crates/language/src/lib.rs b/crates/language/src/lib.rs index dad8af83df..735bd0f3f0 100644 --- a/crates/language/src/lib.rs +++ b/crates/language/src/lib.rs @@ -127,15 +127,15 @@ pub trait File { fn path(&self) -> &Arc; /// Returns the absolute path of this file. - fn abs_path(&self, cx: &AppContext) -> Option; + fn abs_path(&self) -> Option; /// Returns the path of this file relative to the worktree's parent directory (this means it /// includes the name of the worktree's root folder). - fn full_path(&self, cx: &AppContext) -> PathBuf; + fn full_path(&self) -> PathBuf; /// Returns the last component of this handle's absolute path. If this handle refers to the root /// of its worktree, then this method will return the name of the worktree itself. - fn file_name<'a>(&'a self, cx: &'a AppContext) -> Option; + fn file_name(&self) -> Option; fn is_deleted(&self) -> bool; @@ -455,7 +455,7 @@ impl Buffer { }; self.reparse(cx); - self.update_language_server(cx); + self.update_language_server(); } pub fn did_save( @@ -479,7 +479,7 @@ impl Buffer { lsp::DidSaveTextDocumentParams { text_document: lsp::TextDocumentIdentifier { uri: lsp::Url::from_file_path( - self.file.as_ref().unwrap().abs_path(cx).unwrap(), + self.file.as_ref().unwrap().abs_path().unwrap(), ) .unwrap(), }, @@ -1121,7 +1121,7 @@ impl Buffer { Ok(()) } - fn update_language_server(&mut self, cx: &AppContext) { + fn update_language_server(&mut self) { let language_server = if let Some(language_server) = self.language_server.as_mut() { language_server } else { @@ -1131,7 +1131,7 @@ impl Buffer { .file .as_ref() .map_or(Path::new("/").to_path_buf(), |file| { - file.abs_path(cx).unwrap() + file.abs_path().unwrap() }); let version = post_inc(&mut language_server.next_version); @@ -1266,7 +1266,7 @@ impl Buffer { } self.reparse(cx); - self.update_language_server(cx); + self.update_language_server(); cx.emit(Event::Edited); if !was_dirty { diff --git a/crates/project/src/worktree.rs b/crates/project/src/worktree.rs index 1862a342aa..d8ae0e3aa8 100644 --- a/crates/project/src/worktree.rs +++ b/crates/project/src/worktree.rs @@ -616,6 +616,8 @@ impl Worktree { } }; + let local = self.as_local().is_some(); + let worktree_path = self.abs_path.clone(); let worktree_handle = cx.handle(); let mut buffers_to_delete = Vec::new(); for (buffer_id, buffer) in open_buffers { @@ -627,6 +629,8 @@ impl Worktree { .and_then(|entry_id| self.entry_for_id(entry_id)) { File { + is_local: local, + worktree_path: worktree_path.clone(), entry_id: Some(entry.id), mtime: entry.mtime, path: entry.path.clone(), @@ -634,6 +638,8 @@ impl Worktree { } } else if let Some(entry) = self.entry_for_path(old_file.path().as_ref()) { File { + is_local: local, + worktree_path: worktree_path.clone(), entry_id: Some(entry.id), mtime: entry.mtime, path: entry.path.clone(), @@ -641,6 +647,8 @@ impl Worktree { } } else { File { + is_local: local, + worktree_path: worktree_path.clone(), entry_id: None, path: old_file.path().clone(), mtime: old_file.mtime(), @@ -976,12 +984,9 @@ impl LocalWorktree { let (file, contents) = this .update(&mut cx, |this, cx| this.as_local().unwrap().load(&path, cx)) .await?; - let language = this.read_with(&cx, |this, cx| { + let language = this.read_with(&cx, |this, _| { use language::File; - - this.languages() - .select_language(file.full_path(cx)) - .cloned() + this.languages().select_language(file.full_path()).cloned() }); let diagnostics = this.update(&mut cx, |this, _| { this.as_local_mut() @@ -1144,6 +1149,7 @@ impl LocalWorktree { fn load(&self, path: &Path, cx: &mut ModelContext) -> Task> { let handle = cx.handle(); let path = Arc::from(path); + let worktree_path = self.abs_path.clone(); let abs_path = self.absolutize(&path); let background_snapshot = self.background_snapshot.clone(); let fs = self.fs.clone(); @@ -1152,7 +1158,17 @@ impl LocalWorktree { // Eagerly populate the snapshot with an updated entry for the loaded file let entry = refresh_entry(fs.as_ref(), &background_snapshot, path, &abs_path).await?; this.update(&mut cx, |this, cx| this.poll_snapshot(cx)); - Ok((File::new(entry.id, handle, entry.path, entry.mtime), text)) + Ok(( + File { + entry_id: Some(entry.id), + worktree: handle, + worktree_path, + path: entry.path, + mtime: entry.mtime, + is_local: true, + }, + text, + )) }) } @@ -1167,11 +1183,16 @@ impl LocalWorktree { cx.spawn(|this, mut cx| async move { let entry = save.await?; this.update(&mut cx, |this, cx| { - this.as_local_mut() - .unwrap() - .open_buffers - .insert(buffer.id(), buffer.downgrade()); - Ok(File::new(entry.id, cx.handle(), entry.path, entry.mtime)) + let this = this.as_local_mut().unwrap(); + this.open_buffers.insert(buffer.id(), buffer.downgrade()); + Ok(File { + entry_id: Some(entry.id), + worktree: cx.handle(), + worktree_path: this.abs_path.clone(), + path: entry.path, + mtime: entry.mtime, + is_local: true, + }) }) }) } @@ -1360,6 +1381,7 @@ impl RemoteWorktree { let rpc = self.client.clone(); let replica_id = self.replica_id; let remote_worktree_id = self.remote_id; + let root_path = self.snapshot.abs_path.clone(); let path = path.to_string_lossy().to_string(); cx.spawn_weak(|this, mut cx| async move { if let Some(existing_buffer) = existing_buffer { @@ -1380,13 +1402,17 @@ impl RemoteWorktree { let this = this .upgrade(&cx) .ok_or_else(|| anyhow!("worktree was closed"))?; - let file = File::new(entry.id, this.clone(), entry.path, entry.mtime); - let language = this.read_with(&cx, |this, cx| { + let file = File { + entry_id: Some(entry.id), + worktree: this.clone(), + worktree_path: root_path, + path: entry.path, + mtime: entry.mtime, + is_local: false, + }; + let language = this.read_with(&cx, |this, _| { use language::File; - - this.languages() - .select_language(file.full_path(cx)) - .cloned() + this.languages().select_language(file.full_path()).cloned() }); let remote_buffer = response.buffer.ok_or_else(|| anyhow!("empty buffer"))?; let buffer_id = remote_buffer.id as usize; @@ -1868,24 +1894,10 @@ impl fmt::Debug for Snapshot { pub struct File { entry_id: Option, worktree: ModelHandle, + worktree_path: Arc, pub path: Arc, pub mtime: SystemTime, -} - -impl File { - pub fn new( - entry_id: usize, - worktree: ModelHandle, - path: Arc, - mtime: SystemTime, - ) -> Self { - Self { - entry_id: Some(entry_id), - worktree, - path, - mtime, - } - } + is_local: bool, } impl language::File for File { @@ -1905,27 +1917,29 @@ impl language::File for File { &self.path } - fn abs_path(&self, cx: &AppContext) -> Option { - let worktree = self.worktree.read(cx); - worktree - .as_local() - .map(|worktree| worktree.absolutize(&self.path)) + fn abs_path(&self) -> Option { + if self.is_local { + Some(self.worktree_path.join(&self.path)) + } else { + None + } } - fn full_path(&self, cx: &AppContext) -> PathBuf { - let worktree = self.worktree.read(cx); + fn full_path(&self) -> PathBuf { let mut full_path = PathBuf::new(); - full_path.push(worktree.root_name()); + if let Some(worktree_name) = self.worktree_path.file_name() { + full_path.push(worktree_name); + } full_path.push(&self.path); full_path } /// Returns the last component of this handle's absolute path. If this handle refers to the root /// of its worktree, then this method will return the name of the worktree itself. - fn file_name<'a>(&'a self, cx: &'a AppContext) -> Option { + fn file_name<'a>(&'a self) -> Option { self.path .file_name() - .or_else(|| Some(OsStr::new(self.worktree.read(cx).root_name()))) + .or_else(|| self.worktree_path.file_name()) .map(Into::into) } diff --git a/crates/workspace/src/items.rs b/crates/workspace/src/items.rs index d1275aee7c..e9411e93cc 100644 --- a/crates/workspace/src/items.rs +++ b/crates/workspace/src/items.rs @@ -77,7 +77,7 @@ impl ItemView for Editor { .buffer() .read(cx) .file() - .and_then(|file| file.file_name(cx)); + .and_then(|file| file.file_name()); if let Some(name) = filename { name.to_string_lossy().into() } else { @@ -127,8 +127,8 @@ impl ItemView for Editor { cx.spawn(|buffer, mut cx| async move { save_as.await.map(|new_file| { - let (language, language_server) = worktree.read_with(&cx, |worktree, cx| { - let language = worktree.languages().select_language(new_file.full_path(cx)); + let (language, language_server) = worktree.read_with(&cx, |worktree, _| { + let language = worktree.languages().select_language(new_file.full_path()); let language_server = worktree.language_server(); (language.cloned(), language_server.cloned()) });