From b7d4d1791a873af87340ba204ce58bfc3472986d Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Sun, 9 Nov 2025 11:55:56 +0100 Subject: [PATCH] diagnostics: Keep diagnostic excerpt ranges properly ordered (#42298) Fixes ZED-2CQ We were doing the binary search by buffer points, but due to await points within this function we could end up mixing points of differing buffer versions. Release Notes: - N/A *or* Added/Fixed/Improved ... --- crates/diagnostics/src/buffer_diagnostics.rs | 54 +++++++++----- crates/diagnostics/src/diagnostic_renderer.rs | 21 ++---- crates/diagnostics/src/diagnostics.rs | 74 ++++++++++++------- crates/multi_buffer/src/multi_buffer.rs | 4 +- crates/multi_buffer/src/path_key.rs | 18 ++--- crates/sum_tree/src/cursor.rs | 2 +- 6 files changed, 103 insertions(+), 70 deletions(-) diff --git a/crates/diagnostics/src/buffer_diagnostics.rs b/crates/diagnostics/src/buffer_diagnostics.rs index 8fe503a706027fb6ed2f0b9114450eb79c2aa027..01626ddfd2a3f1a4773b2e88a9b8ff001b46680a 100644 --- a/crates/diagnostics/src/buffer_diagnostics.rs +++ b/crates/diagnostics/src/buffer_diagnostics.rs @@ -23,7 +23,7 @@ use project::{ use settings::Settings; use std::{ any::{Any, TypeId}, - cmp::Ordering, + cmp::{self, Ordering}, sync::Arc, }; use text::{Anchor, BufferSnapshot, OffsetRangeExt}; @@ -410,7 +410,7 @@ impl BufferDiagnosticsEditor { // in the editor. // This is done by iterating over the list of diagnostic blocks and // determine what range does the diagnostic block span. - let mut excerpt_ranges: Vec> = Vec::new(); + let mut excerpt_ranges: Vec> = Vec::new(); for diagnostic_block in blocks.iter() { let excerpt_range = context_range_for_entry( @@ -420,30 +420,43 @@ impl BufferDiagnosticsEditor { &mut cx, ) .await; + let initial_range = buffer_snapshot + .anchor_after(diagnostic_block.initial_range.start) + ..buffer_snapshot.anchor_before(diagnostic_block.initial_range.end); - let index = excerpt_ranges - .binary_search_by(|probe| { + let bin_search = |probe: &ExcerptRange| { + let context_start = || { probe .context .start - .cmp(&excerpt_range.start) - .then(probe.context.end.cmp(&excerpt_range.end)) - .then( - probe - .primary - .start - .cmp(&diagnostic_block.initial_range.start), - ) - .then(probe.primary.end.cmp(&diagnostic_block.initial_range.end)) - .then(Ordering::Greater) - }) - .unwrap_or_else(|index| index); + .cmp(&excerpt_range.start, &buffer_snapshot) + }; + let context_end = + || probe.context.end.cmp(&excerpt_range.end, &buffer_snapshot); + let primary_start = || { + probe + .primary + .start + .cmp(&initial_range.start, &buffer_snapshot) + }; + let primary_end = + || probe.primary.end.cmp(&initial_range.end, &buffer_snapshot); + context_start() + .then_with(context_end) + .then_with(primary_start) + .then_with(primary_end) + .then(cmp::Ordering::Greater) + }; + + let index = excerpt_ranges + .binary_search_by(bin_search) + .unwrap_or_else(|i| i); excerpt_ranges.insert( index, ExcerptRange { context: excerpt_range, - primary: diagnostic_block.initial_range.clone(), + primary: initial_range, }, ) } @@ -466,6 +479,13 @@ impl BufferDiagnosticsEditor { buffer_diagnostics_editor .multibuffer .update(cx, |multibuffer, cx| { + let excerpt_ranges = excerpt_ranges + .into_iter() + .map(|range| ExcerptRange { + context: range.context.to_point(&buffer_snapshot), + primary: range.primary.to_point(&buffer_snapshot), + }) + .collect(); multibuffer.set_excerpt_ranges_for_path( PathKey::for_buffer(&buffer, cx), buffer.clone(), diff --git a/crates/diagnostics/src/diagnostic_renderer.rs b/crates/diagnostics/src/diagnostic_renderer.rs index 5eda81faf97878605142a8bf0832b9082dbc414c..6204bf4b52ddb903773beac28627d53c3cce7765 100644 --- a/crates/diagnostics/src/diagnostic_renderer.rs +++ b/crates/diagnostics/src/diagnostic_renderer.rs @@ -39,8 +39,8 @@ impl DiagnosticRenderer { let group_id = primary.diagnostic.group_id; let mut results = vec![]; for entry in diagnostic_group.iter() { + let mut markdown = Self::markdown(&entry.diagnostic); if entry.diagnostic.is_primary { - let mut markdown = Self::markdown(&entry.diagnostic); let diagnostic = &primary.diagnostic; if diagnostic.source.is_some() || diagnostic.code.is_some() { markdown.push_str(" ("); @@ -81,21 +81,12 @@ impl DiagnosticRenderer { diagnostics_editor: diagnostics_editor.clone(), markdown: cx.new(|cx| Markdown::new(markdown.into(), None, None, cx)), }); - } else if entry.range.start.row.abs_diff(primary.range.start.row) < 5 { - let markdown = Self::markdown(&entry.diagnostic); - - results.push(DiagnosticBlock { - initial_range: entry.range.clone(), - severity: entry.diagnostic.severity, - diagnostics_editor: diagnostics_editor.clone(), - markdown: cx.new(|cx| Markdown::new(markdown.into(), None, None, cx)), - }); } else { - let mut markdown = Self::markdown(&entry.diagnostic); - markdown.push_str(&format!( - " ([back](file://#diagnostic-{buffer_id}-{group_id}-{primary_ix}))" - )); - + if entry.range.start.row.abs_diff(primary.range.start.row) >= 5 { + markdown.push_str(&format!( + " ([back](file://#diagnostic-{buffer_id}-{group_id}-{primary_ix}))" + )); + } results.push(DiagnosticBlock { initial_range: entry.range.clone(), severity: entry.diagnostic.severity, diff --git a/crates/diagnostics/src/diagnostics.rs b/crates/diagnostics/src/diagnostics.rs index eabb5b788f25e9308d5352513a4357172959d8eb..c2caefe3f388e12fe91931060fe8980908157e48 100644 --- a/crates/diagnostics/src/diagnostics.rs +++ b/crates/diagnostics/src/diagnostics.rs @@ -498,7 +498,7 @@ impl ProjectDiagnosticsEditor { cx: &mut Context, ) -> Task> { let was_empty = self.multibuffer.read(cx).is_empty(); - let buffer_snapshot = buffer.read(cx).snapshot(); + let mut buffer_snapshot = buffer.read(cx).snapshot(); let buffer_id = buffer_snapshot.remote_id(); let max_severity = if self.include_warnings { @@ -546,6 +546,7 @@ impl ProjectDiagnosticsEditor { } let mut blocks: Vec = Vec::new(); + let diagnostics_toolbar_editor = Arc::new(this.clone()); for (_, group) in grouped { let group_severity = group.iter().map(|d| d.diagnostic.severity).min(); if group_severity.is_none_or(|s| s > max_severity) { @@ -555,7 +556,7 @@ impl ProjectDiagnosticsEditor { crate::diagnostic_renderer::DiagnosticRenderer::diagnostic_blocks_for_group( group, buffer_snapshot.remote_id(), - Some(Arc::new(this.clone())), + Some(diagnostics_toolbar_editor.clone()), cx, ) })?; @@ -563,7 +564,7 @@ impl ProjectDiagnosticsEditor { blocks.extend(more); } - let mut excerpt_ranges: Vec> = this.update(cx, |this, cx| { + let mut excerpt_ranges: Vec> = this.update(cx, |this, cx| { this.multibuffer.update(cx, |multi_buffer, cx| { let is_dirty = multi_buffer .buffer(buffer_id) @@ -573,10 +574,7 @@ impl ProjectDiagnosticsEditor { RetainExcerpts::All | RetainExcerpts::Dirty => multi_buffer .excerpts_for_buffer(buffer_id, cx) .into_iter() - .map(|(_, range)| ExcerptRange { - context: range.context.to_point(&buffer_snapshot), - primary: range.primary.to_point(&buffer_snapshot), - }) + .map(|(_, range)| range) .collect(), } }) @@ -591,24 +589,41 @@ impl ProjectDiagnosticsEditor { cx, ) .await; + buffer_snapshot = cx.update(|_, cx| buffer.read(cx).snapshot())?; + let initial_range = buffer_snapshot.anchor_after(b.initial_range.start) + ..buffer_snapshot.anchor_before(b.initial_range.end); - let i = excerpt_ranges - .binary_search_by(|probe| { + let bin_search = |probe: &ExcerptRange| { + let context_start = || { probe .context .start - .cmp(&excerpt_range.start) - .then(probe.context.end.cmp(&excerpt_range.end)) - .then(probe.primary.start.cmp(&b.initial_range.start)) - .then(probe.primary.end.cmp(&b.initial_range.end)) - .then(cmp::Ordering::Greater) - }) + .cmp(&excerpt_range.start, &buffer_snapshot) + }; + let context_end = + || probe.context.end.cmp(&excerpt_range.end, &buffer_snapshot); + let primary_start = || { + probe + .primary + .start + .cmp(&initial_range.start, &buffer_snapshot) + }; + let primary_end = + || probe.primary.end.cmp(&initial_range.end, &buffer_snapshot); + context_start() + .then_with(context_end) + .then_with(primary_start) + .then_with(primary_end) + .then(cmp::Ordering::Greater) + }; + let i = excerpt_ranges + .binary_search_by(bin_search) .unwrap_or_else(|i| i); excerpt_ranges.insert( i, ExcerptRange { context: excerpt_range, - primary: b.initial_range.clone(), + primary: initial_range, }, ); result_blocks.insert(i, Some(b)); @@ -623,6 +638,13 @@ impl ProjectDiagnosticsEditor { }) } let (anchor_ranges, _) = this.multibuffer.update(cx, |multi_buffer, cx| { + let excerpt_ranges = excerpt_ranges + .into_iter() + .map(|range| ExcerptRange { + context: range.context.to_point(&buffer_snapshot), + primary: range.primary.to_point(&buffer_snapshot), + }) + .collect(); multi_buffer.set_excerpt_ranges_for_path( PathKey::for_buffer(&buffer, cx), buffer.clone(), @@ -968,8 +990,8 @@ async fn context_range_for_entry( context: u32, snapshot: BufferSnapshot, cx: &mut AsyncApp, -) -> Range { - if let Some(rows) = heuristic_syntactic_expand( +) -> Range { + let range = if let Some(rows) = heuristic_syntactic_expand( range.clone(), DIAGNOSTIC_EXPANSION_ROW_LIMIT, snapshot.clone(), @@ -977,15 +999,17 @@ async fn context_range_for_entry( ) .await { - return Range { + Range { start: Point::new(*rows.start(), 0), end: snapshot.clip_point(Point::new(*rows.end(), u32::MAX), Bias::Left), - }; - } - Range { - start: Point::new(range.start.row.saturating_sub(context), 0), - end: snapshot.clip_point(Point::new(range.end.row + context, u32::MAX), Bias::Left), - } + } + } else { + Range { + start: Point::new(range.start.row.saturating_sub(context), 0), + end: snapshot.clip_point(Point::new(range.end.row + context, u32::MAX), Bias::Left), + } + }; + snapshot.anchor_after(range.start)..snapshot.anchor_before(range.end) } /// Expands the input range using syntax information from TreeSitter. This expansion will be limited diff --git a/crates/multi_buffer/src/multi_buffer.rs b/crates/multi_buffer/src/multi_buffer.rs index 90f1bcbe39468fcfa390ce8175414451ddb3b2c7..5be61d1efe153fcd6902b33e46f2f5b84d4055e6 100644 --- a/crates/multi_buffer/src/multi_buffer.rs +++ b/crates/multi_buffer/src/multi_buffer.rs @@ -1148,9 +1148,9 @@ impl MultiBuffer { let mut counts: Vec = Vec::new(); for range in expanded_ranges { if let Some(last_range) = merged_ranges.last_mut() { - debug_assert!( + assert!( last_range.context.start <= range.context.start, - "Last range: {last_range:?} Range: {range:?}" + "ranges must be sorted: {last_range:?} <= {range:?}" ); if last_range.context.end >= range.context.start || last_range.context.end.row + 1 == range.context.start.row diff --git a/crates/multi_buffer/src/path_key.rs b/crates/multi_buffer/src/path_key.rs index c750bb912f0c2767e4c56890d0ab75046c094e71..09d098ea0727906500b37acd8f694f569fea75e2 100644 --- a/crates/multi_buffer/src/path_key.rs +++ b/crates/multi_buffer/src/path_key.rs @@ -172,7 +172,7 @@ impl MultiBuffer { .into_iter() .chunk_by(|id| self.paths_by_excerpt.get(id).cloned()) .into_iter() - .flat_map(|(k, v)| Some((k?, v.into_iter().collect::>()))) + .filter_map(|(k, v)| Some((k?, v.into_iter().collect::>()))) .collect::>(); let snapshot = self.snapshot(cx); @@ -280,7 +280,7 @@ impl MultiBuffer { .excerpts_by_path .range(..path.clone()) .next_back() - .map(|(_, value)| *value.last().unwrap()) + .and_then(|(_, value)| value.last().copied()) .unwrap_or(ExcerptId::min()); let existing = self @@ -299,6 +299,7 @@ impl MultiBuffer { let snapshot = self.snapshot(cx); let mut next_excerpt_id = + // is this right? What if we remove the last excerpt, then we might reallocate with a wrong mapping? if let Some(last_entry) = self.snapshot.borrow().excerpt_ids.last() { last_entry.id.0 + 1 } else { @@ -311,20 +312,16 @@ impl MultiBuffer { excerpts_cursor.next(); loop { - let new = new_iter.peek(); - let existing = if let Some(existing_id) = existing_iter.peek() { - let locator = snapshot.excerpt_locator_for_id(*existing_id); + let existing = if let Some(&existing_id) = existing_iter.peek() { + let locator = snapshot.excerpt_locator_for_id(existing_id); excerpts_cursor.seek_forward(&Some(locator), Bias::Left); if let Some(excerpt) = excerpts_cursor.item() { if excerpt.buffer_id != buffer_snapshot.remote_id() { - to_remove.push(*existing_id); + to_remove.push(existing_id); existing_iter.next(); continue; } - Some(( - *existing_id, - excerpt.range.context.to_point(buffer_snapshot), - )) + Some((existing_id, excerpt.range.context.to_point(buffer_snapshot))) } else { None } @@ -332,6 +329,7 @@ impl MultiBuffer { None }; + let new = new_iter.peek(); if let Some((last_id, last)) = to_insert.last_mut() { if let Some(new) = new && last.context.end >= new.context.start diff --git a/crates/sum_tree/src/cursor.rs b/crates/sum_tree/src/cursor.rs index 7418224c86f51a52a8a621da0f2a0c53dcfcf404..f37eae4f4becaf94c578b233d18e4bff8169252d 100644 --- a/crates/sum_tree/src/cursor.rs +++ b/crates/sum_tree/src/cursor.rs @@ -448,7 +448,7 @@ where aggregate: &mut dyn SeekAggregate<'a, T>, ) -> bool { assert!( - target.cmp(&self.position, self.cx) >= Ordering::Equal, + target.cmp(&self.position, self.cx).is_ge(), "cannot seek backward", );