diagnostics: Keep diagnostic excerpt ranges properly ordered (#42298)

Lukas Wirth created

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 ...

Change summary

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(-)

Detailed changes

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<ExcerptRange<Point>> = Vec::new();
+            let mut excerpt_ranges: Vec<ExcerptRange<_>> = 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<text::Anchor>| {
+                    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(),

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,

crates/diagnostics/src/diagnostics.rs 🔗

@@ -498,7 +498,7 @@ impl ProjectDiagnosticsEditor {
         cx: &mut Context<Self>,
     ) -> Task<Result<()>> {
         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<DiagnosticBlock> = 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<ExcerptRange<Point>> = this.update(cx, |this, cx| {
+            let mut excerpt_ranges: Vec<ExcerptRange<_>> = 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<text::Anchor>| {
+                    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<Point> {
-    if let Some(rows) = heuristic_syntactic_expand(
+) -> Range<text::Anchor> {
+    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

crates/multi_buffer/src/multi_buffer.rs 🔗

@@ -1148,9 +1148,9 @@ impl MultiBuffer {
         let mut counts: Vec<usize> = 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

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::<Vec<_>>())))

+            .filter_map(|(k, v)| Some((k?, v.into_iter().collect::<Vec<_>>())))

             .collect::<Vec<_>>();
         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

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",
         );