Add File.disk_state enum to clarify filesystem states (#20776)

Motivation for this is to make things more understandable while figuring
out #20775.

This is intended to be a refactoring that does not affect behavior, but
there are a few tricky spots:

* Previously `File.mtime()` (now `File.disk_state().mtime()`) would
return last known modification time for deleted files. Looking at uses,
I believe this will not affect anything. If there are behavior changes
here I believe they would be improvements.

* `BufferEvent::DirtyChanged` is now only emitted if dirtiness actually
changed, rather than if it may have changed. This should only be an
efficiency improvement.

Release Notes:

- N/A

Co-authored-by: Mikayla Maki <mikayla@zed.dev>

Cargo.toml

@@ -388,7 +388,7 @@ pet-core = { git = "https://github.com/microsoft/python-environment-tools.git",
 pet-poetry = { git = "https://github.com/microsoft/python-environment-tools.git", rev = "ffcbf3f28c46633abd5448a52b1f396c322e0d6c"  }
 pet-reporter = { git = "https://github.com/microsoft/python-environment-tools.git", rev = "ffcbf3f28c46633abd5448a52b1f396c322e0d6c"  }
 postage = { version = "0.5", features = ["futures-traits"] }
-pretty_assertions = "1.3.0"
+pretty_assertions = { version = "1.3.0", features = ["unstable"] }
 profiling = "1"
 prost = "0.9"
 prost-build = "0.9"

crates/collab/src/tests/random_project_collaboration_tests.rs

@@ -1323,11 +1323,8 @@ impl RandomizedTest for ProjectCollaborationTest {
                     match (host_file, guest_file) {
                         (Some(host_file), Some(guest_file)) => {
                             assert_eq!(guest_file.path(), host_file.path());
-                            assert_eq!(guest_file.is_deleted(), host_file.is_deleted());
+                            assert_eq!(guest_file.disk_state(), host_file.disk_state(),
-                            assert_eq!(
+                                "guest {} disk_state does not match host {} for path {:?} in project {}",
-                                guest_file.mtime(),
-                                host_file.mtime(),
-                                "guest {} mtime does not match host {} for path {:?} in project {}",
                                 guest_user_id,
                                 host_user_id,
                                 guest_file.path(),

crates/copilot/src/copilot.rs

@@ -1229,8 +1229,10 @@ mod tests {
             Some(self)
         }
 
-        fn mtime(&self) -> Option<std::time::SystemTime> {
+        fn disk_state(&self) -> language::DiskState {
-            unimplemented!()
+            language::DiskState::Present {
+                mtime: std::time::UNIX_EPOCH,
+            }
         }
 
         fn path(&self) -> &Arc<Path> {
@@ -1245,10 +1247,6 @@ mod tests {
             unimplemented!()
         }
 
-        fn is_deleted(&self) -> bool {
-            unimplemented!()
-        }
-
         fn as_any(&self) -> &dyn std::any::Any {
             unimplemented!()
         }

crates/editor/src/items.rs

@@ -16,7 +16,8 @@ use gpui::{
     VisualContext, WeakView, WindowContext,
 };
 use language::{
-    proto::serialize_anchor as serialize_text_anchor, Bias, Buffer, CharKind, Point, SelectionGoal,
+    proto::serialize_anchor as serialize_text_anchor, Bias, Buffer, CharKind, DiskState, Point,
+    SelectionGoal,
 };
 use lsp::DiagnosticSeverity;
 use multi_buffer::AnchorRangeExt;
@@ -641,7 +642,7 @@ impl Item for Editor {
             .read(cx)
             .as_singleton()
             .and_then(|buffer| buffer.read(cx).file())
-            .map_or(false, |file| file.is_deleted() && file.is_created());
+            .map_or(false, |file| file.disk_state() == DiskState::Deleted);
 
         h_flex()
             .gap_2()

crates/language/src/buffer.rs

@@ -104,6 +104,7 @@ pub struct Buffer {
     text: TextBuffer,
     diff_base: Option<BufferDiffBase>,
     git_diff: git::diff::BufferDiff,
+    /// Filesystem state, `None` when there is no path.
     file: Option<Arc<dyn File>>,
     /// The mtime of the file when this buffer was last loaded from
     /// or saved to disk.
@@ -371,8 +372,9 @@ pub trait File: Send + Sync {
         self.as_local().is_some()
     }
 
-    /// Returns the file's mtime.
+    /// Returns whether the file is new, exists in storage, or has been deleted. Includes metadata
-    fn mtime(&self) -> Option<SystemTime>;
+    /// only available in some states, such as modification time.
+    fn disk_state(&self) -> DiskState;
 
     /// Returns the path of this file relative to the worktree's root directory.
     fn path(&self) -> &Arc<Path>;
@@ -390,14 +392,6 @@ pub trait File: Send + Sync {
     /// This is needed for looking up project-specific settings.
     fn worktree_id(&self, cx: &AppContext) -> WorktreeId;
 
-    /// Returns whether the file has been deleted.
-    fn is_deleted(&self) -> bool;
-
-    /// Returns whether the file existed on disk at one point
-    fn is_created(&self) -> bool {
-        self.mtime().is_some()
-    }
-
     /// Converts this file into an [`Any`] trait object.
     fn as_any(&self) -> &dyn Any;
 
@@ -408,6 +402,34 @@ pub trait File: Send + Sync {
     fn is_private(&self) -> bool;
 }
 
+/// The file's storage status - whether it's stored (`Present`), and if so when it was last
+/// modified. In the case where the file is not stored, it can be either `New` or `Deleted`. In the
+/// UI these two states are distinguished. For example, the buffer tab does not display a deletion
+/// indicator for new files.
+#[derive(Clone, Copy, Debug, PartialEq)]
+pub enum DiskState {
+    /// File created in Zed that has not been saved.
+    New,
+    /// File present on the filesystem.
+    Present {
+        /// Last known mtime (modification time).
+        mtime: SystemTime,
+    },
+    /// Deleted file that was previously present.
+    Deleted,
+}
+
+impl DiskState {
+    /// Returns the file's last known modification time on disk.
+    pub fn mtime(self) -> Option<SystemTime> {
+        match self {
+            DiskState::New => None,
+            DiskState::Present { mtime } => Some(mtime),
+            DiskState::Deleted => None,
+        }
+    }
+}
+
 /// The file associated with a buffer, in the case where the file is on the local disk.
 pub trait LocalFile: File {
     /// Returns the absolute path of this file
@@ -750,7 +772,7 @@ impl Buffer {
         file: Option<Arc<dyn File>>,
         capability: Capability,
     ) -> Self {
-        let saved_mtime = file.as_ref().and_then(|file| file.mtime());
+        let saved_mtime = file.as_ref().and_then(|file| file.disk_state().mtime());
         let snapshot = buffer.snapshot();
         let git_diff = git::diff::BufferDiff::new(&snapshot);
         let syntax_map = Mutex::new(SyntaxMap::new(&snapshot));
@@ -1014,7 +1036,7 @@ impl Buffer {
         self.reload_task = Some(cx.spawn(|this, mut cx| async move {
             let Some((new_mtime, new_text)) = this.update(&mut cx, |this, cx| {
                 let file = this.file.as_ref()?.as_local()?;
-                Some((file.mtime(), file.load(cx)))
+                Some((file.disk_state().mtime(), file.load(cx)))
             })?
             else {
                 return Ok(());
@@ -1070,6 +1092,7 @@ impl Buffer {
     /// Updates the [`File`] backing this buffer. This should be called when
     /// the file has changed or has been deleted.
     pub fn file_updated(&mut self, new_file: Arc<dyn File>, cx: &mut ModelContext<Self>) {
+        let was_dirty = self.is_dirty();
         let mut file_changed = false;
 
         if let Some(old_file) = self.file.as_ref() {
@@ -1077,21 +1100,12 @@ impl Buffer {
                 file_changed = true;
             }
 
-            if new_file.is_deleted() {
+            let old_state = old_file.disk_state();
-                if !old_file.is_deleted() {
+            let new_state = new_file.disk_state();
-                    file_changed = true;
+            if old_state != new_state {
-                    if !self.is_dirty() {
+                file_changed = true;
-                        cx.emit(BufferEvent::DirtyChanged);
+                if !was_dirty && matches!(new_state, DiskState::Present { .. }) {
-                    }
+                    cx.emit(BufferEvent::ReloadNeeded)
-                }
-            } else {
-                let new_mtime = new_file.mtime();
-                if new_mtime != old_file.mtime() {
-                    file_changed = true;
-
-                    if !self.is_dirty() {
-                        cx.emit(BufferEvent::ReloadNeeded);
-                    }
                 }
             }
         } else {
@@ -1101,6 +1115,9 @@ impl Buffer {
         self.file = Some(new_file);
         if file_changed {
             self.non_text_state_update_count += 1;
+            if was_dirty != self.is_dirty() {
+                cx.emit(BufferEvent::DirtyChanged);
+            }
             cx.emit(BufferEvent::FileHandleChanged);
             cx.notify();
         }
@@ -1742,15 +1759,10 @@ impl Buffer {
     pub fn is_dirty(&self) -> bool {
         self.capability != Capability::ReadOnly
             && (self.has_conflict
-                || self.has_unsaved_edits()
+                || self.file.as_ref().map_or(false, |file| {
-                || self
+                    matches!(file.disk_state(), DiskState::New | DiskState::Deleted)
-                    .file
+                })
-                    .as_ref()
+                || self.has_unsaved_edits())
-                    .map_or(false, |file| file.is_deleted() || !file.is_created()))
-    }
-
-    pub fn is_deleted(&self) -> bool {
-        self.file.as_ref().map_or(false, |file| file.is_deleted())
     }
 
     /// Checks if the buffer and its file have both changed since the buffer
@@ -1762,7 +1774,13 @@ impl Buffer {
         let Some(file) = self.file.as_ref() else {
             return false;
         };
-        file.is_deleted() || (file.mtime() > self.saved_mtime && self.has_unsaved_edits())
+        match file.disk_state() {
+            DiskState::New | DiskState::Deleted => true,
+            DiskState::Present { mtime } => match self.saved_mtime {
+                Some(saved_mtime) => mtime > saved_mtime && self.has_unsaved_edits(),
+                None => true,
+            },
+        }
     }
 
     /// Gets a [`Subscription`] that tracks all of the changes to the buffer's text.
@@ -4403,7 +4421,7 @@ impl File for TestFile {
         None
     }
 
-    fn mtime(&self) -> Option<SystemTime> {
+    fn disk_state(&self) -> DiskState {
         unimplemented!()
     }
 
@@ -4415,10 +4433,6 @@ impl File for TestFile {
         WorktreeId::from_usize(0)
     }
 
-    fn is_deleted(&self) -> bool {
-        unimplemented!()
-    }
-
     fn as_any(&self) -> &dyn std::any::Any {
         unimplemented!()
     }

crates/multi_buffer/src/multi_buffer.rs

@@ -10,9 +10,9 @@ use itertools::Itertools;
 use language::{
     language_settings::{language_settings, LanguageSettings},
     AutoindentMode, Buffer, BufferChunks, BufferRow, BufferSnapshot, Capability, CharClassifier,
-    CharKind, Chunk, CursorShape, DiagnosticEntry, File, IndentGuide, IndentSize, Language,
+    CharKind, Chunk, CursorShape, DiagnosticEntry, DiskState, File, IndentGuide, IndentSize,
-    LanguageScope, OffsetRangeExt, OffsetUtf16, Outline, OutlineItem, Point, PointUtf16, Selection,
+    Language, LanguageScope, OffsetRangeExt, OffsetUtf16, Outline, OutlineItem, Point, PointUtf16,
-    TextDimension, ToOffset as _, ToOffsetUtf16 as _, ToPoint as _, ToPointUtf16 as _,
+    Selection, TextDimension, ToOffset as _, ToOffsetUtf16 as _, ToPoint as _, ToPointUtf16 as _,
     TransactionId, Unclipped,
 };
 use smallvec::SmallVec;
@@ -2035,7 +2035,9 @@ impl MultiBuffer {
             edited |= buffer_edited;
             non_text_state_updated |= buffer_non_text_state_updated;
             is_dirty |= buffer.is_dirty();
-            has_deleted_file |= buffer.file().map_or(false, |file| file.is_deleted());
+            has_deleted_file |= buffer
+                .file()
+                .map_or(false, |file| file.disk_state() == DiskState::Deleted);
             has_conflict |= buffer.has_conflict();
         }
         if edited {

crates/project/src/buffer_store.rs

@@ -20,7 +20,7 @@ use language::{
         deserialize_line_ending, deserialize_version, serialize_line_ending, serialize_version,
         split_operations,
     },
-    Buffer, BufferEvent, Capability, File as _, Language, Operation,
+    Buffer, BufferEvent, Capability, DiskState, File as _, Language, Operation,
 };
 use rpc::{proto, AnyProtoClient, ErrorExt as _, TypedEnvelope};
 use smol::channel::Receiver;
@@ -434,7 +434,10 @@ impl LocalBufferStore {
         let line_ending = buffer.line_ending();
         let version = buffer.version();
         let buffer_id = buffer.remote_id();
-        if buffer.file().is_some_and(|file| !file.is_created()) {
+        if buffer
+            .file()
+            .is_some_and(|file| file.disk_state() == DiskState::New)
+        {
             has_changed_file = true;
         }
 
@@ -444,7 +447,7 @@ impl LocalBufferStore {
 
         cx.spawn(move |this, mut cx| async move {
             let new_file = save.await?;
-            let mtime = new_file.mtime;
+            let mtime = new_file.disk_state().mtime();
             this.update(&mut cx, |this, cx| {
                 if let Some((downstream_client, project_id)) = this.downstream_client(cx) {
                     if has_changed_file {
@@ -658,37 +661,30 @@ impl LocalBufferStore {
                 return None;
             }
 
-            let new_file = if let Some(entry) = old_file
+            let snapshot_entry = old_file
                 .entry_id
                 .and_then(|entry_id| snapshot.entry_for_id(entry_id))
-            {
+                .or_else(|| snapshot.entry_for_path(old_file.path.as_ref()));
+
+            let new_file = if let Some(entry) = snapshot_entry {
                 File {
+                    disk_state: match entry.mtime {
+                        Some(mtime) => DiskState::Present { mtime },
+                        None => old_file.disk_state,
+                    },
                     is_local: true,
                     entry_id: Some(entry.id),
-                    mtime: entry.mtime,
                     path: entry.path.clone(),
                     worktree: worktree.clone(),
-                    is_deleted: false,
-                    is_private: entry.is_private,
-                }
-            } else if let Some(entry) = snapshot.entry_for_path(old_file.path.as_ref()) {
-                File {
-                    is_local: true,
-                    entry_id: Some(entry.id),
-                    mtime: entry.mtime,
-                    path: entry.path.clone(),
-                    worktree: worktree.clone(),
-                    is_deleted: false,
                     is_private: entry.is_private,
                 }
             } else {
                 File {
+                    disk_state: DiskState::Deleted,
                     is_local: true,
                     entry_id: old_file.entry_id,
                     path: old_file.path.clone(),
-                    mtime: old_file.mtime,
                     worktree: worktree.clone(),
-                    is_deleted: true,
                     is_private: old_file.is_private,
                 }
             };
@@ -867,10 +863,9 @@ impl BufferStoreImpl for Model<LocalBufferStore> {
                             Some(Arc::new(File {
                                 worktree,
                                 path,
-                                mtime: None,
+                                disk_state: DiskState::New,
                                 entry_id: None,
                                 is_local: true,
-                                is_deleted: false,
                                 is_private: false,
                             })),
                             Capability::ReadWrite,

crates/project/src/image_store.rs

@@ -9,7 +9,7 @@ use gpui::{
     hash, prelude::*, AppContext, EventEmitter, Img, Model, ModelContext, Subscription, Task,
     WeakModel,
 };
-use language::File;
+use language::{DiskState, File};
 use rpc::{AnyProtoClient, ErrorExt as _};
 use std::ffi::OsStr;
 use std::num::NonZeroU64;
@@ -74,11 +74,12 @@ impl ImageItem {
             file_changed = true;
         }
 
-        if !new_file.is_deleted() {
+        let old_state = old_file.disk_state();
-            let new_mtime = new_file.mtime();
+        let new_state = new_file.disk_state();
-            if new_mtime != old_file.mtime() {
+        if old_state != new_state {
-                file_changed = true;
+            file_changed = true;
-                cx.emit(ImageItemEvent::ReloadNeeded);
+            if matches!(new_state, DiskState::Present { .. }) {
+                cx.emit(ImageItemEvent::ReloadNeeded)
             }
         }
 
@@ -503,37 +504,30 @@ impl LocalImageStore {
                 return;
             }
 
-            let new_file = if let Some(entry) = old_file
+            let snapshot_entry = old_file
                 .entry_id
                 .and_then(|entry_id| snapshot.entry_for_id(entry_id))
-            {
+                .or_else(|| snapshot.entry_for_path(old_file.path.as_ref()));
+
+            let new_file = if let Some(entry) = snapshot_entry {
                 worktree::File {
+                    disk_state: match entry.mtime {
+                        Some(mtime) => DiskState::Present { mtime },
+                        None => old_file.disk_state,
+                    },
                     is_local: true,
                     entry_id: Some(entry.id),
-                    mtime: entry.mtime,
                     path: entry.path.clone(),
                     worktree: worktree.clone(),
-                    is_deleted: false,
-                    is_private: entry.is_private,
-                }
-            } else if let Some(entry) = snapshot.entry_for_path(old_file.path.as_ref()) {
-                worktree::File {
-                    is_local: true,
-                    entry_id: Some(entry.id),
-                    mtime: entry.mtime,
-                    path: entry.path.clone(),
-                    worktree: worktree.clone(),
-                    is_deleted: false,
                     is_private: entry.is_private,
                 }
             } else {
                 worktree::File {
+                    disk_state: DiskState::Deleted,
                     is_local: true,
                     entry_id: old_file.entry_id,
                     path: old_file.path.clone(),
-                    mtime: old_file.mtime,
                     worktree: worktree.clone(),
-                    is_deleted: true,
                     is_private: old_file.is_private,
                 }
             };

crates/project/src/project_tests.rs

@@ -5,12 +5,12 @@ use gpui::{AppContext, SemanticVersion, UpdateGlobal};
 use http_client::Url;
 use language::{
     language_settings::{language_settings, AllLanguageSettings, LanguageSettingsContent},
-    tree_sitter_rust, tree_sitter_typescript, Diagnostic, DiagnosticSet, FakeLspAdapter,
+    tree_sitter_rust, tree_sitter_typescript, Diagnostic, DiagnosticSet, DiskState, FakeLspAdapter,
     LanguageConfig, LanguageMatcher, LanguageName, LineEnding, OffsetRangeExt, Point, ToPoint,
 };
 use lsp::{DiagnosticSeverity, NumberOrString};
 use parking_lot::Mutex;
-use pretty_assertions::assert_eq;
+use pretty_assertions::{assert_eq, assert_matches};
 use serde_json::json;
 #[cfg(not(windows))]
 use std::os;
@@ -3239,10 +3239,22 @@ async fn test_rescan_and_remote_updates(cx: &mut gpui::TestAppContext) {
             Path::new("b/c/file5")
         );
 
-        assert!(!buffer2.read(cx).file().unwrap().is_deleted());
+        assert_matches!(
-        assert!(!buffer3.read(cx).file().unwrap().is_deleted());
+            buffer2.read(cx).file().unwrap().disk_state(),
-        assert!(!buffer4.read(cx).file().unwrap().is_deleted());
+            DiskState::Present { .. }
-        assert!(buffer5.read(cx).file().unwrap().is_deleted());
+        );
+        assert_matches!(
+            buffer3.read(cx).file().unwrap().disk_state(),
+            DiskState::Present { .. }
+        );
+        assert_matches!(
+            buffer4.read(cx).file().unwrap().disk_state(),
+            DiskState::Present { .. }
+        );
+        assert_eq!(
+            buffer5.read(cx).file().unwrap().disk_state(),
+            DiskState::Deleted
+        );
     });
 
     // Update the remote worktree. Check that it becomes consistent with the
@@ -3416,7 +3428,11 @@ async fn test_buffer_is_dirty(cx: &mut gpui::TestAppContext) {
             ]
         );
         events.lock().clear();
-        buffer.did_save(buffer.version(), buffer.file().unwrap().mtime(), cx);
+        buffer.did_save(
+            buffer.version(),
+            buffer.file().unwrap().disk_state().mtime(),
+            cx,
+        );
     });
 
     // after saving, the buffer is not dirty, and emits a saved event.

crates/worktree/src/worktree.rs

@@ -29,6 +29,7 @@ use gpui::{
     Task,
 };
 use ignore::IgnoreStack;
+use language::DiskState;
 use parking_lot::Mutex;
 use paths::local_settings_folder_relative_path;
 use postage::{
@@ -1313,9 +1314,10 @@ impl LocalWorktree {
                         entry_id: None,
                         worktree,
                         path,
-                        mtime: Some(metadata.mtime),
+                        disk_state: DiskState::Present {
+                            mtime: metadata.mtime,
+                        },
                         is_local: true,
-                        is_deleted: false,
                         is_private,
                     })
                 }
@@ -1374,9 +1376,10 @@ impl LocalWorktree {
                         entry_id: None,
                         worktree,
                         path,
-                        mtime: Some(metadata.mtime),
+                        disk_state: DiskState::Present {
+                            mtime: metadata.mtime,
+                        },
                         is_local: true,
-                        is_deleted: false,
                         is_private,
                     })
                 }
@@ -1512,10 +1515,11 @@ impl LocalWorktree {
                 Ok(Arc::new(File {
                     worktree,
                     path,
-                    mtime: Some(metadata.mtime),
+                    disk_state: DiskState::Present {
+                        mtime: metadata.mtime,
+                    },
                     entry_id: None,
                     is_local: true,
-                    is_deleted: false,
                     is_private,
                 }))
             }
@@ -3178,10 +3182,9 @@ impl fmt::Debug for Snapshot {
 pub struct File {
     pub worktree: Model<Worktree>,
     pub path: Arc<Path>,
-    pub mtime: Option<SystemTime>,
+    pub disk_state: DiskState,
     pub entry_id: Option<ProjectEntryId>,
     pub is_local: bool,
-    pub is_deleted: bool,
     pub is_private: bool,
 }
 
@@ -3194,8 +3197,8 @@ impl language::File for File {
         }
     }
 
-    fn mtime(&self) -> Option<SystemTime> {
+    fn disk_state(&self) -> DiskState {
-        self.mtime
+        self.disk_state
     }
 
     fn path(&self) -> &Arc<Path> {
@@ -3238,10 +3241,6 @@ impl language::File for File {
         self.worktree.read(cx).id()
     }
 
-    fn is_deleted(&self) -> bool {
-        self.is_deleted
-    }
-
     fn as_any(&self) -> &dyn Any {
         self
     }
@@ -3251,8 +3250,8 @@ impl language::File for File {
             worktree_id: self.worktree.read(cx).id().to_proto(),
             entry_id: self.entry_id.map(|id| id.to_proto()),
             path: self.path.to_string_lossy().into(),
-            mtime: self.mtime.map(|time| time.into()),
+            mtime: self.disk_state.mtime().map(|time| time.into()),
-            is_deleted: self.is_deleted,
+            is_deleted: self.disk_state == DiskState::Deleted,
         }
     }
 
@@ -3293,10 +3292,13 @@ impl File {
         Arc::new(Self {
             worktree,
             path: entry.path.clone(),
-            mtime: entry.mtime,
+            disk_state: if let Some(mtime) = entry.mtime {
+                DiskState::Present { mtime }
+            } else {
+                DiskState::New
+            },
             entry_id: Some(entry.id),
             is_local: true,
-            is_deleted: false,
             is_private: entry.is_private,
         })
     }
@@ -3316,13 +3318,22 @@ impl File {
             return Err(anyhow!("worktree id does not match file"));
         }
 
+        let disk_state = if proto.is_deleted {
+            DiskState::Deleted
+        } else {
+            if let Some(mtime) = proto.mtime.map(&Into::into) {
+                DiskState::Present { mtime }
+            } else {
+                DiskState::New
+            }
+        };
+
         Ok(Self {
             worktree,
             path: Path::new(&proto.path).into(),
-            mtime: proto.mtime.map(|time| time.into()),
+            disk_state,
             entry_id: proto.entry_id.map(ProjectEntryId::from_proto),
             is_local: false,
-            is_deleted: proto.is_deleted,
             is_private: false,
         })
     }
@@ -3336,10 +3347,9 @@ impl File {
     }
 
     pub fn project_entry_id(&self, _: &AppContext) -> Option<ProjectEntryId> {
-        if self.is_deleted {
+        match self.disk_state {
-            None
+            DiskState::Deleted => None,
-        } else {
+            _ => self.entry_id,
-            self.entry_id
         }
     }
 }