Merge clangd's inactiveRegions with existing diagnostics (#26737)

Closes: https://github.com/zed-industries/zed/issues/13089

This PR attempts to resolve the issues discussed in my previous PR
#26146.

Release Notes:

- Fixed: `inactiveRegions` doesn't replace existing diagnostics anymore

crates/language/src/buffer.rs

@@ -1557,6 +1557,13 @@ impl Buffer {
         self.send_operation(op, true, cx);
     }
 
+    pub fn get_diagnostics(&self, server_id: LanguageServerId) -> Option<&DiagnosticSet> {
+        let Ok(idx) = self.diagnostics.binary_search_by_key(&server_id, |v| v.0) else {
+            return None;
+        };
+        Some(&self.diagnostics[idx].1)
+    }
+
     fn request_autoindent(&mut self, cx: &mut Context<Self>) {
         if let Some(indent_sizes) = self.compute_autoindents() {
             let indent_sizes = cx.background_spawn(indent_sizes);

crates/language/src/language.rs

@@ -229,8 +229,14 @@ impl CachedLspAdapter {
         self.adapter.code_action_kinds()
     }
 
-    pub fn process_diagnostics(&self, params: &mut lsp::PublishDiagnosticsParams) {
+    pub fn process_diagnostics(
-        self.adapter.process_diagnostics(params)
+        &self,
+        params: &mut lsp::PublishDiagnosticsParams,
+        server_id: LanguageServerId,
+        existing_diagnostics: Option<&'_ Buffer>,
+    ) {
+        self.adapter
+            .process_diagnostics(params, server_id, existing_diagnostics)
     }
 
     pub async fn process_completions(&self, completion_items: &mut [lsp::CompletionItem]) {
@@ -443,7 +449,13 @@ pub trait LspAdapter: 'static + Send + Sync {
         delegate: &dyn LspAdapterDelegate,
     ) -> Option<LanguageServerBinary>;
 
-    fn process_diagnostics(&self, _: &mut lsp::PublishDiagnosticsParams) {}
+    fn process_diagnostics(
+        &self,
+        _: &mut lsp::PublishDiagnosticsParams,
+        _: LanguageServerId,
+        _: Option<&'_ Buffer>,
+    ) {
+    }
 
     /// Post-processes completions provided by the language server.
     async fn process_completions(&self, _: &mut [lsp::CompletionItem]) {}
@@ -2060,8 +2072,6 @@ impl LspAdapter for FakeLspAdapter {
         unreachable!();
     }
 
-    fn process_diagnostics(&self, _: &mut lsp::PublishDiagnosticsParams) {}
-
     fn disk_based_diagnostic_sources(&self) -> Vec<String> {
         self.disk_based_diagnostics_sources.clone()
     }

crates/languages/src/c.rs

@@ -4,7 +4,8 @@ use futures::StreamExt;
 use gpui::AsyncApp;
 use http_client::github::{latest_github_release, GitHubLspBinaryVersion};
 pub use language::*;
-use lsp::{InitializeParams, LanguageServerBinary, LanguageServerName};
+use lsp::{DiagnosticTag, InitializeParams, LanguageServerBinary, LanguageServerName};
+use project::lsp_store::clangd_ext;
 use serde_json::json;
 use smol::fs::{self, File};
 use std::{any::Any, env::consts, path::PathBuf, sync::Arc};
@@ -279,10 +280,9 @@ impl super::LspAdapter for CLspAdapter {
                     // enable clangd's dot-to-arrow feature.
                     "editsNearCursor": true
                 },
-                // TODO: inactiveRegions support needs an implementation in clangd_ext.rs
+                "inactiveRegionsCapabilities": {
-                // "inactiveRegionsCapabilities": {
+                    "inactiveRegions": true,
-                //     "inactiveRegions": true,
+                }
-                // }
             }
         });
         if let Some(ref mut original_experimental) = original.capabilities.experimental {
@@ -292,6 +292,40 @@ impl super::LspAdapter for CLspAdapter {
         }
         Ok(original)
     }
