Do not underline unnecessary diagnostics (#31355)

Kirill Bulatov created 6 months ago

Closes
https://github.com/zed-industries/zed/pull/31229#issuecomment-2906946881

Follow
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#diagnosticTag

> Clients are allowed to render diagnostics with this tag faded out
instead of having an error squiggle.

and do not underline any unnecessary diagnostic at all.

Release Notes:

- Fixed clangd's inactive regions diagnostics excessive highlights

Change summary

crates/editor/src/display_map.rs           |  7 +++-
crates/language/src/buffer.rs              |  4 +-
crates/languages/src/c.rs                  | 36 ++----------------------
crates/project/src/lsp_store/clangd_ext.rs |  7 ++--
4 files changed, 14 insertions(+), 40 deletions(-)

Detailed changes

crates/editor/src/display_map.rs

@@ -960,8 +960,11 @@ impl DisplaySnapshot {
             }) {
                 if chunk.is_unnecessary {
                     diagnostic_highlight.fade_out = Some(editor_style.unnecessary_code_fade);
-                }
-                if editor_style.show_underlines {
+                // https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#diagnosticTag
+                // states that
+                // > Clients are allowed to render diagnostics with this tag faded out instead of having an error squiggle.
+                // for the unnecessary diagnostics, so do not underline them.
+                } else if editor_style.show_underlines {
                     let diagnostic_color = super::diagnostic_style(severity, &editor_style.status);
                     diagnostic_highlight.underline = Some(UnderlineStyle {
                         color: Some(diagnostic_color),

crates/language/src/buffer.rs

@@ -492,7 +492,7 @@ pub struct Diff {
     pub edits: Vec<(Range<usize>, Arc<str>)>,
 }
 
-#[derive(Clone, Copy)]
+#[derive(Debug, Clone, Copy)]
 pub(crate) struct DiagnosticEndpoint {
     offset: usize,
     is_start: bool,
@@ -4592,7 +4592,7 @@ impl<'a> Iterator for BufferChunks<'a> {
                 syntax_highlight_id: highlight_id,
                 diagnostic_severity: self.current_diagnostic_severity(),
                 is_unnecessary: self.current_code_is_unnecessary(),
-                ..Default::default()
+                ..Chunk::default()
             })
         } else {
             None

crates/languages/src/c.rs

@@ -4,7 +4,7 @@ use futures::StreamExt;
 use gpui::{App, AsyncApp};
 use http_client::github::{GitHubLspBinaryVersion, latest_github_release};
 pub use language::*;
-use lsp::{DiagnosticTag, InitializeParams, LanguageServerBinary, LanguageServerName};
+use lsp::{InitializeParams, LanguageServerBinary, LanguageServerName};
 use project::lsp_store::clangd_ext;
 use serde_json::json;
 use smol::fs;
@@ -282,38 +282,8 @@ impl super::LspAdapter for CLspAdapter {
         Ok(original)
     }
 
-    fn process_diagnostics(
-        &self,
-        params: &mut lsp::PublishDiagnosticsParams,
-        server_id: LanguageServerId,
-        buffer: Option<&'_ Buffer>,
-    ) {
-        if let Some(buffer) = buffer {
-            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::with_capacity(1);
-                    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);
-        }
+    fn retain_old_diagnostic(&self, previous_diagnostic: &Diagnostic, _: &App) -> bool {
+        clangd_ext::is_inactive_region(previous_diagnostic)
     }
 }
 

crates/project/src/lsp_store/clangd_ext.rs

@@ -10,6 +10,7 @@ use crate::LspStore;
 
 pub const CLANGD_SERVER_NAME: &str = "clangd";
 const INACTIVE_REGION_MESSAGE: &str = "inactive region";
+const INACTIVE_DIAGNOSTIC_SEVERITY: lsp::DiagnosticSeverity = lsp::DiagnosticSeverity::INFORMATION;
 
 #[derive(Debug, Eq, PartialEq, Clone, Deserialize, Serialize)]
 #[serde(rename_all = "camelCase")]
@@ -28,7 +29,7 @@ impl lsp::notification::Notification for InactiveRegions {
 
 pub fn is_inactive_region(diag: &Diagnostic) -> bool {
     diag.is_unnecessary
-        && diag.severity == lsp::DiagnosticSeverity::INFORMATION
+        && diag.severity == INACTIVE_DIAGNOSTIC_SEVERITY
         && diag.message == INACTIVE_REGION_MESSAGE
         && diag
             .source
@@ -59,11 +60,11 @@ pub fn register_notifications(
                         .into_iter()
                         .map(|range| lsp::Diagnostic {
                             range,
-                            severity: Some(lsp::DiagnosticSeverity::INFORMATION),
+                            severity: Some(INACTIVE_DIAGNOSTIC_SEVERITY),
                             source: Some(CLANGD_SERVER_NAME.to_string()),
                             message: INACTIVE_REGION_MESSAGE.to_string(),
                             tags: Some(vec![lsp::DiagnosticTag::UNNECESSARY]),
-                            ..Default::default()
+                            ..lsp::Diagnostic::default()
                         })
                         .collect();
                     let mapped_diagnostics = lsp::PublishDiagnosticsParams {