From 62c9e5146ace04f4d63654650e871d10b13d1be8 Mon Sep 17 00:00:00 2001 From: Cole Miller Date: Sat, 13 Dec 2025 15:27:19 -0500 Subject: [PATCH] more work on randomized test, introduce bias for row_to_base_text_row --- crates/buffer_diff/src/buffer_diff.rs | 156 ++++++++++-- crates/editor/src/split.rs | 227 +++++++++--------- crates/multi_buffer/src/multi_buffer.rs | 6 + crates/multi_buffer/src/multi_buffer_tests.rs | 39 ++- 4 files changed, 293 insertions(+), 135 deletions(-) diff --git a/crates/buffer_diff/src/buffer_diff.rs b/crates/buffer_diff/src/buffer_diff.rs index 195824101698a42dfce2fe9545b0d6f9f12dfdf9..4dda287fb2524b8faad55521b1350ff0f045da30 100644 --- a/crates/buffer_diff/src/buffer_diff.rs +++ b/crates/buffer_diff/src/buffer_diff.rs @@ -322,7 +322,12 @@ impl BufferDiffSnapshot { (new_id == old_id && new_version == old_version) || (new_empty && old_empty) } - pub fn row_to_base_text_row(&self, row: BufferRow, buffer: &text::BufferSnapshot) -> u32 { + pub fn row_to_base_text_row( + &self, + row: BufferRow, + bias: Bias, + buffer: &text::BufferSnapshot, + ) -> u32 { // TODO(split-diff) expose a parameter to reuse a cursor to avoid repeatedly seeking from the start // Find the last hunk that starts before this position. @@ -339,15 +344,24 @@ impl BufferDiffSnapshot { let unclipped_point = if let Some(hunk) = cursor.item() && hunk.buffer_range.start.cmp(&position, buffer).is_le() { - let mut unclipped_point = cursor - .end() - .diff_base_byte_range - .end - .to_point(self.base_text()); - if position.cmp(&cursor.end().buffer_range.end, buffer).is_ge() { + let unclipped_point = if position.cmp(&cursor.end().buffer_range.end, buffer).is_ge() { + let mut unclipped_point = cursor + .end() + .diff_base_byte_range + .end + .to_point(self.base_text()); unclipped_point += Point::new(row, 0) - cursor.end().buffer_range.end.to_point(buffer); - } + unclipped_point + } else if bias == Bias::Right { + cursor + .end() + .diff_base_byte_range + .end + .to_point(self.base_text()) + } else { + hunk.diff_base_byte_range.start.to_point(self.base_text()) + }; // Move the cursor so that at the next step we can clip with the start of the next hunk. cursor.next(); unclipped_point @@ -868,7 +882,7 @@ fn compare_hunks( start.get_or_insert(new_hunk.buffer_range.start); base_text_start.get_or_insert(new_hunk.diff_base_byte_range.start); end.replace(new_hunk.buffer_range.end); - base_text_end.get_or_insert(new_hunk.diff_base_byte_range.end); + base_text_end.replace(new_hunk.diff_base_byte_range.end); new_cursor.next(); } Ordering::Equal => { @@ -882,11 +896,16 @@ fn compare_hunks( .is_ge() { end.replace(old_hunk.buffer_range.end); - base_text_end.replace(old_hunk.diff_base_byte_range.end); } else { end.replace(new_hunk.buffer_range.end); - base_text_end.replace(new_hunk.diff_base_byte_range.end); } + + base_text_end.replace( + old_hunk + .diff_base_byte_range + .end + .max(new_hunk.diff_base_byte_range.end), + ); } new_cursor.next(); @@ -904,15 +923,17 @@ fn compare_hunks( (Some(new_hunk), None) => { start.get_or_insert(new_hunk.buffer_range.start); base_text_start.get_or_insert(new_hunk.diff_base_byte_range.start); + // TODO(cole) it seems like this could move end backward? end.replace(new_hunk.buffer_range.end); - base_text_end.replace(new_hunk.diff_base_byte_range.end); + base_text_end = base_text_end.max(Some(new_hunk.diff_base_byte_range.end)); new_cursor.next(); } (None, Some(old_hunk)) => { start.get_or_insert(old_hunk.buffer_range.start); base_text_start.get_or_insert(old_hunk.diff_base_byte_range.start); + // TODO(cole) it seems like this could move end backward? end.replace(old_hunk.buffer_range.end); - base_text_end.replace(old_hunk.diff_base_byte_range.end); + base_text_end = base_text_end.max(Some(old_hunk.diff_base_byte_range.end)); old_cursor.next(); } (None, None) => break, @@ -1550,7 +1571,7 @@ pub fn assert_hunks( #[cfg(test)] mod tests { - use std::fmt::Write as _; + use std::{fmt::Write as _, sync::mpsc}; use super::*; use gpui::TestAppContext; @@ -2466,21 +2487,106 @@ mod tests { let buffer_snapshot = buffer.snapshot(); let diff = BufferDiffSnapshot::new_sync(buffer_snapshot.clone(), base_text, cx); let expected_results = [ - // don't format me - (0, 0), - (1, 2), - (2, 2), - (3, 5), - (4, 5), - (5, 7), - (6, 9), + // main buffer row, base text row (right bias), base text row (left bias) + (0, 0, 0), + (1, 2, 1), + (2, 2, 2), + (3, 5, 3), + (4, 5, 5), + (5, 7, 7), + (6, 9, 9), ]; - for (buffer_row, expected) in expected_results { + for (buffer_row, expected_right, expected_left) in expected_results { + assert_eq!( + diff.row_to_base_text_row(buffer_row, Bias::Right, &buffer_snapshot), + expected_right, + "{buffer_row}" + ); assert_eq!( - diff.row_to_base_text_row(buffer_row, &buffer_snapshot), - expected, + diff.row_to_base_text_row(buffer_row, Bias::Left, &buffer_snapshot), + expected_left, "{buffer_row}" ); } } + + #[gpui::test] + async fn test_changed_ranges(cx: &mut gpui::TestAppContext) { + let base_text = " + one + two + three + four + five + six + " + .unindent(); + let buffer_text = " + one + TWO + three + four + FIVE + six + " + .unindent(); + let buffer = cx.new(|cx| language::Buffer::local(buffer_text, cx)); + let diff = cx.new(|cx| { + BufferDiff::new_with_base_text(&base_text, &buffer.read(cx).text_snapshot(), cx) + }); + let (tx, rx) = mpsc::channel(); + let subscription = + cx.update(|cx| cx.subscribe(&diff, move |_, event, _| tx.send(event.clone()).unwrap())); + + let snapshot = buffer.update(cx, |buffer, cx| { + buffer.set_text( + " + ONE + TWO + THREE + FOUR + FIVE + SIX + " + .unindent(), + cx, + ); + buffer.text_snapshot() + }); + let update = diff + .update(cx, |diff, cx| { + diff.update_diff( + snapshot.clone(), + Some(base_text.as_str().into()), + false, + None, + cx, + ) + }) + .await; + diff.update(cx, |diff, cx| { + diff.set_snapshot(update, &snapshot, false, cx) + }); + cx.run_until_parked(); + drop(subscription); + let events = rx.into_iter().collect::>(); + match events.as_slice() { + [ + BufferDiffEvent::DiffChanged { + changed_range: _, + base_text_changed_range, + }, + ] => { + // TODO(cole) this seems like it should pass but currently fails (see compare_hunks) + // assert_eq!( + // *changed_range, + // Some(Anchor::min_max_range_for_buffer( + // buffer.read_with(cx, |buffer, _| buffer.remote_id()) + // )) + // ); + assert_eq!(*base_text_changed_range, Some(0..base_text.len())); + } + _ => panic!("unexpected events: {:?}", events), + } + } } diff --git a/crates/editor/src/split.rs b/crates/editor/src/split.rs index 687b19a1fe661a293f1651316925205287fcacd1..aa4c11d7f1bab6dc4a8e00f6e647358bea35313d 100644 --- a/crates/editor/src/split.rs +++ b/crates/editor/src/split.rs @@ -10,7 +10,7 @@ use language::{Buffer, Capability}; use multi_buffer::{Anchor, ExcerptId, ExcerptRange, ExpandExcerptDirection, MultiBuffer, PathKey}; use project::Project; use rope::Point; -use text::OffsetRangeExt as _; +use text::{Bias, OffsetRangeExt as _}; use ui::{ App, Context, InteractiveElement as _, IntoElement as _, ParentElement as _, Render, Styled as _, Window, div, @@ -341,7 +341,7 @@ impl SplittableEditor { #[cfg(test)] impl SplittableEditor { - fn check_invariants(&self, cx: &App) { + fn check_invariants(&self, quiesced: bool, cx: &App) { use buffer_diff::DiffHunkStatusKind; use collections::HashSet; use multi_buffer::MultiBufferOffset; @@ -410,50 +410,35 @@ impl SplittableEditor { let secondary_excerpts = secondary.multibuffer.read(cx).excerpt_ids(); assert_eq!(primary_excerpts.len(), secondary_excerpts.len(),); - let primary_diff_hunks = self - .primary_multibuffer - .read(cx) - .snapshot(cx) - .diff_hunks() - .collect::>(); - let secondary_diff_hunks = secondary - .multibuffer - .read(cx) - .snapshot(cx) - .diff_hunks() - .collect::>(); - assert_eq!( - primary_diff_hunks.len(), - secondary_diff_hunks.len(), - "\n\nprimary: {primary_diff_hunks:#?}\nsecondary: {secondary_diff_hunks:#?}", - ); - - // self.primary_multibuffer.read(cx).check_invariants(cx); - // secondary.multibuffer.read(cx).check_invariants(cx); - // Assertions:... - // - // left.display_lines().filter(is_unmodified) == right.display_lines().filter(is_unmodified) - // - // left excerpts and right excerpts bijectivity - // - // - - // let primary_buffer_text = self - // .primary_multibuffer - // .read(cx) - // .text_summary_for_range(Anchor::min()..Anchor::max()); - // let secondary_buffer_text = secondary - // .multibuffer - // .read(cx) - // .text_summary_for_range(Anchor::min()..Anchor::max()); - // let primary_buffer_base_text = self - // .primary_multibuffer - // .read(cx) - // .base_text_summary_for_range(Anchor::min()..Anchor::max()); - // let secondary_buffer_base_text = secondary - // .multibuffer - // .read(cx) - // .base_text_summary_for_range(Anchor::min()..Anchor::max()); + if quiesced { + let primary_snapshot = self.primary_multibuffer.read(cx).snapshot(cx); + let secondary_snapshot = secondary.multibuffer.read(cx).snapshot(cx); + let primary_diff_hunks = primary_snapshot + .diff_hunks() + .map(|hunk| hunk.diff_base_byte_range.clone()) + .collect::>(); + let secondary_diff_hunks = secondary_snapshot + .diff_hunks() + .map(|hunk| hunk.diff_base_byte_range.clone()) + .collect::>(); + pretty_assertions::assert_eq!(primary_diff_hunks, secondary_diff_hunks); + + let primary_unmodified_rows = primary_snapshot + .text() + .split("\n") + .zip(primary_snapshot.row_infos(MultiBufferRow(0))) + .filter(|(_, row_info)| row_info.diff_status.is_none()) + .map(|(line, _)| line.to_owned()) + .collect::>(); + let secondary_unmodified_rows = secondary_snapshot + .text() + .split("\n") + .zip(secondary_snapshot.row_infos(MultiBufferRow(0))) + .filter(|(_, row_info)| row_info.diff_status.is_none()) + .map(|(line, _)| line.to_owned()) + .collect::>(); + pretty_assertions::assert_eq!(primary_unmodified_rows, secondary_unmodified_rows); + } } fn randomly_edit_excerpts( @@ -535,62 +520,6 @@ impl SplittableEditor { } } } - - fn randomly_mutate( - &mut self, - rng: &mut impl rand::Rng, - mutation_count: usize, - cx: &mut Context, - ) { - use rand::prelude::*; - - if rng.random_bool(0.7) { - let buffers = self.primary_editor.read(cx).buffer().read(cx).all_buffers(); - let buffer = buffers.iter().choose(rng); - - if let Some(buffer) = buffer { - buffer.update(cx, |buffer, cx| { - if rng.random() { - log::info!("randomly editing single buffer"); - buffer.randomly_edit(rng, mutation_count, cx); - } else { - log::info!("randomly undoing/redoing in single buffer"); - buffer.randomly_undo_redo(rng, cx); - } - }); - } else { - log::info!("randomly editing multibuffer"); - self.primary_multibuffer.update(cx, |multibuffer, cx| { - multibuffer.randomly_edit(rng, mutation_count, cx); - }); - } - } else if rng.random() { - self.randomly_edit_excerpts(rng, mutation_count, cx); - } else { - log::info!("updating diffs and excerpts"); - for buffer in self.primary_multibuffer.read(cx).all_buffers() { - let diff = self - .primary_multibuffer - .read(cx) - .diff_for(buffer.read(cx).remote_id()) - .unwrap(); - let buffer_snapshot = buffer.read(cx).text_snapshot(); - diff.update(cx, |diff, cx| { - diff.recalculate_diff_sync(&buffer_snapshot, cx); - }); - // TODO(split-diff) might be a good idea to try to separate the diff recalculation from the excerpt recalculation - let diff_snapshot = diff.read(cx).snapshot(cx); - let ranges = diff_snapshot - .hunks(&buffer_snapshot) - .map(|hunk| hunk.range.clone()) - .collect::>(); - let path = PathKey::for_buffer(&buffer, cx); - self.set_excerpts_for_path(path, buffer, ranges, 2, diff, cx); - } - } - - self.check_invariants(cx); - } } impl EventEmitter for SplittableEditor {} @@ -655,9 +584,13 @@ impl SecondaryEditor { .into_iter() .map(|(_, excerpt_range)| { let point_range_to_base_text_point_range = |range: Range| { - let start_row = - diff_snapshot.row_to_base_text_row(range.start.row, main_buffer); - let end_row = diff_snapshot.row_to_base_text_row(range.end.row, main_buffer); + let start_row = diff_snapshot.row_to_base_text_row( + range.start.row, + Bias::Left, + main_buffer, + ); + let end_row = + diff_snapshot.row_to_base_text_row(range.end.row, Bias::Right, main_buffer); let end_column = diff_snapshot.base_text().line_len(end_row); Point::new(start_row, 0)..Point::new(end_row, end_column) }; @@ -692,7 +625,7 @@ mod tests { use fs::FakeFs; use gpui::AppContext as _; use language::Capability; - use multi_buffer::MultiBuffer; + use multi_buffer::{MultiBuffer, PathKey}; use project::Project; use rand::rngs::StdRng; use settings::SettingsStore; @@ -712,6 +645,8 @@ mod tests { #[gpui::test(iterations = 100)] async fn test_random_split_editor(mut rng: StdRng, cx: &mut gpui::TestAppContext) { + use rand::prelude::*; + init_test(cx); let project = Project::test(FakeFs::new(cx.executor()), [], cx).await; let (workspace, cx) = @@ -728,10 +663,84 @@ mod tests { editor }); - for _ in 0..10 { + let operations = std::env::var("OPERATIONS") + .map(|i| i.parse().expect("invalid `OPERATIONS` variable")) + .unwrap_or(20); + let rng = &mut rng; + for _ in 0..operations { editor.update(cx, |editor, cx| { - editor.randomly_mutate(&mut rng, 5, cx); - }) + let buffers = editor + .primary_editor + .read(cx) + .buffer() + .read(cx) + .all_buffers(); + + if buffers.is_empty() { + editor.randomly_edit_excerpts(rng, 2, cx); + editor.check_invariants(true, cx); + return; + } + + let quiesced = match rng.random_range(0..100) { + 0..=69 if !buffers.is_empty() => { + let buffer = buffers.iter().choose(rng).unwrap(); + buffer.update(cx, |buffer, cx| { + if rng.random() { + log::info!("randomly editing single buffer"); + buffer.randomly_edit(rng, 5, cx); + } else { + log::info!("randomly undoing/redoing in single buffer"); + buffer.randomly_undo_redo(rng, cx); + } + }); + false + } + 70..=79 => { + log::info!("mutating excerpts"); + editor.randomly_edit_excerpts(rng, 2, cx); + false + } + 80..=89 if !buffers.is_empty() => { + log::info!("recalculating buffer diff"); + let buffer = buffers.iter().choose(rng).unwrap(); + let diff = editor + .primary_multibuffer + .read(cx) + .diff_for(buffer.read(cx).remote_id()) + .unwrap(); + let buffer_snapshot = buffer.read(cx).text_snapshot(); + diff.update(cx, |diff, cx| { + diff.recalculate_diff_sync(&buffer_snapshot, cx); + }); + false + } + _ => { + log::info!("quiescing"); + for buffer in buffers { + let buffer_snapshot = buffer.read(cx).text_snapshot(); + let diff = editor + .primary_multibuffer + .read(cx) + .diff_for(buffer.read(cx).remote_id()) + .unwrap(); + diff.update(cx, |diff, cx| { + diff.recalculate_diff_sync(&buffer_snapshot, cx); + }); + let diff_snapshot = diff.read(cx).snapshot(cx); + let ranges = diff_snapshot + .hunks(&buffer_snapshot) + .map(|hunk| hunk.range.clone()) + .collect::>(); + let path = PathKey::for_buffer(&buffer, cx); + editor.set_excerpts_for_path(path, buffer, ranges, 2, diff, cx); + } + true + } + }; + + editor.check_invariants(quiesced, cx); + }); } } } diff --git a/crates/multi_buffer/src/multi_buffer.rs b/crates/multi_buffer/src/multi_buffer.rs index 976da812a6d52a2aabbf65156c160e70fbc42e72..2b6b10825e8a3ac61656871ba771bf989f57c521 100644 --- a/crates/multi_buffer/src/multi_buffer.rs +++ b/crates/multi_buffer/src/multi_buffer.rs @@ -3143,6 +3143,11 @@ impl MultiBuffer { inserted_hunk_info: Some(hunk), .. }) => excerpts.item().is_some_and(|excerpt| { + if let Some(diff) = snapshot.diffs.get(&excerpt.buffer_id) + && let Some(main_buffer) = &diff.main_buffer + { + return hunk.hunk_start_anchor.is_valid(main_buffer); + } hunk.hunk_start_anchor.is_valid(&excerpt.buffer) }), _ => true, @@ -3259,6 +3264,7 @@ impl MultiBuffer { log::trace!("skipping hunk that starts before excerpt"); continue; } + hunk_buffer_range.end.to_point(&excerpt.buffer); let hunk_excerpt_start = excerpt_start + hunk_buffer_range.start.saturating_sub(excerpt_buffer_start); let hunk_excerpt_end = excerpt_end diff --git a/crates/multi_buffer/src/multi_buffer_tests.rs b/crates/multi_buffer/src/multi_buffer_tests.rs index 6146bf289b36f6504c95ef2dfd2e2877f10c0b31..bfa3146e8e4c0488d06ca03e42a72031ce81252a 100644 --- a/crates/multi_buffer/src/multi_buffer_tests.rs +++ b/crates/multi_buffer/src/multi_buffer_tests.rs @@ -3744,7 +3744,7 @@ async fn test_inverted_diff(cx: &mut TestAppContext) { .new(|cx| BufferDiff::new_with_base_text(base_text, &buffer.read(cx).text_snapshot(), cx)); cx.run_until_parked(); - let base_text_buffer = diff.read_with(cx, |diff, cx| diff.base_text_buffer()); + let base_text_buffer = diff.read_with(cx, |diff, _| diff.base_text_buffer()); let multibuffer = cx.new(|cx| { let mut multibuffer = MultiBuffer::singleton(base_text_buffer.clone(), cx); @@ -3824,6 +3824,43 @@ async fn test_inverted_diff(cx: &mut TestAppContext) { }, ); + buffer.update(cx, |buffer, cx| { + buffer.set_text("ZERO\nONE\nTWO\n", cx); + }); + cx.run_until_parked(); + let update = diff + .update(cx, |diff, cx| { + diff.update_diff( + buffer.read(cx).text_snapshot(), + Some(base_text.into()), + false, + None, + cx, + ) + }) + .await; + diff.update(cx, |diff, cx| { + diff.set_snapshot(update, &buffer.read(cx).text_snapshot(), false, cx); + }); + cx.run_until_parked(); + + assert_new_snapshot( + &multibuffer, + &mut snapshot, + &mut subscription, + cx, + indoc! { + " + - one + - two + - three + - four + - five + - six + " + }, + ); + diff.update(cx, |diff, cx| { diff.set_base_text( Some("new base\n".into()),