editor: Support walking through overlapping diagnostics (#11139)

Kalle Ahlström created 2 years ago

While looking into how to implement #4901, noticed that the current
`Goto next/previous diagnostic` behaved a bit weirdly. That is, when
there are multiple errors that have overlapping ranges, only the first
one can be chosen to be active by the `go_to_diagnostic_impl`.

### Previous behavior:


https://github.com/zed-industries/zed/assets/71292737/95897675-f5ee-40e5-869f-0a40066eb8e3

Doesn't go through all the diagnostics, and going backwards and forwards
doesn't show the same diagnostic always.

### New behavior:


https://github.com/zed-industries/zed/assets/71292737/81f7945a-7ad8-4a34-b286-cc2799b10500

Should always go through the diagnostics in a consistent manner.

Release Notes:
* Improved the behavioral consistency of "Go to Next/Previous
Diagnostic"

Change summary

crates/editor/src/editor.rs   | 44 +++++++++++++++++++++++++-----------
crates/language/src/buffer.rs | 16 ++++++++++++
2 files changed, 45 insertions(+), 15 deletions(-)

Detailed changes

crates/editor/src/editor.rs

@@ -1447,6 +1447,7 @@ impl CodeActionsMenu {
 struct ActiveDiagnosticGroup {
     primary_range: Range<Anchor>,
     primary_message: String,
+    group_id: usize,
     blocks: HashMap<BlockId, Diagnostic>,
     is_valid: bool,
 }
@@ -8015,7 +8016,7 @@ impl Editor {
         });
         let mut search_start = if let Some(active_primary_range) = active_primary_range.as_ref() {
             if active_primary_range.contains(&selection.head()) {
-                *active_primary_range.end()
+                *active_primary_range.start()
             } else {
                 selection.head()
             }
@@ -8024,24 +8025,38 @@ impl Editor {
         };
         let snapshot = self.snapshot(cx);
         loop {
-            let mut diagnostics = if direction == Direction::Prev {
+            let diagnostics = if direction == Direction::Prev {
                 buffer.diagnostics_in_range::<_, usize>(0..search_start, true)
             } else {
                 buffer.diagnostics_in_range::<_, usize>(search_start..buffer.len(), false)
             }
             .filter(|diagnostic| !snapshot.intersects_fold(diagnostic.range.start));
-            let group = diagnostics.find_map(|entry| {
-                if entry.diagnostic.is_primary
-                    && entry.diagnostic.severity <= DiagnosticSeverity::WARNING
-                    && !entry.range.is_empty()
-                    && Some(entry.range.end) != active_primary_range.as_ref().map(|r| *r.end())
-                    && !entry.range.contains(&search_start)
-                {
-                    Some((entry.range, entry.diagnostic.group_id))
-                } else {
-                    None
-                }
-            });
+            let group = diagnostics
+                // relies on diagnostics_in_range to return diagnostics with the same starting range to
+                // be sorted in a stable way
+                // skip until we are at current active diagnostic, if it exists
+                .skip_while(|entry| {
+                    (match direction {
+                        Direction::Prev => entry.range.start >= search_start,
+                        Direction::Next => entry.range.start <= search_start,
+                    }) && self
+                        .active_diagnostics
+                        .as_ref()
+                        .is_some_and(|a| a.group_id != entry.diagnostic.group_id)
+                })
+                .find_map(|entry| {
+                    if entry.diagnostic.is_primary
+                        && entry.diagnostic.severity <= DiagnosticSeverity::WARNING
+                        && !entry.range.is_empty()
+                        // if we match with the active diagnostic, skip it
+                        && Some(entry.diagnostic.group_id)
+                            != self.active_diagnostics.as_ref().map(|d| d.group_id)
+                    {
+                        Some((entry.range, entry.diagnostic.group_id))
+                    } else {
+                        None
+                    }
+                });
 
             if let Some((primary_range, group_id)) = group {
                 if self.activate_diagnostics(group_id, cx) {
@@ -9042,6 +9057,7 @@ impl Editor {
             Some(ActiveDiagnosticGroup {
                 primary_range,
                 primary_message,
+                group_id,
                 blocks,
                 is_valid: true,
             })

crates/language/src/buffer.rs

@@ -3159,7 +3159,21 @@ impl BufferSnapshot {
                 .iter_mut()
                 .enumerate()
                 .flat_map(|(ix, iter)| Some((ix, iter.peek()?)))
-                .min_by(|(_, a), (_, b)| a.range.start.cmp(&b.range.start))?;
+                .min_by(|(_, a), (_, b)| {
+                    let cmp = a
+                        .range
+                        .start
+                        .cmp(&b.range.start)
+                        // when range is equal, sort by diagnostic severity
+                        .then(a.diagnostic.severity.cmp(&b.diagnostic.severity))
+                        // and stabilize order with group_id
+                        .then(a.diagnostic.group_id.cmp(&b.diagnostic.group_id));
+                    if reversed {
+                        cmp.reverse()
+                    } else {
+                        cmp
+                    }
+                })?;
             iterators[next_ix].next()
         })
     }