From 0b87be71e605d334df117058c413891e949ea498 Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Wed, 6 Mar 2024 18:55:36 -0700 Subject: [PATCH] Make anchor_in_excerpt Optional (#8975) We were seeing panics due to callers assuming they had valid excerpt_ids, but that cannot easily be guaranteed across await points as anyone may remove an excerpt. Release Notes: - Fixed a panic when hovering in a multibuffer --- crates/assistant/src/assistant_panel.rs | 4 +- crates/diagnostics/src/diagnostics.rs | 18 ++++---- crates/editor/src/hover_links.rs | 27 ++++++----- crates/editor/src/hover_popover.rs | 23 +++++----- crates/editor/src/inlay_hint_cache.rs | 44 ++++++++++-------- crates/editor/src/items.rs | 4 +- crates/language_tools/src/syntax_tree_view.rs | 8 +++- crates/multi_buffer/src/multi_buffer.rs | 46 +++++++++++-------- 8 files changed, 101 insertions(+), 73 deletions(-) diff --git a/crates/assistant/src/assistant_panel.rs b/crates/assistant/src/assistant_panel.rs index 28623ea8b1..fd590ea514 100644 --- a/crates/assistant/src/assistant_panel.rs +++ b/crates/assistant/src/assistant_panel.rs @@ -2413,7 +2413,9 @@ impl ConversationEditor { .read(cx) .messages(cx) .map(|message| BlockProperties { - position: buffer.anchor_in_excerpt(excerpt_id, message.anchor), + position: buffer + .anchor_in_excerpt(excerpt_id, message.anchor) + .unwrap(), height: 2, style: BlockStyle::Sticky, render: Arc::new({ diff --git a/crates/diagnostics/src/diagnostics.rs b/crates/diagnostics/src/diagnostics.rs index 6d6a946fa0..d211eed516 100644 --- a/crates/diagnostics/src/diagnostics.rs +++ b/crates/diagnostics/src/diagnostics.rs @@ -517,15 +517,15 @@ impl ProjectDiagnosticsEditor { self.editor.update(cx, |editor, cx| { editor.remove_blocks(blocks_to_remove, None, cx); let block_ids = editor.insert_blocks( - blocks_to_add.into_iter().map(|block| { + blocks_to_add.into_iter().flat_map(|block| { let (excerpt_id, text_anchor) = block.position; - BlockProperties { - position: excerpts_snapshot.anchor_in_excerpt(excerpt_id, text_anchor), + Some(BlockProperties { + position: excerpts_snapshot.anchor_in_excerpt(excerpt_id, text_anchor)?, height: block.height, style: block.style, render: block.render, disposition: block.disposition, - } + }) }), Some(Autoscroll::fit()), cx, @@ -589,14 +589,16 @@ impl ProjectDiagnosticsEditor { Ok(ix) | Err(ix) => ix, }; if let Some(group) = groups.get(group_ix) { - let offset = excerpts_snapshot + if let Some(offset) = excerpts_snapshot .anchor_in_excerpt( group.excerpts[group.primary_excerpt_ix], group.primary_diagnostic.range.start, ) - .to_offset(&excerpts_snapshot); - selection.start = offset; - selection.end = offset; + .map(|anchor| anchor.to_offset(&excerpts_snapshot)) + { + selection.start = offset; + selection.end = offset; + } } } } diff --git a/crates/editor/src/hover_links.rs b/crates/editor/src/hover_links.rs index e1afaa6591..ae15d79ce1 100644 --- a/crates/editor/src/hover_links.rs +++ b/crates/editor/src/hover_links.rs @@ -13,7 +13,7 @@ use project::{ }; use std::ops::Range; use theme::ActiveTheme as _; -use util::TryFutureExt; +use util::{maybe, TryFutureExt}; #[derive(Debug)] pub struct HoveredLinkState { @@ -424,12 +424,13 @@ pub fn show_link_definition( TriggerPoint::Text(_) => { if let Some((url_range, url)) = find_url(&buffer, buffer_position, cx.clone()) { this.update(&mut cx, |_, _| { - let start = snapshot.anchor_in_excerpt(excerpt_id, url_range.start); - let end = snapshot.anchor_in_excerpt(excerpt_id, url_range.end); - ( - Some(RangeInEditor::Text(start..end)), - vec![HoverLink::Url(url)], - ) + let range = maybe!({ + let start = + snapshot.anchor_in_excerpt(excerpt_id, url_range.start)?; + let end = snapshot.anchor_in_excerpt(excerpt_id, url_range.end)?; + Some(RangeInEditor::Text(start..end)) + }); + (range, vec![HoverLink::Url(url)]) }) .ok() } else if let Some(project) = project { @@ -449,12 +450,14 @@ pub fn show_link_definition( .map(|definition_result| { ( definition_result.iter().find_map(|link| { - link.origin.as_ref().map(|origin| { - let start = snapshot - .anchor_in_excerpt(excerpt_id, origin.range.start); + link.origin.as_ref().and_then(|origin| { + let start = snapshot.anchor_in_excerpt( + excerpt_id, + origin.range.start, + )?; let end = snapshot - .anchor_in_excerpt(excerpt_id, origin.range.end); - RangeInEditor::Text(start..end) + .anchor_in_excerpt(excerpt_id, origin.range.end)?; + Some(RangeInEditor::Text(start..end)) }) }), definition_result.into_iter().map(HoverLink::Text).collect(), diff --git a/crates/editor/src/hover_popover.rs b/crates/editor/src/hover_popover.rs index b2a168fb83..ed90fd7130 100644 --- a/crates/editor/src/hover_popover.rs +++ b/crates/editor/src/hover_popover.rs @@ -294,18 +294,19 @@ fn show_hover( let hover_popover = match hover_result { Some(hover_result) if !hover_result.is_empty() => { // Create symbol range of anchors for highlighting and filtering of future requests. - let range = if let Some(range) = hover_result.range { - let start = snapshot - .buffer_snapshot - .anchor_in_excerpt(excerpt_id, range.start); - let end = snapshot - .buffer_snapshot - .anchor_in_excerpt(excerpt_id, range.end); + let range = hover_result + .range + .and_then(|range| { + let start = snapshot + .buffer_snapshot + .anchor_in_excerpt(excerpt_id, range.start)?; + let end = snapshot + .buffer_snapshot + .anchor_in_excerpt(excerpt_id, range.end)?; - start..end - } else { - anchor..anchor - }; + Some(start..end) + }) + .unwrap_or_else(|| anchor..anchor); let language_registry = project.update(&mut cx, |p, _| p.languages().clone())?; diff --git a/crates/editor/src/inlay_hint_cache.rs b/crates/editor/src/inlay_hint_cache.rs index a796c39a25..73a997e351 100644 --- a/crates/editor/src/inlay_hint_cache.rs +++ b/crates/editor/src/inlay_hint_cache.rs @@ -460,14 +460,15 @@ impl InlayHintCache { if !old_kinds.contains(&cached_hint.kind) && new_kinds.contains(&cached_hint.kind) { - to_insert.push(Inlay::hint( - cached_hint_id.id(), - multi_buffer_snapshot.anchor_in_excerpt( - *excerpt_id, - cached_hint.position, - ), - &cached_hint, - )); + if let Some(anchor) = multi_buffer_snapshot + .anchor_in_excerpt(*excerpt_id, cached_hint.position) + { + to_insert.push(Inlay::hint( + cached_hint_id.id(), + anchor, + &cached_hint, + )); + } } excerpt_cache.next(); } @@ -483,12 +484,15 @@ impl InlayHintCache { let maybe_missed_cached_hint = &excerpt_cached_hints.hints_by_id[cached_hint_id]; let cached_hint_kind = maybe_missed_cached_hint.kind; if !old_kinds.contains(&cached_hint_kind) && new_kinds.contains(&cached_hint_kind) { - to_insert.push(Inlay::hint( - cached_hint_id.id(), - multi_buffer_snapshot - .anchor_in_excerpt(*excerpt_id, maybe_missed_cached_hint.position), - &maybe_missed_cached_hint, - )); + if let Some(anchor) = multi_buffer_snapshot + .anchor_in_excerpt(*excerpt_id, maybe_missed_cached_hint.position) + { + to_insert.push(Inlay::hint( + cached_hint_id.id(), + anchor, + &maybe_missed_cached_hint, + )); + } } } } @@ -1200,11 +1204,13 @@ fn apply_hint_update( .allowed_hint_kinds .contains(&new_hint.kind) { - let new_hint_position = - multi_buffer_snapshot.anchor_in_excerpt(query.excerpt_id, new_hint.position); - splice - .to_insert - .push(Inlay::hint(new_inlay_id, new_hint_position, &new_hint)); + if let Some(new_hint_position) = + multi_buffer_snapshot.anchor_in_excerpt(query.excerpt_id, new_hint.position) + { + splice + .to_insert + .push(Inlay::hint(new_inlay_id, new_hint_position, &new_hint)); + } } let new_id = InlayId::Hint(new_inlay_id); cached_excerpt_hints.hints_by_id.insert(new_id, new_hint); diff --git a/crates/editor/src/items.rs b/crates/editor/src/items.rs index 07ef6a9d84..298e242dd9 100644 --- a/crates/editor/src/items.rs +++ b/crates/editor/src/items.rs @@ -1154,8 +1154,8 @@ impl SearchableItem for Editor { let end = excerpt .buffer .anchor_before(excerpt_range.start + range.end); - buffer.anchor_in_excerpt(excerpt.id, start) - ..buffer.anchor_in_excerpt(excerpt.id, end) + buffer.anchor_in_excerpt(excerpt.id, start).unwrap() + ..buffer.anchor_in_excerpt(excerpt.id, end).unwrap() }), ); } diff --git a/crates/language_tools/src/syntax_tree_view.rs b/crates/language_tools/src/syntax_tree_view.rs index bca193bc94..0340180442 100644 --- a/crates/language_tools/src/syntax_tree_view.rs +++ b/crates/language_tools/src/syntax_tree_view.rs @@ -227,8 +227,12 @@ impl SyntaxTreeView { let multibuffer = editor_state.editor.read(cx).buffer(); let multibuffer = multibuffer.read(cx).snapshot(cx); let excerpt_id = buffer_state.excerpt_id; - let range = multibuffer.anchor_in_excerpt(excerpt_id, range.start) - ..multibuffer.anchor_in_excerpt(excerpt_id, range.end); + let range = multibuffer + .anchor_in_excerpt(excerpt_id, range.start) + .unwrap() + ..multibuffer + .anchor_in_excerpt(excerpt_id, range.end) + .unwrap(); // Update the editor with the anchor range. editor_state.editor.update(cx, |editor, cx| { diff --git a/crates/multi_buffer/src/multi_buffer.rs b/crates/multi_buffer/src/multi_buffer.rs index 60b8af4a53..ce50a9345a 100644 --- a/crates/multi_buffer/src/multi_buffer.rs +++ b/crates/multi_buffer/src/multi_buffer.rs @@ -2788,7 +2788,13 @@ impl MultiBufferSnapshot { } } - pub fn anchor_in_excerpt(&self, excerpt_id: ExcerptId, text_anchor: text::Anchor) -> Anchor { + /// Returns an anchor for the given excerpt and text anchor, + /// returns None if the excerpt_id is no longer valid. + pub fn anchor_in_excerpt( + &self, + excerpt_id: ExcerptId, + text_anchor: text::Anchor, + ) -> Option { let locator = self.excerpt_locator_for_id(excerpt_id); let mut cursor = self.excerpts.cursor::>(); cursor.seek(locator, Bias::Left, &()); @@ -2796,14 +2802,14 @@ impl MultiBufferSnapshot { if excerpt.id == excerpt_id { let text_anchor = excerpt.clip_anchor(text_anchor); drop(cursor); - return Anchor { + return Some(Anchor { buffer_id: Some(excerpt.buffer_id), excerpt_id, text_anchor, - }; + }); } } - panic!("excerpt not found"); + None } pub fn can_resolve(&self, anchor: &Anchor) -> bool { @@ -3283,13 +3289,15 @@ impl MultiBufferSnapshot { outline .items .into_iter() - .map(|item| OutlineItem { - depth: item.depth, - range: self.anchor_in_excerpt(*excerpt_id, item.range.start) - ..self.anchor_in_excerpt(*excerpt_id, item.range.end), - text: item.text, - highlight_ranges: item.highlight_ranges, - name_ranges: item.name_ranges, + .flat_map(|item| { + Some(OutlineItem { + depth: item.depth, + range: self.anchor_in_excerpt(*excerpt_id, item.range.start)? + ..self.anchor_in_excerpt(*excerpt_id, item.range.end)?, + text: item.text, + highlight_ranges: item.highlight_ranges, + name_ranges: item.name_ranges, + }) }) .collect(), )) @@ -3310,13 +3318,15 @@ impl MultiBufferSnapshot { .symbols_containing(anchor.text_anchor, theme) .into_iter() .flatten() - .map(|item| OutlineItem { - depth: item.depth, - range: self.anchor_in_excerpt(excerpt_id, item.range.start) - ..self.anchor_in_excerpt(excerpt_id, item.range.end), - text: item.text, - highlight_ranges: item.highlight_ranges, - name_ranges: item.name_ranges, + .flat_map(|item| { + Some(OutlineItem { + depth: item.depth, + range: self.anchor_in_excerpt(excerpt_id, item.range.start)? + ..self.anchor_in_excerpt(excerpt_id, item.range.end)?, + text: item.text, + highlight_ranges: item.highlight_ranges, + name_ranges: item.name_ranges, + }) }) .collect(), ))