Don't emit event when LSP reports consecutive empty diagnostics

Antonio Scandurra created 3 years ago

This is related to #849: in that pull request we avoided *storing*
empty diagnostics, but we'd still report an event when receiving
consecutive empty diagnostics. So if the project diagnostics editor
was open, it could happen that opening a buffer would cause the
language server to report zero diagnostics. We would therefore close
the buffer because there were no diagnostics, but doing so would cause
the LSP to report another event with zero diagnostics. This would repeat
forever, causing Zed to use a lot of CPU and the UI not to refresh properly.

With this commit we will simply avoid emitting a `DiagnosticsUpdated` event
altogether if no diagnostics were present before *and* the LSP is reporting
a `PublishDiagnostics` event with no diagnostics in it.

Change summary

crates/project/src/project.rs  | 33 ++++++++++++++++++++++---
crates/project/src/worktree.rs | 46 +++++++++++++++++++----------------
2 files changed, 54 insertions(+), 25 deletions(-)

Detailed changes

crates/project/src/project.rs

@@ -154,7 +154,7 @@ pub struct ProjectPath {
     pub path: Arc<Path>,
 }
 
-#[derive(Clone, Debug, Default, PartialEq, Serialize)]
+#[derive(Copy, Clone, Debug, Default, PartialEq, Serialize)]
 pub struct DiagnosticSummary {
     pub error_count: usize,
     pub warning_count: usize,
@@ -1961,13 +1961,15 @@ impl Project {
             self.update_buffer_diagnostics(&buffer, diagnostics.clone(), version, cx)?;
         }
 
-        worktree.update(cx, |worktree, cx| {
+        let updated = worktree.update(cx, |worktree, cx| {
             worktree
                 .as_local_mut()
                 .ok_or_else(|| anyhow!("not a local worktree"))?
                 .update_diagnostics(project_path.path.clone(), diagnostics, cx)
         })?;
-        cx.emit(Event::DiagnosticsUpdated(project_path));
+        if updated {
+            cx.emit(Event::DiagnosticsUpdated(project_path));
+        }
         Ok(())
     }
 
@@ -4879,7 +4881,7 @@ mod tests {
     };
     use lsp::Url;
     use serde_json::json;
-    use std::{cell::RefCell, os::unix, path::PathBuf, rc::Rc};
+    use std::{cell::RefCell, os::unix, path::PathBuf, rc::Rc, task::Poll};
     use unindent::Unindent as _;
     use util::{assert_set_eq, test::temp_tree};
     use worktree::WorktreeHandle as _;
@@ -5564,6 +5566,29 @@ mod tests {
                 }]
             )
         });
+
+        // Ensure publishing empty diagnostics twice only results in one update event.
+        fake_server.notify::<lsp::notification::PublishDiagnostics>(
+            lsp::PublishDiagnosticsParams {
+                uri: Url::from_file_path("/dir/a.rs").unwrap(),
+                version: None,
+                diagnostics: Default::default(),
+            },
+        );
+        assert_eq!(
+            events.next().await.unwrap(),
+            Event::DiagnosticsUpdated((worktree_id, Path::new("a.rs")).into())
+        );
+
+        fake_server.notify::<lsp::notification::PublishDiagnostics>(
+            lsp::PublishDiagnosticsParams {
+                uri: Url::from_file_path("/dir/a.rs").unwrap(),
+                version: None,
+                diagnostics: Default::default(),
+            },
+        );
+        cx.foreground().run_until_parked();
+        assert_eq!(futures::poll!(events.next()), Poll::Pending);
     }
 
     #[gpui::test]

crates/project/src/worktree.rs

@@ -564,33 +564,37 @@ impl LocalWorktree {
         worktree_path: Arc<Path>,
         diagnostics: Vec<DiagnosticEntry<PointUtf16>>,
         _: &mut ModelContext<Worktree>,
-    ) -> Result<()> {
-        let summary = DiagnosticSummary::new(&diagnostics);
-        if summary.is_empty() {
+    ) -> Result<bool> {
+        self.diagnostics.remove(&worktree_path);
+        let old_summary = self
+            .diagnostic_summaries
+            .remove(&PathKey(worktree_path.clone()))
+            .unwrap_or_default();
+        let new_summary = DiagnosticSummary::new(&diagnostics);
+        if !new_summary.is_empty() {
             self.diagnostic_summaries
-                .remove(&PathKey(worktree_path.clone()));
-            self.diagnostics.remove(&worktree_path);
-        } else {
-            self.diagnostic_summaries
-                .insert(PathKey(worktree_path.clone()), summary.clone());
+                .insert(PathKey(worktree_path.clone()), new_summary);
             self.diagnostics.insert(worktree_path.clone(), diagnostics);
         }
 
-        if let Some(share) = self.share.as_ref() {
-            self.client
-                .send(proto::UpdateDiagnosticSummary {
-                    project_id: share.project_id,
-                    worktree_id: self.id().to_proto(),
-                    summary: Some(proto::DiagnosticSummary {
-                        path: worktree_path.to_string_lossy().to_string(),
-                        error_count: summary.error_count as u32,
-                        warning_count: summary.warning_count as u32,
-                    }),
-                })
-                .log_err();
+        let updated = !old_summary.is_empty() || !new_summary.is_empty();
+        if updated {
+            if let Some(share) = self.share.as_ref() {
+                self.client
+                    .send(proto::UpdateDiagnosticSummary {
+                        project_id: share.project_id,
+                        worktree_id: self.id().to_proto(),
+                        summary: Some(proto::DiagnosticSummary {
+                            path: worktree_path.to_string_lossy().to_string(),
+                            error_count: new_summary.error_count as u32,
+                            warning_count: new_summary.warning_count as u32,
+                        }),
+                    })
+                    .log_err();
+            }
         }
 
-        Ok(())
+        Ok(updated)
     }
 
     pub fn scan_complete(&self) -> impl Future<Output = ()> {