multi_buffer: Fix handling of `ExcerptId::max()` (#38887)

Lukas Wirth created

This removes a hack from `MultiBuffer::anchor_at` that works around
missing logic for handling `ExcerptId::max()` by implementing that said
missing logic.

Generally, `ExcerptId::min()` is already being handled correctly due to
how `Cursor` seeking works, we tend to seek to or beyond a seek target,
meaning `min` will always match the first excerpt as expected. `max` on
the other hand will always seek beyond the last excerpt resulting in no
excerpt being found, so any code path dealing with the excerpt sumtree
will have to specially check for this special excerpt ID to work
correctly.

Release Notes:

- N/A *or* Added/Fixed/Improved ...

Change summary

crates/editor/src/editor.rs                |  51 +++----
crates/editor/src/element.rs               |   4 
crates/editor/src/hover_links.rs           |  14 -
crates/editor/src/hover_popover.rs         |   4 
crates/editor/src/inlay_hint_cache.rs      |   2 
crates/editor/src/items.rs                 |  18 -
crates/editor/src/movement.rs              |   8 
crates/editor/src/scroll.rs                |   6 
crates/editor/src/selections_collection.rs |   9 +
crates/multi_buffer/src/anchor.rs          |   7 +
crates/multi_buffer/src/multi_buffer.rs    | 153 ++++++++---------------
crates/vim/src/helix.rs                    |   2 
12 files changed, 114 insertions(+), 164 deletions(-)

Detailed changes

crates/editor/src/editor.rs 🔗

@@ -3211,22 +3211,27 @@ impl Editor {
                 let background_executor = cx.background_executor().clone();
                 let editor_id = cx.entity().entity_id().as_u64() as ItemId;
                 self.serialize_selections = cx.background_spawn(async move {
-                            background_executor.timer(SERIALIZATION_THROTTLE_TIME).await;
-                            let db_selections = selections
-                                .iter()
-                                .map(|selection| {
-                                    (
-                                        selection.start.to_offset(&snapshot),
-                                        selection.end.to_offset(&snapshot),
-                                    )
-                                })
-                                .collect();
+                    background_executor.timer(SERIALIZATION_THROTTLE_TIME).await;
+                    let db_selections = selections
+                        .iter()
+                        .map(|selection| {
+                            (
+                                selection.start.to_offset(&snapshot),
+                                selection.end.to_offset(&snapshot),
+                            )
+                        })
+                        .collect();
 
-                            DB.save_editor_selections(editor_id, workspace_id, db_selections)
-                                .await
-                                .with_context(|| format!("persisting editor selections for editor {editor_id}, workspace {workspace_id:?}"))
-                                .log_err();
-                        });
+                    DB.save_editor_selections(editor_id, workspace_id, db_selections)
+                        .await
+                        .with_context(|| {
+                            format!(
+                                "persisting editor selections for editor {editor_id}, \
+                                workspace {workspace_id:?}"
+                            )
+                        })
+                        .log_err();
+                });
             }
         }
 
@@ -6871,17 +6876,7 @@ impl Editor {
                                 continue;
                             }
 
