diff --git a/Cargo.lock b/Cargo.lock index cda9a18032..053c989970 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11152,6 +11152,8 @@ dependencies = [ "log", "parking_lot", "pretty_assertions", + "proptest", + "proptest-derive", "rand 0.9.4", "rope", "serde", diff --git a/crates/editor/src/display_map.rs b/crates/editor/src/display_map.rs index dfef0499bb..c6c19d57c8 100644 --- a/crates/editor/src/display_map.rs +++ b/crates/editor/src/display_map.rs @@ -2707,6 +2707,7 @@ pub mod tests { use lsp::LanguageServerId; use futures::stream::StreamExt; + use multi_buffer::PathKey; use rand::{Rng, prelude::*}; use settings::{SettingsContent, SettingsStore}; use std::{env, sync::Arc}; @@ -4120,6 +4121,197 @@ pub mod tests { chunks } + /// Asserts that every header-like block in the snapshot references a + /// buffer that is still present in the multibuffer: the invariant whose + /// violation panics at render time with "buffer snapshot not found for + /// excerpt boundary" (ZED-7G6). + #[track_caller] + fn assert_headers_resolve(snapshot: &DisplaySnapshot) { + let end_row = DisplayRow(snapshot.max_point().row().0 + 1); + for (row, block) in snapshot.blocks_in_range(DisplayRow(0)..end_row) { + let excerpt = match block { + Block::BufferHeader { excerpt, .. } | Block::ExcerptBoundary { excerpt, .. } => { + excerpt + } + Block::FoldedBuffer { first_excerpt, .. } => first_excerpt, + _ => continue, + }; + assert!( + snapshot + .buffer_snapshot() + .buffer_for_id(excerpt.buffer_id()) + .is_some(), + "stale header block {:?} at {row:?} references buffer {:?}, \ + which is no longer in the multibuffer", + block.id(), + excerpt.buffer_id(), + ); + } + } + + /// Deterministic end-to-end regression test for ZED-7G6 ("buffer snapshot + /// not found for excerpt boundary"), driving a real `DisplayMap` with + /// ordinary operations. In a diff-backed multibuffer with all hunks + /// expanded, two folds inside an expanded deleted hunk are ordered only by + /// their diff base anchors. Replacing the diff's base text used to invert + /// that order (comparison filtered diff base anchors on validity, which + /// the base edit revoked for one anchor of the pair), silently unsorting + /// the fold map's persistent fold tree; subsequent syncs walked it with + /// forward-only cursors and emitted edits that misdescribed the changed + /// rows, until removing a buffer left its header block referencing a + /// buffer absent from the snapshot -- the state whose render-time + /// resolution panics. + /// + /// On pre-fix code this fails in the display map layers' internal + /// checks; in production builds, where those checks don't run, the same + /// corruption propagated to the stale header instead. + #[gpui::test] + async fn test_removing_buffer_removes_header_after_diff_base_changes( + cx: &mut gpui::TestAppContext, + ) { + cx.update(|cx| init_test(cx, &|_| {})); + + fn excerpt_buffer( + multibuffer: &Entity, + path: u64, + buffer: &Entity, + cx: &mut gpui::TestAppContext, + ) { + multibuffer.update(cx, |multibuffer, cx| { + let max_point = buffer.read(cx).max_point(); + multibuffer.set_excerpts_for_path( + PathKey::sorted(path), + buffer.clone(), + [Point::zero()..max_point], + 0, + cx, + ); + }); + cx.run_until_parked(); + } + + async fn set_base_text( + diff: &Entity, + buffer: &Entity, + base_text: &str, + cx: &mut gpui::TestAppContext, + ) { + let snapshot = buffer.read_with(cx, |buffer, _| buffer.text_snapshot()); + diff.update(cx, |diff, cx| { + diff.set_base_text(Some(base_text.to_string().into()), snapshot, cx) + }) + .await; + cx.run_until_parked(); + } + + #[track_caller] + fn assert_headers(display_map: &Entity, cx: &mut gpui::TestAppContext) { + cx.run_until_parked(); + let snapshot = display_map.update(cx, |display_map, cx| display_map.snapshot(cx)); + assert_headers_resolve(&snapshot); + } + + let buffer_a = cx.new(|cx| Buffer::local("bbb\nccc\nddd\n", cx)); + let diff_a = cx.new(|cx| { + buffer_diff::BufferDiff::new_with_base_text( + "DEL1\nDEL2\nbbb\nccc\nddd\n", + &buffer_a.read(cx).text_snapshot(), + cx, + ) + }); + let buffer_b = cx.new(|cx| Buffer::local("xxx\nyyy\n", cx)); + let buffer_b_id = buffer_b.read_with(cx, |buffer, _| buffer.remote_id()); + let multibuffer = cx.new(|cx| { + let mut multibuffer = MultiBuffer::new(language::Capability::ReadWrite); + multibuffer.set_all_diff_hunks_expanded(cx); + multibuffer + }); + excerpt_buffer(&multibuffer, 0, &buffer_a, cx); + excerpt_buffer(&multibuffer, 1, &buffer_b, cx); + multibuffer.update(cx, |multibuffer, cx| { + multibuffer.add_diff(diff_a.clone(), cx); + }); + cx.run_until_parked(); + + let display_map = cx.new(|cx| { + DisplayMap::new( + multibuffer.clone(), + test_font(), + px(14.0), + None, + 1, + 1, + FoldPlaceholder::test(), + DiagnosticSeverity::Warning, + cx, + ) + }); + assert_headers(&display_map, cx); + + // Two folds inside the expanded deleted hunk (rows 0 and 1 are + // materialized from the base text), sharing a buffer position and + // ordered only by their diff base anchors: a narrow fold within + // "DEL1", then a wider fold from "DEL2" into the buffer's own rows. + display_map.update(cx, |display_map, cx| { + display_map.fold( + vec![ + Crease::simple(Point::new(0, 1)..Point::new(1, 0), FoldPlaceholder::test()), + Crease::simple(Point::new(1, 1)..Point::new(2, 2), FoldPlaceholder::test()), + ], + cx, + ); + }); + assert_headers(&display_map, cx); + + // Keep "DEL1" (the first fold's base anchors survive) but delete + // "DEL2" (the second fold's start anchor is tombstoned): the folds' + // relative order must not change. + set_base_text(&diff_a, &buffer_a, "DEL1\nbbb\nccc\nddd\n", cx).await; + assert_headers(&display_map, cx); + + // Churn the buffers, the diff base, and buffer B's excerpts the way + // the original fuzz sequence did, syncing the display map after each + // group of operations. + multibuffer.update(cx, |multibuffer, cx| { + multibuffer.remove_excerpts_for_buffer(buffer_b_id, cx); + }); + buffer_a.update(cx, |buffer, cx| { + buffer.edit([(4..5, "")], None, cx); + }); + assert_headers(&display_map, cx); + + excerpt_buffer(&multibuffer, 1, &buffer_b, cx); + set_base_text(&diff_a, &buffer_a, "DEL1\nbbb\nccc\nddd\n", cx).await; + buffer_a.update(cx, |buffer, cx| { + buffer.edit([(1..1, "Q\n")], None, cx); + }); + assert_headers(&display_map, cx); + + set_base_text(&diff_a, &buffer_a, "DEL1\nbbb\nccc\nddd\n", cx).await; + assert_headers(&display_map, cx); + + set_base_text(&diff_a, &buffer_a, "DEL2\nbbb\nccc\nddd\n", cx).await; + buffer_a.update(cx, |buffer, cx| { + buffer.edit([(2..2, "Q\n")], None, cx); + }); + assert_headers(&display_map, cx); + + multibuffer.update(cx, |multibuffer, cx| { + multibuffer.remove_excerpts_for_buffer(buffer_b_id, cx); + }); + excerpt_buffer(&multibuffer, 1, &buffer_b, cx); + assert_headers(&display_map, cx); + + // Removing B must remove its header block: with the fold tree + // corrupted, the removal edit's rows were misdescribed by the time + // they reached the block map, B's header row went uncovered, and the + // header survived pointing at a buffer absent from the snapshot. + multibuffer.update(cx, |multibuffer, cx| { + multibuffer.remove_excerpts_for_buffer(buffer_b_id, cx); + }); + assert_headers(&display_map, cx); + } + fn init_test(cx: &mut App, f: &dyn Fn(&mut SettingsContent)) { let settings = SettingsStore::test(cx); cx.set_global(settings); diff --git a/crates/editor/src/display_map/block_map.rs b/crates/editor/src/display_map/block_map.rs index 22fb21e4a1..4306d239e3 100644 --- a/crates/editor/src/display_map/block_map.rs +++ b/crates/editor/src/display_map/block_map.rs @@ -4677,6 +4677,545 @@ mod tests { } } + /// Deterministic demonstration of the root cause behind ZED-7G6's crash + /// family: multibuffer anchor comparison is not stable across diff base + /// text changes, which unsorts the fold map's persistent fold tree. + /// + /// Folds inside an expanded deleted hunk anchor at the same buffer + /// position and are ordered only by their `diff_base_anchor`s into the + /// diff's base text. When anchor comparison filtered those on + /// *validity*, a base text change that deleted the region one anchor + /// pointed into made comparison fall back to the text anchor's bias — + /// opposite to the answer it gave when the folds were inserted — so the + /// persistent tree was unsorted without having been touched. Sync then + /// walked it with forward-only cursors, emitting edits that misdescribed + /// the change, and the block map kept stale headers for rows that did + /// change. + /// + /// Fixed by filtering diff base anchors on *resolvability* against the + /// current base buffer instead (monotone: buffer ids never change and + /// versions only grow), so anchors into deleted base text keep comparing + /// positionally through their tombstones and the answer never flips. + #[gpui::test] + async fn test_folds_stay_sorted_when_diff_base_text_replaced(cx: &mut gpui::TestAppContext) { + cx.update(init_test); + + let text = "bbb\nccc\nddd\n"; + let base_text = "DEL1\nDEL2\nbbb\nccc\nddd\n"; + let buffer = cx.new(|cx| Buffer::local(text, cx)); + let buffer_text_snapshot = buffer.read_with(cx, |buffer, _| buffer.text_snapshot()); + let diff = + cx.new(|cx| BufferDiff::new_with_base_text(base_text, &buffer_text_snapshot, cx)); + + let multibuffer = cx.new(|cx| { + let mut multibuffer = MultiBuffer::new(Capability::ReadWrite); + multibuffer.set_all_diff_hunks_expanded(cx); + multibuffer + }); + multibuffer.update(cx, |multibuffer, cx| { + let max_point = buffer.read(cx).max_point(); + multibuffer.set_excerpts_for_buffer(buffer.clone(), [Point::zero()..max_point], 0, cx); + multibuffer.add_diff(diff.clone(), cx); + }); + cx.run_until_parked(); + + let subscription = multibuffer.update(cx, |multibuffer, _| multibuffer.subscribe()); + let snapshot = multibuffer.read_with(cx, |multibuffer, cx| multibuffer.snapshot(cx)); + assert_eq!( + snapshot.text(), + "DEL1\nDEL2\nbbb\nccc\nddd\n", + "expected the deleted hunk's rows to be materialized at the top" + ); + let (mut inlay_map, inlay_snapshot) = InlayMap::new(snapshot.clone()); + let (mut fold_map, _) = FoldMap::new(inlay_snapshot.clone()); + + // Two folds inside the deleted hunk share a buffer position and are + // ordered only by their anchors into the diff's base text: a narrow + // fold within "DEL1", then a wider fold from "DEL2" into the + // buffer's own content. + let (mut writer, ..) = fold_map.write(inlay_snapshot, Vec::new()); + let (fold_snapshot, _) = writer.fold(vec![ + (Point::new(0, 1)..Point::new(1, 0), FoldPlaceholder::test()), + (Point::new(1, 1)..Point::new(2, 2), FoldPlaceholder::test()), + ]); + assert_eq!( + fold_snapshot.fold_count(), + 2, + "both folds should anchor inside the expanded deleted hunk" + ); + + // Keep "DEL1" (the first fold's base anchors stay visible) but + // delete "DEL2" (the second fold's start anchor becomes a tombstone). + // The folds' relative order must survive this: comparison that + // filtered out the tombstoned anchor fell back to the text anchor's + // bias and reversed it. + let new_base_text = "DEL1\nbbb\nccc\nddd\n"; + diff.update(cx, |diff, cx| { + diff.set_base_text(Some(new_base_text.into()), buffer_text_snapshot.clone(), cx) + }) + .await; + cx.run_until_parked(); + + let snapshot = multibuffer.read_with(cx, |multibuffer, cx| multibuffer.snapshot(cx)); + let edits = subscription.consume().into_inner(); + let (inlay_snapshot, inlay_edits) = inlay_map.sync(snapshot, edits); + // `read` checks that the fold tree is still sorted. An unsorted tree + // makes subsequent syncs emit edits that misdescribe the actual + // changes, which is what leaves block map headers stale in + // production. + fold_map.read(inlay_snapshot, inlay_edits); + } + + /// Regression fuzz for ZED-7G6 ("buffer snapshot not found for excerpt + /// boundary"): header blocks cache an `ExcerptBoundaryInfo`, so when a + /// buffer leaves the multibuffer without the block map re-syncing the + /// header's rows, the stale block panics at render time. Excerpt removal + /// is fuzzed together with diffs, folded buffers, text folds, and inlays, + /// because the crash fires in diff-backed multibuffers like the project + /// diff. + /// + /// Deeper settings have found real bugs; run them locally when touching + /// this area, e.g. `ITERATIONS=1000 OPERATIONS=40`. Found so far: + /// `InlayMap::sync` panicking on inlays whose anchors predate a path key + /// reuse (minimized into `inlay_map`'s + /// `test_sync_after_path_key_reused_for_different_buffer`), a + /// `FoldMap` canonical-form violation ("found adjacent isomorphic + /// transforms", `SEED=612 OPERATIONS=40`), and the fold tree going out of + /// order (`SEED=144 OPERATIONS=50`): with all diff hunks expanded, fold + /// anchors carry diff base anchors, and replacing a diff's base text + /// changes how those anchors compare, unsorting the persistent fold tree + /// that `FoldMap` seeks through. During the investigation, disabling + /// the fold and wrap maps' test-only invariants the way production + /// builds run let the unsorted tree propagate: `FoldMap::sync` panicked + /// with "cannot seek backward" (`SEED=332 OPERATIONS=60`), matching a + /// large family of open Sentry crashes with that message, and the wrap + /// layer emitted edits that no longer covered the rows that actually + /// changed (`SEED=612 OPERATIONS=50`, caught by + /// `assert_wrap_edits_cover_changes`), which is the mechanism that + /// leaves the block map holding stale header blocks. The inlay panic and + /// the fold tree disorder are fixed by making anchor comparison track + /// resolution; the canonical-form violation is still unfixed. + #[gpui::test(iterations = 20)] + async fn test_random_excerpt_removal_with_diffs( + cx: &mut gpui::TestAppContext, + mut rng: StdRng, + ) { + cx.update(init_test); + + let operations = env::var("OPERATIONS") + .map(|i| i.parse().expect("invalid `OPERATIONS` variable")) + .unwrap_or(20); + + let wrap_width = if rng.random_bool(0.2) { + None + } else { + Some(px(rng.random_range(0.0..=100.0))) + }; + let tab_size = 1.try_into().unwrap(); + let font_size = px(14.0); + let buffer_start_header_height = rng.random_range(1..=5); + let excerpt_header_height = rng.random_range(1..=5); + log::info!("Wrap width: {wrap_width:?}"); + + // Half the time, build the multibuffer the way a split diff's left + // hand side does: excerpts of read-only base text buffers, with + // inverted diffs pointing at the main buffers. + let split_style = rng.random_bool(0.5); + let multibuffer; + let mut known_buffers; + let mut diffs = Vec::new(); + if split_style { + multibuffer = cx.new(|_cx| MultiBuffer::new(Capability::ReadWrite)); + known_buffers = Vec::new(); + for _ in 0..rng.random_range(1..=3) { + let text = random_buffer_text(&mut rng); + let main_buffer = cx.new(|cx| Buffer::local(text.clone(), cx)); + let base_text = randomized_text(&text, &mut rng); + let diff = cx.new(|cx| { + BufferDiff::new_with_base_text( + &base_text, + &main_buffer.read(cx).text_snapshot(), + cx, + ) + }); + let base_text_buffer = + diff.read_with(cx, |diff, _| diff.base_text_buffer().clone()); + multibuffer.update(cx, |multibuffer, cx| { + let max_point = base_text_buffer.read(cx).max_point(); + multibuffer.set_excerpts_for_buffer( + base_text_buffer.clone(), + [Point::zero()..max_point], + 0, + cx, + ); + multibuffer.add_inverted_diff(diff.clone(), main_buffer.clone(), cx); + }); + known_buffers.push(base_text_buffer); + diffs.push((diff, main_buffer)); + } + } else { + multibuffer = cx.update(|cx| MultiBuffer::build_random(&mut rng, cx)); + // Diff views like the project diff show all hunks expanded, which + // materializes deleted rows from base buffers inline. + if rng.random_bool(0.5) { + multibuffer.update(cx, |multibuffer, cx| { + multibuffer.set_all_diff_hunks_expanded(cx); + }); + } + known_buffers = multibuffer.read_with(cx, |multibuffer, _| { + multibuffer.all_buffers_iter().collect::>() + }); + + // Attach diffs to a random subset of buffers, with base texts that + // differ from the buffer text so that hunks exist. + for buffer in known_buffers.clone() { + if !rng.random_bool(0.5) { + continue; + } + let (base_text, buffer_snapshot) = buffer.read_with(cx, |buffer, _| { + ( + randomized_text(&buffer.text(), &mut rng), + buffer.text_snapshot(), + ) + }); + let diff = + cx.new(|cx| BufferDiff::new_with_base_text(&base_text, &buffer_snapshot, cx)); + multibuffer.update(cx, |multibuffer, cx| multibuffer.add_diff(diff.clone(), cx)); + diffs.push((diff, buffer)); + } + } + + let mut buffer_snapshot = + multibuffer.read_with(cx, |multibuffer, cx| multibuffer.snapshot(cx)); + let (mut inlay_map, inlay_snapshot) = InlayMap::new(buffer_snapshot.clone()); + let (mut fold_map, fold_snapshot) = FoldMap::new(inlay_snapshot); + let (mut tab_map, tab_snapshot) = TabMap::new(fold_snapshot, 4.try_into().unwrap()); + let font = test_font(); + let (wrap_map, wraps_snapshot) = + cx.update(|cx| WrapMap::new(tab_snapshot, font, font_size, wrap_width, cx)); + let mut previous_wrap_text = wraps_snapshot.text(); + let mut block_map = BlockMap::new( + wraps_snapshot, + buffer_start_header_height, + excerpt_header_height, + ); + + let subscription = multibuffer.update(cx, |multibuffer, _| multibuffer.subscribe()); + + let mut next_inlay_id = 0; + let mut operations_until_sync = 0; + for _ in 0..operations { + // Apply several mutations before syncing the display layers, so + // that edit composition across mutations is exercised the way it is + // when more than one change lands between frames. + let sync_now = operations_until_sync == 0; + if sync_now { + operations_until_sync = rng.random_range(1..=4); + } + operations_until_sync -= 1; + + match rng.random_range(0..100) { + 0..=9 => { + let wrap_width = if rng.random_bool(0.2) { + None + } else { + Some(px(rng.random_range(0.0..=100.0))) + }; + log::info!("Setting wrap width to {wrap_width:?}"); + wrap_map.update(cx, |map, cx| map.set_wrap_width(wrap_width, cx)); + } + 10..=24 => { + // Folding reads the block map, so bring the display layers + // up to date with any mutations batched since the last sync. + buffer_snapshot = + multibuffer.read_with(cx, |multibuffer, cx| multibuffer.snapshot(cx)); + let pending_edits = subscription.consume().into_inner(); + let (inlay_snapshot, inlay_edits) = + inlay_map.sync(buffer_snapshot.clone(), pending_edits); + let (fold_snapshot, fold_edits) = fold_map.read(inlay_snapshot, inlay_edits); + let (tab_snapshot, tab_edits) = + tab_map.sync(fold_snapshot, fold_edits, tab_size); + let (wraps_snapshot, wrap_edits) = wrap_map.update(cx, |wrap_map, cx| { + wrap_map.sync(tab_snapshot, tab_edits, cx) + }); + assert_wrap_edits_cover_changes( + &mut previous_wrap_text, + &wraps_snapshot, + &wrap_edits, + ); + let mut writer = block_map.write(wraps_snapshot, wrap_edits, None); + let folded_buffers = writer + .block_map + .folded_buffers + .iter() + .cloned() + .collect::>(); + let mut unfolded_buffers = buffer_snapshot + .buffer_ids_for_range(Anchor::Min..Anchor::Max) + .collect::>(); + unfolded_buffers.dedup(); + unfolded_buffers.retain(|buffer_id| !folded_buffers.contains(buffer_id)); + + multibuffer.read_with(cx, |multibuffer, cx| { + if !unfolded_buffers.is_empty() + && (folded_buffers.is_empty() || rng.random_bool(0.5)) + { + let buffer_to_fold = *unfolded_buffers.choose(&mut rng).unwrap(); + log::info!("Folding {buffer_to_fold:?}"); + writer.fold_buffers([buffer_to_fold], multibuffer, cx); + } else if !folded_buffers.is_empty() { + let buffer_to_unfold = *folded_buffers.choose(&mut rng).unwrap(); + log::info!("Unfolding {buffer_to_unfold:?}"); + writer.unfold_buffers([buffer_to_unfold], multibuffer, cx); + } + }); + } + // Remove all excerpts for a buffer, as happens when a file + // leaves the project diff. + 25..=44 => { + let buffer_ids = multibuffer.read_with(cx, |multibuffer, cx| { + multibuffer + .snapshot(cx) + .all_buffer_ids() + .collect::>() + }); + if buffer_ids.is_empty() { + continue; + } + let buffer_id = *buffer_ids.choose(&mut rng).unwrap(); + log::info!("Removing excerpts for buffer {buffer_id:?}"); + multibuffer.update(cx, |multibuffer, cx| { + multibuffer.remove_excerpts_for_buffer(buffer_id, cx); + }); + } + // Add excerpts for a buffer that currently has none, as + // happens when a file (re-)enters the project diff. + 45..=59 => { + let present = multibuffer.read_with(cx, |multibuffer, cx| { + multibuffer + .snapshot(cx) + .all_buffer_ids() + .collect::>() + }); + let absent = known_buffers + .iter() + .filter(|buffer| { + !present.contains(&buffer.read_with(cx, |buffer, _| buffer.remote_id())) + }) + .cloned() + .collect::>(); + + let buffer = if absent.is_empty() || rng.random_bool(0.3) { + let text = random_buffer_text(&mut rng); + log::info!("Creating new buffer with text: {text:?}"); + let buffer = cx.new(|cx| Buffer::local(text, cx)); + known_buffers.push(buffer.clone()); + buffer + } else { + absent.choose(&mut rng).unwrap().clone() + }; + + let max_row = + buffer.read_with(cx, |buffer, _| buffer.snapshot().max_point().row); + let ranges = (0..rng.random_range(1..=3)) + .map(|_| { + let start_row = rng.random_range(0..=max_row); + let end_row = rng.random_range(start_row..=max_row); + Point::new(start_row, 0)..Point::new(end_row, 0) + }) + .collect::>(); + log::info!("Setting excerpts {ranges:?}"); + multibuffer.update(cx, |multibuffer, cx| { + multibuffer.set_excerpts_for_buffer(buffer.clone(), ranges, 0, cx); + // Re-attach the buffer's diff if it has one, like the + // project diff does when a file re-enters. + if let Some((diff, _)) = + diffs.iter().find(|(_, candidate)| *candidate == buffer) + { + multibuffer.add_diff(diff.clone(), cx); + } + }); + } + // Change a diff's base text, as happens when the diff base + // changes or HEAD advances. + 60..=69 if !diffs.is_empty() => { + let (diff, buffer) = diffs.choose(&mut rng).unwrap().clone(); + let new_base_text = diff.read_with(cx, |diff, cx| { + randomized_text(&diff.base_text_string(cx).unwrap_or_default(), &mut rng) + }); + let buffer_text_snapshot = + buffer.read_with(cx, |buffer, _| buffer.text_snapshot()); + log::info!("Setting diff base text to {new_base_text:?}"); + diff.update(cx, |diff, cx| { + diff.set_base_text(Some(new_base_text.into()), buffer_text_snapshot, cx) + }) + .await; + cx.run_until_parked(); + } + // Fold or unfold random text ranges, so that fold edits are + // propagated through the tab and wrap layers alongside the + // multibuffer mutations. Mutating the inlay and fold maps + // requires them to be current, as `DisplayMap` keeps them, so + // sync the pending mutations through first. + 70..=81 => { + buffer_snapshot = + multibuffer.read_with(cx, |multibuffer, cx| multibuffer.snapshot(cx)); + let pending_edits = subscription.consume().into_inner(); + let (inlay_snapshot, inlay_edits) = + inlay_map.sync(buffer_snapshot.clone(), pending_edits); + let (fold_snapshot, fold_edits) = fold_map.read(inlay_snapshot, inlay_edits); + let (tab_snapshot, tab_edits) = + tab_map.sync(fold_snapshot, fold_edits, tab_size); + let (wraps_snapshot, wrap_edits) = wrap_map.update(cx, |wrap_map, cx| { + wrap_map.sync(tab_snapshot, tab_edits, cx) + }); + assert_wrap_edits_cover_changes( + &mut previous_wrap_text, + &wraps_snapshot, + &wrap_edits, + ); + block_map.read(wraps_snapshot, wrap_edits, None); + + if rng.random_bool(0.5) { + for (fold_snapshot, fold_edits) in fold_map.randomly_mutate(&mut rng) { + let (tab_snapshot, tab_edits) = + tab_map.sync(fold_snapshot, fold_edits, tab_size); + let (wraps_snapshot, wrap_edits) = wrap_map + .update(cx, |wrap_map, cx| { + wrap_map.sync(tab_snapshot, tab_edits, cx) + }); + assert_wrap_edits_cover_changes( + &mut previous_wrap_text, + &wraps_snapshot, + &wrap_edits, + ); + block_map.read(wraps_snapshot, wrap_edits, None); + } + } else { + let (inlay_snapshot, inlay_edits) = + inlay_map.randomly_mutate(&mut next_inlay_id, &mut rng); + let (fold_snapshot, fold_edits) = + fold_map.read(inlay_snapshot, inlay_edits); + let (tab_snapshot, tab_edits) = + tab_map.sync(fold_snapshot, fold_edits, tab_size); + let (wraps_snapshot, wrap_edits) = wrap_map.update(cx, |wrap_map, cx| { + wrap_map.sync(tab_snapshot, tab_edits, cx) + }); + assert_wrap_edits_cover_changes( + &mut previous_wrap_text, + &wraps_snapshot, + &wrap_edits, + ); + block_map.read(wraps_snapshot, wrap_edits, None); + } + } + // Update a path's excerpts through `update_path_excerpts`, + // sometimes reusing a path key for a different buffer, as + // split diffs do when a file's diff base buffer is recreated. + 82..=89 => { + let paths = multibuffer.read_with(cx, |multibuffer, cx| { + let snapshot = multibuffer.snapshot(cx); + snapshot + .all_buffer_ids() + .filter_map(|id| snapshot.path_for_buffer(id).cloned()) + .collect::>() + }); + if paths.is_empty() || known_buffers.is_empty() { + continue; + } + let path = paths.choose(&mut rng).unwrap().clone(); + + let buffer = if rng.random_bool(0.5) { + let text = random_buffer_text(&mut rng); + let buffer = cx.new(|cx| Buffer::local(text, cx)); + known_buffers.push(buffer.clone()); + buffer + } else { + known_buffers.choose(&mut rng).unwrap().clone() + }; + let max_row = + buffer.read_with(cx, |buffer, _| buffer.snapshot().max_point().row); + let ranges = (0..rng.random_range(0..=3)) + .map(|_| { + let start_row = rng.random_range(0..=max_row); + let end_row = rng.random_range(start_row..=max_row); + Point::new(start_row, 0)..Point::new(end_row, 0) + }) + .collect::>(); + log::info!("Setting excerpts for path {path:?} with ranges {ranges:?}"); + multibuffer.update(cx, |multibuffer, cx| { + multibuffer.set_excerpts_for_path(path, buffer, ranges, 0, cx); + }); + } + _ => { + // Base text buffers are read-only in production, so only + // edit buffers that are actually editable. + let buffers = multibuffer.read_with(cx, |multibuffer, cx| { + multibuffer + .all_buffers_iter() + .filter(|buffer| !buffer.read(cx).read_only()) + .collect::>() + }); + let editable = buffers + .into_iter() + .chain(diffs.iter().map(|(_, buffer)| buffer.clone())) + .collect::>(); + let Some(buffer) = editable.choose(&mut rng).cloned() else { + continue; + }; + buffer.update(cx, |buffer, cx| { + let edit_count = rng.random_range(1..=3); + buffer.randomly_edit(&mut rng, edit_count, cx); + }); + // Recalculate any diff for the edited buffer, mirroring + // the recalculation that follows a buffer edit. + let snapshot = buffer.read_with(cx, |buffer, _| buffer.text_snapshot()); + for (diff, diff_buffer) in &diffs { + if *diff_buffer == buffer { + diff.update(cx, |diff, cx| diff.recalculate_diff_sync(&snapshot, cx)); + } + } + } + } + + if !sync_now { + continue; + } + + buffer_snapshot = multibuffer.read_with(cx, |multibuffer, cx| multibuffer.snapshot(cx)); + let buffer_edits = subscription.consume().into_inner(); + let (inlay_snapshot, inlay_edits) = + inlay_map.sync(buffer_snapshot.clone(), buffer_edits); + let (fold_snapshot, fold_edits) = fold_map.read(inlay_snapshot, inlay_edits); + let (tab_snapshot, tab_edits) = tab_map.sync(fold_snapshot, fold_edits, tab_size); + let (wraps_snapshot, wrap_edits) = wrap_map.update(cx, |wrap_map, cx| { + wrap_map.sync(tab_snapshot, tab_edits, cx) + }); + assert_wrap_edits_cover_changes(&mut previous_wrap_text, &wraps_snapshot, &wrap_edits); + let blocks_snapshot = block_map.read(wraps_snapshot.clone(), wrap_edits, None); + log::info!("blocks text: {:?}", blocks_snapshot.text()); + assert_excerpts_resolve(&buffer_snapshot); + + let max_row = blocks_snapshot.max_point().row; + for (row, block) in blocks_snapshot.blocks_in_range(BlockRow(0)..BlockRow(max_row + 1)) + { + let excerpt = match block { + Block::BufferHeader { excerpt, .. } + | Block::ExcerptBoundary { excerpt, .. } => excerpt, + Block::FoldedBuffer { first_excerpt, .. } => first_excerpt, + _ => continue, + }; + assert!( + buffer_snapshot.buffer_for_id(excerpt.buffer_id()).is_some(), + "stale header block {:?} at {row:?} references buffer {:?}, \ + which is no longer in the multibuffer", + block.id(), + excerpt.buffer_id(), + ); + } + } + } + #[gpui::test] fn test_remove_intersecting_replace_blocks_edge_case(cx: &mut gpui::TestAppContext) { cx.update(init_test); @@ -5086,6 +5625,96 @@ mod tests { assets::Assets.load_test_fonts(cx); } + /// Verifies the edit coverage invariant at the wrap boundary: applying the + /// emitted edits to the previous wrap text must reproduce the new wrap + /// text exactly. A row that changed without being covered by an edit is + /// precisely the kind of under-invalidation that leaves the block map + /// holding stale header blocks (ZED-7G6), because every layer below the + /// block map propagates its changes only through these edits. Checking at + /// the wrap boundary catches under-coverage introduced by any layer + /// beneath it. + #[track_caller] + fn assert_wrap_edits_cover_changes( + previous_wrap_text: &mut String, + wrap_snapshot: &WrapSnapshot, + wrap_edits: &WrapPatch, + ) { + let new_text = wrap_snapshot.text(); + let old_rows = previous_wrap_text.split('\n').collect::>(); + let new_rows = new_text.split('\n').collect::>(); + + let mut reconstructed = old_rows.clone(); + for edit in wrap_edits.edits().iter().rev() { + assert!( + edit.old.start.0 as usize <= reconstructed.len() + && edit.old.end.0 as usize <= old_rows.len(), + "edit {edit:?} lies outside the previous wrap text ({} rows)", + old_rows.len(), + ); + assert!( + edit.new.end.0 as usize <= new_rows.len(), + "edit {edit:?} lies outside the new wrap text ({} rows)", + new_rows.len(), + ); + reconstructed.splice( + edit.old.start.0 as usize..edit.old.end.0 as usize, + new_rows[edit.new.start.0 as usize..edit.new.end.0 as usize] + .iter() + .copied(), + ); + } + assert_eq!( + reconstructed.join("\n"), + new_text, + "wrap edits do not cover all changed rows; edits: {wrap_edits:?}", + ); + *previous_wrap_text = new_text; + } + + /// Every excerpt must resolve its buffer, independently of any block: a + /// snapshot holding an excerpt whose buffer is absent would panic even when + /// the header rendering it was just computed. + #[track_caller] + fn assert_excerpts_resolve(buffer_snapshot: &MultiBufferSnapshot) { + for excerpt in buffer_snapshot.excerpts() { + let buffer_id = excerpt.context.start.buffer_id; + assert!( + buffer_snapshot.buffer_for_id(buffer_id).is_some(), + "multibuffer snapshot holds an excerpt for buffer {buffer_id:?}, \ + which is absent from its buffers", + ); + } + } + + /// Empty and single-line buffers are generated often because excerpts + /// holding no text produce zero-width edits, which `Patch` discards. + fn random_buffer_text(rng: &mut StdRng) -> String { + let len = match rng.random_range(0..100) { + 0..=29 => 0, + 30..=59 => rng.random_range(1..4), + _ => rng.random_range(4..30), + }; + RandomCharIter::new(rng).take(len).collect() + } + + fn randomized_text(text: &str, rng: &mut StdRng) -> String { + let mut chars = text.chars().collect::>(); + for _ in 0..rng.random_range(1..=3) { + if chars.is_empty() || rng.random_bool(0.5) { + let insert_at = rng.random_range(0..=chars.len()); + let insert_len = rng.random_range(1..5); + for (offset, c) in RandomCharIter::new(&mut *rng).take(insert_len).enumerate() { + chars.insert(insert_at + offset, c); + } + } else { + let start = rng.random_range(0..chars.len()); + let end = rng.random_range(start + 1..=chars.len()); + chars.drain(start..end); + } + } + chars.into_iter().collect() + } + impl Block { fn as_custom(&self) -> Option<&CustomBlock> { match self { diff --git a/crates/editor/src/display_map/fold_map.rs b/crates/editor/src/display_map/fold_map.rs index 8bd67ac8da..2739a857ae 100644 --- a/crates/editor/src/display_map/fold_map.rs +++ b/crates/editor/src/display_map/fold_map.rs @@ -411,7 +411,8 @@ impl FoldMap { #[ztracing::instrument(skip_all)] fn check_invariants(&self) { - if cfg!(test) { + #[cfg(test)] + { assert_eq!( self.snapshot.transforms.summary().input.len, self.snapshot.inlay_snapshot.len().0, @@ -432,8 +433,20 @@ impl FoldMap { let mut folds = self.snapshot.folds.iter().peekable(); while let Some(fold) = folds.next() { if let Some(next_fold) = folds.peek() { - let comparison = fold.range.cmp(&next_fold.range, self.snapshot.buffer()); - assert!(comparison.is_le()); + let buffer = self.snapshot.buffer(); + let comparison = fold.range.cmp(&next_fold.range, buffer); + assert!( + comparison.is_le(), + "folds are out of order:\n\ + fold: {:?}\n resolves to {:?}..{:?}\n\ + next_fold: {:?}\n resolves to {:?}..{:?}", + fold.range, + fold.range.start.to_offset(buffer), + fold.range.end.to_offset(buffer), + next_fold.range, + next_fold.range.start.to_offset(buffer), + next_fold.range.end.to_offset(buffer), + ); } } } diff --git a/crates/editor/src/display_map/inlay_map.rs b/crates/editor/src/display_map/inlay_map.rs index a0eba30fa0..8528188cc7 100644 --- a/crates/editor/src/display_map/inlay_map.rs +++ b/crates/editor/src/display_map/inlay_map.rs @@ -1459,8 +1459,9 @@ mod tests { hover_links::InlayHighlight, }; use collections::HashMap; - use gpui::{App, HighlightStyle}; - use multi_buffer::Anchor; + use gpui::{App, AppContext as _, HighlightStyle}; + use language::Buffer; + use multi_buffer::{Anchor, PathKey}; use project::{InlayHint, InlayHintLabel, ResolveState}; use rand::prelude::*; use settings::SettingsStore; @@ -2354,6 +2355,95 @@ mod tests { } } + /// Reproduces the "cannot summarize backward" crash family: when a path + /// key is reused for a different buffer, as happens when a diff's base + /// buffer is recreated, an inlay anchored in the departed buffer resolves + /// to the end of the reused path's region while still sorting before + /// anchors into the new buffer (same-path anchors order by buffer id). + /// That breaks the resolved-offset ordering `InlayMap::sync`'s binary + /// search over `self.inlays` relies on, so the scan reaches a valid inlay + /// belonging to a region before the edit and pushes it after content that + /// was already built, panicking in the rope layer. + #[gpui::test] + fn test_sync_after_path_key_reused_for_different_buffer(cx: &mut App) { + init_test(cx); + + let buffer_x = cx.new(|cx| Buffer::local("xxx xxx xxx\nxxx\n", cx)); + let buffer_y = cx.new(|cx| Buffer::local("yyy yyy yyy\nyyy\n", cx)); + let path = PathKey::sorted(0); + let multibuffer = cx.new(|_| MultiBuffer::new(language::Capability::ReadWrite)); + multibuffer.update(cx, |multibuffer, cx| { + let max_point_x = buffer_x.read(cx).max_point(); + multibuffer.set_excerpts_for_path( + path.clone(), + buffer_x.clone(), + [Point::zero()..max_point_x], + 0, + cx, + ); + }); + + let subscription = multibuffer.update(cx, |multibuffer, _| multibuffer.subscribe()); + let snapshot = multibuffer.read(cx).snapshot(cx); + let (mut inlay_map, _) = InlayMap::new(snapshot.clone()); + + // Anchor two inlays in `buffer_x` while it holds the path. Two are + // needed so that the binary search over the inlays probes one of them + // and is steered away from the valid inlay added below. + inlay_map.splice( + &[], + vec![ + Inlay::mock_hint(0, snapshot.anchor_after(MultiBufferOffset(5)), "|stale|"), + Inlay::mock_hint(2, snapshot.anchor_after(MultiBufferOffset(7)), "|stale|"), + ], + ); + + // Reuse the path for `buffer_y`. The stale inlay's anchor now resolves + // to the end of the path's region while still sorting first. + multibuffer.update(cx, |multibuffer, cx| { + let max_point_y = buffer_y.read(cx).max_point(); + multibuffer.set_excerpts_for_path( + path.clone(), + buffer_y.clone(), + [Point::zero()..max_point_y], + 0, + cx, + ); + }); + let snapshot = multibuffer.read(cx).snapshot(cx); + let edits = subscription.consume().into_inner(); + inlay_map.sync(snapshot.clone(), edits); + + // Anchor a valid inlay early in `buffer_y`'s region. + inlay_map.splice( + &[], + vec![Inlay::mock_hint( + 1, + snapshot.anchor_after(MultiBufferOffset(2)), + "|valid|", + )], + ); + + // Append another buffer, producing an edit at the end of the + // multibuffer: between the valid inlay's position and where the stale + // inlay now resolves. + let buffer_z = cx.new(|cx| Buffer::local("zzz zzz zzz\nzzz\n", cx)); + multibuffer.update(cx, |multibuffer, cx| { + let max_point_z = buffer_z.read(cx).max_point(); + multibuffer.set_excerpts_for_path( + PathKey::sorted(1), + buffer_z.clone(), + [Point::zero()..max_point_z], + 0, + cx, + ); + }); + let snapshot = multibuffer.read(cx).snapshot(cx); + let edits = subscription.consume().into_inner(); + let (inlay_snapshot, _) = inlay_map.sync(snapshot, edits); + inlay_snapshot.check_invariants(); + } + fn init_test(cx: &mut App) { let store = SettingsStore::test(cx); cx.set_global(store); diff --git a/crates/multi_buffer/Cargo.toml b/crates/multi_buffer/Cargo.toml index dcdca9b7cf..f0fbe34e98 100644 --- a/crates/multi_buffer/Cargo.toml +++ b/crates/multi_buffer/Cargo.toml @@ -53,6 +53,8 @@ gpui = { workspace = true, features = ["test-support"] } indoc.workspace = true language = { workspace = true, features = ["test-support"] } pretty_assertions.workspace = true +proptest.workspace = true +proptest-derive.workspace = true rand.workspace = true settings = { workspace = true, features = ["test-support"] } text = { workspace = true, features = ["test-support"] } diff --git a/crates/multi_buffer/src/anchor.rs b/crates/multi_buffer/src/anchor.rs index ef80afd385..d4ea75c0fd 100644 --- a/crates/multi_buffer/src/anchor.rs +++ b/crates/multi_buffer/src/anchor.rs @@ -38,6 +38,7 @@ pub(crate) enum AnchorSeekTarget<'a> { // buffer no longer exists at its original path key in the multibuffer Missing { path_key: &'a PathKey, + buffer_id: BufferId, }, // we have excerpts for the buffer at the expected path key Excerpt { @@ -63,9 +64,13 @@ impl std::fmt::Debug for AnchorSeekTarget<'_> { .field("path_key", path_key) .field("anchor", anchor) .finish(), - Self::Missing { path_key } => f + Self::Missing { + path_key, + buffer_id, + } => f .debug_struct("Missing") .field("path_key", path_key) + .field("buffer_id", buffer_id) .finish(), Self::Empty => f.debug_struct("Empty").finish(), } @@ -144,10 +149,14 @@ impl ExcerptAnchor { && let Some(base_text) = find_diff_state(&snapshot.diffs, self.text_anchor.buffer_id) .map(|diff| diff.base_text()) { - let self_anchor = self.diff_base_anchor.filter(|a| a.is_valid(base_text)); - let other_anchor = other.diff_base_anchor.filter(|a| a.is_valid(base_text)); - return match (self_anchor, other_anchor) { - (Some(a), Some(b)) => a.cmp(&b, base_text), + return match (self.diff_base_anchor, other.diff_base_anchor) { + (Some(self_anchor), Some(other_anchor)) => { + if base_text.can_resolve(&self_anchor) && base_text.can_resolve(&other_anchor) { + self_anchor.cmp(&other_anchor, base_text) + } else { + self_anchor.buffer_id.cmp(&other_anchor.buffer_id) + } + } (Some(_), None) => match other.text_anchor().bias { Bias::Left => Ordering::Greater, Bias::Right => Ordering::Less, @@ -174,7 +183,7 @@ impl ExcerptAnchor { let ret = Self::in_buffer(self.path, text_anchor); if let Some(diff_base_anchor) = self.diff_base_anchor { if let Some(diff) = find_diff_state(&snapshot.diffs, self.text_anchor.buffer_id) - && diff_base_anchor.is_valid(&diff.base_text()) + && diff.base_text().can_resolve(&diff_base_anchor) { ret.with_diff_base_anchor(diff_base_anchor.bias_left(diff.base_text())) } else { @@ -196,7 +205,7 @@ impl ExcerptAnchor { let ret = Self::in_buffer(self.path, text_anchor); if let Some(diff_base_anchor) = self.diff_base_anchor { if let Some(diff) = find_diff_state(&snapshot.diffs, self.text_anchor.buffer_id) - && diff_base_anchor.is_valid(&diff.base_text()) + && diff.base_text().can_resolve(&diff_base_anchor) { ret.with_diff_base_anchor(diff_base_anchor.bias_right(diff.base_text())) } else { @@ -272,7 +281,10 @@ impl ExcerptAnchor { .get(&self.buffer_id()) .filter(|state| &state.path_key == path_key) else { - return Some(AnchorSeekTarget::Missing { path_key }); + return Some(AnchorSeekTarget::Missing { + path_key, + buffer_id: self.buffer_id(), + }); }; Some(AnchorSeekTarget::Excerpt { diff --git a/crates/multi_buffer/src/multi_buffer.rs b/crates/multi_buffer/src/multi_buffer.rs index 3cd6cc56a0..4dae5ecb58 100644 --- a/crates/multi_buffer/src/multi_buffer.rs +++ b/crates/multi_buffer/src/multi_buffer.rs @@ -764,9 +764,39 @@ impl ExcerptBoundaryInfo { self.start_text_anchor().buffer_id } pub fn buffer<'a>(&self, snapshot: &'a MultiBufferSnapshot) -> &'a BufferSnapshot { - snapshot - .buffer_for_id(self.buffer_id()) - .expect("buffer snapshot not found for excerpt boundary") + let buffer_id = self.buffer_id(); + snapshot.buffer_for_id(buffer_id).unwrap_or_else(|| { + let mut excerpt_buffer_ids = Vec::new(); + for excerpt in snapshot.excerpts.iter() { + let excerpt_ids = ( + excerpt.buffer_id, + excerpt.range.context.start.buffer_id, + excerpt.path_key_index, + ); + if excerpt_buffer_ids.last() != Some(&excerpt_ids) { + excerpt_buffer_ids.push(excerpt_ids); + } + } + let present_buffer_ids = snapshot + .buffers + .iter() + .map(|(id, state)| (*id, state.path_key_index)) + .collect::>(); + panic!( + "buffer snapshot not found for excerpt boundary: sought buffer {buffer_id} at \ + path index {:?}; buffers present (id, path index): {present_buffer_ids:?}; \ + excerpts present (buffer id, anchor buffer id, path index): \ + {excerpt_buffer_ids:?}; singleton={}, has_inverted_diff={}, \ + trailing_excerpt_update_count={}", + match self.start_anchor { + Anchor::Excerpt(anchor) => Some(anchor.path), + Anchor::Min | Anchor::Max => None, + }, + snapshot.singleton, + snapshot.has_inverted_diff, + snapshot.trailing_excerpt_update_count, + ) + }) } } @@ -4899,10 +4929,10 @@ impl MultiBufferSnapshot { hunk_info, .. }) => { + let base_text = self.diff_state(*buffer_id).map(|diff| diff.base_text()); if let Some(diff_base_anchor) = anchor.diff_base_anchor - && let Some(base_text) = - self.diff_state(*buffer_id).map(|diff| diff.base_text()) - && diff_base_anchor.is_valid(&base_text) + && let Some(base_text) = base_text + && base_text.can_resolve(&diff_base_anchor) { // The anchor carries a diff-base position — resolve it // to a location inside the deleted hunk. @@ -4921,30 +4951,37 @@ impl MultiBufferSnapshot { diff_transforms.next(); continue; } - } else if at_transform_end - && anchor - .text_anchor() - .cmp(&hunk_info.hunk_start_anchor, excerpt_buffer) - .is_gt() - { - // The anchor has no (valid) diff-base position, so it - // belongs in the buffer content, not in the deleted - // hunk. However, after an edit deletes the text between - // the hunk boundary and this anchor, both resolve to - // the same excerpt_position—landing us here on the - // DeletedHunk left behind by the shared cursor. Use the - // CRDT ordering to detect that the anchor is strictly - // *past* the hunk boundary and skip to the following - // BufferContent. - diff_transforms.next(); - continue; + } else { + let departed_base_sorts_after = anchor + .diff_base_anchor + .zip(base_text) + .is_some_and(|(diff_base_anchor, base_text)| { + diff_base_anchor.buffer_id > base_text.remote_id() + }); + if at_transform_end + && (departed_base_sorts_after + || anchor + .text_anchor() + .cmp(&hunk_info.hunk_start_anchor, excerpt_buffer) + .is_gt()) + { + diff_transforms.next(); + continue; + } } } _ => { // On a BufferContent (or no transform). If the anchor // carries a diff_base_anchor it needs a DeletedHunk, so // advance to find one. - if at_transform_end && anchor.diff_base_anchor.is_some() { + if at_transform_end + && (anchor.diff_base_anchor.is_some() + || matches!( + diff_transforms.next_item(), + Some(DiffTransform::DeletedHunk { buffer_id, .. }) + if *buffer_id == anchor.text_anchor.buffer_id + )) + { diff_transforms.next(); continue; } @@ -6877,6 +6914,21 @@ impl MultiBufferSnapshot { "excerpt path key not found in active path keys: {:#?}", excerpt.path_key ); + assert!( + self.buffers.get(&excerpt.buffer_id).is_some(), + "excerpt references buffer {:#?}, which has no snapshot", + excerpt.buffer_id + ); + assert_eq!( + excerpt.range.context.start.buffer_id, excerpt.buffer_id, + "excerpt context start anchor belongs to a different buffer: {:#?}", + excerpt.range + ); + assert_eq!( + excerpt.range.context.end.buffer_id, excerpt.buffer_id, + "excerpt context end anchor belongs to a different buffer: {:#?}", + excerpt.range + ); assert_eq!( self.path_keys.get_index(excerpt.path_key_index.0 as usize), Some(&excerpt.path_key), @@ -7490,13 +7542,17 @@ impl sum_tree::SeekTarget<'_, ExcerptSummary, ExcerptSummary> for AnchorSeekTarg _cx: ::Context<'_>, ) -> cmp::Ordering { match self { - AnchorSeekTarget::Missing { path_key } => { - // Want to end up after any excerpts for (a different buffer at) the original path - match Ord::cmp(*path_key, &cursor_location.path_key) { - Ordering::Less => Ordering::Less, - Ordering::Equal | Ordering::Greater => Ordering::Greater, - } - } + AnchorSeekTarget::Missing { + path_key, + buffer_id, + } => match Ord::cmp(*path_key, &cursor_location.path_key) { + Ordering::Less => Ordering::Less, + Ordering::Equal => match &cursor_location.max_anchor { + Some(max_anchor) if *buffer_id < max_anchor.buffer_id => Ordering::Less, + _ => Ordering::Greater, + }, + Ordering::Greater => Ordering::Greater, + }, AnchorSeekTarget::Excerpt { path_key, path_key_index, diff --git a/crates/multi_buffer/src/multi_buffer_tests.rs b/crates/multi_buffer/src/multi_buffer_tests.rs index 5acbafe187..8bb16554ca 100644 --- a/crates/multi_buffer/src/multi_buffer_tests.rs +++ b/crates/multi_buffer/src/multi_buffer_tests.rs @@ -6583,3 +6583,473 @@ fn test_is_valid_anchor_past_last_excerpt_for_buffer(cx: &mut TestAppContext) { ); }); } + +/// The contract between anchor comparison and anchor resolution that every +/// sorted-input consumer relies on (`summaries_for_anchors*`, forward-only +/// cursors, and persistent trees ordered by anchor comparison such as the +/// editor's fold tree): +/// +/// 1. Within one snapshot, comparison tracks resolution: if `a.cmp(b)` is +/// `Less`, `a` must not resolve after `b`, and anchors that compare +/// `Equal` must resolve to the same offset. +/// 2. Across structural changes (edits, diff base text changes, a path key +/// reused for a different buffer, buffers leaving), a pair's comparison +/// never inverts: `Less` may weaken to `Equal`, but must not become +/// `Greater`. (Even the collapse to `Equal` is hazardous for consumers +/// that sort ranges with end tie-breaks, but it is tolerated here so +/// that outright inversions stay visible.) +/// +/// Shrinking yields a minimal operation sequence for whichever violating +/// mechanism it finds. Mechanisms found so far, each fixed and pinned by a +/// distilled test: +/// - stale-path anchors sorted by buffer id but resolved to the end of +/// their path's region +/// (`test_stale_path_anchor_comparison_tracks_resolution`); +/// - diff base anchors' validity filter flipping comparison results when a +/// base text edit deleted the region one anchor of a pair pointed into +/// (`test_folds_stay_sorted_when_diff_base_text_replaced` in the +/// editor's block_map tests); +/// - plain anchors resolved before phantom deleted-hunk rows they compare +/// after (`test_collapsed_plain_anchor_resolves_past_deleted_hunk_rows`). +#[gpui::property_test(config = proptest::prelude::ProptestConfig { + failure_persistence: Some(Box::new( + proptest::test_runner::FileFailurePersistence::WithSource("proptest-regressions"), + )), + ..Default::default() +})] +async fn test_anchor_comparison_tracks_resolution( + cx: &mut TestAppContext, + #[strategy = proptest::collection::vec(proptest::prelude::any::(), 1..16)] + ops: Vec, +) { + let base_text_variants = [ + "DEL1\nDEL2\nbbb\nccc\nddd\n", + "DEL1\nbbb\nccc\nddd\n", + "DEL2\nbbb\nccc\nddd\n", + "bbb\nccc\nddd\n", + ]; + + let mut buffer_a = cx.new(|cx| Buffer::local("bbb\nccc\nddd\n", cx)); + let mut diff_a = cx.new(|cx| { + BufferDiff::new_with_base_text( + base_text_variants[0], + &buffer_a.read(cx).text_snapshot(), + cx, + ) + }); + let buffer_b = cx.new(|cx| Buffer::local("xxx\nyyy\n", cx)); + let multibuffer = cx.new(|cx| { + let mut multibuffer = MultiBuffer::new(Capability::ReadWrite); + // Diff views expand all hunks, which materializes deleted rows from + // the base text and gives anchors in those rows `diff_base_anchor`s. + multibuffer.set_all_diff_hunks_expanded(cx); + multibuffer + }); + multibuffer.update(cx, |multibuffer, cx| { + let max_point_a = buffer_a.read(cx).max_point(); + let max_point_b = buffer_b.read(cx).max_point(); + multibuffer.set_excerpts_for_path( + PathKey::sorted(0), + buffer_a.clone(), + [Point::zero()..max_point_a], + 0, + cx, + ); + multibuffer.set_excerpts_for_path( + PathKey::sorted(1), + buffer_b.clone(), + [Point::zero()..max_point_b], + 0, + cx, + ); + multibuffer.add_diff(diff_a.clone(), cx); + }); + cx.run_until_parked(); + + let mut anchors: Vec = Vec::new(); + let mut previous_orderings: std::collections::HashMap<(usize, usize), cmp::Ordering> = + Default::default(); + let mut buffer_b_present = true; + let mut departed_diffs: Vec> = Vec::new(); + + for (step, op) in ops.iter().enumerate() { + match op { + AnchorContractOp::CreateAnchor { + position, + left_bias, + } => { + let snapshot = + multibuffer.read_with(cx, |multibuffer, cx| multibuffer.snapshot(cx)); + let offset = MultiBufferOffset(snapshot.len().0 * (*position as usize) / 1000); + let bias = if *left_bias { Bias::Left } else { Bias::Right }; + anchors.push(snapshot.anchor_at(offset, bias)); + } + AnchorContractOp::EditBuffer { position, insert } => { + buffer_a.update(cx, |buffer, cx| { + let offset = buffer.len() * (*position as usize) / 1000; + if *insert || buffer.len() == 0 { + buffer.edit([(offset..offset, "Q\n")], None, cx); + } else { + let end = (offset + 1).min(buffer.len()); + buffer.edit([(offset..end, "")], None, cx); + } + }); + } + AnchorContractOp::ChangeBaseText { variant } => { + let buffer_a_text_snapshot = + buffer_a.read_with(cx, |buffer, _| buffer.text_snapshot()); + diff_a + .update(cx, |diff, cx| { + diff.set_base_text( + Some(base_text_variants[*variant].into()), + buffer_a_text_snapshot, + cx, + ) + }) + .await; + } + AnchorContractOp::RekeyPath => { + buffer_a = cx.new(|cx| Buffer::local("bbb\nccc\nddd\n", cx)); + diff_a = cx.new(|cx| { + BufferDiff::new_with_base_text( + base_text_variants[0], + &buffer_a.read(cx).text_snapshot(), + cx, + ) + }); + // Departed diffs belong to the departed buffer; reinstalling + // them would no longer exercise base buffer swaps. + departed_diffs.clear(); + multibuffer.update(cx, |multibuffer, cx| { + let max_point = buffer_a.read(cx).max_point(); + multibuffer.set_excerpts_for_path( + PathKey::sorted(0), + buffer_a.clone(), + [Point::zero()..max_point], + 0, + cx, + ); + multibuffer.add_diff(diff_a.clone(), cx); + }); + } + AnchorContractOp::SwapDiff { reinstall, variant } => { + let buffer_a_text_snapshot = + buffer_a.read_with(cx, |buffer, _| buffer.text_snapshot()); + let next_diff = if *reinstall && let Some(previous_diff) = departed_diffs.pop() { + // Recompute the reinstalled diff against the buffer's + // current text; its base text buffer (and so the buffer + // id anchors compare by) is preserved. + let base_text = previous_diff + .read_with(cx, |diff, cx| diff.base_text_string(cx)) + .map(Arc::from); + previous_diff + .update(cx, |diff, cx| { + diff.set_base_text(base_text, buffer_a_text_snapshot, cx) + }) + .await; + previous_diff + } else { + cx.new(|cx| { + BufferDiff::new_with_base_text( + base_text_variants[*variant], + &buffer_a_text_snapshot, + cx, + ) + }) + }; + departed_diffs.push(std::mem::replace(&mut diff_a, next_diff)); + multibuffer.update(cx, |multibuffer, cx| { + multibuffer.add_diff(diff_a.clone(), cx); + }); + } + AnchorContractOp::BiasAnchor { index, right } => { + if !anchors.is_empty() { + let snapshot = + multibuffer.read_with(cx, |multibuffer, cx| multibuffer.snapshot(cx)); + let k = (anchors.len() * (*index as usize) / 1001).min(anchors.len() - 1); + let rebiased = if *right { + anchors[k].bias_right(&snapshot) + } else { + anchors[k].bias_left(&snapshot) + }; + anchors.push(rebiased); + } + } + AnchorContractOp::ToggleSecondBuffer => { + multibuffer.update(cx, |multibuffer, cx| { + if buffer_b_present { + multibuffer.remove_excerpts_for_buffer(buffer_b.read(cx).remote_id(), cx); + } else { + let max_point = buffer_b.read(cx).max_point(); + multibuffer.set_excerpts_for_path( + PathKey::sorted(1), + buffer_b.clone(), + [Point::zero()..max_point], + 0, + cx, + ); + } + }); + buffer_b_present = !buffer_b_present; + } + } + cx.run_until_parked(); + + let snapshot = multibuffer.read_with(cx, |multibuffer, cx| multibuffer.snapshot(cx)); + for i in 0..anchors.len() { + for j in i + 1..anchors.len() { + let ordering = anchors[i].cmp(&anchors[j], &snapshot); + let offset_i = anchors[i].to_offset(&snapshot); + let offset_j = anchors[j].to_offset(&snapshot); + let resolution_consistent = match ordering { + cmp::Ordering::Less => offset_i <= offset_j, + cmp::Ordering::Equal => offset_i == offset_j, + cmp::Ordering::Greater => offset_i >= offset_j, + }; + assert!( + resolution_consistent, + "after step {step} ({op:?}), comparison contradicts resolution:\n\ + anchor {i}: {:?}\n resolves to {offset_i:?}\n\ + anchor {j}: {:?}\n resolves to {offset_j:?}\n\ + but compares {ordering:?}", + anchors[i], anchors[j], + ); + + if let Some(previous) = previous_orderings.insert((i, j), ordering) { + let inverted = matches!( + (previous, ordering), + (cmp::Ordering::Less, cmp::Ordering::Greater) + | (cmp::Ordering::Greater, cmp::Ordering::Less) + ); + assert!( + !inverted, + "after step {step} ({op:?}), comparison inverted over time:\n\ + anchor {i}: {:?}\n\ + anchor {j}: {:?}\n\ + compared {previous:?} before this step, {ordering:?} after", + anchors[i], anchors[j], + ); + } + } + } + } +} + +/// Distilled from `test_anchor_comparison_tracks_resolution`'s minimal +/// shrunk counterexample (create anchor, re-key the path, create anchor). +/// After a path key is reused for a different buffer, an anchor into the +/// departed buffer sorts against anchors into the new buffer by buffer id, +/// so resolution must land it on the matching side of the path's region: +/// the start when the departed buffer's id is lower than the current +/// occupant's, the end otherwise. When resolution instead ignored that +/// order (it always chose the end), sorted-input consumers feeding +/// comparison-sorted anchors to forward-only cursors walked backward in +/// offset space — the "cannot seek backward" crash family (e.g. ZED-95K). +#[gpui::test] +async fn test_stale_path_anchor_comparison_tracks_resolution(cx: &mut TestAppContext) { + let old_buffer = cx.new(|cx| Buffer::local("bbb\nccc\nddd\n", cx)); + let multibuffer = cx.new(|_| MultiBuffer::new(Capability::ReadWrite)); + multibuffer.update(cx, |multibuffer, cx| { + let max_point = old_buffer.read(cx).max_point(); + multibuffer.set_excerpts_for_path( + PathKey::sorted(0), + old_buffer.clone(), + [Point::zero()..max_point], + 0, + cx, + ); + }); + let stale = multibuffer.read_with(cx, |multibuffer, cx| { + multibuffer + .snapshot(cx) + .anchor_at(MultiBufferOffset(0), Bias::Right) + }); + + let new_buffer = cx.new(|cx| Buffer::local("bbb\nccc\nddd\n", cx)); + multibuffer.update(cx, |multibuffer, cx| { + let max_point = new_buffer.read(cx).max_point(); + multibuffer.set_excerpts_for_path( + PathKey::sorted(0), + new_buffer.clone(), + [Point::zero()..max_point], + 0, + cx, + ); + }); + cx.run_until_parked(); + + let snapshot = multibuffer.read_with(cx, |multibuffer, cx| multibuffer.snapshot(cx)); + let fresh = snapshot.anchor_at(MultiBufferOffset(0), Bias::Right); + + let ordering = stale.cmp(&fresh, &snapshot); + let stale_offset = stale.to_offset(&snapshot); + let fresh_offset = fresh.to_offset(&snapshot); + let resolution_consistent = match ordering { + cmp::Ordering::Less => stale_offset <= fresh_offset, + cmp::Ordering::Equal => stale_offset == fresh_offset, + cmp::Ordering::Greater => stale_offset >= fresh_offset, + }; + assert!( + resolution_consistent, + "stale anchor {stale:?} resolves to {stale_offset:?} and fresh anchor {fresh:?} \ + resolves to {fresh_offset:?}, but they compare {ordering:?}" + ); +} + +/// Distilled from `test_anchor_comparison_tracks_resolution`'s second +/// shrunk counterexample. An expanded deleted hunk's phantom rows occupy +/// its attachment anchor's tie space in the buffer's CRDT order: anchor +/// comparison places a plain buffer anchor after the hunk's rows exactly +/// when it sorts past the attachment anchor, which can only happen once +/// the buffer text separating them has been deleted. Resolution must +/// place the anchor on the same side. It used to depend on which +/// transform the walk's cursor rested on: arriving on the deleted hunk +/// itself applied the CRDT rule, but arriving on the preceding buffer +/// content returned early, parking the anchor before rows it compares +/// after — so `to_offset` and a shared-cursor `summaries_for_anchors` +/// disagreed about the same anchor, and comparison-sorted consumers fed +/// forward-only cursors positions that moved backward. +#[gpui::test] +async fn test_collapsed_plain_anchor_resolves_past_deleted_hunk_rows(cx: &mut TestAppContext) { + let base_text = "DEL1\nDEL2\nbbb\nccc\nddd\n"; + let buffer_a = cx.new(|cx| Buffer::local("bbb\nccc\nddd\n", cx)); + let diff_a = cx.new(|cx| { + BufferDiff::new_with_base_text(base_text, &buffer_a.read(cx).text_snapshot(), cx) + }); + let multibuffer = cx.new(|cx| { + let mut multibuffer = MultiBuffer::new(Capability::ReadWrite); + multibuffer.set_all_diff_hunks_expanded(cx); + multibuffer + }); + multibuffer.update(cx, |multibuffer, cx| { + let max_point_a = buffer_a.read(cx).max_point(); + multibuffer.set_excerpts_for_path( + PathKey::sorted(0), + buffer_a.clone(), + [Point::zero()..max_point_a], + 0, + cx, + ); + multibuffer.add_diff(diff_a.clone(), cx); + }); + cx.run_until_parked(); + + // Delete the first "d" (insertion offset 8), then anchor before the last + // "d" (insertion offset 10). + buffer_a.update(cx, |buffer, cx| { + buffer.edit([(8..9, "")], None, cx); + }); + cx.run_until_parked(); + let snapshot = multibuffer.read_with(cx, |multibuffer, cx| multibuffer.snapshot(cx)); + assert_eq!(snapshot.text(), "DEL1\nDEL2\nbbb\nccc\ndd\n"); + let plain_anchor = snapshot.anchor_at(MultiBufferOffset(19), Bias::Left); + + // Recompute the diff against the edited buffer: a "-ddd/+dd" hunk + // appears, attached at the buffer position both anchors will collapse + // to. Anchor one character into its phantom "ddd" row. + let buffer_a_text_snapshot = buffer_a.read_with(cx, |buffer, _| buffer.text_snapshot()); + diff_a + .update(cx, |diff, cx| { + diff.set_base_text(Some(base_text.into()), buffer_a_text_snapshot, cx) + }) + .await; + cx.run_until_parked(); + let snapshot = multibuffer.read_with(cx, |multibuffer, cx| multibuffer.snapshot(cx)); + assert_eq!(snapshot.text(), "DEL1\nDEL2\nbbb\nccc\nddd\ndd\n"); + let hunk_anchor = snapshot.anchor_at(MultiBufferOffset(19), Bias::Right); + + assert_eq!( + plain_anchor.cmp(&hunk_anchor, &snapshot), + cmp::Ordering::Greater + ); + assert_eq!(plain_anchor.to_offset(&snapshot), MultiBufferOffset(23)); + assert_eq!(hunk_anchor.to_offset(&snapshot), MultiBufferOffset(19)); + + // Delete the "d" separating the two anchors (insertion offset 9). Their + // text anchors now both collapse to the seam where the phantom row + // hangs, and only the CRDT order remembers which side of it each + // belongs on. + buffer_a.update(cx, |buffer, cx| { + buffer.edit([(8..9, "")], None, cx); + }); + cx.run_until_parked(); + let snapshot = multibuffer.read_with(cx, |multibuffer, cx| multibuffer.snapshot(cx)); + assert_eq!(snapshot.text(), "DEL1\nDEL2\nbbb\nccc\nddd\nd\n"); + + assert_eq!( + plain_anchor.cmp(&hunk_anchor, &snapshot), + cmp::Ordering::Greater, + "comparison must not change: the buffer order of the anchors is immutable" + ); + assert_eq!( + hunk_anchor.to_offset(&snapshot), + MultiBufferOffset(19), + "the hunk-interior anchor stays one character into the phantom row" + ); + assert_eq!( + plain_anchor.to_offset(&snapshot), + MultiBufferOffset(22), + "the plain anchor sorts past the hunk's attachment anchor, so it must \ + resolve past the phantom rows" + ); + assert_eq!( + snapshot.summaries_for_anchors::(&[hunk_anchor, plain_anchor]), + vec![MultiBufferOffset(19), MultiBufferOffset(22)], + "the shared-cursor batch path must agree with individual resolution" + ); +} + +/// Operations for `test_anchor_comparison_tracks_resolution`, generated up +/// front as plain data so proptest can shrink the failing sequence. +#[derive(Debug, Clone, proptest_derive::Arbitrary)] +enum AnchorContractOp { + /// Create an anchor at a position scaled onto the current snapshot + /// length, so positions stay valid regardless of preceding operations. + #[proptest(weight = 4)] + CreateAnchor { + #[proptest(strategy = "0u16..=1000")] + position: u16, + left_bias: bool, + }, + /// Edit the path 0 buffer at a scaled position. + #[proptest(weight = 3)] + EditBuffer { + #[proptest(strategy = "0u16..=1000")] + position: u16, + insert: bool, + }, + /// Replace the diff's base text, keeping or dropping the deleted lines + /// that anchors inside expanded hunks point into. + #[proptest(weight = 3)] + ChangeBaseText { + #[proptest(strategy = "0usize..4")] + variant: usize, + }, + /// Reuse path 0 for a fresh buffer with a fresh diff, as split diffs do + /// when a file's diff base buffer is recreated. + #[proptest(weight = 2)] + RekeyPath, + /// Replace the diff wholesale with one whose base text lives in a + /// *different* buffer, or reinstall a previously departed diff. + /// Anchors' diff base anchors then point into a base buffer that isn't + /// the diff's current one. + #[proptest(weight = 2)] + SwapDiff { + reinstall: bool, + #[proptest(strategy = "0usize..4")] + variant: usize, + }, + /// Remove or re-add the second buffer's excerpts, so the multibuffer + /// has structure before and after the diffed region. + #[proptest(weight = 2)] + ToggleSecondBuffer, + /// Re-bias an existing anchor, appending the result as a new anchor. + /// Callers like hover links and completions re-bias anchors that can + /// point into expanded deleted hunk rows. + #[proptest(weight = 2)] + BiasAnchor { + #[proptest(strategy = "0u16..=1000")] + index: u16, + right: bool, + }, +} diff --git a/crates/text/src/anchor.rs b/crates/text/src/anchor.rs index 4dbe0e377a..2a9cbeddf2 100644 --- a/crates/text/src/anchor.rs +++ b/crates/text/src/anchor.rs @@ -148,10 +148,10 @@ impl Anchor { /// Returns true when the [`Anchor`] is located inside a visible fragment. pub fn is_valid(&self, buffer: &BufferSnapshot) -> bool { - if self.is_min() || self.is_max() { - true - } else if self.buffer_id != buffer.remote_id { + if self.buffer_id != buffer.remote_id { false + } else if self.is_min() || self.is_max() { + true } else { let Some(fragment_id) = buffer.try_fragment_id_for_anchor(self) else { return false; diff --git a/crates/text/src/text.rs b/crates/text/src/text.rs index 280d1bda7d..3e16ddff16 100644 --- a/crates/text/src/text.rs +++ b/crates/text/src/text.rs @@ -2669,9 +2669,8 @@ impl BufferSnapshot { } pub fn can_resolve(&self, anchor: &Anchor) -> bool { - anchor.is_min() - || anchor.is_max() - || (self.remote_id == anchor.buffer_id && self.version.observed(anchor.timestamp())) + self.remote_id == anchor.buffer_id + && (anchor.is_min() || anchor.is_max() || self.version.observed(anchor.timestamp())) } pub fn clip_offset(&self, offset: usize, bias: Bias) -> usize {