+
+    fn process_diagnostics(
+        &self,
+        params: &mut lsp::PublishDiagnosticsParams,
+        server_id: LanguageServerId,
+        buffer_access: Option<&'_ Buffer>,
+    ) {
+        if let Some(buffer) = buffer_access {
+            let snapshot = buffer.snapshot();
+            let inactive_regions = buffer
+                .get_diagnostics(server_id)
+                .into_iter()
+                .flat_map(|v| v.iter())
+                .filter(|diag| clangd_ext::is_inactive_region(&diag.diagnostic))
+                .map(move |diag| {
+                    let range =
+                        language::range_to_lsp(diag.range.to_point_utf16(&snapshot)).unwrap();
+                    let mut tags = vec![];
+                    if diag.diagnostic.is_unnecessary {
+                        tags.push(DiagnosticTag::UNNECESSARY);
+                    }
+                    lsp::Diagnostic {
+                        range,
+                        severity: Some(diag.diagnostic.severity),
+                        source: diag.diagnostic.source.clone(),
+                        tags: Some(tags),
+                        message: diag.diagnostic.message.clone(),
+                        code: diag.diagnostic.code.clone(),
+                        ..Default::default()
+                    }
+                });
+            params.diagnostics.extend(inactive_regions);
+        }
+    }
 }
 
 async fn get_cached_server_binary(container_dir: PathBuf) -> Option<LanguageServerBinary> {

crates/languages/src/rust.rs

@@ -255,7 +255,12 @@ impl LspAdapter for RustLspAdapter {
         Some("rust-analyzer/flycheck".into())
     }
 
-    fn process_diagnostics(&self, params: &mut lsp::PublishDiagnosticsParams) {
+    fn process_diagnostics(
+        &self,
+        params: &mut lsp::PublishDiagnosticsParams,
+        _: LanguageServerId,
+        _: Option<&'_ Buffer>,
+    ) {
         static REGEX: LazyLock<Regex> =
             LazyLock::new(|| Regex::new(r"(?m)`([^`]+)\n`$").expect("Failed to create REGEX"));
 
@@ -959,7 +964,7 @@ mod tests {
                 },
             ],
         };
-        RustLspAdapter.process_diagnostics(&mut params);
+        RustLspAdapter.process_diagnostics(&mut params, LanguageServerId(0), None);
 
         assert_eq!(params.diagnostics[0].message, "use of moved value `a`");
 

crates/project/src/lsp_store.rs

@@ -77,7 +77,6 @@ use std::{
     ops::{ControlFlow, Range},
     path::{self, Path, PathBuf},
     rc::Rc,
-    str,
     sync::Arc,
     time::{Duration, Instant},
 };
@@ -452,8 +451,17 @@ impl LocalLspStore {
                 move |mut params, cx| {
                     let adapter = adapter.clone();
                     if let Some(this) = this.upgrade() {
-                        adapter.process_diagnostics(&mut params);
                         this.update(cx, |this, cx| {
+                            {
+                                let buffer = params
+                                    .uri
+                                    .to_file_path()
+                                    .map(|file_path| this.get_buffer(&file_path, cx))
+                                    .ok()
+                                    .flatten();
+                                adapter.process_diagnostics(&mut params, server_id, buffer);
+                            }
+
                             this.update_diagnostics(
                                 server_id,
                                 params,
@@ -2162,7 +2170,7 @@ impl LocalLspStore {
             Patch::new(snapshot.edits_since::<PointUtf16>(saved_version).collect())
         });
 
-        let mut sanitized_diagnostics = Vec::new();
+        let mut sanitized_diagnostics = Vec::with_capacity(diagnostics.len());
 
         for entry in diagnostics {
             let start;
@@ -6331,6 +6339,18 @@ impl LspStore {
         version: Option<i32>,
         diagnostics: Vec<DiagnosticEntry<Unclipped<PointUtf16>>>,
         cx: &mut Context<Self>,
+    ) -> anyhow::Result<()> {
+        self.merge_diagnostic_entries(server_id, abs_path, version, diagnostics, |_| false, cx)
+    }
+
+    pub fn merge_diagnostic_entries<F: Fn(&Diagnostic) -> bool + Clone>(
+        &mut self,
+        server_id: LanguageServerId,
+        abs_path: PathBuf,
+        version: Option<i32>,
+        mut diagnostics: Vec<DiagnosticEntry<Unclipped<PointUtf16>>>,
+        filter: F,
+        cx: &mut Context<Self>,
     ) -> Result<(), anyhow::Error> {
         let Some((worktree, relative_path)) =
             self.worktree_store.read(cx).find_worktree(&abs_path, cx)
@@ -6345,6 +6365,28 @@ impl LspStore {
         };
 
         if let Some(buffer) = self.buffer_store.read(cx).get_by_path(&project_path, cx) {
+            let snapshot = self
+                .as_local_mut()
+                .unwrap()
+                .buffer_snapshot_for_lsp_version(&buffer, server_id, version, cx)?;
+
+            diagnostics.extend(
+                buffer
+                    .read(cx)
+                    .get_diagnostics(server_id)
+                    .into_iter()
+                    .flat_map(|diag| {
+                        diag.iter().filter(|v| filter(&v.diagnostic)).map(|v| {
+                            let start = Unclipped(v.range.start.to_point_utf16(&snapshot));
+                            let end = Unclipped(v.range.end.to_point_utf16(&snapshot));
+                            DiagnosticEntry {
+                                range: start..end,
+                                diagnostic: v.diagnostic.clone(),
+                            }
+                        })
+                    }),
+            );
+
             self.as_local_mut().unwrap().update_buffer_diagnostics(
                 &buffer,
                 server_id,
@@ -8369,11 +8411,45 @@ impl LspStore {
         }
     }
 
+    fn get_buffer<'a>(&self, abs_path: &Path, cx: &'a App) -> Option<&'a Buffer> {
+        let (worktree, relative_path) =
+            self.worktree_store.read(cx).find_worktree(&abs_path, cx)?;
+
+        let project_path = ProjectPath {
+            worktree_id: worktree.read(cx).id(),
+            path: relative_path.into(),
+        };
+
+        Some(
+            self.buffer_store()
+                .read(cx)
+                .get_by_path(&project_path, cx)?
+                .read(cx),
+        )
+    }
+
     pub fn update_diagnostics(
+        &mut self,
+        language_server_id: LanguageServerId,
+        params: lsp::PublishDiagnosticsParams,
+        disk_based_sources: &[String],
+        cx: &mut Context<Self>,
+    ) -> Result<()> {
+        self.merge_diagnostics(
+            language_server_id,
+            params,
+            disk_based_sources,
+            |_| false,
+            cx,
+        )
+    }
+
+    pub fn merge_diagnostics<F: Fn(&Diagnostic) -> bool + Clone>(
         &mut self,
         language_server_id: LanguageServerId,
         mut params: lsp::PublishDiagnosticsParams,
         disk_based_sources: &[String],
+        filter: F,
         cx: &mut Context<Self>,
     ) -> Result<()> {
         if !self.mode.is_local() {
@@ -8480,11 +8556,12 @@ impl LspStore {
             }
         }
 
-        self.update_diagnostic_entries(
+        self.merge_diagnostic_entries(
             language_server_id,
             abs_path,
             params.version,
             diagnostics,
+            filter,
             cx,
         )?;
         Ok(())

crates/project/src/lsp_store/clangd_ext.rs

@@ -2,13 +2,14 @@ use std::sync::Arc;
 
 use ::serde::{Deserialize, Serialize};
 use gpui::WeakEntity;
-use language::CachedLspAdapter;
+use language::{CachedLspAdapter, Diagnostic};
 use lsp::LanguageServer;
 use util::ResultExt as _;
 
 use crate::LspStore;
 
 pub const CLANGD_SERVER_NAME: &str = "clangd";
+const INACTIVE_REGION_MESSAGE: &str = "inactive region";
 
 #[derive(Debug, Eq, PartialEq, Clone, Deserialize, Serialize)]
 #[serde(rename_all = "camelCase")]
@@ -25,6 +26,16 @@ impl lsp::notification::Notification for InactiveRegions {
     const METHOD: &'static str = "textDocument/inactiveRegions";
 }
 
+pub fn is_inactive_region(diag: &Diagnostic) -> bool {
+    diag.is_unnecessary
+        && diag.severity == lsp::DiagnosticSeverity::INFORMATION
+        && diag.message == INACTIVE_REGION_MESSAGE
+        && diag
+            .source
+            .as_ref()
+            .is_some_and(|v| v == CLANGD_SERVER_NAME)
+}
+
 pub fn register_notifications(
     lsp_store: WeakEntity<LspStore>,
     language_server: &LanguageServer,
@@ -35,10 +46,6 @@ pub fn register_notifications(
     }
     let server_id = language_server.server_id();
 
-    // TODO: inactiveRegions support needs do add diagnostics, not replace them as `this.update_diagnostics` call below does
-    if true {
-        return;
-    }
     language_server
         .on_notification::<InactiveRegions, _>({
             let adapter = adapter.clone();
@@ -54,7 +61,7 @@ pub fn register_notifications(
                             range,
                             severity: Some(lsp::DiagnosticSeverity::INFORMATION),
                             source: Some(CLANGD_SERVER_NAME.to_string()),
-                            message: "inactive region".to_string(),
+                            message: INACTIVE_REGION_MESSAGE.to_string(),
                             tags: Some(vec![lsp::DiagnosticTag::UNNECESSARY]),
                             ..Default::default()
                         })
@@ -64,10 +71,11 @@ pub fn register_notifications(
                         version: params.text_document.version,
                         diagnostics,
                     };
-                    this.update_diagnostics(
+                    this.merge_diagnostics(
                         server_id,
                         mapped_diagnostics,
                         &adapter.disk_based_diagnostic_sources,
+                        |diag| !is_inactive_region(diag),
                         cx,
                     )
                     .log_err();