Skip to content

multi_buffer: Fix update_path_excerpts inserting excerpts out of order #49290

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.

Sign up for GitHub

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
Veykril merged 1 commit into from
Feb 16, 2026
Merged

multi_buffer: Fix update_path_excerpts inserting excerpts out of order #49290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
83 changes: 83 additions & 0 deletions crates/multi_buffer/src/multi_buffer_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5041,3 +5041,86 @@ fn test_range_to_buffer_ranges_with_range_bounds(cx: &mut App) {
assert_eq!(ranges_unbounded_trailing[0].2, te_excerpt_1_id);
assert_eq!(ranges_unbounded_trailing[1].2, te_excerpt_2_id);
}

#[gpui::test]
fn test_cannot_seek_backward_after_excerpt_replacement(cx: &mut TestAppContext) {
let buffer_b_text: String = (0..50).map(|i| format!("line_b {i}\n")).collect();
let buffer_b = cx.new(|cx| Buffer::local(buffer_b_text, cx));

let buffer_c_text: String = (0..10).map(|i| format!("line_c {i}\n")).collect();
let buffer_c = cx.new(|cx| Buffer::local(buffer_c_text, cx));

let buffer_d_text: String = (0..10).map(|i| format!("line_d {i}\n")).collect();
let buffer_d = cx.new(|cx| Buffer::local(buffer_d_text, cx));

let path_b = PathKey::with_sort_prefix(0, rel_path("bbb.rs").into_arc());
let path_c = PathKey::with_sort_prefix(0, rel_path("ddd.rs").into_arc());
let path_d = PathKey::with_sort_prefix(0, rel_path("ccc.rs").into_arc());

let multibuffer = cx.new(|_| MultiBuffer::new(Capability::ReadWrite));

multibuffer.update(cx, |multibuffer, cx| {
multibuffer.set_excerpts_for_path(
path_b.clone(),
buffer_b.clone(),
vec![
Point::row_range(0..3),
Point::row_range(15..18),
Point::row_range(30..33),
],
0,
cx,
);
});

multibuffer.update(cx, |multibuffer, cx| {
multibuffer.set_excerpts_for_path(
path_c.clone(),
buffer_c.clone(),
vec![Point::row_range(0..3)],
0,
cx,
);
});

let (anchor_in_e_b2, anchor_in_e_b3) = multibuffer.read_with(cx, |multibuffer, cx| {
let snapshot = multibuffer.snapshot(cx);
let excerpt_ids: Vec<ExcerptId> = snapshot.excerpts().map(|(id, _, _)| id).collect();
assert_eq!(excerpt_ids.len(), 4, "expected 4 excerpts (3×B + 1×C)");

let e_b2_id = excerpt_ids[1];
let e_b3_id = excerpt_ids[2];

let e_b2 = snapshot.excerpt(e_b2_id).expect("E_B2 should exist");
let e_b3 = snapshot.excerpt(e_b3_id).expect("E_B3 should exist");

let anchor_b2 = Anchor::in_buffer(e_b2_id, e_b2.range.context.start);
let anchor_b3 = Anchor::in_buffer(e_b3_id, e_b3.range.context.start);
(anchor_b2, anchor_b3)
});

multibuffer.update(cx, |multibuffer, cx| {
multibuffer.set_excerpts_for_path(
path_b.clone(),
buffer_b.clone(),
vec![Point::row_range(0..3), Point::row_range(28..36)],
0,
cx,
);
});

multibuffer.update(cx, |multibuffer, cx| {
multibuffer.set_excerpts_for_path(
path_d.clone(),
buffer_d.clone(),
vec![Point::row_range(0..3)],
0,
cx,
);
});

multibuffer.read_with(cx, |multibuffer, cx| {
let snapshot = multibuffer.snapshot(cx);
snapshot.summaries_for_anchors::<Point, _>(&[anchor_in_e_b2, anchor_in_e_b3]);
});
}
123 changes: 77 additions & 46 deletions crates/multi_buffer/src/path_key.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,8 +316,7 @@ impl MultiBuffer {
let snapshot = self.snapshot(cx);

let mut next_excerpt_id =
// todo(lw): is this right? What if we remove the last excerpt, then we might reallocate with a wrong mapping?
if let Some(last_entry) = self.snapshot.borrow().excerpt_ids.last() {
if let Some(last_entry) = self.snapshot.get_mut().excerpt_ids.last() {
last_entry.id.0 + 1
} else {
1
Expand Down Expand Up @@ -347,7 +346,11 @@ impl MultiBuffer {
};

let new = new_iter.peek();
// Try to merge the next new range or existing excerpt into the last
// queued insert.
if let Some((last_id, last)) = to_insert.last_mut() {
// Next new range overlaps the last queued insert: absorb it by
// extending the insert's end.
if let Some(new) = new
&& last.context.end >= new.context.start
{
Expand All @@ -356,6 +359,9 @@ impl MultiBuffer {
new_iter.next();
continue;
}
// Next existing excerpt overlaps the last queued insert: absorb
// it by extending the insert's end, and record the existing
// excerpt as replaced so anchors in it resolve to the new one.
if let Some((existing_id, existing_range)) = &existing
&& last.context.end >= existing_range.start
{
Expand All @@ -372,63 +378,90 @@ impl MultiBuffer {

match (new, existing) {
(None, None) => break,

// No more new ranges; remove the remaining existing excerpt.
(None, Some((existing_id, _))) => {
existing_iter.next();
to_remove.push(existing_id);
continue;
}

// No more existing excerpts; queue the new range for insertion.
(Some(_), None) => {
added_a_new_excerpt = true;
let new_id = next_excerpt_id();
excerpt_ids.push(new_id);
to_insert.push((new_id, new_iter.next().unwrap()));
continue;
}
(Some(new), Some((_, existing_range))) => {
if existing_range.end < new.context.start {
let existing_id = existing_iter.next().unwrap();
to_remove.push(existing_id);
continue;
} else if existing_range.start > new.context.end {
let new_id = next_excerpt_id();
excerpt_ids.push(new_id);
to_insert.push((new_id, new_iter.next().unwrap()));
continue;
}

// Existing excerpt ends before the new range starts, so it
// has no corresponding new range and must be removed. Flush
// pending inserts and advance `insert_after` past it so that
// future inserts receive locators *after* this excerpt's
// locator, preserving forward ordering.
(Some(new), Some((_, existing_range)))
if existing_range.end < new.context.start =>
{
self.insert_excerpts_with_ids_after(
insert_after,
buffer.clone(),
mem::take(&mut to_insert),
cx,
);
insert_after = existing_iter.next().unwrap();
to_remove.push(insert_after);
}
// New range ends before the existing excerpt starts, so the
// new range has no corresponding existing excerpt. Queue it
// for insertion at the current `insert_after` position
// (before the existing excerpt), which is the correct
// spatial ordering.
(Some(new), Some((_, existing_range)))
if existing_range.start > new.context.end =>
{
let new_id = next_excerpt_id();
excerpt_ids.push(new_id);
to_insert.push((new_id, new_iter.next().unwrap()));
}
// Exact match: keep the existing excerpt in place, flush
// any pending inserts before it, and use it as the new
// `insert_after` anchor.
(Some(new), Some((_, existing_range)))
if existing_range.start == new.context.start
&& existing_range.end == new.context.end
{
self.insert_excerpts_with_ids_after(
insert_after,
buffer.clone(),
mem::take(&mut to_insert),
cx,
);
insert_after = existing_iter.next().unwrap();
excerpt_ids.push(insert_after);
new_iter.next();
} else {
let existing_id = existing_iter.next().unwrap();
let new_id = next_excerpt_id();
self.snapshot
.get_mut()
.replaced_excerpts
.insert(existing_id, new_id);
to_remove.push(existing_id);
let mut range = new_iter.next().unwrap();
range.context.start = range.context.start.min(existing_range.start);
range.context.end = range.context.end.max(existing_range.end);
excerpt_ids.push(new_id);
to_insert.push((new_id, range));
}
&& existing_range.end == new.context.end =>
{
self.insert_excerpts_with_ids_after(
insert_after,
buffer.clone(),
mem::take(&mut to_insert),
cx,
);
insert_after = existing_iter.next().unwrap();
excerpt_ids.push(insert_after);
new_iter.next();
}

// Partial overlap: replace the existing excerpt with a new
// one whose range is the union of both, and record the
// replacement so that anchors in the old excerpt resolve to
// the new one.
(Some(_), Some((_, existing_range))) => {
let existing_id = existing_iter.next().unwrap();
let new_id = next_excerpt_id();
self.snapshot
.get_mut()
.replaced_excerpts
.insert(existing_id, new_id);
to_remove.push(existing_id);
let mut range = new_iter.next().unwrap();
range.context.start = range.context.start.min(existing_range.start);
range.context.end = range.context.end.max(existing_range.end);
excerpt_ids.push(new_id);
to_insert.push((new_id, range));
}
};
}

self.insert_excerpts_with_ids_after(insert_after, buffer, to_insert, cx);
// todo(lw): There is a logic bug somewhere that causes the to_remove vector to be not ordered correctly
to_remove.sort_by_cached_key(|&id| snapshot.excerpt_locator_for_id(id));
self.remove_excerpts(to_remove, cx);

if excerpt_ids.is_empty() {
Expand All @@ -437,10 +470,8 @@ impl MultiBuffer {
for excerpt_id in &excerpt_ids {
self.paths_by_excerpt.insert(*excerpt_id, path.clone());
}
let snapshot = &*self.snapshot.get_mut();
let mut excerpt_ids: Vec<_> = excerpt_ids.iter().dedup().cloned().collect();
excerpt_ids.sort_by_cached_key(|&id| snapshot.excerpt_locator_for_id(id));
self.excerpts_by_path.insert(path, excerpt_ids);
self.excerpts_by_path
.insert(path, excerpt_ids.iter().dedup().cloned().collect());
}

(excerpt_ids, added_a_new_excerpt)
Expand Down