Rework project diagnostics to prevent showing inconsistent state (#10922)

For a long time, we've had problems where diagnostics can end up showing
up inconsistently in different views. This PR is my attempt to prevent
that, and to simplify the system in the process. There are some UX
changes.

Diagnostic behaviors that have *not* changed:

* In-buffer diagnostics update immediately when LSPs send diagnostics
updates.
* The diagnostic counts in the status bar indicator also update
immediately.

Diagnostic behaviors that this PR changes:

* [x] The tab title for the project diagnostics view now simply shows
the same counts as the status bar indicator - the project's current
totals. Previously, this tab title showed something slightly different -
the numbers of diagnostics *currently shown* in the diagnostics view's
excerpts. But it was pretty confusing that you could sometimes see two
different diagnostic counts.
* [x] The project diagnostics view **never** updates its excerpts while
the user might be in the middle of typing it that view, unless the user
expressed an intent for the excerpts to update (by e.g. saving the
buffer). This was the behavior we originally implemented, but has
changed a few times since then, in attempts to fix other issues. I've
restored that invariant.

    Times when the excerpts will update:
     * diagnostics are updated while the diagnostics view is not focused
     * the user changes focus away from the diagnostics view
* the language server sends a `work done progress end` message for its
disk-based diagnostics token (i.e. cargo check finishes)
* the user saves a buffer associated with a language server, and then a
debounce timer expires

* [x] The project diagnostics view indicates when its diagnostics are
stale. States:
* when diagnostics have been updated while the diagnostics view was
focused:
        * the indicator shows a 'refresh' icon
        * clicking the indicator updates the excerpts
* when diagnostics have been updated, but a file has been saved, so that
the diagnostics will soon update, the indicator is disabled

With these UX changes, the only 'complex' part of the our diagnostics
presentation is the Project Diagnostics view's excerpt management,
because it needs to implement the deferred updates in order to avoid
disrupting the user while they may be typing. I want to take some steps
to reduce the potential for bugs in this view.

* [x] Reduce the amount of state that the view uses, and simplify its
implementation
* [x] Add a randomized test that checks the invariant that a mutated
diagnostics view matches a freshly computed diagnostics view


##  Release Notes

- Reworked the project diagnostics view:
- Fixed an issue where the project diagnostics view could update its
excerpts while you were typing in it.
- Fixed bugs where the project diagnostics view could show the wrong
excerpts.
- Changed the diagnostics view to always update its excerpts eagerly
when not focused.
- Added an indicator to the project diagnostics view's toolbar, showing
when diagnostics have been changed.

---------

Co-authored-by: Richard Feldman <oss@rtfeldman.com>

Cargo.lock

@@ -3181,13 +3181,17 @@ dependencies = [
  "anyhow",
  "client",
  "collections",
+ "ctor",
  "editor",
+ "env_logger",
  "futures 0.3.28",
  "gpui",
  "language",
  "log",
  "lsp",
+ "pretty_assertions",
  "project",
+ "rand 0.8.5",
  "schemars",
  "serde",
  "serde_json",

crates/diagnostics/Cargo.toml

@@ -15,13 +15,16 @@ doctest = false
 [dependencies]
 anyhow.workspace = true
 collections.workspace = true
+ctor.workspace = true
 editor.workspace = true
+env_logger.workspace = true
 futures.workspace = true
 gpui.workspace = true
 language.workspace = true
 log.workspace = true
 lsp.workspace = true
 project.workspace = true
+rand.workspace = true
 schemars.workspace = true
 serde.workspace = true
 settings.workspace = true
@@ -40,3 +43,4 @@ serde_json.workspace = true
 theme = { workspace = true, features = ["test-support"] }
 unindent.workspace = true
 workspace = { workspace = true, features = ["test-support"] }
+pretty_assertions.workspace = true

crates/diagnostics/src/items.rs

@@ -1,13 +1,11 @@
 use std::time::Duration;
 
-use collections::HashSet;
 use editor::Editor;
 use gpui::{
     percentage, rems, Animation, AnimationExt, EventEmitter, IntoElement, ParentElement, Render,
     Styled, Subscription, Transformation, View, ViewContext, WeakView,
 };
 use language::Diagnostic;
-use lsp::LanguageServerId;
 use ui::{h_flex, prelude::*, Button, ButtonLike, Color, Icon, IconName, Label, Tooltip};
 use workspace::{item::ItemHandle, StatusItemView, ToolbarItemEvent, Workspace};
 
@@ -18,7 +16,6 @@ pub struct DiagnosticIndicator {
     active_editor: Option<WeakView<Editor>>,
     workspace: WeakView<Workspace>,
     current_diagnostic: Option<Diagnostic>,
-    in_progress_checks: HashSet<LanguageServerId>,
     _observe_active_editor: Option<Subscription>,
 }
 
@@ -64,7 +61,20 @@ impl Render for DiagnosticIndicator {
                 .child(Label::new(warning_count.to_string()).size(LabelSize::Small)),
         };
 
-        let status = if !self.in_progress_checks.is_empty() {
+        let has_in_progress_checks = self
+            .workspace
+            .upgrade()
+            .and_then(|workspace| {
+                workspace
+                    .read(cx)
+                    .project()
+                    .read(cx)
+                    .language_servers_running_disk_based_diagnostics()
+                    .next()
+            })
+            .is_some();
+
+        let status = if has_in_progress_checks {
             Some(
                 h_flex()
                     .gap_2()
@@ -126,15 +136,13 @@ impl DiagnosticIndicator {
     pub fn new(workspace: &Workspace, cx: &mut ViewContext<Self>) -> Self {
         let project = workspace.project();
         cx.subscribe(project, |this, project, event, cx| match event {
-            project::Event::DiskBasedDiagnosticsStarted { language_server_id } => {
+            project::Event::DiskBasedDiagnosticsStarted { .. } => {
-                this.in_progress_checks.insert(*language_server_id);
                 cx.notify();
             }
 
-            project::Event::DiskBasedDiagnosticsFinished { language_server_id }
+            project::Event::DiskBasedDiagnosticsFinished { .. }
-            | project::Event::LanguageServerRemoved(language_server_id) => {
+            | project::Event::LanguageServerRemoved(_) => {
                 this.summary = project.read(cx).diagnostic_summary(false, cx);
-                this.in_progress_checks.remove(language_server_id);
                 cx.notify();
             }
 
@@ -149,10 +157,6 @@ impl DiagnosticIndicator {
 
         Self {
             summary: project.read(cx).diagnostic_summary(false, cx),
-            in_progress_checks: project
-                .read(cx)
-                .language_servers_running_disk_based_diagnostics()
-                .collect(),
             active_editor: None,
             workspace: workspace.weak_handle(),
             current_diagnostic: None,

crates/diagnostics/src/toolbar_controls.rs

@@ -1,5 +1,5 @@
 use crate::ProjectDiagnosticsEditor;
-use gpui::{div, EventEmitter, ParentElement, Render, ViewContext, WeakView};
+use gpui::{EventEmitter, ParentElement, Render, ViewContext, WeakView};
 use ui::prelude::*;
 use ui::{IconButton, IconName, Tooltip};
 use workspace::{item::ItemHandle, ToolbarItemEvent, ToolbarItemLocation, ToolbarItemView};
@@ -10,12 +10,23 @@ pub struct ToolbarControls {
 
 impl Render for ToolbarControls {
     fn render(&mut self, cx: &mut ViewContext<Self>) -> impl IntoElement {
-        let include_warnings = self
+        let mut include_warnings = false;
-            .editor
+        let mut has_stale_excerpts = false;
-            .as_ref()
+        let mut is_updating = false;
-            .and_then(|editor| editor.upgrade())
+
-            .map(|editor| editor.read(cx).include_warnings)
+        if let Some(editor) = self.editor.as_ref().and_then(|editor| editor.upgrade()) {
-            .unwrap_or(false);
+            let editor = editor.read(cx);
+
+            include_warnings = editor.include_warnings;
+            has_stale_excerpts = !editor.paths_to_update.is_empty();
+            is_updating = editor.update_paths_tx.len() > 0
+                || editor
+                    .project
+                    .read(cx)
+                    .language_servers_running_disk_based_diagnostics()
+                    .next()
+                    .is_some();
+        }
 
         let tooltip = if include_warnings {
             "Exclude Warnings"
@@ -23,11 +34,31 @@ impl Render for ToolbarControls {
             "Include Warnings"
         };
 
-        div().child(
+        h_flex()
+            .when(has_stale_excerpts, |div| {
+                div.child(
+                    IconButton::new("update-excerpts", IconName::Update)
+                        .icon_color(Color::Info)
+                        .disabled(is_updating)
+                        .tooltip(move |cx| Tooltip::text("Update excerpts", cx))
+                        .on_click(cx.listener(|this, _, cx| {
+                            if let Some(editor) =
+                                this.editor.as_ref().and_then(|editor| editor.upgrade())
+                            {
+                                editor.update(cx, |editor, _| {
+                                    editor.enqueue_update_stale_excerpts(None);
+                                });
+                            }
+                        })),
+                )
+            })
+            .child(
                 IconButton::new("toggle-warnings", IconName::ExclamationTriangle)
                     .tooltip(move |cx| Tooltip::text(tooltip, cx))
                     .on_click(cx.listener(|this, _, cx| {
-                    if let Some(editor) = this.editor.as_ref().and_then(|editor| editor.upgrade()) {
+                        if let Some(editor) =
+                            this.editor.as_ref().and_then(|editor| editor.upgrade())
+                        {
                             editor.update(cx, |editor, cx| {
                                 editor.toggle_warnings(&Default::default(), cx);
                             });

crates/fs/src/fs.rs

@@ -714,6 +714,15 @@ impl FakeFs {
         Ok(())
     }
 
+    pub fn read_file_sync(&self, path: impl AsRef<Path>) -> Result<Vec<u8>> {
+        let path = path.as_ref();
+        let path = normalize_path(path);
+        let state = self.state.lock();
+        let entry = state.read_path(&path)?;
+        let entry = entry.lock();
+        entry.file_content(&path).cloned()
+    }
+
     async fn load_internal(&self, path: impl AsRef<Path>) -> Result<Vec<u8>> {
         let path = path.as_ref();
         let path = normalize_path(path);

crates/project/src/project.rs

@@ -2699,7 +2699,6 @@ impl Project {
 
                 for (_, _, server) in self.language_servers_for_worktree(worktree_id) {
                     let text = include_text(server.as_ref()).then(|| buffer.read(cx).text());
-
                     server
                         .notify::<lsp::notification::DidSaveTextDocument>(
                             lsp::DidSaveTextDocumentParams {
@@ -2710,46 +2709,8 @@ impl Project {
                         .log_err();
                 }
 
-                let language_server_ids = self.language_server_ids_for_buffer(buffer.read(cx), cx);
+                for language_server_id in self.language_server_ids_for_buffer(buffer.read(cx), cx) {
-                for language_server_id in language_server_ids {
+                    self.simulate_disk_based_diagnostics_events_if_needed(language_server_id, cx);
-                    if let Some(LanguageServerState::Running {
-                        adapter,
-                        simulate_disk_based_diagnostics_completion,
-                        ..
-                    }) = self.language_servers.get_mut(&language_server_id)
-                    {
-                        // After saving a buffer using a language server that doesn't provide
-                        // a disk-based progress token, kick off a timer that will reset every
-                        // time the buffer is saved. If the timer eventually fires, simulate
-                        // disk-based diagnostics being finished so that other pieces of UI
-                        // (e.g., project diagnostics view, diagnostic status bar) can update.
-                        // We don't emit an event right away because the language server might take
-                        // some time to publish diagnostics.
-                        if adapter.disk_based_diagnostics_progress_token.is_none() {
-                            const DISK_BASED_DIAGNOSTICS_DEBOUNCE: Duration =
-                                Duration::from_secs(1);
-
-                            let task = cx.spawn(move |this, mut cx| async move {
-                                cx.background_executor().timer(DISK_BASED_DIAGNOSTICS_DEBOUNCE).await;
-                                if let Some(this) = this.upgrade() {
-                                    this.update(&mut cx, |this, cx| {
-                                        this.disk_based_diagnostics_finished(
-                                            language_server_id,
-                                            cx,
-                                        );
-                                        this.enqueue_buffer_ordered_message(
-                                                BufferOrderedMessage::LanguageServerUpdate {
-                                                    language_server_id,
-                                                    message:proto::update_language_server::Variant::DiskBasedDiagnosticsUpdated(Default::default())
-                                                },
-                                            )
-                                            .ok();
-                                    }).ok();
-                                }
-                            });
-                            *simulate_disk_based_diagnostics_completion = Some(task);
-                        }
-                    }
                 }
             }
             BufferEvent::FileHandleChanged => {
@@ -2783,6 +2744,57 @@ impl Project {
         None
     }
 
+    // After saving a buffer using a language server that doesn't provide a disk-based progress token,
+    // kick off a timer that will reset every time the buffer is saved. If the timer eventually fires,
+    // simulate disk-based diagnostics being finished so that other pieces of UI (e.g., project
+    // diagnostics view, diagnostic status bar) can update. We don't emit an event right away because
+    // the language server might take some time to publish diagnostics.
+    fn simulate_disk_based_diagnostics_events_if_needed(
+        &mut self,
+        language_server_id: LanguageServerId,
+        cx: &mut ModelContext<Self>,
+    ) {
+        const DISK_BASED_DIAGNOSTICS_DEBOUNCE: Duration = Duration::from_secs(1);
+
+        let Some(LanguageServerState::Running {
+            simulate_disk_based_diagnostics_completion,
+            adapter,
+            ..
+        }) = self.language_servers.get_mut(&language_server_id)
+        else {
+            return;
+        };
+
+        if adapter.disk_based_diagnostics_progress_token.is_some() {
+            return;
+        }
+
+        let prev_task = simulate_disk_based_diagnostics_completion.replace(cx.spawn(
+            move |this, mut cx| async move {
+                cx.background_executor()
+                    .timer(DISK_BASED_DIAGNOSTICS_DEBOUNCE)
+                    .await;
+
+                this.update(&mut cx, |this, cx| {
+                    this.disk_based_diagnostics_finished(language_server_id, cx);
+
+                    if let Some(LanguageServerState::Running {
+                        simulate_disk_based_diagnostics_completion,
+                        ..
+                    }) = this.language_servers.get_mut(&language_server_id)
+                    {
+                        *simulate_disk_based_diagnostics_completion = None;
+                    }
+                })
+                .ok();
+            },
+        ));
+
+        if prev_task.is_none() {
+            self.disk_based_diagnostics_started(language_server_id, cx);
+        }
+    }
+
     fn request_buffer_diff_recalculation(
         &mut self,
         buffer: &Model<Buffer>,
@@ -4041,13 +4053,7 @@ impl Project {
         match progress {
             lsp::WorkDoneProgress::Begin(report) => {
                 if is_disk_based_diagnostics_progress {
-                    language_server_status.has_pending_diagnostic_updates = true;
                     self.disk_based_diagnostics_started(language_server_id, cx);
-                    self.enqueue_buffer_ordered_message(BufferOrderedMessage::LanguageServerUpdate {
-                            language_server_id,
-                            message: proto::update_language_server::Variant::DiskBasedDiagnosticsUpdating(Default::default())
-                        })
-                        .ok();
                 } else {
                     self.on_lsp_work_start(
                         language_server_id,
@@ -4092,18 +4098,7 @@ impl Project {
                 language_server_status.progress_tokens.remove(&token);
 
                 if is_disk_based_diagnostics_progress {
-                    language_server_status.has_pending_diagnostic_updates = false;
                     self.disk_based_diagnostics_finished(language_server_id, cx);
-                    self.enqueue_buffer_ordered_message(
-                        BufferOrderedMessage::LanguageServerUpdate {
-                            language_server_id,
-                            message:
-                                proto::update_language_server::Variant::DiskBasedDiagnosticsUpdated(
-                                    Default::default(),
-                                ),
-                        },
-                    )
-                    .ok();
                 } else {
                     self.on_lsp_work_end(language_server_id, token.clone(), cx);
                 }
@@ -7708,13 +7703,7 @@ impl Project {
 
     pub fn diagnostic_summary(&self, include_ignored: bool, cx: &AppContext) -> DiagnosticSummary {
         let mut summary = DiagnosticSummary::default();
-        for (_, _, path_summary) in
+        for (_, _, path_summary) in self.diagnostic_summaries(include_ignored, cx) {
-            self.diagnostic_summaries(include_ignored, cx)
-                .filter(|(path, _, _)| {
-                    let worktree = self.entry_for_path(path, cx).map(|entry| entry.is_ignored);
-                    include_ignored || worktree == Some(false)
-                })
-        {
             summary.error_count += path_summary.error_count;
             summary.warning_count += path_summary.warning_count;
         }
@@ -7726,20 +7715,23 @@ impl Project {
         include_ignored: bool,
         cx: &'a AppContext,
     ) -> impl Iterator<Item = (ProjectPath, LanguageServerId, DiagnosticSummary)> + 'a {
-        self.visible_worktrees(cx)
+        self.visible_worktrees(cx).flat_map(move |worktree| {
-            .flat_map(move |worktree| {
             let worktree = worktree.read(cx);
             let worktree_id = worktree.id();
             worktree
                 .diagnostic_summaries()
-                    .map(move |(path, server_id, summary)| {
+                .filter_map(move |(path, server_id, summary)| {
-                        (ProjectPath { worktree_id, path }, server_id, summary)
+                    if include_ignored
+                        || worktree
+                            .entry_for_path(path.as_ref())
+                            .map_or(false, |entry| !entry.is_ignored)
+                    {
+                        Some((ProjectPath { worktree_id, path }, server_id, summary))
+                    } else {
+                        None
+                    }
                 })
         })
-            .filter(move |(path, _, _)| {
-                let worktree = self.entry_for_path(path, cx).map(|entry| entry.is_ignored);
-                include_ignored || worktree == Some(false)
-            })
     }
 
     pub fn disk_based_diagnostics_started(
@@ -7747,7 +7739,22 @@ impl Project {
         language_server_id: LanguageServerId,
         cx: &mut ModelContext<Self>,
     ) {
+        if let Some(language_server_status) =
+            self.language_server_statuses.get_mut(&language_server_id)
+        {
+            language_server_status.has_pending_diagnostic_updates = true;
+        }
+
         cx.emit(Event::DiskBasedDiagnosticsStarted { language_server_id });
+        if self.is_local() {
+            self.enqueue_buffer_ordered_message(BufferOrderedMessage::LanguageServerUpdate {
+                language_server_id,
+                message: proto::update_language_server::Variant::DiskBasedDiagnosticsUpdating(
+                    Default::default(),
+                ),
+            })
+            .ok();
+        }
     }
 
     pub fn disk_based_diagnostics_finished(
@@ -7755,7 +7762,23 @@ impl Project {
         language_server_id: LanguageServerId,
         cx: &mut ModelContext<Self>,
     ) {
+        if let Some(language_server_status) =
+            self.language_server_statuses.get_mut(&language_server_id)
+        {
+            language_server_status.has_pending_diagnostic_updates = false;
+        }
+
         cx.emit(Event::DiskBasedDiagnosticsFinished { language_server_id });
+
+        if self.is_local() {
+            self.enqueue_buffer_ordered_message(BufferOrderedMessage::LanguageServerUpdate {
+                language_server_id,
+                message: proto::update_language_server::Variant::DiskBasedDiagnosticsUpdated(
+                    Default::default(),
+                ),
+            })
+            .ok();
+        }
     }
 
     pub fn active_entry(&self) -> Option<ProjectEntryId> {