-                            let range = Anchor {
-                                buffer_id: Some(buffer_id),
-                                excerpt_id,
-                                text_anchor: start,
-                                diff_base_anchor: None,
-                            }..Anchor {
-                                buffer_id: Some(buffer_id),
-                                excerpt_id,
-                                text_anchor: end,
-                                diff_base_anchor: None,
-                            };
+                            let range = Anchor::range_in_buffer(excerpt_id, buffer_id, start..end);
                             if highlight.kind == lsp::DocumentHighlightKind::WRITE {
                                 write_ranges.push(range);
                             } else {
@@ -12886,7 +12881,7 @@ impl Editor {
         self.hide_mouse_cursor(HideMouseCursorOrigin::MovementAction, cx);
         self.change_selections(Default::default(), window, cx, |s| {
             s.move_heads_with(|map, head, _| (movement::right(map, head), SelectionGoal::None));
-        })
+        });
     }
 
     pub fn move_up(&mut self, _: &MoveUp, window: &mut Window, cx: &mut Context<Self>) {
@@ -15873,7 +15868,7 @@ impl Editor {
             let snapshot = multi_buffer.snapshot(cx);
             if let Some(buffer_id) = snapshot.buffer_id_for_excerpt(excerpt)
                 && let Some(buffer) = multi_buffer.buffer(buffer_id)
-                && let Some(excerpt_range) = snapshot.buffer_range_for_excerpt(excerpt)
+                && let Some(excerpt_range) = snapshot.context_range_for_excerpt(excerpt)
             {
                 let buffer_snapshot = buffer.read(cx).snapshot();
                 let excerpt_end_row = Point::from_anchor(&excerpt_range.end, &buffer_snapshot).row;

crates/editor/src/element.rs 🔗

@@ -1311,10 +1311,10 @@ impl EditorElement {
 
         let range = snapshot
             .buffer_snapshot
-            .anchor_at(start.to_point(&snapshot.display_snapshot), Bias::Left)
+            .anchor_before(start.to_point(&snapshot.display_snapshot))
             ..snapshot
                 .buffer_snapshot
-                .anchor_at(end.to_point(&snapshot.display_snapshot), Bias::Right);
+                .anchor_after(end.to_point(&snapshot.display_snapshot));
 
         let Some(selection) = snapshot.remote_selections_in_range(&range, hub, cx).next() else {
             return;

crates/editor/src/hover_links.rs 🔗

@@ -301,14 +301,10 @@ pub fn update_inlay_link_and_hover_points(
     let mut hover_updated = false;
     if let Some(hovered_offset) = hovered_offset {
         let buffer_snapshot = editor.buffer().read(cx).snapshot(cx);
-        let previous_valid_anchor = buffer_snapshot.anchor_at(
-            point_for_position.previous_valid.to_point(snapshot),
-            Bias::Left,
-        );
-        let next_valid_anchor = buffer_snapshot.anchor_at(
-            point_for_position.next_valid.to_point(snapshot),
-            Bias::Right,
-        );
+        let previous_valid_anchor =
+            buffer_snapshot.anchor_before(point_for_position.previous_valid.to_point(snapshot));
+        let next_valid_anchor =
+            buffer_snapshot.anchor_after(point_for_position.next_valid.to_point(snapshot));
         if let Some(hovered_hint) = editor
             .visible_inlay_hints(cx)
             .into_iter()
@@ -1400,7 +1396,7 @@ mod tests {
             let buffer_snapshot = editor.buffer().update(cx, |buffer, cx| buffer.snapshot(cx));
             let expected_highlight = InlayHighlight {
                 inlay: InlayId::Hint(0),
-                inlay_position: buffer_snapshot.anchor_at(inlay_range.start, Bias::Right),
+                inlay_position: buffer_snapshot.anchor_after(inlay_range.start),
                 range: 0..hint_label.len(),
             };
             assert_set_eq!(actual_highlights, vec![&expected_highlight]);

crates/editor/src/hover_popover.rs 🔗

@@ -1785,7 +1785,7 @@ mod tests {
                 popover.symbol_range,
                 RangeInEditor::Inlay(InlayHighlight {
                     inlay: InlayId::Hint(0),
-                    inlay_position: buffer_snapshot.anchor_at(inlay_range.start, Bias::Right),
+                    inlay_position: buffer_snapshot.anchor_after(inlay_range.start),
                     range: ": ".len()..": ".len() + new_type_label.len(),
                 }),
                 "Popover range should match the new type label part"
@@ -1840,7 +1840,7 @@ mod tests {
                 popover.symbol_range,
                 RangeInEditor::Inlay(InlayHighlight {
                     inlay: InlayId::Hint(0),
-                    inlay_position: buffer_snapshot.anchor_at(inlay_range.start, Bias::Right),
+                    inlay_position: buffer_snapshot.anchor_after(inlay_range.start),
                     range: ": ".len() + new_type_label.len() + "<".len()
                         ..": ".len() + new_type_label.len() + "<".len() + struct_label.len(),
                 }),

crates/editor/src/inlay_hint_cache.rs 🔗

@@ -2251,7 +2251,7 @@ pub mod tests {
             .unwrap();
     }
 
-    #[gpui::test(iterations = 10)]
+    #[gpui::test(iterations = 4)]
     async fn test_large_buffer_inlay_requests_split(cx: &mut gpui::TestAppContext) {
         init_test(cx, |settings| {
             settings.defaults.inlay_hints = Some(InlayHintSettingsContent {

crates/editor/src/items.rs 🔗

@@ -578,12 +578,11 @@ fn deserialize_selection(
 
 fn deserialize_anchor(buffer: &MultiBufferSnapshot, anchor: proto::EditorAnchor) -> Option<Anchor> {
     let excerpt_id = ExcerptId::from_proto(anchor.excerpt_id);
-    Some(Anchor {
+    Some(Anchor::in_buffer(
         excerpt_id,
-        text_anchor: language::proto::deserialize_anchor(anchor.anchor?)?,
-        buffer_id: buffer.buffer_id_for_excerpt(excerpt_id),
-        diff_base_anchor: None,
-    })
+        buffer.buffer_id_for_excerpt(excerpt_id)?,
+        language::proto::deserialize_anchor(anchor.anchor?)?,
+    ))
 }
 
 impl Item for Editor {
@@ -1752,13 +1751,8 @@ impl SearchableItem for Editor {
                                         .anchor_after(search_range.start + match_range.start);
                                     let end = search_buffer
                                         .anchor_before(search_range.start + match_range.end);
-                                    Anchor {
-                                        diff_base_anchor: Some(start),
-                                        ..deleted_hunk_anchor
-                                    }..Anchor {
-                                        diff_base_anchor: Some(end),
-                                        ..deleted_hunk_anchor
-                                    }
+                                    deleted_hunk_anchor.with_diff_base_anchor(start)
+                                        ..deleted_hunk_anchor.with_diff_base_anchor(end)
                                 } else {
                                     let start = search_buffer
                                         .anchor_after(search_range.start + match_range.start);

crates/editor/src/movement.rs 🔗

@@ -1018,22 +1018,22 @@ mod tests {
                 [
                     Inlay::edit_prediction(
                         post_inc(&mut id),
-                        buffer_snapshot.anchor_at(offset, Bias::Left),
+                        buffer_snapshot.anchor_before(offset),
                         "test",
                     ),
                     Inlay::edit_prediction(
                         post_inc(&mut id),
-                        buffer_snapshot.anchor_at(offset, Bias::Right),
+                        buffer_snapshot.anchor_after(offset),
                         "test",
                     ),
                     Inlay::mock_hint(
                         post_inc(&mut id),
-                        buffer_snapshot.anchor_at(offset, Bias::Left),
+                        buffer_snapshot.anchor_before(offset),
                         "test",
                     ),
                     Inlay::mock_hint(
                         post_inc(&mut id),
-                        buffer_snapshot.anchor_at(offset, Bias::Right),
+                        buffer_snapshot.anchor_after(offset),
                         "test",
                     ),
                 ]

crates/editor/src/scroll.rs 🔗

@@ -244,9 +244,7 @@ impl ScrollManager {
                 Bias::Left,
             )
             .to_point(map);
-        let top_anchor = map
-            .buffer_snapshot
-            .anchor_at(scroll_top_buffer_point, Bias::Right);
+        let top_anchor = map.buffer_snapshot.anchor_after(scroll_top_buffer_point);
 
         self.set_anchor(
             ScrollAnchor {
@@ -767,7 +765,7 @@ impl Editor {
                 .buffer()
                 .read(cx)
                 .snapshot(cx)
-                .anchor_at(Point::new(top_row, 0), Bias::Left);
+                .anchor_before(Point::new(top_row, 0));
             let scroll_anchor = ScrollAnchor {
                 offset: gpui::Point::new(x, y),
                 anchor: top_anchor,

crates/editor/src/selections_collection.rs 🔗

@@ -440,6 +440,15 @@ pub struct MutableSelectionsCollection<'a> {
     cx: &'a mut App,
 }
 
+impl<'a> fmt::Debug for MutableSelectionsCollection<'a> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_struct("MutableSelectionsCollection")
+            .field("collection", &self.collection)
+            .field("selections_changed", &self.selections_changed)
+            .finish()
+    }
+}
+
 impl<'a> MutableSelectionsCollection<'a> {
     pub fn display_map(&mut self) -> DisplaySnapshot {
         self.collection.display_map(self.cx)

crates/multi_buffer/src/anchor.rs 🔗

@@ -16,6 +16,13 @@ pub struct Anchor {
 }
 
 impl Anchor {
+    pub fn with_diff_base_anchor(self, diff_base_anchor: text::Anchor) -> Self {
+        Self {
+            diff_base_anchor: Some(diff_base_anchor),
+            ..self
+        }
+    }
+
     pub fn in_buffer(
         excerpt_id: ExcerptId,
         buffer_id: BufferId,

crates/multi_buffer/src/multi_buffer.rs 🔗

@@ -1310,11 +1310,9 @@ impl MultiBuffer {
             let end_locator = snapshot.excerpt_locator_for_id(selection.end.excerpt_id);
 
             cursor.seek(&Some(start_locator), Bias::Left);
-            while let Some(excerpt) = cursor.item() {
-                if excerpt.locator > *end_locator {
-                    break;
-                }
-
+            while let Some(excerpt) = cursor.item()
+                && excerpt.locator <= *end_locator
+            {
                 let mut start = excerpt.range.context.start;
                 let mut end = excerpt.range.context.end;
                 if excerpt.id == selection.start.excerpt_id {
@@ -4793,7 +4791,6 @@ impl MultiBufferSnapshot {
     where
         D: TextDimension,
     {
-        // let mut range = range.start..range.end;
         let mut summary = D::zero(());
         let mut cursor = self.excerpts.cursor::<ExcerptOffset>(());
         cursor.seek(&range.start, Bias::Right);
@@ -4917,13 +4914,13 @@ impl MultiBufferSnapshot {
         let locator = self.excerpt_locator_for_id(anchor.excerpt_id);
 
         cursor.seek(&Some(locator), Bias::Left);
-        if cursor.item().is_none() {
-            cursor.next();
+        if cursor.item().is_none() && anchor.excerpt_id == ExcerptId::max() {
+            cursor.prev();
         }
 
         let mut position = cursor.start().1;
         if let Some(excerpt) = cursor.item()
-            && excerpt.id == anchor.excerpt_id
+            && (excerpt.id == anchor.excerpt_id || anchor.excerpt_id == ExcerptId::max())
         {
             let excerpt_buffer_start = excerpt
                 .buffer
@@ -5086,20 +5083,14 @@ impl MultiBufferSnapshot {
             let old_locator = self.excerpt_locator_for_id(old_excerpt_id);
             cursor.seek_forward(&Some(old_locator), Bias::Left);
 
-            if cursor.item().is_none() {
-                cursor.next();
-            }
-
             let next_excerpt = cursor.item();
             let prev_excerpt = cursor.prev_item();
 
             // Process all of the anchors for this excerpt.
-            while let Some((_, anchor)) = anchors.peek() {
-                if anchor.excerpt_id != old_excerpt_id {
-                    break;
-                }
-                let (anchor_ix, anchor) = anchors.next().unwrap();
-                let mut anchor = *anchor;
+            while let Some((anchor_ix, &anchor)) =
+                anchors.next_if(|(_, anchor)| anchor.excerpt_id == old_excerpt_id)
+            {
+                let mut anchor = anchor;
 
                 // Leave min and max anchors unchanged if invalid or
                 // if the old excerpt still exists at this location
@@ -5135,12 +5126,7 @@ impl MultiBufferSnapshot {
                         {
                             text_anchor = excerpt.range.context.end;
                         }
-                        Anchor {
-                            buffer_id: Some(excerpt.buffer_id),
-                            excerpt_id: excerpt.id,
-                            text_anchor,
-                            diff_base_anchor: None,
-                        }
+                        Anchor::in_buffer(excerpt.id, excerpt.buffer_id, text_anchor)
                     } else if let Some(excerpt) = prev_excerpt {
                         let mut text_anchor = excerpt
                             .range
@@ -5153,12 +5139,7 @@ impl MultiBufferSnapshot {
                         {
                             text_anchor = excerpt.range.context.start;
                         }
-                        Anchor {
-                            buffer_id: Some(excerpt.buffer_id),
-                            excerpt_id: excerpt.id,
-                            text_anchor,
-                            diff_base_anchor: None,
-                        }
+                        Anchor::in_buffer(excerpt.id, excerpt.buffer_id, text_anchor)
                     } else if anchor.text_anchor.bias == Bias::Left {
                         Anchor::min()
                     } else {
@@ -5240,24 +5221,15 @@ impl MultiBufferSnapshot {
             let buffer_start = excerpt.range.context.start.to_offset(&excerpt.buffer);
             let text_anchor =
                 excerpt.clip_anchor(excerpt.buffer.anchor_at(buffer_start + overshoot, bias));
-            Anchor {
-                buffer_id: Some(excerpt.buffer_id),
-                excerpt_id: excerpt.id,
-                text_anchor,
-                diff_base_anchor,
+            let anchor = Anchor::in_buffer(excerpt.id, excerpt.buffer_id, text_anchor);
+            match diff_base_anchor {
+                Some(diff_base_anchor) => anchor.with_diff_base_anchor(diff_base_anchor),
+                None => anchor,
             }
+        } else if excerpt_offset.is_zero() && bias == Bias::Left {
+            Anchor::min()
         } else {
-            let mut anchor = if excerpt_offset.is_zero() && bias == Bias::Left {
-                Anchor::min()
-            } else {
-                Anchor::max()
-            };
-
-            // TODO this is a hack, because all APIs should be able to handle ExcerptId::min and max.
-            if let Some((excerpt_id, _, _)) = self.as_singleton() {
-                anchor.excerpt_id = *excerpt_id;
-            }
-            anchor
+            Anchor::max()
         }
     }
 
@@ -5269,22 +5241,12 @@ impl MultiBufferSnapshot {
         text_anchor: text::Anchor,
     ) -> Option<Anchor> {
         let excerpt_id = self.latest_excerpt_id(excerpt_id);
-        let locator = self.excerpt_locator_for_id(excerpt_id);
-        let mut cursor = self.excerpts.cursor::<Option<&Locator>>(());
-        cursor.seek(locator, Bias::Left);
-        if let Some(excerpt) = cursor.item()
-            && excerpt.id == excerpt_id
-        {
-            let text_anchor = excerpt.clip_anchor(text_anchor);
-            drop(cursor);
-            return Some(Anchor {
-                buffer_id: Some(excerpt.buffer_id),
-                excerpt_id,
-                text_anchor,
-                diff_base_anchor: None,
-            });
-        }
-        None
+        let excerpt = self.excerpt(excerpt_id)?;
+        Some(Anchor::in_buffer(
+            excerpt_id,
+            excerpt.buffer_id,
+            text_anchor,
+        ))
     }
 
     pub fn context_range_for_excerpt(&self, excerpt_id: ExcerptId) -> Option<Range<text::Anchor>> {
@@ -5320,8 +5282,8 @@ impl MultiBufferSnapshot {
         }
     }
 
-    pub fn excerpt_before(&self, id: ExcerptId) -> Option<MultiBufferExcerpt<'_>> {
-        let start_locator = self.excerpt_locator_for_id(id);
+    pub fn excerpt_before(&self, excerpt_id: ExcerptId) -> Option<MultiBufferExcerpt<'_>> {
+        let start_locator = self.excerpt_locator_for_id(excerpt_id);
         let mut excerpts = self
             .excerpts
             .cursor::<Dimensions<Option<&Locator>, ExcerptDimension<usize>>>(());
@@ -6240,7 +6202,14 @@ impl MultiBufferSnapshot {
             .excerpts
             .cursor::<Dimensions<Option<&Locator>, ExcerptDimension<Point>>>(());
         let locator = self.excerpt_locator_for_id(excerpt_id);
-        if cursor.seek(&Some(locator), Bias::Left) {
+        let mut sought_exact = cursor.seek(&Some(locator), Bias::Left);
+        if excerpt_id == ExcerptId::max() {
+            sought_exact = true;
+            cursor.prev();
+        } else if excerpt_id == ExcerptId::min() {
+            sought_exact = true;
+        }
+        if sought_exact {
             let start = cursor.start().1.clone();
             let end = cursor.end().1;
             let mut diff_transforms = self
@@ -6258,17 +6227,6 @@ impl MultiBufferSnapshot {
         }
     }
 
-    pub fn buffer_range_for_excerpt(&self, excerpt_id: ExcerptId) -> Option<Range<text::Anchor>> {
-        let mut cursor = self.excerpts.cursor::<Option<&Locator>>(());
-        let locator = self.excerpt_locator_for_id(excerpt_id);
-        if cursor.seek(&Some(locator), Bias::Left)
-            && let Some(excerpt) = cursor.item()
-        {
-            return Some(excerpt.range.context.clone());
-        }
-        None
-    }
-
     fn excerpt(&self, excerpt_id: ExcerptId) -> Option<&Excerpt> {
         let mut cursor = self.excerpts.cursor::<Option<&Locator>>(());
         let locator = self.excerpt_locator_for_id(excerpt_id);
@@ -6277,6 +6235,9 @@ impl MultiBufferSnapshot {
             && excerpt.id == excerpt_id
         {
             return Some(excerpt);
+        } else if excerpt_id == ExcerptId::max() {
+            cursor.prev();
+            return cursor.item();
         }
         None
     }
@@ -6345,18 +6306,10 @@ impl MultiBufferSnapshot {
                     .selections_in_range(query_range, include_local)
                     .flat_map(move |(replica_id, line_mode, cursor_shape, selections)| {
                         selections.map(move |selection| {
-                            let mut start = Anchor {
-                                buffer_id: Some(excerpt.buffer_id),
-                                excerpt_id: excerpt.id,
-                                text_anchor: selection.start,
-                                diff_base_anchor: None,
-                            };
-                            let mut end = Anchor {
-                                buffer_id: Some(excerpt.buffer_id),
-                                excerpt_id: excerpt.id,
-                                text_anchor: selection.end,
-                                diff_base_anchor: None,
-                            };
+                            let mut start =
+                                Anchor::in_buffer(excerpt.id, excerpt.buffer_id, selection.start);
+                            let mut end =
+                                Anchor::in_buffer(excerpt.id, excerpt.buffer_id, selection.end);
                             if range.start.cmp(&start, self).is_gt() {
                                 start = range.start;
                             }
@@ -7073,21 +7026,19 @@ impl<'a> MultiBufferExcerpt<'a> {
     }
 
     pub fn start_anchor(&self) -> Anchor {
-        Anchor {
-            buffer_id: Some(self.excerpt.buffer_id),
-            excerpt_id: self.excerpt.id,
-            text_anchor: self.excerpt.range.context.start,
-            diff_base_anchor: None,
-        }
+        Anchor::in_buffer(
+            self.excerpt.id,
+            self.excerpt.buffer_id,
+            self.excerpt.range.context.start,
+        )
     }
 
     pub fn end_anchor(&self) -> Anchor {
-        Anchor {
-            buffer_id: Some(self.excerpt.buffer_id),
-            excerpt_id: self.excerpt.id,
-            text_anchor: self.excerpt.range.context.end,
-            diff_base_anchor: None,
-        }
+        Anchor::in_buffer(
+            self.excerpt.id,
+            self.excerpt.buffer_id,
+            self.excerpt.range.context.end,
+        )
     }
 
     pub fn buffer(&self) -> &'a BufferSnapshot {

crates/vim/src/helix.rs 🔗

@@ -467,7 +467,7 @@ impl Vim {
                         let was_empty = range.is_empty();
                         let was_reversed = selection.reversed;
                         (
-                            map.buffer_snapshot.anchor_at(start_offset, Bias::Left),
+                            map.buffer_snapshot.anchor_before(start_offset),
                             end_offset - start_offset,
                             was_empty,
                             was_reversed,