fix(lsp): independent diagnostic publishing should include all diagnostic sources on each publish (#13483)

cli/lsp/diagnostics.rs

@@ -40,6 +40,54 @@ pub type DiagnosticVec = Vec<DiagnosticRecord>;
 type DiagnosticMap =
   HashMap<ModuleSpecifier, (Option<i32>, Vec<lsp::Diagnostic>)>;
 type TsDiagnosticsMap = HashMap<String, Vec<diagnostics::Diagnostic>>;
+type DiagnosticsByVersionMap = HashMap<Option<i32>, Vec<lsp::Diagnostic>>;
+
+#[derive(Clone)]
+struct DiagnosticsPublisher {
+  client: Client,
+  all_diagnostics:
+    Arc<Mutex<HashMap<ModuleSpecifier, DiagnosticsByVersionMap>>>,
+}
+
+impl DiagnosticsPublisher {
+  pub fn new(client: Client) -> Self {
+    Self {
+      client,
+      all_diagnostics: Default::default(),
+    }
+  }
+
+  pub async fn publish(
+    &self,
+    diagnostics: DiagnosticVec,
+    token: &CancellationToken,
+  ) {
+    let mut all_diagnostics = self.all_diagnostics.lock().await;
+    for (specifier, version, diagnostics) in diagnostics {
+      if token.is_cancelled() {
+        return;
+      }
+
+      // the versions of all the published diagnostics should be the same, but just
+      // in case they're not keep track of that
+      let diagnostics_by_version =
+        all_diagnostics.entry(specifier.clone()).or_default();
+      let mut version_diagnostics =
+        diagnostics_by_version.entry(version).or_default();
+      version_diagnostics.extend(diagnostics);
+
+      self
+        .client
+        .publish_diagnostics(specifier, version_diagnostics.clone(), version)
+        .await;
+    }
+  }
+
+  pub async fn clear(&self) {
+    let mut all_diagnostics = self.all_diagnostics.lock().await;
+    all_diagnostics.clear();
+  }
+}
 
 #[derive(Debug)]
 pub(crate) struct DiagnosticsServer {
@@ -113,6 +161,7 @@ impl DiagnosticsServer {
         let mut ts_handle: Option<tokio::task::JoinHandle<()>> = None;
         let mut lint_handle: Option<tokio::task::JoinHandle<()>> = None;
         let mut deps_handle: Option<tokio::task::JoinHandle<()>> = None;
+        let diagnostics_publisher = DiagnosticsPublisher::new(client.clone());
 
         loop {
           match rx.recv().await {
@@ -122,6 +171,7 @@ impl DiagnosticsServer {
               // cancel the previous run
               token.cancel();
               token = CancellationToken::new();
+              diagnostics_publisher.clear().await;
 
               let (snapshot, config, maybe_lint_config) = {
                 let language_server = language_server.lock().await;
@@ -135,8 +185,8 @@ impl DiagnosticsServer {
               let previous_ts_handle = ts_handle.take();
               ts_handle = Some(tokio::spawn({
                 let performance = performance.clone();
+                let diagnostics_publisher = diagnostics_publisher.clone();
                 let ts_server = ts_server.clone();
-                let client = client.clone();
                 let token = token.clone();
                 let stored_ts_diagnostics = stored_ts_diagnostics.clone();
                 let snapshot = snapshot.clone();
@@ -184,20 +234,19 @@ impl DiagnosticsServer {
                         .collect();
                     }
 
-                    for (specifier, version, diagnostics) in diagnostics {
+                    diagnostics_publisher.publish(diagnostics, &token).await;
-                      client
+
-                        .publish_diagnostics(specifier, diagnostics, version)
+                    if !token.is_cancelled() {
-                        .await;
-                    }
                       performance.measure(mark);
                     }
                   }
+                }
               }));
 
               let previous_deps_handle = deps_handle.take();
               deps_handle = Some(tokio::spawn({
                 let performance = performance.clone();
-                let client = client.clone();
+                let diagnostics_publisher = diagnostics_publisher.clone();
                 let token = token.clone();
                 let snapshot = snapshot.clone();
                 let config = config.clone();
@@ -214,12 +263,9 @@ impl DiagnosticsServer {
                   )
                   .await;
 
+                  diagnostics_publisher.publish(diagnostics, &token).await;
+
                   if !token.is_cancelled() {
-                    for (specifier, version, diagnostics) in diagnostics {
-                      client
-                        .publish_diagnostics(specifier, diagnostics, version)
-                        .await;
-                    }
                     performance.measure(mark);
                   }
                 }
@@ -228,7 +274,7 @@ impl DiagnosticsServer {
               let previous_lint_handle = lint_handle.take();
               lint_handle = Some(tokio::spawn({
                 let performance = performance.clone();
-                let client = client.clone();
+                let diagnostics_publisher = diagnostics_publisher.clone();
                 let token = token.clone();
                 let snapshot = snapshot.clone();
                 let config = config.clone();
@@ -246,12 +292,9 @@ impl DiagnosticsServer {
                   )
                   .await;
 
+                  diagnostics_publisher.publish(diagnostics, &token).await;
+
                   if !token.is_cancelled() {
-                    for (specifier, version, diagnostics) in diagnostics {
-                      client
-                        .publish_diagnostics(specifier, diagnostics, version)
-                        .await;
-                    }
                     performance.measure(mark);
                   }
                 }

cli/tests/integration/lsp_tests.rs

@@ -10,6 +10,7 @@ use deno_core::serde_json::Value;
 use deno_core::url::Url;
 use lspower::lsp;
 use pretty_assertions::assert_eq;
+use std::collections::HashSet;
 use std::fs;
 use tempfile::TempDir;
 use test_util::deno_exe_path;
@@ -155,17 +156,32 @@ impl TestSession {
 struct CollectedDiagnostics(Vec<lsp::PublishDiagnosticsParams>);
 
 impl CollectedDiagnostics {
-  pub fn all(&self) -> Vec<lsp::Diagnostic> {
+  /// Gets the diagnostics that the editor will see after all the publishes.
+  pub fn viewed(&self) -> Vec<lsp::Diagnostic> {
     self
-      .0
+      .viewed_messages()
-      .iter()
+      .into_iter()
-      .flat_map(|p| p.diagnostics.clone().into_iter())
+      .flat_map(|m| m.diagnostics)
       .collect()
   }
 
+  /// Gets the messages that the editor will see after all the publishes.
+  pub fn viewed_messages(&self) -> Vec<lsp::PublishDiagnosticsParams> {
+    // go over the publishes in reverse order in order to get
+    // the final messages that will be shown in the editor
+    let mut messages = Vec::new();
+    let mut had_specifier = HashSet::new();
+    for message in self.0.iter().rev() {
+      if had_specifier.insert(message.uri.clone()) {
+        messages.insert(0, message.clone());
+      }
+    }
+    messages
+  }
+
   pub fn with_source(&self, source: &str) -> lsp::PublishDiagnosticsParams {
     self
-      .0
+      .viewed_messages()
       .iter()
       .find(|p| {
         p.diagnostics
@@ -183,7 +199,7 @@ impl CollectedDiagnostics {
   ) -> lsp::PublishDiagnosticsParams {
     let specifier = ModuleSpecifier::parse(specifier).unwrap();
     self
-      .0
+      .viewed_messages()
       .iter()
       .find(|p| {
         p.uri == specifier
@@ -2622,29 +2638,22 @@ fn lsp_code_actions() {
 
 #[test]
 fn lsp_code_actions_deno_cache() {
-  let mut client = init("initialize_params.json");
+  let mut session = TestSession::from_file("initialize_params.json");
-  client
+  let diagnostics = session.did_open(json!({
-    .write_notification("textDocument/didOpen", json!({
     "textDocument": {
       "uri": "file:///a/file.ts",
       "languageId": "typescript",
       "version": 1,
       "text": "import * as a from \"https://deno.land/x/a/mod.ts\";\n\nconsole.log(a);\n"
     }
-    }))
+  }));
-    .unwrap();
-  let (id, method, _) = client.read_request::<Value>().unwrap();
-  assert_eq!(method, "workspace/configuration");
-  client
-    .write_response(id, json!([{ "enable": true }]))
-    .unwrap();
-  let diagnostics = read_diagnostics(&mut client);
   assert_eq!(
     diagnostics.with_source("deno"),
     load_fixture_as("diagnostics_deno_deps.json")
   );
 
-  let (maybe_res, maybe_err) = client
+  let (maybe_res, maybe_err) = session
+    .client
     .write_request(
       "textDocument/codeAction",
       load_fixture("code_action_params_cache.json"),
@@ -2655,7 +2664,7 @@ fn lsp_code_actions_deno_cache() {
     maybe_res,
     Some(load_fixture("code_action_response_cache.json"))
   );
-  shutdown(&mut client);
+  session.shutdown_and_exit();
 }
 
 #[test]
@@ -3215,26 +3224,22 @@ fn lsp_cache_location() {
   client
     .write_request::<_, _, Value>("initialize", params)
     .unwrap();
-
   client.write_notification("initialized", json!({})).unwrap();
-  did_open(
+  let mut session = TestSession::from_client(client);
-    &mut client,
+
-    json!({
+  session.did_open(json!({
     "textDocument": {
       "uri": "file:///a/file_01.ts",
       "languageId": "typescript",
       "version": 1,
       "text": "export const a = \"a\";\n",
     }
-    }),
+  }));
-  );
+  let diagnostics =
-  let diagnostics = did_open(
+    session.did_open(load_fixture("did_open_params_import_hover.json"));
-    &mut client,
+  assert_eq!(diagnostics.viewed().len(), 7);
-    load_fixture("did_open_params_import_hover.json"),
+  let (maybe_res, maybe_err) = session
-  );
+    .client
-  let diagnostics = diagnostics.into_iter().flat_map(|x| x.diagnostics);
-  assert_eq!(diagnostics.count(), 7);
-  let (maybe_res, maybe_err) = client
     .write_request::<_, _, Value>(
       "deno/cache",
       json!({
@@ -3247,7 +3252,8 @@ fn lsp_cache_location() {
     .unwrap();
   assert!(maybe_err.is_none());
   assert!(maybe_res.is_some());
-  let (maybe_res, maybe_err) = client
+  let (maybe_res, maybe_err) = session
+    .client
     .write_request(
       "textDocument/hover",
       json!({
@@ -3281,7 +3287,8 @@ fn lsp_cache_location() {
       }
     }))
   );
-  let (maybe_res, maybe_err) = client
+  let (maybe_res, maybe_err) = session
+    .client
     .write_request::<_, _, Value>(
       "textDocument/hover",
       json!({
@@ -3318,7 +3325,7 @@ fn lsp_cache_location() {
   let cache_path = temp_dir.path().join(".cache");
   assert!(cache_path.is_dir());
   assert!(cache_path.join("gen").is_dir());
-  shutdown(&mut client);
+  session.shutdown_and_exit();
 }
 
 /// Sets the TLS root certificate on startup, which allows the LSP to connect to
@@ -3351,7 +3358,7 @@ fn lsp_tls_cert() {
   }));
   let diagnostics =
     session.did_open(load_fixture("did_open_params_tls_cert.json"));
-  let diagnostics = diagnostics.all();
+  let diagnostics = diagnostics.viewed();
   assert_eq!(diagnostics.len(), 7);
   let (maybe_res, maybe_err) = session
     .client
@@ -3585,7 +3592,7 @@ fn lsp_diagnostics_deno_types() {
   assert!(maybe_res.is_some());
   assert!(maybe_err.is_none());
   let diagnostics = read_diagnostics(&mut client);
-  assert_eq!(diagnostics.all().len(), 5);
+  assert_eq!(diagnostics.viewed().len(), 5);
   shutdown(&mut client);
 }
 
@@ -3671,7 +3678,7 @@ fn lsp_diagnostics_refresh_dependents() {
     )
     .unwrap();
   let diagnostics = session.read_diagnostics();
-  assert_eq!(diagnostics.all().len(), 0); // no diagnostics now
+  assert_eq!(diagnostics.viewed().len(), 0); // no diagnostics now
 
   session.shutdown_and_exit();
   assert_eq!(session.client.queue_len(), 0);
@@ -4405,18 +4412,16 @@ fn lsp_lint_with_config() {
   client
     .write_request::<_, _, Value>("initialize", params)
     .unwrap();
+  let mut session = TestSession::from_client(client);
 
-  let diagnostics = did_open(&mut client, load_fixture("did_open_lint.json"));
+  let diagnostics = session.did_open(load_fixture("did_open_lint.json"));
-  let diagnostics = diagnostics
+  let diagnostics = diagnostics.viewed();
-    .into_iter()
-    .flat_map(|x| x.diagnostics)
-    .collect::<Vec<_>>();
   assert_eq!(diagnostics.len(), 1);
   assert_eq!(
     diagnostics[0].code,
     Some(lsp::NumberOrString::String("ban-untagged-todo".to_string()))
   );
-  shutdown(&mut client);
+  session.shutdown_and_exit();
 }
 
 #[test]