From f8965317c394b758106bd83aebb94ddaea0ef00e Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Tue, 25 Nov 2025 14:41:19 +0100 Subject: [PATCH] multi_buffer: Fix up some anchor checks (#43454) Release Notes: - N/A *or* Added/Fixed/Improved ... --- Cargo.lock | 1 + crates/agent_ui/src/inline_assistant.rs | 1 + crates/assistant_text_thread/Cargo.toml | 1 + .../src/assistant_text_thread_tests.rs | 7 ++- .../assistant_text_thread/src/text_thread.rs | 12 +++-- crates/buffer_diff/src/buffer_diff.rs | 1 + crates/editor/src/selections_collection.rs | 23 ++++++++++ crates/multi_buffer/src/anchor.rs | 44 +++++++++++++++---- crates/multi_buffer/src/multi_buffer.rs | 43 +++++++++++++----- crates/multi_buffer/src/multi_buffer_tests.rs | 2 + crates/multi_buffer/src/path_key.rs | 13 ++++-- crates/remote/src/transport/wsl.rs | 25 +++++------ crates/rope/src/rope.rs | 6 +-- crates/text/src/anchor.rs | 20 ++++++++- crates/text/src/text.rs | 13 +++--- 15 files changed, 154 insertions(+), 58 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 93961b4181aa1ad721ba8d740736d86c2ae32ca2..2698d882403b159f8ed350c59cc8e98ab467360d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -884,6 +884,7 @@ dependencies = [ "fuzzy", "gpui", "indoc", + "itertools 0.14.0", "language", "language_model", "log", diff --git a/crates/agent_ui/src/inline_assistant.rs b/crates/agent_ui/src/inline_assistant.rs index f822c79f2589c757173bcd2699ef6abf2ac51027..81242135757561a6c829cc9cabf8893294d9e875 100644 --- a/crates/agent_ui/src/inline_assistant.rs +++ b/crates/agent_ui/src/inline_assistant.rs @@ -1445,6 +1445,7 @@ impl InlineAssistant { multi_buffer.update(cx, |multi_buffer, cx| { multi_buffer.push_excerpts( old_buffer.clone(), + // todo(lw): buffer_start and buffer_end might come from different snapshots! Some(ExcerptRange::new(buffer_start..buffer_end)), cx, ); diff --git a/crates/assistant_text_thread/Cargo.toml b/crates/assistant_text_thread/Cargo.toml index 8dfdfa3828340217456088a246eee5b1568a7a77..7c8fcca3bfa81f6f2de570fa68ecc795cb81b257 100644 --- a/crates/assistant_text_thread/Cargo.toml +++ b/crates/assistant_text_thread/Cargo.toml @@ -29,6 +29,7 @@ fs.workspace = true futures.workspace = true fuzzy.workspace = true gpui.workspace = true +itertools.workspace = true language.workspace = true language_model.workspace = true log.workspace = true diff --git a/crates/assistant_text_thread/src/assistant_text_thread_tests.rs b/crates/assistant_text_thread/src/assistant_text_thread_tests.rs index 75a414dfc4428b3c101a72454bb185b5a171d692..0743641bf5ce33850f28987d834b2e79771cff6f 100644 --- a/crates/assistant_text_thread/src/assistant_text_thread_tests.rs +++ b/crates/assistant_text_thread/src/assistant_text_thread_tests.rs @@ -880,10 +880,9 @@ async fn test_random_context_collaboration(cx: &mut TestAppContext, mut rng: Std let num_sections = rng.random_range(0..=3); let mut section_start = 0; for _ in 0..num_sections { - let mut section_end = rng.random_range(section_start..=output_text.len()); - while !output_text.is_char_boundary(section_end) { - section_end += 1; - } + let section_end = output_text.floor_char_boundary( + rng.random_range(section_start..=output_text.len()), + ); events.push(Ok(SlashCommandEvent::StartSection { icon: IconName::Ai, label: "section".into(), diff --git a/crates/assistant_text_thread/src/text_thread.rs b/crates/assistant_text_thread/src/text_thread.rs index a50e410ab7d1bd1eb34ba367dfbfd36a7b2ec826..2bc4ceec4c243a654abf04b19b4e2ba93a1fef4f 100644 --- a/crates/assistant_text_thread/src/text_thread.rs +++ b/crates/assistant_text_thread/src/text_thread.rs @@ -16,6 +16,7 @@ use gpui::{ App, AppContext as _, Context, Entity, EventEmitter, RenderImage, SharedString, Subscription, Task, }; +use itertools::Itertools as _; use language::{AnchorRangeExt, Bias, Buffer, LanguageRegistry, OffsetRangeExt, Point, ToOffset}; use language_model::{ LanguageModel, LanguageModelCacheConfiguration, LanguageModelCompletionEvent, @@ -1853,14 +1854,17 @@ impl TextThread { } if ensure_trailing_newline - && buffer.contains_str_at(command_range_end, "\n") + && buffer + .chars_at(command_range_end) + .next() + .is_some_and(|c| c == '\n') { - let newline_offset = insert_position.saturating_sub(1); - if buffer.contains_str_at(newline_offset, "\n") + if let Some((prev_char, '\n')) = + buffer.reversed_chars_at(insert_position).next_tuple() && last_section_range.is_none_or(|last_section_range| { !last_section_range .to_offset(buffer) - .contains(&newline_offset) + .contains(&(insert_position - prev_char.len_utf8())) }) { deletions.push((command_range_end..command_range_end + 1, "")); diff --git a/crates/buffer_diff/src/buffer_diff.rs b/crates/buffer_diff/src/buffer_diff.rs index d6ae5545200bb47976554814e346be3039fa276e..52c6463b9bcccd242ef18e5f3dcb518bd335686d 100644 --- a/crates/buffer_diff/src/buffer_diff.rs +++ b/crates/buffer_diff/src/buffer_diff.rs @@ -147,6 +147,7 @@ impl std::fmt::Debug for BufferDiffInner { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("BufferDiffSnapshot") .field("hunks", &self.hunks) + .field("remote_id", &self.base_text.remote_id()) .finish() } } diff --git a/crates/editor/src/selections_collection.rs b/crates/editor/src/selections_collection.rs index c1b8d11db94de7394b36ec706f42622993b63785..f8ff9da763403b0946e99a4e39c934ff43ad6634 100644 --- a/crates/editor/src/selections_collection.rs +++ b/crates/editor/src/selections_collection.rs @@ -415,6 +415,29 @@ impl SelectionsCollection { !mutable_collection.disjoint.is_empty() || mutable_collection.pending.is_some(), "There must be at least one selection" ); + if cfg!(debug_assertions) { + mutable_collection.disjoint.iter().for_each(|selection| { + assert!( + snapshot.can_resolve(&selection.start), + "disjoint selection start is not resolvable for the given snapshot:\n{selection:?}", + ); + assert!( + snapshot.can_resolve(&selection.end), + "disjoint selection end is not resolvable for the given snapshot: {selection:?}", + ); + }); + if let Some(pending) = &mutable_collection.pending { + let selection = &pending.selection; + assert!( + snapshot.can_resolve(&selection.start), + "pending selection start is not resolvable for the given snapshot: {pending:?}", + ); + assert!( + snapshot.can_resolve(&selection.end), + "pending selection end is not resolvable for the given snapshot: {pending:?}", + ); + } + } (mutable_collection.selections_changed, result) } diff --git a/crates/multi_buffer/src/anchor.rs b/crates/multi_buffer/src/anchor.rs index 57b5244b3f276265c31f1431701a2bd7d8e59aef..b8c1680574a86354d92f39c544c202642293f619 100644 --- a/crates/multi_buffer/src/anchor.rs +++ b/crates/multi_buffer/src/anchor.rs @@ -9,14 +9,33 @@ use std::{ use sum_tree::Bias; use text::BufferId; -#[derive(Clone, Copy, Eq, PartialEq, Debug, Hash)] +#[derive(Clone, Copy, Eq, PartialEq, Hash)] pub struct Anchor { + /// Invariant: If buffer id is `None`, excerpt id must be `ExcerptId::min()` or `ExcerptId::max()`. pub buffer_id: Option, pub excerpt_id: ExcerptId, pub text_anchor: text::Anchor, pub diff_base_anchor: Option, } +impl std::fmt::Debug for Anchor { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + if *self == Self::min() { + return f.write_str("Anchor::MIN"); + } + if *self == Self::max() { + return f.write_str("Anchor::MAX"); + } + + f.debug_struct("Anchor") + .field("buffer_id", &self.buffer_id) + .field("excerpt_id", &self.excerpt_id) + .field("text_anchor", &self.text_anchor) + .field("diff_base_anchor", &self.diff_base_anchor) + .finish() + } +} + impl Anchor { pub fn with_diff_base_anchor(self, diff_base_anchor: text::Anchor) -> Self { Self { @@ -30,6 +49,10 @@ impl Anchor { buffer_id: BufferId, text_anchor: text::Anchor, ) -> Self { + debug_assert!( + text_anchor.buffer_id.is_none_or(|id| id == buffer_id), + "buffer id does not match the one in the text anchor: {buffer_id:?} {text_anchor:?}", + ); Self { buffer_id: Some(buffer_id), excerpt_id, @@ -77,7 +100,12 @@ impl Anchor { if excerpt_id_cmp.is_ne() { return excerpt_id_cmp; } - if self_excerpt_id == ExcerptId::min() || self_excerpt_id == ExcerptId::max() { + if self_excerpt_id == ExcerptId::max() + && self.text_anchor == text::Anchor::MAX + && self.text_anchor == text::Anchor::MAX + && self.diff_base_anchor.is_none() + && other.diff_base_anchor.is_none() + { return Ordering::Equal; } if let Some(excerpt) = snapshot.excerpt(self_excerpt_id) { @@ -119,8 +147,8 @@ impl Anchor { && let Some(excerpt) = snapshot.excerpt(self.excerpt_id) { return Self { - buffer_id: self.buffer_id, - excerpt_id: self.excerpt_id, + buffer_id: Some(excerpt.buffer_id), + excerpt_id: excerpt.id, text_anchor: self.text_anchor.bias_left(&excerpt.buffer), diff_base_anchor: self.diff_base_anchor.map(|a| { if let Some(base_text) = snapshot @@ -143,8 +171,8 @@ impl Anchor { && let Some(excerpt) = snapshot.excerpt(self.excerpt_id) { return Self { - buffer_id: self.buffer_id, - excerpt_id: self.excerpt_id, + buffer_id: Some(excerpt.buffer_id), + excerpt_id: excerpt.id, text_anchor: self.text_anchor.bias_right(&excerpt.buffer), diff_base_anchor: self.diff_base_anchor.map(|a| { if let Some(base_text) = snapshot @@ -174,8 +202,8 @@ impl Anchor { } pub fn is_valid(&self, snapshot: &MultiBufferSnapshot) -> bool { - if *self == Anchor::min() || *self == Anchor::max() { - true + if *self == Anchor::min() || self.excerpt_id == ExcerptId::max() { + !snapshot.is_empty() } else if let Some(excerpt) = snapshot.excerpt(self.excerpt_id) { (self.text_anchor == excerpt.range.context.start || self.text_anchor == excerpt.range.context.end diff --git a/crates/multi_buffer/src/multi_buffer.rs b/crates/multi_buffer/src/multi_buffer.rs index 93fa26e02936884bc4b9dfd19bdea37455f1fd6e..7922692d30eb3a79e835f5e4b94313c3ea886a7c 100644 --- a/crates/multi_buffer/src/multi_buffer.rs +++ b/crates/multi_buffer/src/multi_buffer.rs @@ -5076,8 +5076,7 @@ impl MultiBufferSnapshot { excerpt_id: ExcerptId, text_anchor: Range, ) -> Option> { - let excerpt_id = self.latest_excerpt_id(excerpt_id); - let excerpt = self.excerpt(excerpt_id)?; + let excerpt = self.excerpt(self.latest_excerpt_id(excerpt_id))?; Some( self.anchor_in_excerpt_(excerpt, text_anchor.start)? @@ -5092,8 +5091,7 @@ impl MultiBufferSnapshot { excerpt_id: ExcerptId, text_anchor: text::Anchor, ) -> Option { - let excerpt_id = self.latest_excerpt_id(excerpt_id); - let excerpt = self.excerpt(excerpt_id)?; + let excerpt = self.excerpt(self.latest_excerpt_id(excerpt_id))?; self.anchor_in_excerpt_(excerpt, text_anchor) } @@ -5130,7 +5128,8 @@ impl MultiBufferSnapshot { } pub fn can_resolve(&self, anchor: &Anchor) -> bool { - if anchor.excerpt_id == ExcerptId::min() || anchor.excerpt_id == ExcerptId::max() { + if *anchor == Anchor::min() || anchor.excerpt_id == ExcerptId::max() { + // todo(lw): should be `!self.is_empty()` true } else if let Some(excerpt) = self.excerpt(anchor.excerpt_id) { excerpt.buffer.can_resolve(&anchor.text_anchor) @@ -5791,8 +5790,8 @@ impl MultiBufferSnapshot { .and_then(|(buffer, _)| buffer.file()) } - pub fn language_at(&self, point: T) -> Option<&Arc> { - self.point_to_buffer_offset(point) + pub fn language_at(&self, offset: T) -> Option<&Arc> { + self.point_to_buffer_offset(offset) .and_then(|(buffer, offset)| buffer.language_at(offset)) } @@ -5992,13 +5991,27 @@ impl MultiBufferSnapshot { theme: Option<&SyntaxTheme>, ) -> Option<(BufferId, Vec>)> { let anchor = self.anchor_before(offset); - let excerpt_id = anchor.excerpt_id; - let excerpt = self.excerpt(excerpt_id)?; - let buffer_id = excerpt.buffer_id; + let excerpt @ &Excerpt { + id: excerpt_id, + buffer_id, + ref buffer, + .. + } = self.excerpt(anchor.excerpt_id)?; + if cfg!(debug_assertions) { + match anchor.buffer_id { + // we clearly are hitting this according to sentry, but in what situations can this occur? + Some(anchor_buffer_id) => { + assert_eq!( + anchor_buffer_id, buffer_id, + "anchor {anchor:?} does not match with resolved excerpt {excerpt:?}" + ) + } + None => assert_eq!(anchor, Anchor::max()), + } + }; Some(( buffer_id, - excerpt - .buffer + buffer .symbols_containing(anchor.text_anchor, theme) .into_iter() .flat_map(|item| { @@ -6114,6 +6127,12 @@ impl MultiBufferSnapshot { } } + /// Returns the excerpt for the given id. The returned excerpt is guaranteed + /// to have the same excerpt id as the one passed in, with the exception of + /// `ExcerptId::max()`. + /// + /// Callers of this function should generally use the resulting excerpt's `id` field + /// afterwards. fn excerpt(&self, excerpt_id: ExcerptId) -> Option<&Excerpt> { let mut cursor = self.excerpts.cursor::>(()); let locator = self.excerpt_locator_for_id(excerpt_id); diff --git a/crates/multi_buffer/src/multi_buffer_tests.rs b/crates/multi_buffer/src/multi_buffer_tests.rs index 526c77db85a3efabb0e64b184dbed6fa90097558..9517f1f76ece2f34aa5c95eb27b408e1ef004b99 100644 --- a/crates/multi_buffer/src/multi_buffer_tests.rs +++ b/crates/multi_buffer/src/multi_buffer_tests.rs @@ -3050,7 +3050,9 @@ async fn test_random_multibuffer(cx: &mut TestAppContext, mut rng: StdRng) { for _ in 0..10 { let end_ix = rng.random_range(0..=text_rope.len()); + let end_ix = text_rope.floor_char_boundary(end_ix); let start_ix = rng.random_range(0..=end_ix); + let start_ix = text_rope.floor_char_boundary(start_ix); assert_eq!( snapshot .bytes_in_range(MultiBufferOffset(start_ix)..MultiBufferOffset(end_ix)) diff --git a/crates/multi_buffer/src/path_key.rs b/crates/multi_buffer/src/path_key.rs index 926ceff202837d13fe14350ee0334cbf4036bd89..530bb4aa6435fb9a3aa768d84a2bbcf829eb72c6 100644 --- a/crates/multi_buffer/src/path_key.rs +++ b/crates/multi_buffer/src/path_key.rs @@ -57,7 +57,7 @@ impl MultiBuffer { let snapshot = self.read(cx); let excerpt = snapshot.excerpt(*excerpt_id)?; Some(Anchor::in_buffer( - *excerpt_id, + excerpt.id, excerpt.buffer_id, excerpt.range.context.start, )) @@ -182,11 +182,16 @@ impl MultiBuffer { }; let ids_to_expand = HashSet::from_iter(ids); + let mut excerpt_id_ = None; let expanded_ranges = excerpt_ids.iter().filter_map(|excerpt_id| { let excerpt = snapshot.excerpt(*excerpt_id)?; + let excerpt_id = excerpt.id; + if excerpt_id_.is_none() { + excerpt_id_ = Some(excerpt_id); + } let mut context = excerpt.range.context.to_point(&excerpt.buffer); - if ids_to_expand.contains(excerpt_id) { + if ids_to_expand.contains(&excerpt_id) { match direction { ExpandExcerptDirection::Up => { context.start.row = context.start.row.saturating_sub(line_count); @@ -222,10 +227,10 @@ impl MultiBuffer { } merged_ranges.push(range) } - let Some(excerpt_id) = excerpt_ids.first() else { + let Some(excerpt_id) = excerpt_id_ else { continue; }; - let Some(buffer_id) = &snapshot.buffer_id_for_excerpt(*excerpt_id) else { + let Some(buffer_id) = &snapshot.buffer_id_for_excerpt(excerpt_id) else { continue; }; diff --git a/crates/remote/src/transport/wsl.rs b/crates/remote/src/transport/wsl.rs index 9fdf14d9fed6e6caf108171e292d4c2f33709ce7..3239f8813159a42a95c607a1b893845d4d5ae3c8 100644 --- a/crates/remote/src/transport/wsl.rs +++ b/crates/remote/src/transport/wsl.rs @@ -141,10 +141,6 @@ impl WslRemoteConnection { windows_path_to_wsl_path_impl(&self.connection_options, source, self.can_exec).await } - fn wsl_command(&self, program: &str, args: &[impl AsRef]) -> process::Command { - wsl_command_impl(&self.connection_options, program, args, self.can_exec) - } - async fn run_wsl_command(&self, program: &str, args: &[&str]) -> Result { run_wsl_command_impl(&self.connection_options, program, args, self.can_exec).await } @@ -345,16 +341,17 @@ impl RemoteConnection for WslRemoteConnection { if reconnect { proxy_args.push("--reconnect".to_owned()); } - let proxy_process = match self - .wsl_command("env", &proxy_args) - .kill_on_drop(true) - .spawn() - { - Ok(process) => process, - Err(error) => { - return Task::ready(Err(anyhow!("failed to spawn remote server: {}", error))); - } - }; + + let proxy_process = + match wsl_command_impl(&self.connection_options, "env", &proxy_args, self.can_exec) + .kill_on_drop(true) + .spawn() + { + Ok(process) => process, + Err(error) => { + return Task::ready(Err(anyhow!("failed to spawn remote server: {}", error))); + } + }; super::handle_rpc_messages_over_child_process_stdio( proxy_process, diff --git a/crates/rope/src/rope.rs b/crates/rope/src/rope.rs index ad39022c0d6181bd5d5f4fdfc1b84ea4a667340d..32894fb84469287fb1474efc57d8180bdee13466 100644 --- a/crates/rope/src/rope.rs +++ b/crates/rope/src/rope.rs @@ -715,10 +715,8 @@ impl<'a> Chunks<'a> { range.start }; let chunk_offset = offset - chunks.start(); - if let Some(chunk) = chunks.item() - && !chunk.text.is_char_boundary(chunk_offset) - { - panic!("byte index {} is not a char boundary", offset); + if let Some(chunk) = chunks.item() { + chunk.assert_char_boundary(chunk_offset); } Self { chunks, diff --git a/crates/text/src/anchor.rs b/crates/text/src/anchor.rs index cf2febdfc505b426fd8d224a2dc29f18d22cd1a8..63a9ff6f1863041594fba7ebea0b3feaba6b8db7 100644 --- a/crates/text/src/anchor.rs +++ b/crates/text/src/anchor.rs @@ -6,7 +6,7 @@ use std::{cmp::Ordering, fmt::Debug, ops::Range}; use sum_tree::{Bias, Dimensions}; /// A timestamped position in a buffer -#[derive(Copy, Clone, Eq, PartialEq, Debug, Hash)] +#[derive(Copy, Clone, Eq, PartialEq, Hash)] pub struct Anchor { pub timestamp: clock::Lamport, /// The byte offset in the buffer @@ -16,6 +16,24 @@ pub struct Anchor { pub buffer_id: Option, } +impl Debug for Anchor { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + if *self == Self::MIN { + return f.write_str("Anchor::MIN"); + } + if *self == Self::MAX { + return f.write_str("Anchor::MAX"); + } + + f.debug_struct("Anchor") + .field("timestamp", &self.timestamp) + .field("offset", &self.offset) + .field("bias", &self.bias) + .field("buffer_id", &self.buffer_id) + .finish() + } +} + impl Anchor { pub const MIN: Self = Self { timestamp: clock::Lamport::MIN, diff --git a/crates/text/src/text.rs b/crates/text/src/text.rs index fe9fe26f1bcc89b66753703e03f0a8bfeec628bd..5f87e5441d2bb97863b0086ac273e4d4d8acfdc9 100644 --- a/crates/text/src/text.rs +++ b/crates/text/src/text.rs @@ -2444,7 +2444,9 @@ impl BufferSnapshot { } else if bias == Bias::Right && offset == self.len() { Anchor::MAX } else { - if offset > self.visible_text.len() { + if cfg!(debug_assertions) { + self.visible_text.assert_char_boundary(offset); + } else if offset > self.visible_text.len() { panic!("offset {} is out of bounds", offset) } let (start, _, item) = self.fragments.find::(&None, &offset, bias); @@ -3137,12 +3139,9 @@ impl ToOffset for Point { impl ToOffset for usize { fn to_offset(&self, snapshot: &BufferSnapshot) -> usize { - assert!( - *self <= snapshot.len(), - "offset {} is out of range, snapshot length is {}", - self, - snapshot.len() - ); + if cfg!(debug_assertions) { + snapshot.as_rope().assert_char_boundary(*self); + } *self } }