Merge pull request #589 from zed-industries/fold-map-test-failures

Antonio Scandurra created

Avoid re-using excerpt IDs in `MultiBuffer`

Change summary

crates/editor/src/display_map/fold_map.rs |  8 ++
crates/editor/src/editor.rs               | 19 +++-
crates/editor/src/multi_buffer.rs         | 84 ++++++++++++++----------
crates/editor/src/multi_buffer/anchor.rs  | 49 ++++----------
crates/language/src/proto.rs              | 44 -------------
crates/text/src/locator.rs                | 29 +++++++-
6 files changed, 109 insertions(+), 124 deletions(-)

Detailed changes

crates/editor/src/display_map/fold_map.rs 🔗

@@ -241,6 +241,14 @@ impl FoldMap {
                 self.buffer.lock().len(),
                 "transform tree does not match buffer's length"
             );
+
+            let mut folds = self.folds.iter().peekable();
+            while let Some(fold) = folds.next() {
+                if let Some(next_fold) = folds.peek() {
+                    let comparison = fold.0.cmp(&next_fold.0, &self.buffer.lock()).unwrap();
+                    assert!(comparison.is_le());
+                }
+            }
         }
     }
 

crates/editor/src/editor.rs 🔗

@@ -4524,25 +4524,32 @@ impl Editor {
         self.remove_blocks([rename.block_id].into_iter().collect(), cx);
         self.clear_highlighted_ranges::<Rename>(cx);
 
-        let editor = rename.editor.read(cx);
-        let snapshot = self.buffer.read(cx).snapshot(cx);
-        let selection = editor.newest_selection_with_snapshot::<usize>(&snapshot);
+        let selection_in_rename_editor = rename.editor.read(cx).newest_selection::<usize>(cx);
 
         // Update the selection to match the position of the selection inside
         // the rename editor.
+        let snapshot = self.buffer.read(cx).read(cx);
         let rename_range = rename.range.to_offset(&snapshot);
         let start = snapshot
-            .clip_offset(rename_range.start + selection.start, Bias::Left)
+            .clip_offset(
+                rename_range.start + selection_in_rename_editor.start,
+                Bias::Left,
+            )
             .min(rename_range.end);
         let end = snapshot
-            .clip_offset(rename_range.start + selection.end, Bias::Left)
+            .clip_offset(
+                rename_range.start + selection_in_rename_editor.end,
+                Bias::Left,
+            )
             .min(rename_range.end);
+        drop(snapshot);
+
         self.update_selections(
             vec![Selection {
                 id: self.newest_anchor_selection().id,
                 start,
                 end,
-                reversed: selection.reversed,
+                reversed: selection_in_rename_editor.reversed,
                 goal: SelectionGoal::None,
             }],
             None,

crates/editor/src/multi_buffer.rs 🔗

@@ -36,6 +36,7 @@ pub type ExcerptId = Locator;
 pub struct MultiBuffer {
     snapshot: RefCell<MultiBufferSnapshot>,
     buffers: RefCell<HashMap<usize, BufferState>>,
+    used_excerpt_ids: SumTree<ExcerptId>,
     subscriptions: Topic,
     singleton: bool,
     replica_id: ReplicaId,
@@ -155,6 +156,7 @@ impl MultiBuffer {
         Self {
             snapshot: Default::default(),
             buffers: Default::default(),
+            used_excerpt_ids: Default::default(),
             subscriptions: Default::default(),
             singleton: false,
             replica_id,
@@ -192,6 +194,7 @@ impl MultiBuffer {
         Self {
             snapshot: RefCell::new(self.snapshot.borrow().clone()),
             buffers: RefCell::new(buffers),
+            used_excerpt_ids: Default::default(),
             subscriptions: Default::default(),
             singleton: self.singleton,
             replica_id: self.replica_id,
@@ -748,20 +751,27 @@ impl MultiBuffer {
         let mut cursor = snapshot.excerpts.cursor::<Option<&ExcerptId>>();
         let mut new_excerpts = cursor.slice(&Some(prev_excerpt_id), Bias::Right, &());
 
-        let mut prev_id = ExcerptId::min();
         let edit_start = new_excerpts.summary().text.bytes;
         new_excerpts.update_last(
             |excerpt| {
                 excerpt.has_trailing_newline = true;
-                prev_id = excerpt.id.clone();
             },
             &(),
         );
 
-        let mut next_id = ExcerptId::max();
-        if let Some(next_excerpt) = cursor.item() {
-            next_id = next_excerpt.id.clone();
-        }
+        let mut used_cursor = self.used_excerpt_ids.cursor::<Locator>();
+        used_cursor.seek(prev_excerpt_id, Bias::Right, &());
+        let mut prev_id = if let Some(excerpt_id) = used_cursor.prev_item() {
+            excerpt_id.clone()
+        } else {
+            ExcerptId::min()
+        };
+        let next_id = if let Some(excerpt_id) = used_cursor.item() {
+            excerpt_id.clone()
+        } else {
+            ExcerptId::max()
+        };
+        drop(used_cursor);
 
         let mut ids = Vec::new();
         while let Some(range) = ranges.next() {
@@ -782,6 +792,10 @@ impl MultiBuffer {
             prev_id = id.clone();
             ids.push(id);
         }
+        self.used_excerpt_ids.edit(
+            ids.iter().cloned().map(sum_tree::Edit::Insert).collect(),
+            &(),
+        );
 
         let edit_end = new_excerpts.summary().text.bytes;
 
@@ -963,10 +977,13 @@ impl MultiBuffer {
     ) -> Option<(ModelHandle<Buffer>, language::Anchor)> {
         let snapshot = self.read(cx);
         let anchor = snapshot.anchor_before(position);
-        Some((
-            self.buffers.borrow()[&anchor.buffer_id?].buffer.clone(),
-            anchor.text_anchor,
-        ))
+        let buffer = self
+            .buffers
+            .borrow()
+            .get(&anchor.buffer_id?)?
+            .buffer
+            .clone();
+        Some((buffer, anchor.text_anchor))
     }
 
     fn on_buffer_event(
@@ -1020,14 +1037,19 @@ impl MultiBuffer {
 
         let snapshot = self.snapshot(cx);
         let anchor = snapshot.anchor_before(position);
-        anchor.buffer_id.map_or(false, |buffer_id| {
-            let buffer = self.buffers.borrow()[&buffer_id].buffer.clone();
-            buffer
-                .read(cx)
-                .completion_triggers()
-                .iter()
-                .any(|string| string == text)
-        })
+        anchor
+            .buffer_id
+            .and_then(|buffer_id| {
+                let buffer = self.buffers.borrow().get(&buffer_id)?.buffer.clone();
+                Some(
+                    buffer
+                        .read(cx)
+                        .completion_triggers()
+                        .iter()
+                        .any(|string| string == text),
+                )
+            })
+            .unwrap_or(false)
     }
 
     pub fn language<'a>(&self, cx: &'a AppContext) -> Option<&'a Arc<Language>> {
@@ -1757,7 +1779,7 @@ impl MultiBufferSnapshot {
 
         let mut position = D::from_text_summary(&cursor.start().text);
         if let Some(excerpt) = cursor.item() {
-            if excerpt.id == anchor.excerpt_id && Some(excerpt.buffer_id) == anchor.buffer_id {
+            if excerpt.id == anchor.excerpt_id {
                 let excerpt_buffer_start = excerpt.range.start.summary::<D>(&excerpt.buffer);
                 let excerpt_buffer_end = excerpt.range.end.summary::<D>(&excerpt.buffer);
                 let buffer_position = cmp::min(
@@ -1788,10 +1810,9 @@ impl MultiBufferSnapshot {
         let mut summaries = Vec::new();
         while let Some(anchor) = anchors.peek() {
             let excerpt_id = &anchor.excerpt_id;
-            let buffer_id = anchor.buffer_id;
             let excerpt_anchors = iter::from_fn(|| {
                 let anchor = anchors.peek()?;
-                if anchor.excerpt_id == *excerpt_id && anchor.buffer_id == buffer_id {
+                if anchor.excerpt_id == *excerpt_id {
                     Some(&anchors.next().unwrap().text_anchor)
                 } else {
                     None
@@ -1805,7 +1826,7 @@ impl MultiBufferSnapshot {
 
             let position = D::from_text_summary(&cursor.start().text);
             if let Some(excerpt) = cursor.item() {
-                if excerpt.id == *excerpt_id && Some(excerpt.buffer_id) == buffer_id {
+                if excerpt.id == *excerpt_id {
                     let excerpt_buffer_start = excerpt.range.start.summary::<D>(&excerpt.buffer);
                     let excerpt_buffer_end = excerpt.range.end.summary::<D>(&excerpt.buffer);
                     summaries.extend(
@@ -1998,10 +2019,8 @@ impl MultiBufferSnapshot {
     pub fn can_resolve(&self, anchor: &Anchor) -> bool {
         if anchor.excerpt_id == ExcerptId::min() || anchor.excerpt_id == ExcerptId::max() {
             true
-        } else if let Some((buffer_id, buffer_snapshot)) =
-            self.buffer_snapshot_for_excerpt(&anchor.excerpt_id)
-        {
-            anchor.buffer_id == Some(buffer_id) && buffer_snapshot.can_resolve(&anchor.text_anchor)
+        } else if let Some(excerpt) = self.excerpt(&anchor.excerpt_id) {
+            excerpt.buffer.can_resolve(&anchor.text_anchor)
         } else {
             false
         }
@@ -2231,15 +2250,12 @@ impl MultiBufferSnapshot {
         ))
     }
 
-    fn buffer_snapshot_for_excerpt<'a>(
-        &'a self,
-        excerpt_id: &'a ExcerptId,
-    ) -> Option<(usize, &'a BufferSnapshot)> {
+    fn excerpt<'a>(&'a self, excerpt_id: &'a ExcerptId) -> Option<&'a Excerpt> {
         let mut cursor = self.excerpts.cursor::<Option<&ExcerptId>>();
         cursor.seek(&Some(excerpt_id), Bias::Left, &());
         if let Some(excerpt) = cursor.item() {
             if excerpt.id == *excerpt_id {
-                return Some((excerpt.buffer_id, &excerpt.buffer));
+                return Some(excerpt);
             }
         }
         None
@@ -3213,8 +3229,8 @@ mod tests {
         let snapshot_2 = multibuffer.read(cx).snapshot(cx);
         assert_eq!(snapshot_2.text(), "ABCD\nGHIJ\nMNOP");
 
-        // The old excerpt id has been reused.
-        assert_eq!(excerpt_id_2, excerpt_id_1);
+        // The old excerpt id doesn't get reused.
+        assert_ne!(excerpt_id_2, excerpt_id_1);
 
         // Resolve some anchors from the previous snapshot in the new snapshot.
         // Although there is still an excerpt with the same id, it is for
@@ -3266,7 +3282,7 @@ mod tests {
         ];
         assert_eq!(
             snapshot_3.summaries_for_anchors::<usize, _>(&anchors),
-            &[0, 2, 9, 13]
+            &[0, 2, 5, 13]
         );
 
         let new_anchors = snapshot_3.refresh_anchors(&anchors);

crates/editor/src/multi_buffer/anchor.rs 🔗

@@ -40,21 +40,8 @@ impl Anchor {
         if excerpt_id_cmp.is_eq() {
             if self.excerpt_id == ExcerptId::min() || self.excerpt_id == ExcerptId::max() {
                 Ok(Ordering::Equal)
-            } else if let Some((buffer_id, buffer_snapshot)) =
-                snapshot.buffer_snapshot_for_excerpt(&self.excerpt_id)
-            {
-                // Even though the anchor refers to a valid excerpt the underlying buffer might have
-                // changed. In that case, treat the anchor as if it were at the start of that
-                // excerpt.
-                if self.buffer_id == Some(buffer_id) && other.buffer_id == Some(buffer_id) {
-                    self.text_anchor.cmp(&other.text_anchor, buffer_snapshot)
-                } else if self.buffer_id == Some(buffer_id) {
-                    Ok(Ordering::Greater)
-                } else if other.buffer_id == Some(buffer_id) {
-                    Ok(Ordering::Less)
-                } else {
-                    Ok(Ordering::Equal)
-                }
+            } else if let Some(excerpt) = snapshot.excerpt(&self.excerpt_id) {
+                self.text_anchor.cmp(&other.text_anchor, &excerpt.buffer)
             } else {
                 Ok(Ordering::Equal)
             }
@@ -65,16 +52,12 @@ impl Anchor {
 
     pub fn bias_left(&self, snapshot: &MultiBufferSnapshot) -> Anchor {
         if self.text_anchor.bias != Bias::Left {
-            if let Some((buffer_id, buffer_snapshot)) =
-                snapshot.buffer_snapshot_for_excerpt(&self.excerpt_id)
-            {
-                if self.buffer_id == Some(buffer_id) {
-                    return Self {
-                        buffer_id: self.buffer_id,
-                        excerpt_id: self.excerpt_id.clone(),
-                        text_anchor: self.text_anchor.bias_left(buffer_snapshot),
-                    };
-                }
+            if let Some(excerpt) = snapshot.excerpt(&self.excerpt_id) {
+                return Self {
+                    buffer_id: self.buffer_id,
+                    excerpt_id: self.excerpt_id.clone(),
+                    text_anchor: self.text_anchor.bias_left(&excerpt.buffer),
+                };
             }
         }
         self.clone()
@@ -82,16 +65,12 @@ impl Anchor {
 
     pub fn bias_right(&self, snapshot: &MultiBufferSnapshot) -> Anchor {
         if self.text_anchor.bias != Bias::Right {
-            if let Some((buffer_id, buffer_snapshot)) =
-                snapshot.buffer_snapshot_for_excerpt(&self.excerpt_id)
-            {
-                if self.buffer_id == Some(buffer_id) {
-                    return Self {
-                        buffer_id: self.buffer_id,
-                        excerpt_id: self.excerpt_id.clone(),
-                        text_anchor: self.text_anchor.bias_right(buffer_snapshot),
-                    };
-                }
+            if let Some(excerpt) = snapshot.excerpt(&self.excerpt_id) {
+                return Self {
+                    buffer_id: self.buffer_id,
+                    excerpt_id: self.excerpt_id.clone(),
+                    text_anchor: self.text_anchor.bias_right(&excerpt.buffer),
+                };
             }
         }
         self.clone()

crates/language/src/proto.rs 🔗

@@ -4,7 +4,6 @@ use crate::{
 };
 use anyhow::{anyhow, Result};
 use clock::ReplicaId;
-use collections::HashSet;
 use lsp::DiagnosticSeverity;
 use rpc::proto;
 use std::{ops::Range, sync::Arc};
@@ -100,26 +99,6 @@ pub fn serialize_undo_map_entry(
     }
 }
 
-pub fn serialize_buffer_fragment(fragment: &text::Fragment) -> proto::BufferFragment {
-    proto::BufferFragment {
-        replica_id: fragment.insertion_timestamp.replica_id as u32,
-        local_timestamp: fragment.insertion_timestamp.local,
-        lamport_timestamp: fragment.insertion_timestamp.lamport,
-        insertion_offset: fragment.insertion_offset as u32,
-        len: fragment.len as u32,
-        visible: fragment.visible,
-        deletions: fragment
-            .deletions
-            .iter()
-            .map(|clock| proto::VectorClockEntry {
-                replica_id: clock.replica_id as u32,
-                timestamp: clock.value,
-            })
-            .collect(),
-        max_undos: serialize_version(&fragment.max_undos),
-    }
-}
-
 pub fn serialize_selections(selections: &Arc<[Selection<Anchor>]>) -> Vec<proto::Selection> {
     selections
         .iter()
@@ -290,29 +269,6 @@ pub fn deserialize_undo_map_entry(
     )
 }
 
-pub fn deserialize_buffer_fragment(
-    message: proto::BufferFragment,
-    ix: usize,
-    count: usize,
-) -> Fragment {
-    Fragment {
-        id: locator::Locator::from_index(ix, count),
-        insertion_timestamp: InsertionTimestamp {
-            replica_id: message.replica_id as ReplicaId,
-            local: message.local_timestamp,
-            lamport: message.lamport_timestamp,
-        },
-        insertion_offset: message.insertion_offset as usize,
-        len: message.len as usize,
-        visible: message.visible,
-        deletions: HashSet::from_iter(message.deletions.into_iter().map(|entry| clock::Local {
-            replica_id: entry.replica_id as ReplicaId,
-            value: entry.timestamp,
-        })),
-        max_undos: deserialize_version(message.max_undos),
-    }
-}
-
 pub fn deserialize_selections(selections: Vec<proto::Selection>) -> Arc<[Selection<Anchor>]> {
     Arc::from(
         selections

crates/text/src/locator.rs 🔗

@@ -19,11 +19,6 @@ impl Locator {
         Self(smallvec![u64::MAX])
     }
 
-    pub fn from_index(ix: usize, count: usize) -> Self {
-        let id = (1 + ix as u64) * (u64::MAX / (count as u64 + 2));
-        Self(smallvec![id])
-    }
-
     pub fn assign(&mut self, other: &Self) {
         self.0.resize(other.0.len(), 0);
         self.0.copy_from_slice(&other.0);
@@ -54,6 +49,30 @@ impl Default for Locator {
     }
 }
 
+impl sum_tree::Item for Locator {
+    type Summary = Locator;
+
+    fn summary(&self) -> Self::Summary {
+        self.clone()
+    }
+}
+
+impl sum_tree::KeyedItem for Locator {
+    type Key = Locator;
+
+    fn key(&self) -> Self::Key {
+        self.clone()
+    }
+}
+
+impl sum_tree::Summary for Locator {
+    type Context = ();
+
+    fn add_summary(&mut self, summary: &Self, _: &()) {
+        self.assign(summary);
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;