From b1143bc7f77046050f35ba556fbafd6c921cf014 Mon Sep 17 00:00:00 2001 From: Sofus Addington <sofus@addington.dk> Date: Mon, 27 Jan 2025 08:22:21 +0100 Subject: [PATCH 1/9] Pull diagnostics --- helix-core/src/syntax.rs | 2 + helix-lsp/src/client.rs | 34 +++++ helix-lsp/src/lib.rs | 2 + helix-term/src/application.rs | 20 +++ helix-term/src/commands.rs | 4 + helix-term/src/events.rs | 3 +- helix-term/src/handlers.rs | 5 +- helix-term/src/handlers/diagnostics.rs | 204 ++++++++++++++++++++++++- helix-view/src/document.rs | 7 + helix-view/src/editor.rs | 7 +- helix-view/src/events.rs | 1 + helix-view/src/handlers.rs | 1 + helix-view/src/handlers/lsp.rs | 11 +- 13 files changed, 295 insertions(+), 6 deletions(-) diff --git a/helix-core/src/syntax.rs b/helix-core/src/syntax.rs index 7c50a579..3614e2d3 100644 --- a/helix-core/src/syntax.rs +++ b/helix-core/src/syntax.rs @@ -332,6 +332,7 @@ pub enum LanguageServerFeature { WorkspaceSymbols, // Symbols, use bitflags, see above? Diagnostics, + PullDiagnostics, RenameSymbol, InlayHints, } @@ -355,6 +356,7 @@ impl Display for LanguageServerFeature { DocumentSymbols => "document-symbols", WorkspaceSymbols => "workspace-symbols", Diagnostics => "diagnostics", + PullDiagnostics => "pull-diagnostics", RenameSymbol => "rename-symbol", InlayHints => "inlay-hints", }; diff --git a/helix-lsp/src/client.rs b/helix-lsp/src/client.rs index bf8a8695..7d92d55f 100644 --- a/helix-lsp/src/client.rs +++ b/helix-lsp/src/client.rs @@ -348,6 +348,7 @@ impl Client { Some(OneOf::Left(true) | OneOf::Right(_)) ), LanguageServerFeature::Diagnostics => true, // there's no extra server capability + LanguageServerFeature::PullDiagnostics => capabilities.diagnostic_provider.is_some(), LanguageServerFeature::RenameSymbol => matches!( capabilities.rename_provider, Some(OneOf::Left(true)) | Some(OneOf::Right(_)) @@ -581,6 +582,9 @@ impl Client { did_rename: Some(true), ..Default::default() }), + diagnostic: Some(lsp::DiagnosticWorkspaceClientCapabilities { + refresh_support: Some(true), + }), ..Default::default() }), text_document: Some(lsp::TextDocumentClientCapabilities { @@ -658,6 +662,10 @@ impl Client { }), ..Default::default() }), + diagnostic: Some(lsp::DiagnosticClientCapabilities { + dynamic_registration: Some(false), + related_document_support: Some(true), + }), publish_diagnostics: Some(lsp::PublishDiagnosticsClientCapabilities { version_support: Some(true), tag_support: Some(lsp::TagSupport { @@ -1224,6 +1232,32 @@ impl Client { }) } + pub fn text_document_diagnostic( + &self, + text_document: lsp::TextDocumentIdentifier, + previous_result_id: Option<String>, + ) -> Option<impl Future<Output = Result<Value>>> { + let capabilities = self.capabilities(); + + // Return early if the server does not support pull diagnostic. + let identifier = match capabilities.diagnostic_provider.as_ref()? { + lsp::DiagnosticServerCapabilities::Options(cap) => cap.identifier.clone(), + lsp::DiagnosticServerCapabilities::RegistrationOptions(cap) => { + cap.diagnostic_options.identifier.clone() + } + }; + + let params = lsp::DocumentDiagnosticParams { + text_document, + identifier, + previous_result_id, + work_done_progress_params: lsp::WorkDoneProgressParams::default(), + partial_result_params: lsp::PartialResultParams::default(), + }; + + Some(self.call::<lsp::request::DocumentDiagnosticRequest>(params)) + } + pub fn text_document_document_highlight( &self, text_document: lsp::TextDocumentIdentifier, diff --git a/helix-lsp/src/lib.rs b/helix-lsp/src/lib.rs index ba41cbc5..b9a9ee4d 100644 --- a/helix-lsp/src/lib.rs +++ b/helix-lsp/src/lib.rs @@ -463,6 +463,7 @@ pub enum MethodCall { RegisterCapability(lsp::RegistrationParams), UnregisterCapability(lsp::UnregistrationParams), ShowDocument(lsp::ShowDocumentParams), + WorkspaceDiagnosticRefresh, } impl MethodCall { @@ -494,6 +495,7 @@ impl MethodCall { let params: lsp::ShowDocumentParams = params.parse()?; Self::ShowDocument(params) } + lsp::request::WorkspaceDiagnosticRefresh::METHOD => Self::WorkspaceDiagnosticRefresh, _ => { return Err(Error::Unhandled); } diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index cb270b86..f80cffd4 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -725,6 +725,15 @@ impl Application { doc.text(), language_id, ); + + if language_server + .supports_feature(syntax::LanguageServerFeature::PullDiagnostics) + { + handlers::diagnostics::pull_diagnostics_for_document( + doc, + language_server, + ); + } } } Notification::PublishDiagnostics(params) => { @@ -745,6 +754,7 @@ impl Application { uri, params.version, params.diagnostics, + None, ); } Notification::ShowMessage(params) => { @@ -1020,6 +1030,16 @@ impl Application { let result = self.handle_show_document(params, offset_encoding); Ok(json!(result)) } + Ok(MethodCall::WorkspaceDiagnosticRefresh) => { + for document in self.editor.documents() { + let language_server = language_server!(); + handlers::diagnostics::pull_diagnostics_for_document( + document, + language_server, + ); + } + Ok(serde_json::Value::Null) + } }; let language_server = language_server!(); diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index a197792e..5b542abf 100644 --- a/helix-term/src/commands.rs +++ b/helix-term/src/commands.rs @@ -917,6 +917,10 @@ fn goto_buffer(editor: &mut Editor, direction: Direction, count: usize) { let id = *id; + if let Some(doc) = editor.document(id) { + helix_event::dispatch(helix_view::events::DocumentDidOpen { doc }); + }; + editor.switch(id, Action::Replace); } diff --git a/helix-term/src/events.rs b/helix-term/src/events.rs index 15d81152..9d06627c 100644 --- a/helix-term/src/events.rs +++ b/helix-term/src/events.rs @@ -1,7 +1,7 @@ use helix_event::{events, register_event}; use helix_view::document::Mode; use helix_view::events::{ - DiagnosticsDidChange, DocumentDidChange, DocumentFocusLost, SelectionDidChange, + DiagnosticsDidChange, DocumentDidChange, DocumentDidOpen, DocumentFocusLost, SelectionDidChange, }; use crate::commands; @@ -18,6 +18,7 @@ pub fn register() { register_event::<PostInsertChar>(); register_event::<PostCommand>(); register_event::<DocumentDidChange>(); + register_event::<DocumentDidOpen>(); register_event::<DocumentFocusLost>(); register_event::<SelectionDidChange>(); register_event::<DiagnosticsDidChange>(); diff --git a/helix-term/src/handlers.rs b/helix-term/src/handlers.rs index b580e678..034c5067 100644 --- a/helix-term/src/handlers.rs +++ b/helix-term/src/handlers.rs @@ -6,13 +6,14 @@ use helix_event::AsyncHook; use crate::config::Config; use crate::events; use crate::handlers::auto_save::AutoSaveHandler; +use crate::handlers::diagnostics::PullDiagnosticsHandler; use crate::handlers::signature_help::SignatureHelpHandler; pub use helix_view::handlers::Handlers; mod auto_save; pub mod completion; -mod diagnostics; +pub mod diagnostics; mod signature_help; mod snippet; @@ -22,11 +23,13 @@ pub fn setup(config: Arc<ArcSwap<Config>>) -> Handlers { let event_tx = completion::CompletionHandler::new(config).spawn(); let signature_hints = SignatureHelpHandler::new().spawn(); let auto_save = AutoSaveHandler::new().spawn(); + let pull_diagnostics = PullDiagnosticsHandler::new().spawn(); let handlers = Handlers { completions: helix_view::handlers::completion::CompletionHandler::new(event_tx), signature_hints, auto_save, + pull_diagnostics, }; completion::register_hooks(&handlers); diff --git a/helix-term/src/handlers/diagnostics.rs b/helix-term/src/handlers/diagnostics.rs index 3e44d416..c8566cfa 100644 --- a/helix-term/src/handlers/diagnostics.rs +++ b/helix-term/src/handlers/diagnostics.rs @@ -1,12 +1,22 @@ +use std::collections::{HashMap, HashSet}; +use std::time::Duration; + +use helix_core::syntax::LanguageServerFeature; +use helix_core::Uri; use helix_event::{register_hook, send_blocking}; +use helix_lsp::{lsp, LanguageServerId}; use helix_view::document::Mode; -use helix_view::events::DiagnosticsDidChange; +use helix_view::events::{DiagnosticsDidChange, DocumentDidChange, DocumentDidOpen}; use helix_view::handlers::diagnostics::DiagnosticEvent; +use helix_view::handlers::lsp::PullDiagnosticsEvent; use helix_view::handlers::Handlers; +use helix_view::{DocumentId, Editor}; +use tokio::time::Instant; use crate::events::OnModeSwitch; +use crate::job; -pub(super) fn register_hooks(_handlers: &Handlers) { +pub(super) fn register_hooks(handlers: &Handlers) { register_hook!(move |event: &mut DiagnosticsDidChange<'_>| { if event.editor.mode != Mode::Insert { for (view, _) in event.editor.tree.views_mut() { @@ -21,4 +31,194 @@ pub(super) fn register_hooks(_handlers: &Handlers) { } Ok(()) }); + + let tx = handlers.pull_diagnostics.clone(); + register_hook!(move |event: &mut DocumentDidChange<'_>| { + if event + .doc + .has_language_server_with_feature(LanguageServerFeature::PullDiagnostics) + { + let document_id = event.doc.id(); + send_blocking(&tx, PullDiagnosticsEvent { document_id }); + } + Ok(()) + }); + + register_hook!(move |event: &mut DocumentDidOpen<'_>| { + if event + .doc + .has_language_server_with_feature(LanguageServerFeature::PullDiagnostics) + { + let document_id = event.doc.id(); + job::dispatch_blocking(move |editor, _| { + let Some(doc) = editor.document(document_id) else { + return; + }; + + let language_servers = + doc.language_servers_with_feature(LanguageServerFeature::PullDiagnostics); + + for language_server in language_servers { + pull_diagnostics_for_document(doc, language_server); + } + }) + } + + Ok(()) + }); +} + +#[derive(Debug)] +pub(super) struct PullDiagnosticsHandler { + document_ids: HashSet<DocumentId>, +} + +impl PullDiagnosticsHandler { + pub fn new() -> PullDiagnosticsHandler { + PullDiagnosticsHandler { + document_ids: [].into(), + } + } +} + +impl helix_event::AsyncHook for PullDiagnosticsHandler { + type Event = PullDiagnosticsEvent; + + fn handle_event( + &mut self, + event: Self::Event, + existing_debounce: Option<tokio::time::Instant>, + ) -> Option<tokio::time::Instant> { + if existing_debounce.is_none() { + dispatch_pull_diagnostic_for_document(event.document_id); + } + + self.document_ids.insert(event.document_id); + Some(Instant::now() + Duration::from_millis(500)) + } + + fn finish_debounce(&mut self) { + for document_id in self.document_ids.clone() { + dispatch_pull_diagnostic_for_document(document_id); + } + } +} + +fn dispatch_pull_diagnostic_for_document(document_id: DocumentId) { + job::dispatch_blocking(move |editor, _| { + let Some(doc) = editor.document(document_id) else { + return; + }; + + let language_servers = + doc.language_servers_with_feature(LanguageServerFeature::PullDiagnostics); + + for language_server in language_servers { + pull_diagnostics_for_document(doc, language_server); + } + }) +} + +pub fn pull_diagnostics_for_document( + doc: &helix_view::Document, + language_server: &helix_lsp::Client, +) { + let Some(future) = language_server + .text_document_diagnostic(doc.identifier(), doc.previous_diagnostic_id.clone()) + else { + return; + }; + + let Some(uri) = doc.uri() else { + return; + }; + + let server_id = language_server.id(); + let document_id = doc.id(); + + tokio::spawn(async move { + match future.await { + Ok(res) => { + job::dispatch(move |editor, _| { + let response = match serde_json::from_value(res) { + Ok(result) => result, + Err(_) => return, + }; + + handle_pull_diagnostics_response(editor, response, server_id, uri, document_id) + }) + .await + } + Err(err) => log::error!("Pull diagnostic request failed: {err}"), + } + }); +} + +fn handle_pull_diagnostics_response( + editor: &mut Editor, + response: lsp::DocumentDiagnosticReport, + server_id: LanguageServerId, + uri: Uri, + document_id: DocumentId, +) { + let Some(doc) = editor.document_mut(document_id) else { + return; + }; + + match response { + lsp::DocumentDiagnosticReport::Full(report) => { + // Diagnostic for requested file + editor.handle_lsp_diagnostics( + server_id, + uri, + None, + report.full_document_diagnostic_report.items, + report.full_document_diagnostic_report.result_id, + ); + + // Diagnostic for related files + handle_document_diagnostic_report_kind( + editor, + document_id, + report.related_documents, + server_id, + ); + } + lsp::DocumentDiagnosticReport::Unchanged(report) => { + doc.previous_diagnostic_id = + Some(report.unchanged_document_diagnostic_report.result_id); + + handle_document_diagnostic_report_kind( + editor, + document_id, + report.related_documents, + server_id, + ); + } + } +} + +fn handle_document_diagnostic_report_kind( + editor: &mut Editor, + document_id: DocumentId, + report: Option<HashMap<lsp::Url, lsp::DocumentDiagnosticReportKind>>, + server_id: LanguageServerId, +) { + for (url, report) in report.into_iter().flatten() { + match report { + lsp::DocumentDiagnosticReportKind::Full(report) => { + let Ok(uri) = Uri::try_from(url) else { + return; + }; + + editor.handle_lsp_diagnostics(server_id, uri, None, report.items, report.result_id); + } + lsp::DocumentDiagnosticReportKind::Unchanged(report) => { + let Some(doc) = editor.document_mut(document_id) else { + return; + }; + doc.previous_diagnostic_id = Some(report.result_id); + } + } + } } diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 06a708f0..8f90aba4 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -196,6 +196,8 @@ pub struct Document { pub focused_at: std::time::Instant, pub readonly: bool, + + pub previous_diagnostic_id: Option<String>, } /// Inlay hints for a single `(Document, View)` combo. @@ -698,6 +700,7 @@ impl Document { focused_at: std::time::Instant::now(), readonly: false, jump_labels: HashMap::new(), + previous_diagnostic_id: None, } } @@ -2174,6 +2177,10 @@ impl Document { pub fn reset_all_inlay_hints(&mut self) { self.inlay_hints = Default::default(); } + + pub fn has_language_server_with_feature(&self, feature: LanguageServerFeature) -> bool { + self.language_servers_with_feature(feature).next().is_some() + } } #[derive(Debug, Default)] diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs index cdc48a54..6581dd35 100644 --- a/helix-view/src/editor.rs +++ b/helix-view/src/editor.rs @@ -4,7 +4,7 @@ use crate::{ document::{ DocumentOpenError, DocumentSavedEventFuture, DocumentSavedEventResult, Mode, SavePoint, }, - events::DocumentFocusLost, + events::{DocumentDidOpen, DocumentFocusLost}, graphics::{CursorKind, Rect}, handlers::Handlers, info::Info, @@ -1778,6 +1778,11 @@ impl Editor { }; self.switch(id, action); + + if let Some(doc) = self.document_mut(id) { + helix_event::dispatch(DocumentDidOpen { doc }); + }; + Ok(id) } diff --git a/helix-view/src/events.rs b/helix-view/src/events.rs index eb97268c..cb3ed90c 100644 --- a/helix-view/src/events.rs +++ b/helix-view/src/events.rs @@ -11,6 +11,7 @@ events! { changes: &'a ChangeSet, ghost_transaction: bool } + DocumentDidOpen<'a> { doc: &'a Document} SelectionDidChange<'a> { doc: &'a mut Document, view: ViewId } DiagnosticsDidChange<'a> { editor: &'a mut Editor, doc: DocumentId } // called **after** a document loses focus (but not when its closed) diff --git a/helix-view/src/handlers.rs b/helix-view/src/handlers.rs index a26c4ddb..bb2221a9 100644 --- a/helix-view/src/handlers.rs +++ b/helix-view/src/handlers.rs @@ -21,6 +21,7 @@ pub struct Handlers { pub completions: CompletionHandler, pub signature_hints: Sender<lsp::SignatureHelpEvent>, pub auto_save: Sender<AutoSaveEvent>, + pub pull_diagnostics: Sender<lsp::PullDiagnosticsEvent>, } impl Handlers { diff --git a/helix-view/src/handlers/lsp.rs b/helix-view/src/handlers/lsp.rs index dc2e1755..60d50f9e 100644 --- a/helix-view/src/handlers/lsp.rs +++ b/helix-view/src/handlers/lsp.rs @@ -3,7 +3,7 @@ use std::fmt::Display; use crate::editor::Action; use crate::events::DiagnosticsDidChange; -use crate::Editor; +use crate::{DocumentId, Editor}; use helix_core::Uri; use helix_lsp::util::generate_transaction_from_edits; use helix_lsp::{lsp, LanguageServerId, OffsetEncoding}; @@ -22,6 +22,10 @@ pub enum SignatureHelpEvent { RequestComplete { open: bool }, } +pub struct PullDiagnosticsEvent { + pub document_id: DocumentId, +} + #[derive(Debug)] pub struct ApplyEditError { pub kind: ApplyEditErrorKind, @@ -280,6 +284,7 @@ impl Editor { uri: Uri, version: Option<i32>, mut diagnostics: Vec<lsp::Diagnostic>, + report_id: Option<String>, ) { let doc = self .documents @@ -357,6 +362,10 @@ impl Editor { ); doc.replace_diagnostics(diagnostics, &unchanged_diag_sources, Some(server_id)); + if report_id.is_some() { + doc.previous_diagnostic_id = report_id; + } + let doc = doc.id(); helix_event::dispatch(DiagnosticsDidChange { editor: self, doc }); } From 6949048e3bc81aa01fcf670bcb0f34a0cf888642 Mon Sep 17 00:00:00 2001 From: Sofus Addington <sofus@addington.dk> Date: Sat, 22 Feb 2025 17:13:01 +0100 Subject: [PATCH 2/9] Separate publish and pull diagnostic provider --- helix-core/src/diagnostic.rs | 17 +++++++- helix-term/src/application.rs | 17 ++++++-- helix-term/src/commands/lsp.rs | 9 ++-- helix-term/src/commands/typed.rs | 2 +- helix-term/src/handlers/diagnostics.rs | 19 +++++---- helix-view/src/document.rs | 26 ++++++++---- helix-view/src/editor.rs | 58 ++++++++++++++------------ helix-view/src/gutter.rs | 2 +- helix-view/src/handlers/lsp.rs | 19 +++++---- 9 files changed, 106 insertions(+), 63 deletions(-) diff --git a/helix-core/src/diagnostic.rs b/helix-core/src/diagnostic.rs index 333c9409..78d48edb 100644 --- a/helix-core/src/diagnostic.rs +++ b/helix-core/src/diagnostic.rs @@ -50,8 +50,21 @@ pub struct Diagnostic { pub data: Option<serde_json::Value>, } -// TODO turn this into an enum + feature flag when lsp becomes optional -pub type DiagnosticProvider = LanguageServerId; +// TODO turn this into a feature flag when lsp becomes optional +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +pub enum DiagnosticProvider { + PublishDiagnosticProvider(LanguageServerId), + PullDiagnosticProvider(LanguageServerId), +} + +impl From<DiagnosticProvider> for LanguageServerId { + fn from(value: DiagnosticProvider) -> Self { + match value { + DiagnosticProvider::PublishDiagnosticProvider(id) => id, + DiagnosticProvider::PullDiagnosticProvider(id) => id, + } + } +} // while I would prefer having this in helix-lsp that necessitates a bunch of // conversions I would rather not add. I think its fine since this just a very diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index f80cffd4..2d41c34e 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -1,6 +1,9 @@ use arc_swap::{access::Map, ArcSwap}; use futures_util::Stream; -use helix_core::{diagnostic::Severity, pos_at_coords, syntax, Range, Selection}; +use helix_core::{ + diagnostic::{DiagnosticProvider, Severity}, + pos_at_coords, syntax, Range, Selection, +}; use helix_lsp::{ lsp::{self, notification::Notification}, util::lsp_range_to_range, @@ -749,8 +752,12 @@ impl Application { log::error!("Discarding publishDiagnostic notification sent by an uninitialized server: {}", language_server.name()); return; } + + let diagnostic_provider = + DiagnosticProvider::PublishDiagnosticProvider(language_server.id()); + self.editor.handle_lsp_diagnostics( - language_server.id(), + diagnostic_provider, uri, params.version, params.diagnostics, @@ -864,14 +871,16 @@ impl Application { // we need to clear those and remove the entries from the list if this leads to // an empty diagnostic list for said files for diags in self.editor.diagnostics.values_mut() { - diags.retain(|(_, lsp_id)| *lsp_id != server_id); + diags.retain(|(_, diagnostic_provider)| { + Into::<LanguageServerId>::into(*diagnostic_provider) != server_id + }); } self.editor.diagnostics.retain(|_, diags| !diags.is_empty()); // Clear any diagnostics for documents with this server open. for doc in self.editor.documents_mut() { - doc.clear_diagnostics(Some(server_id)); + doc.clear_all_language_server_diagnostics(Some(server_id)); } // Remove the language server from the registry. diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs index c5442924..c506f7bb 100644 --- a/helix-term/src/commands/lsp.rs +++ b/helix-term/src/commands/lsp.rs @@ -14,7 +14,8 @@ use tui::{text::Span, widgets::Row}; use super::{align_view, push_jump, Align, Context, Editor}; use helix_core::{ - syntax::LanguageServerFeature, text_annotations::InlineAnnotation, Selection, Uri, + diagnostic::DiagnosticProvider, syntax::LanguageServerFeature, + text_annotations::InlineAnnotation, Selection, Uri, }; use helix_stdx::path; use helix_view::{ @@ -209,7 +210,7 @@ type DiagnosticsPicker = Picker<PickerDiagnostic, DiagnosticStyles>; fn diag_picker( cx: &Context, - diagnostics: BTreeMap<Uri, Vec<(lsp::Diagnostic, LanguageServerId)>>, + diagnostics: BTreeMap<Uri, Vec<(lsp::Diagnostic, DiagnosticProvider)>>, format: DiagnosticsFormat, ) -> DiagnosticsPicker { // TODO: drop current_path comparison and instead use workspace: bool flag? @@ -219,8 +220,8 @@ fn diag_picker( for (uri, diags) in diagnostics { flat_diag.reserve(diags.len()); - for (diag, ls) in diags { - if let Some(ls) = cx.editor.language_server_by_id(ls) { + for (diag, provider) in diags { + if let Some(ls) = cx.editor.language_server_by_id(provider.into()) { flat_diag.push(PickerDiagnostic { location: Location { uri: uri.clone(), diff --git a/helix-term/src/commands/typed.rs b/helix-term/src/commands/typed.rs index bcb6e543..87580a7e 100644 --- a/helix-term/src/commands/typed.rs +++ b/helix-term/src/commands/typed.rs @@ -1623,7 +1623,7 @@ fn lsp_stop(cx: &mut compositor::Context, args: Args, event: PromptEvent) -> any for doc in cx.editor.documents_mut() { if let Some(client) = doc.remove_language_server_by_name(ls_name) { - doc.clear_diagnostics(Some(client.id())); + doc.clear_all_language_server_diagnostics(Some(client.id())); doc.reset_all_inlay_hints(); doc.inlay_hints_oudated = true; } diff --git a/helix-term/src/handlers/diagnostics.rs b/helix-term/src/handlers/diagnostics.rs index c8566cfa..9e28ef52 100644 --- a/helix-term/src/handlers/diagnostics.rs +++ b/helix-term/src/handlers/diagnostics.rs @@ -1,10 +1,11 @@ use std::collections::{HashMap, HashSet}; use std::time::Duration; +use helix_core::diagnostic::DiagnosticProvider; use helix_core::syntax::LanguageServerFeature; use helix_core::Uri; use helix_event::{register_hook, send_blocking}; -use helix_lsp::{lsp, LanguageServerId}; +use helix_lsp::lsp; use helix_view::document::Mode; use helix_view::events::{DiagnosticsDidChange, DocumentDidChange, DocumentDidOpen}; use helix_view::handlers::diagnostics::DiagnosticEvent; @@ -133,7 +134,7 @@ pub fn pull_diagnostics_for_document( return; }; - let server_id = language_server.id(); + let provider = DiagnosticProvider::PullDiagnosticProvider(language_server.id()); let document_id = doc.id(); tokio::spawn(async move { @@ -145,7 +146,7 @@ pub fn pull_diagnostics_for_document( Err(_) => return, }; - handle_pull_diagnostics_response(editor, response, server_id, uri, document_id) + handle_pull_diagnostics_response(editor, response, provider, uri, document_id) }) .await } @@ -157,7 +158,7 @@ pub fn pull_diagnostics_for_document( fn handle_pull_diagnostics_response( editor: &mut Editor, response: lsp::DocumentDiagnosticReport, - server_id: LanguageServerId, + provider: DiagnosticProvider, uri: Uri, document_id: DocumentId, ) { @@ -169,7 +170,7 @@ fn handle_pull_diagnostics_response( lsp::DocumentDiagnosticReport::Full(report) => { // Diagnostic for requested file editor.handle_lsp_diagnostics( - server_id, + provider, uri, None, report.full_document_diagnostic_report.items, @@ -181,7 +182,7 @@ fn handle_pull_diagnostics_response( editor, document_id, report.related_documents, - server_id, + provider, ); } lsp::DocumentDiagnosticReport::Unchanged(report) => { @@ -192,7 +193,7 @@ fn handle_pull_diagnostics_response( editor, document_id, report.related_documents, - server_id, + provider, ); } } @@ -202,7 +203,7 @@ fn handle_document_diagnostic_report_kind( editor: &mut Editor, document_id: DocumentId, report: Option<HashMap<lsp::Url, lsp::DocumentDiagnosticReportKind>>, - server_id: LanguageServerId, + provider: DiagnosticProvider, ) { for (url, report) in report.into_iter().flatten() { match report { @@ -211,7 +212,7 @@ fn handle_document_diagnostic_report_kind( return; }; - editor.handle_lsp_diagnostics(server_id, uri, None, report.items, report.result_id); + editor.handle_lsp_diagnostics(provider, uri, None, report.items, report.result_id); } lsp::DocumentDiagnosticReportKind::Unchanged(report) => { let Some(doc) = editor.document_mut(document_id) else { diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 8f90aba4..5e066bb5 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -5,6 +5,7 @@ use futures_util::future::BoxFuture; use futures_util::FutureExt; use helix_core::auto_pairs::AutoPairs; use helix_core::chars::char_is_word; +use helix_core::diagnostic::DiagnosticProvider; use helix_core::doc_formatter::TextFormat; use helix_core::encoding::Encoding; use helix_core::snippets::{ActiveSnippet, SnippetRenderCtx}; @@ -1934,7 +1935,7 @@ impl Document { text: &Rope, language_config: Option<&LanguageConfiguration>, diagnostic: &helix_lsp::lsp::Diagnostic, - language_server_id: LanguageServerId, + language_server_id: DiagnosticProvider, offset_encoding: helix_lsp::OffsetEncoding, ) -> Option<Diagnostic> { use helix_core::diagnostic::{Range, Severity::*}; @@ -2027,13 +2028,13 @@ impl Document { &mut self, diagnostics: impl IntoIterator<Item = Diagnostic>, unchanged_sources: &[String], - language_server_id: Option<LanguageServerId>, + diagnostic_provider: Option<DiagnosticProvider>, ) { if unchanged_sources.is_empty() { - self.clear_diagnostics(language_server_id); + self.clear_diagnostics(diagnostic_provider); } else { self.diagnostics.retain(|d| { - if language_server_id.is_some_and(|id| id != d.provider) { + if diagnostic_provider.is_some_and(|provider| provider != d.provider) { return true; } @@ -2049,10 +2050,19 @@ impl Document { .sort_by_key(|diagnostic| (diagnostic.range, diagnostic.severity, diagnostic.provider)); } - /// clears diagnostics for a given language server id if set, otherwise all diagnostics are cleared - pub fn clear_diagnostics(&mut self, language_server_id: Option<LanguageServerId>) { - if let Some(id) = language_server_id { - self.diagnostics.retain(|d| d.provider != id); + /// clears diagnostics for a given diagnostic provider if set, otherwise all diagnostics are cleared + pub fn clear_diagnostics(&mut self, provider: Option<DiagnosticProvider>) { + if let Some(provider) = provider { + self.diagnostics.retain(|d| d.provider != provider); + } else { + self.diagnostics.clear(); + } + } + + /// clears diagnostics for a given language_server if set, otherwise all diagnostics are cleared + pub fn clear_all_language_server_diagnostics(&mut self, server_id: Option<LanguageServerId>) { + if let Some(server_id) = server_id { + self.diagnostics.retain(|d| server_id != d.provider.into()); } else { self.diagnostics.clear(); } diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs index 6581dd35..bcfe68d7 100644 --- a/helix-view/src/editor.rs +++ b/helix-view/src/editor.rs @@ -45,6 +45,7 @@ use anyhow::{anyhow, bail, Error}; pub use helix_core::diagnostic::Severity; use helix_core::{ auto_pairs::AutoPairs, + diagnostic::DiagnosticProvider, syntax::{self, AutoPairConfig, IndentationHeuristic, LanguageServerFeature, SoftWrap}, Change, LineEnding, Position, Range, Selection, Uri, NATIVE_LINE_ENDING, }; @@ -1056,7 +1057,7 @@ pub struct Editor { pub macro_recording: Option<(char, Vec<KeyEvent>)>, pub macro_replaying: Vec<char>, pub language_servers: helix_lsp::Registry, - pub diagnostics: BTreeMap<Uri, Vec<(lsp::Diagnostic, LanguageServerId)>>, + pub diagnostics: BTreeMap<Uri, Vec<(lsp::Diagnostic, DiagnosticProvider)>>, pub diff_providers: DiffProviderRegistry, pub debugger: Option<dap::Client>, @@ -2006,7 +2007,7 @@ impl Editor { /// Returns all supported diagnostics for the document pub fn doc_diagnostics<'a>( language_servers: &'a helix_lsp::Registry, - diagnostics: &'a BTreeMap<Uri, Vec<(lsp::Diagnostic, LanguageServerId)>>, + diagnostics: &'a BTreeMap<Uri, Vec<(lsp::Diagnostic, DiagnosticProvider)>>, document: &Document, ) -> impl Iterator<Item = helix_core::Diagnostic> + 'a { Editor::doc_diagnostics_with_filter(language_servers, diagnostics, document, |_, _| true) @@ -2016,9 +2017,9 @@ impl Editor { /// filtered by `filter` which is invocated with the raw `lsp::Diagnostic` and the language server id it came from pub fn doc_diagnostics_with_filter<'a>( language_servers: &'a helix_lsp::Registry, - diagnostics: &'a BTreeMap<Uri, Vec<(lsp::Diagnostic, LanguageServerId)>>, + diagnostics: &'a BTreeMap<Uri, Vec<(lsp::Diagnostic, DiagnosticProvider)>>, document: &Document, - filter: impl Fn(&lsp::Diagnostic, LanguageServerId) -> bool + 'a, + filter: impl Fn(&lsp::Diagnostic, DiagnosticProvider) -> bool + 'a, ) -> impl Iterator<Item = helix_core::Diagnostic> + 'a { let text = document.text().clone(); let language_config = document.language.clone(); @@ -2026,30 +2027,33 @@ impl Editor { .uri() .and_then(|uri| diagnostics.get(&uri)) .map(|diags| { - diags.iter().filter_map(move |(diagnostic, lsp_id)| { - let ls = language_servers.get_by_id(*lsp_id)?; - language_config - .as_ref() - .and_then(|c| { - c.language_servers.iter().find(|features| { - features.name == ls.name() - && features.has_feature(LanguageServerFeature::Diagnostics) + diags + .iter() + .filter_map(move |(diagnostic, diagnostic_provider)| { + let ls = language_servers + .get_by_id(Into::<LanguageServerId>::into(*diagnostic_provider))?; + language_config + .as_ref() + .and_then(|c| { + c.language_servers.iter().find(|features| { + features.name == ls.name() + && features.has_feature(LanguageServerFeature::Diagnostics) + }) }) - }) - .and_then(|_| { - if filter(diagnostic, *lsp_id) { - Document::lsp_diagnostic_to_diagnostic( - &text, - language_config.as_deref(), - diagnostic, - *lsp_id, - ls.offset_encoding(), - ) - } else { - None - } - }) - }) + .and_then(|_| { + if filter(diagnostic, *diagnostic_provider) { + Document::lsp_diagnostic_to_diagnostic( + &text, + language_config.as_deref(), + diagnostic, + *diagnostic_provider, + ls.offset_encoding(), + ) + } else { + None + } + }) + }) }) .into_iter() .flatten() diff --git a/helix-view/src/gutter.rs b/helix-view/src/gutter.rs index 7cd91271..b8456ffc 100644 --- a/helix-view/src/gutter.rs +++ b/helix-view/src/gutter.rs @@ -71,7 +71,7 @@ pub fn diagnostic<'doc>( d.line == line && doc .language_servers_with_feature(LanguageServerFeature::Diagnostics) - .any(|ls| ls.id() == d.provider) + .any(|ls| ls.id() == d.provider.into()) }); diagnostics_on_line.max_by_key(|d| d.severity).map(|d| { write!(out, "●").ok(); diff --git a/helix-view/src/handlers/lsp.rs b/helix-view/src/handlers/lsp.rs index 60d50f9e..989cd936 100644 --- a/helix-view/src/handlers/lsp.rs +++ b/helix-view/src/handlers/lsp.rs @@ -4,9 +4,10 @@ use std::fmt::Display; use crate::editor::Action; use crate::events::DiagnosticsDidChange; use crate::{DocumentId, Editor}; +use helix_core::diagnostic::DiagnosticProvider; use helix_core::Uri; use helix_lsp::util::generate_transaction_from_edits; -use helix_lsp::{lsp, LanguageServerId, OffsetEncoding}; +use helix_lsp::{lsp, OffsetEncoding}; #[derive(Debug, PartialEq, Eq, Clone, Copy)] pub enum SignatureHelpInvoked { @@ -280,7 +281,7 @@ impl Editor { pub fn handle_lsp_diagnostics( &mut self, - server_id: LanguageServerId, + diagnostic_provider: DiagnosticProvider, uri: Uri, version: Option<i32>, mut diagnostics: Vec<lsp::Diagnostic>, @@ -315,7 +316,7 @@ impl Editor { let old_diagnostics = old_diagnostics .iter() .filter(|(d, d_server)| { - *d_server == server_id && d.source.as_ref() == Some(source) + *d_server == diagnostic_provider && d.source.as_ref() == Some(source) }) .map(|(d, _)| d); if new_diagnostics.eq(old_diagnostics) { @@ -324,7 +325,7 @@ impl Editor { } } - let diagnostics = diagnostics.into_iter().map(|d| (d, server_id)); + let diagnostics = diagnostics.into_iter().map(|d| (d, diagnostic_provider)); // Insert the original lsp::Diagnostics here because we may have no open document // for diagnostic message and so we can't calculate the exact position. @@ -333,7 +334,7 @@ impl Editor { Entry::Occupied(o) => { let current_diagnostics = o.into_mut(); // there may entries of other language servers, which is why we can't overwrite the whole entry - current_diagnostics.retain(|(_, lsp_id)| *lsp_id != server_id); + current_diagnostics.retain(|(_, lsp_id)| *lsp_id != diagnostic_provider); current_diagnostics.extend(diagnostics); current_diagnostics // Sort diagnostics first by severity and then by line numbers. @@ -348,7 +349,7 @@ impl Editor { if let Some(doc) = doc { let diagnostic_of_language_server_and_not_in_unchanged_sources = |diagnostic: &lsp::Diagnostic, ls_id| { - ls_id == server_id + ls_id == diagnostic_provider && diagnostic .source .as_ref() @@ -360,7 +361,11 @@ impl Editor { doc, diagnostic_of_language_server_and_not_in_unchanged_sources, ); - doc.replace_diagnostics(diagnostics, &unchanged_diag_sources, Some(server_id)); + doc.replace_diagnostics( + diagnostics, + &unchanged_diag_sources, + Some(diagnostic_provider), + ); if report_id.is_some() { doc.previous_diagnostic_id = report_id; From 8b59087d32bfba5b1fb53bdb0b03fa460b8ba6a9 Mon Sep 17 00:00:00 2001 From: Sofus Addington <sofus@addington.dk> Date: Mon, 3 Mar 2025 07:05:48 +0100 Subject: [PATCH 3/9] Skip additional dispatch and eliminate helper function --- helix-term/src/application.rs | 1 - helix-term/src/handlers/diagnostics.rs | 85 ++++++++------------------ helix-view/src/handlers/lsp.rs | 5 -- 3 files changed, 27 insertions(+), 64 deletions(-) diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index 2d41c34e..92ab8261 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -761,7 +761,6 @@ impl Application { uri, params.version, params.diagnostics, - None, ); } Notification::ShowMessage(params) => { diff --git a/helix-term/src/handlers/diagnostics.rs b/helix-term/src/handlers/diagnostics.rs index 9e28ef52..a93fc396 100644 --- a/helix-term/src/handlers/diagnostics.rs +++ b/helix-term/src/handlers/diagnostics.rs @@ -1,4 +1,4 @@ -use std::collections::{HashMap, HashSet}; +use std::collections::HashSet; use std::time::Duration; use helix_core::diagnostic::DiagnosticProvider; @@ -46,23 +46,11 @@ pub(super) fn register_hooks(handlers: &Handlers) { }); register_hook!(move |event: &mut DocumentDidOpen<'_>| { - if event + for language_server in event .doc - .has_language_server_with_feature(LanguageServerFeature::PullDiagnostics) + .language_servers_with_feature(LanguageServerFeature::PullDiagnostics) { - let document_id = event.doc.id(); - job::dispatch_blocking(move |editor, _| { - let Some(doc) = editor.document(document_id) else { - return; - }; - - let language_servers = - doc.language_servers_with_feature(LanguageServerFeature::PullDiagnostics); - - for language_server in language_servers { - pull_diagnostics_for_document(doc, language_server); - } - }) + pull_diagnostics_for_document(event.doc, language_server); } Ok(()) @@ -162,64 +150,45 @@ fn handle_pull_diagnostics_response( uri: Uri, document_id: DocumentId, ) { - let Some(doc) = editor.document_mut(document_id) else { - return; - }; - - match response { + let (result_id, related_documents) = match response { lsp::DocumentDiagnosticReport::Full(report) => { - // Diagnostic for requested file editor.handle_lsp_diagnostics( provider, uri, None, report.full_document_diagnostic_report.items, + ); + + ( report.full_document_diagnostic_report.result_id, - ); - - // Diagnostic for related files - handle_document_diagnostic_report_kind( - editor, - document_id, report.related_documents, - provider, - ); + ) } - lsp::DocumentDiagnosticReport::Unchanged(report) => { - doc.previous_diagnostic_id = - Some(report.unchanged_document_diagnostic_report.result_id); + lsp::DocumentDiagnosticReport::Unchanged(report) => ( + Some(report.unchanged_document_diagnostic_report.result_id), + report.related_documents, + ), + }; - handle_document_diagnostic_report_kind( - editor, - document_id, - report.related_documents, - provider, - ); - } - } -} + if let Some(doc) = editor.document_mut(document_id) { + doc.previous_diagnostic_id = result_id; + }; -fn handle_document_diagnostic_report_kind( - editor: &mut Editor, - document_id: DocumentId, - report: Option<HashMap<lsp::Url, lsp::DocumentDiagnosticReportKind>>, - provider: DiagnosticProvider, -) { - for (url, report) in report.into_iter().flatten() { - match report { + for (url, report) in related_documents.into_iter().flatten() { + let result_id = match report { lsp::DocumentDiagnosticReportKind::Full(report) => { let Ok(uri) = Uri::try_from(url) else { - return; + continue; }; - editor.handle_lsp_diagnostics(provider, uri, None, report.items, report.result_id); - } - lsp::DocumentDiagnosticReportKind::Unchanged(report) => { - let Some(doc) = editor.document_mut(document_id) else { - return; - }; - doc.previous_diagnostic_id = Some(report.result_id); + editor.handle_lsp_diagnostics(provider, uri, None, report.items); + report.result_id } + lsp::DocumentDiagnosticReportKind::Unchanged(report) => Some(report.result_id), + }; + + if let Some(doc) = editor.document_mut(document_id) { + doc.previous_diagnostic_id = result_id; } } } diff --git a/helix-view/src/handlers/lsp.rs b/helix-view/src/handlers/lsp.rs index 989cd936..b9932727 100644 --- a/helix-view/src/handlers/lsp.rs +++ b/helix-view/src/handlers/lsp.rs @@ -285,7 +285,6 @@ impl Editor { uri: Uri, version: Option<i32>, mut diagnostics: Vec<lsp::Diagnostic>, - report_id: Option<String>, ) { let doc = self .documents @@ -367,10 +366,6 @@ impl Editor { Some(diagnostic_provider), ); - if report_id.is_some() { - doc.previous_diagnostic_id = report_id; - } - let doc = doc.id(); helix_event::dispatch(DiagnosticsDidChange { editor: self, doc }); } From 96cf04288db8d3d7f6df6d688cd147c70a9967bc Mon Sep 17 00:00:00 2001 From: Sofus Addington <sofus@addington.dk> Date: Fri, 7 Mar 2025 06:07:09 +0100 Subject: [PATCH 4/9] Prepare for indentifier in diagnostic provider --- helix-core/src/diagnostic.rs | 67 +++++++++++++++++++++++++- helix-term/src/application.rs | 4 +- helix-term/src/handlers/diagnostics.rs | 4 +- helix-view/src/document.rs | 32 ++++++++---- helix-view/src/editor.rs | 7 ++- helix-view/src/gutter.rs | 2 +- helix-view/src/handlers/lsp.rs | 20 +++++--- 7 files changed, 109 insertions(+), 27 deletions(-) diff --git a/helix-core/src/diagnostic.rs b/helix-core/src/diagnostic.rs index 78d48edb..a133fae1 100644 --- a/helix-core/src/diagnostic.rs +++ b/helix-core/src/diagnostic.rs @@ -51,7 +51,7 @@ pub struct Diagnostic { } // TODO turn this into a feature flag when lsp becomes optional -#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] pub enum DiagnosticProvider { PublishDiagnosticProvider(LanguageServerId), PullDiagnosticProvider(LanguageServerId), @@ -74,6 +74,33 @@ slotmap::new_key_type! { pub struct LanguageServerId; } +impl DiagnosticProvider { + pub fn server_id(&self) -> &LanguageServerId { + match self { + DiagnosticProvider::PublishDiagnosticProvider(language_server_id) => language_server_id, + DiagnosticProvider::PullDiagnosticProvider(language_server_id) => language_server_id, + } + } + + pub fn has_server_id(&self, server_id: &LanguageServerId) -> bool { + match self { + DiagnosticProvider::PublishDiagnosticProvider(language_server_id) => { + language_server_id == server_id + } + DiagnosticProvider::PullDiagnosticProvider(language_server_id) => { + language_server_id == server_id + } + } + } + + pub fn equals(&self, diagnostic_provider: &DiagnosticProvider) -> bool { + let server_id = self.server_id(); + let other_server_id = diagnostic_provider.server_id(); + + server_id == other_server_id + } +} + impl fmt::Display for LanguageServerId { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{:?}", self.0) @@ -86,3 +113,41 @@ impl Diagnostic { self.severity.unwrap_or(Severity::Warning) } } + +#[cfg(test)] +mod tests { + use slotmap::KeyData; + + use super::DiagnosticProvider; + use crate::diagnostic::LanguageServerId; + + #[test] + fn can_compare_equal_diagnostic_provider() { + let first_provider = + DiagnosticProvider::PublishDiagnosticProvider(LanguageServerId(KeyData::from_ffi(1))); + let second_provider = + DiagnosticProvider::PublishDiagnosticProvider(LanguageServerId(KeyData::from_ffi(1))); + + assert!(first_provider.equals(&second_provider)); + } + + #[test] + fn can_distinguish_diagnostic_provider() { + let first_provider = + DiagnosticProvider::PublishDiagnosticProvider(LanguageServerId(KeyData::from_ffi(1))); + let second_provider = + DiagnosticProvider::PullDiagnosticProvider(LanguageServerId(KeyData::from_ffi(1))); + + assert!(first_provider.equals(&second_provider)); + } + + #[test] + fn can_compare_language_server_id() { + let provider = + DiagnosticProvider::PublishDiagnosticProvider(LanguageServerId(KeyData::from_ffi(1))); + + let language_server_id = LanguageServerId(KeyData::from_ffi(1)); + + assert!(provider.has_server_id(&language_server_id)); + } +} diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index 92ab8261..57c82634 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -757,7 +757,7 @@ impl Application { DiagnosticProvider::PublishDiagnosticProvider(language_server.id()); self.editor.handle_lsp_diagnostics( - diagnostic_provider, + &diagnostic_provider, uri, params.version, params.diagnostics, @@ -871,7 +871,7 @@ impl Application { // an empty diagnostic list for said files for diags in self.editor.diagnostics.values_mut() { diags.retain(|(_, diagnostic_provider)| { - Into::<LanguageServerId>::into(*diagnostic_provider) != server_id + !diagnostic_provider.has_server_id(&server_id) }); } diff --git a/helix-term/src/handlers/diagnostics.rs b/helix-term/src/handlers/diagnostics.rs index a93fc396..bdd1408c 100644 --- a/helix-term/src/handlers/diagnostics.rs +++ b/helix-term/src/handlers/diagnostics.rs @@ -153,7 +153,7 @@ fn handle_pull_diagnostics_response( let (result_id, related_documents) = match response { lsp::DocumentDiagnosticReport::Full(report) => { editor.handle_lsp_diagnostics( - provider, + &provider, uri, None, report.full_document_diagnostic_report.items, @@ -181,7 +181,7 @@ fn handle_pull_diagnostics_response( continue; }; - editor.handle_lsp_diagnostics(provider, uri, None, report.items); + editor.handle_lsp_diagnostics(&provider, uri, None, report.items); report.result_id } lsp::DocumentDiagnosticReportKind::Unchanged(report) => Some(report.result_id), diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 5e066bb5..1398520b 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -1407,8 +1407,13 @@ impl Document { true }); - self.diagnostics - .sort_by_key(|diagnostic| (diagnostic.range, diagnostic.severity, diagnostic.provider)); + self.diagnostics.sort_by_key(|diagnostic| { + ( + diagnostic.range, + diagnostic.severity, + diagnostic.provider.clone(), + ) + }); // Update the inlay hint annotations' positions, helping ensure they are displayed in the proper place let apply_inlay_hint_changes = |annotations: &mut Vec<InlineAnnotation>| { @@ -2028,13 +2033,16 @@ impl Document { &mut self, diagnostics: impl IntoIterator<Item = Diagnostic>, unchanged_sources: &[String], - diagnostic_provider: Option<DiagnosticProvider>, + diagnostic_provider: Option<&DiagnosticProvider>, ) { if unchanged_sources.is_empty() { self.clear_diagnostics(diagnostic_provider); } else { self.diagnostics.retain(|d| { - if diagnostic_provider.is_some_and(|provider| provider != d.provider) { + if diagnostic_provider + .as_ref() + .is_some_and(|provider| !provider.equals(&d.provider)) + { return true; } @@ -2046,14 +2054,19 @@ impl Document { }); } self.diagnostics.extend(diagnostics); - self.diagnostics - .sort_by_key(|diagnostic| (diagnostic.range, diagnostic.severity, diagnostic.provider)); + self.diagnostics.sort_by_key(|diagnostic| { + ( + diagnostic.range, + diagnostic.severity, + diagnostic.provider.clone(), + ) + }); } /// clears diagnostics for a given diagnostic provider if set, otherwise all diagnostics are cleared - pub fn clear_diagnostics(&mut self, provider: Option<DiagnosticProvider>) { + pub fn clear_diagnostics(&mut self, provider: Option<&DiagnosticProvider>) { if let Some(provider) = provider { - self.diagnostics.retain(|d| d.provider != provider); + self.diagnostics.retain(|d| !d.provider.equals(provider)); } else { self.diagnostics.clear(); } @@ -2062,7 +2075,8 @@ impl Document { /// clears diagnostics for a given language_server if set, otherwise all diagnostics are cleared pub fn clear_all_language_server_diagnostics(&mut self, server_id: Option<LanguageServerId>) { if let Some(server_id) = server_id { - self.diagnostics.retain(|d| server_id != d.provider.into()); + self.diagnostics + .retain(|d| !d.provider.has_server_id(&server_id)); } else { self.diagnostics.clear(); } diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs index bcfe68d7..041efb3b 100644 --- a/helix-view/src/editor.rs +++ b/helix-view/src/editor.rs @@ -2030,8 +2030,7 @@ impl Editor { diags .iter() .filter_map(move |(diagnostic, diagnostic_provider)| { - let ls = language_servers - .get_by_id(Into::<LanguageServerId>::into(*diagnostic_provider))?; + let ls = language_servers.get_by_id(*diagnostic_provider.server_id())?; language_config .as_ref() .and_then(|c| { @@ -2041,12 +2040,12 @@ impl Editor { }) }) .and_then(|_| { - if filter(diagnostic, *diagnostic_provider) { + if filter(diagnostic, diagnostic_provider.clone()) { Document::lsp_diagnostic_to_diagnostic( &text, language_config.as_deref(), diagnostic, - *diagnostic_provider, + diagnostic_provider.clone(), ls.offset_encoding(), ) } else { diff --git a/helix-view/src/gutter.rs b/helix-view/src/gutter.rs index b8456ffc..5e5c1182 100644 --- a/helix-view/src/gutter.rs +++ b/helix-view/src/gutter.rs @@ -71,7 +71,7 @@ pub fn diagnostic<'doc>( d.line == line && doc .language_servers_with_feature(LanguageServerFeature::Diagnostics) - .any(|ls| ls.id() == d.provider.into()) + .any(|ls| d.provider.has_server_id(&ls.id())) }); diagnostics_on_line.max_by_key(|d| d.severity).map(|d| { write!(out, "●").ok(); diff --git a/helix-view/src/handlers/lsp.rs b/helix-view/src/handlers/lsp.rs index b9932727..49d03905 100644 --- a/helix-view/src/handlers/lsp.rs +++ b/helix-view/src/handlers/lsp.rs @@ -281,7 +281,7 @@ impl Editor { pub fn handle_lsp_diagnostics( &mut self, - diagnostic_provider: DiagnosticProvider, + diagnostic_provider: &DiagnosticProvider, uri: Uri, version: Option<i32>, mut diagnostics: Vec<lsp::Diagnostic>, @@ -314,8 +314,8 @@ impl Editor { .filter(|d| d.source.as_ref() == Some(source)); let old_diagnostics = old_diagnostics .iter() - .filter(|(d, d_server)| { - *d_server == diagnostic_provider && d.source.as_ref() == Some(source) + .filter(|(d, d_provider)| { + diagnostic_provider.equals(d_provider) && d.source.as_ref() == Some(source) }) .map(|(d, _)| d); if new_diagnostics.eq(old_diagnostics) { @@ -324,7 +324,9 @@ impl Editor { } } - let diagnostics = diagnostics.into_iter().map(|d| (d, diagnostic_provider)); + let diagnostics = diagnostics + .into_iter() + .map(|d| (d, diagnostic_provider.clone())); // Insert the original lsp::Diagnostics here because we may have no open document // for diagnostic message and so we can't calculate the exact position. @@ -333,7 +335,8 @@ impl Editor { Entry::Occupied(o) => { let current_diagnostics = o.into_mut(); // there may entries of other language servers, which is why we can't overwrite the whole entry - current_diagnostics.retain(|(_, lsp_id)| *lsp_id != diagnostic_provider); + current_diagnostics + .retain(|(_, ls_provider)| !diagnostic_provider.equals(ls_provider)); current_diagnostics.extend(diagnostics); current_diagnostics // Sort diagnostics first by severity and then by line numbers. @@ -343,12 +346,13 @@ impl Editor { // Sort diagnostics first by severity and then by line numbers. // Note: The `lsp::DiagnosticSeverity` enum is already defined in decreasing order - diagnostics.sort_by_key(|(d, server_id)| (d.severity, d.range.start, *server_id)); + diagnostics + .sort_by_key(|(d, ls_provider)| (d.severity, d.range.start, ls_provider.clone())); if let Some(doc) = doc { let diagnostic_of_language_server_and_not_in_unchanged_sources = - |diagnostic: &lsp::Diagnostic, ls_id| { - ls_id == diagnostic_provider + |diagnostic: &lsp::Diagnostic, ls_provider| { + diagnostic_provider.equals(&ls_provider) && diagnostic .source .as_ref() From 14982fb5251cbf5ae61d01e2e1ec7243195e1b2c Mon Sep 17 00:00:00 2001 From: Sofus Addington <sofus@addington.dk> Date: Tue, 4 Mar 2025 06:01:08 +0100 Subject: [PATCH 5/9] Add identifier to diagnostic provider --- helix-core/src/diagnostic.rs | 153 +++++++++++++++++++------ helix-term/src/application.rs | 2 +- helix-term/src/handlers/diagnostics.rs | 13 ++- 3 files changed, 128 insertions(+), 40 deletions(-) diff --git a/helix-core/src/diagnostic.rs b/helix-core/src/diagnostic.rs index a133fae1..3f326037 100644 --- a/helix-core/src/diagnostic.rs +++ b/helix-core/src/diagnostic.rs @@ -53,15 +53,75 @@ pub struct Diagnostic { // TODO turn this into a feature flag when lsp becomes optional #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] pub enum DiagnosticProvider { - PublishDiagnosticProvider(LanguageServerId), - PullDiagnosticProvider(LanguageServerId), + Lsp { + server_id: LanguageServerId, + identifier: Option<String>, + }, + // In the future, other non-LSP providers like spell checking go here... +} + +impl DiagnosticProvider { + pub fn from_server_id(server_id: LanguageServerId) -> DiagnosticProvider { + DiagnosticProvider::Lsp { + server_id, + identifier: None, + } + } + + pub fn from_server_and_identifier( + server_id: LanguageServerId, + identifier: Option<String>, + ) -> DiagnosticProvider { + DiagnosticProvider::Lsp { + server_id, + identifier, + } + } + + pub fn server_id(&self) -> &LanguageServerId { + match self { + DiagnosticProvider::Lsp { + server_id, + identifier: _, + } => server_id, + } + } + + pub fn has_server_id(&self, server_id: &LanguageServerId) -> bool { + match self { + DiagnosticProvider::Lsp { + server_id: id, + identifier: _, + } => server_id == id, + } + } + + pub fn equals(&self, diagnostic_provider: &DiagnosticProvider) -> bool { + let (other_identifier, other_server_id) = match diagnostic_provider { + DiagnosticProvider::Lsp { + server_id, + identifier, + } => (identifier, server_id), + }; + + let (identifier, server_id) = match self { + DiagnosticProvider::Lsp { + server_id, + identifier, + } => (identifier, server_id), + }; + + identifier == other_identifier && server_id == other_server_id + } } impl From<DiagnosticProvider> for LanguageServerId { fn from(value: DiagnosticProvider) -> Self { match value { - DiagnosticProvider::PublishDiagnosticProvider(id) => id, - DiagnosticProvider::PullDiagnosticProvider(id) => id, + DiagnosticProvider::Lsp { + server_id, + identifier: _, + } => server_id, } } } @@ -74,33 +134,6 @@ slotmap::new_key_type! { pub struct LanguageServerId; } -impl DiagnosticProvider { - pub fn server_id(&self) -> &LanguageServerId { - match self { - DiagnosticProvider::PublishDiagnosticProvider(language_server_id) => language_server_id, - DiagnosticProvider::PullDiagnosticProvider(language_server_id) => language_server_id, - } - } - - pub fn has_server_id(&self, server_id: &LanguageServerId) -> bool { - match self { - DiagnosticProvider::PublishDiagnosticProvider(language_server_id) => { - language_server_id == server_id - } - DiagnosticProvider::PullDiagnosticProvider(language_server_id) => { - language_server_id == server_id - } - } - } - - pub fn equals(&self, diagnostic_provider: &DiagnosticProvider) -> bool { - let server_id = self.server_id(); - let other_server_id = diagnostic_provider.server_id(); - - server_id == other_server_id - } -} - impl fmt::Display for LanguageServerId { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{:?}", self.0) @@ -124,9 +157,23 @@ mod tests { #[test] fn can_compare_equal_diagnostic_provider() { let first_provider = - DiagnosticProvider::PublishDiagnosticProvider(LanguageServerId(KeyData::from_ffi(1))); + DiagnosticProvider::from_server_id(LanguageServerId(KeyData::from_ffi(1))); let second_provider = - DiagnosticProvider::PublishDiagnosticProvider(LanguageServerId(KeyData::from_ffi(1))); + DiagnosticProvider::from_server_id(LanguageServerId(KeyData::from_ffi(1))); + + assert!(first_provider.equals(&second_provider)); + } + + #[test] + fn can_compare_equal_diagnostic_provider_with_identifier() { + let first_provider = DiagnosticProvider::from_server_and_identifier( + LanguageServerId(KeyData::from_ffi(1)), + Some("provider".to_string()), + ); + let second_provider = DiagnosticProvider::from_server_and_identifier( + LanguageServerId(KeyData::from_ffi(1)), + Some("provider".to_string()), + ); assert!(first_provider.equals(&second_provider)); } @@ -134,17 +181,47 @@ mod tests { #[test] fn can_distinguish_diagnostic_provider() { let first_provider = - DiagnosticProvider::PublishDiagnosticProvider(LanguageServerId(KeyData::from_ffi(1))); + DiagnosticProvider::from_server_id(LanguageServerId(KeyData::from_ffi(1))); let second_provider = - DiagnosticProvider::PullDiagnosticProvider(LanguageServerId(KeyData::from_ffi(1))); + DiagnosticProvider::from_server_id(LanguageServerId(KeyData::from_ffi(2))); - assert!(first_provider.equals(&second_provider)); + assert!(!first_provider.equals(&second_provider)); + } + + #[test] + fn can_distinguish_diagnostic_provider_by_identifier() { + let first_provider = DiagnosticProvider::from_server_and_identifier( + LanguageServerId(KeyData::from_ffi(1)), + Some("provider".to_string()), + ); + let second_provider = DiagnosticProvider::from_server_and_identifier( + LanguageServerId(KeyData::from_ffi(1)), + None, + ); + + assert!(!first_provider.equals(&second_provider)); + } + + #[test] + fn can_distinguish_diagnostic_provider_by_language_server_id() { + let first_provider = DiagnosticProvider::from_server_and_identifier( + LanguageServerId(KeyData::from_ffi(1)), + Some("provider".to_string()), + ); + let second_provider = DiagnosticProvider::from_server_and_identifier( + LanguageServerId(KeyData::from_ffi(2)), + Some("provider".to_string()), + ); + + assert!(!first_provider.equals(&second_provider)); } #[test] fn can_compare_language_server_id() { - let provider = - DiagnosticProvider::PublishDiagnosticProvider(LanguageServerId(KeyData::from_ffi(1))); + let provider = DiagnosticProvider::from_server_and_identifier( + LanguageServerId(KeyData::from_ffi(1)), + Some("provider".to_string()), + ); let language_server_id = LanguageServerId(KeyData::from_ffi(1)); diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index 57c82634..6e66041b 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -754,7 +754,7 @@ impl Application { } let diagnostic_provider = - DiagnosticProvider::PublishDiagnosticProvider(language_server.id()); + DiagnosticProvider::from_server_id(language_server.id()); self.editor.handle_lsp_diagnostics( &diagnostic_provider, diff --git a/helix-term/src/handlers/diagnostics.rs b/helix-term/src/handlers/diagnostics.rs index bdd1408c..c04b6e9a 100644 --- a/helix-term/src/handlers/diagnostics.rs +++ b/helix-term/src/handlers/diagnostics.rs @@ -122,7 +122,18 @@ pub fn pull_diagnostics_for_document( return; }; - let provider = DiagnosticProvider::PullDiagnosticProvider(language_server.id()); + let identifier = language_server + .capabilities() + .diagnostic_provider + .as_ref() + .and_then(|diagnostic_provider| match diagnostic_provider { + lsp::DiagnosticServerCapabilities::Options(options) => options.identifier.clone(), + lsp::DiagnosticServerCapabilities::RegistrationOptions(options) => { + options.diagnostic_options.identifier.clone() + } + }); + + let provider = DiagnosticProvider::from_server_and_identifier(language_server.id(), identifier); let document_id = doc.id(); tokio::spawn(async move { From 9be3681f0eadf1bf4d459b9a3980678b2eee5488 Mon Sep 17 00:00:00 2001 From: Sofus Addington <sofus@addington.dk> Date: Thu, 6 Mar 2025 19:08:02 +0100 Subject: [PATCH 6/9] Drop hashset and query for all open documents --- helix-term/src/handlers/diagnostics.rs | 29 ++++++++++++++++---------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/helix-term/src/handlers/diagnostics.rs b/helix-term/src/handlers/diagnostics.rs index c04b6e9a..ac2df70a 100644 --- a/helix-term/src/handlers/diagnostics.rs +++ b/helix-term/src/handlers/diagnostics.rs @@ -1,4 +1,3 @@ -use std::collections::HashSet; use std::time::Duration; use helix_core::diagnostic::DiagnosticProvider; @@ -58,15 +57,11 @@ pub(super) fn register_hooks(handlers: &Handlers) { } #[derive(Debug)] -pub(super) struct PullDiagnosticsHandler { - document_ids: HashSet<DocumentId>, -} +pub(super) struct PullDiagnosticsHandler {} impl PullDiagnosticsHandler { pub fn new() -> PullDiagnosticsHandler { - PullDiagnosticsHandler { - document_ids: [].into(), - } + PullDiagnosticsHandler {} } } @@ -82,14 +77,11 @@ impl helix_event::AsyncHook for PullDiagnosticsHandler { dispatch_pull_diagnostic_for_document(event.document_id); } - self.document_ids.insert(event.document_id); Some(Instant::now() + Duration::from_millis(500)) } fn finish_debounce(&mut self) { - for document_id in self.document_ids.clone() { - dispatch_pull_diagnostic_for_document(document_id); - } + dispatch_pull_diagnostic_for_open_documents(); } } @@ -108,6 +100,21 @@ fn dispatch_pull_diagnostic_for_document(document_id: DocumentId) { }) } +fn dispatch_pull_diagnostic_for_open_documents() { + job::dispatch_blocking(move |editor, _| { + let documents = editor.documents.values(); + + for document in documents { + let language_servers = + document.language_servers_with_feature(LanguageServerFeature::PullDiagnostics); + + for language_server in language_servers { + pull_diagnostics_for_document(document, language_server); + } + } + }) +} + pub fn pull_diagnostics_for_document( doc: &helix_view::Document, language_server: &helix_lsp::Client, From b8f47d7ca9d6499cb523cd29a588711a4570dc2b Mon Sep 17 00:00:00 2001 From: Sofus Addington <sofus@addington.dk> Date: Sat, 8 Mar 2025 06:11:46 +0100 Subject: [PATCH 7/9] Separate timeout for language servers without interfile dependencies --- helix-term/src/handlers/diagnostics.rs | 55 +++++++++++++++++++++----- 1 file changed, 46 insertions(+), 9 deletions(-) diff --git a/helix-term/src/handlers/diagnostics.rs b/helix-term/src/handlers/diagnostics.rs index ac2df70a..39799c5f 100644 --- a/helix-term/src/handlers/diagnostics.rs +++ b/helix-term/src/handlers/diagnostics.rs @@ -57,27 +57,45 @@ pub(super) fn register_hooks(handlers: &Handlers) { } #[derive(Debug)] -pub(super) struct PullDiagnosticsHandler {} +pub(super) struct PullDiagnosticsHandler { + no_inter_file_dependency_timeout: Option<tokio::time::Instant>, +} impl PullDiagnosticsHandler { pub fn new() -> PullDiagnosticsHandler { - PullDiagnosticsHandler {} + PullDiagnosticsHandler { + no_inter_file_dependency_timeout: None, + } } } +const TIMEOUT: Duration = Duration::from_millis(500); +const TIMEOUT_NO_INTER_FILE_DEPENDENCY: Duration = Duration::from_millis(125); + impl helix_event::AsyncHook for PullDiagnosticsHandler { type Event = PullDiagnosticsEvent; fn handle_event( &mut self, event: Self::Event, - existing_debounce: Option<tokio::time::Instant>, + timeout: Option<tokio::time::Instant>, ) -> Option<tokio::time::Instant> { - if existing_debounce.is_none() { - dispatch_pull_diagnostic_for_document(event.document_id); + if timeout.is_none() { + dispatch_pull_diagnostic_for_document(event.document_id, false); + self.no_inter_file_dependency_timeout = Some(Instant::now()); } - Some(Instant::now() + Duration::from_millis(500)) + if self + .no_inter_file_dependency_timeout + .is_some_and(|nifd_timeout| { + nifd_timeout.duration_since(Instant::now()) > TIMEOUT_NO_INTER_FILE_DEPENDENCY + }) + { + dispatch_pull_diagnostic_for_document(event.document_id, true); + self.no_inter_file_dependency_timeout = Some(Instant::now()); + }; + + Some(Instant::now() + TIMEOUT) } fn finish_debounce(&mut self) { @@ -85,14 +103,33 @@ impl helix_event::AsyncHook for PullDiagnosticsHandler { } } -fn dispatch_pull_diagnostic_for_document(document_id: DocumentId) { +fn dispatch_pull_diagnostic_for_document( + document_id: DocumentId, + exclude_language_servers_without_inter_file_dependency: bool, +) { job::dispatch_blocking(move |editor, _| { let Some(doc) = editor.document(document_id) else { return; }; - let language_servers = - doc.language_servers_with_feature(LanguageServerFeature::PullDiagnostics); + let language_servers = doc + .language_servers_with_feature(LanguageServerFeature::PullDiagnostics) + .filter(|ls| { + if !exclude_language_servers_without_inter_file_dependency { + return true; + }; + ls.capabilities() + .diagnostic_provider + .as_ref() + .is_some_and(|dp| match dp { + lsp::DiagnosticServerCapabilities::Options(options) => { + options.inter_file_dependencies + } + lsp::DiagnosticServerCapabilities::RegistrationOptions(options) => { + options.diagnostic_options.inter_file_dependencies + } + }) + }); for language_server in language_servers { pull_diagnostics_for_document(doc, language_server); From a9c2953001771e6432fb242ac2eeecff44431268 Mon Sep 17 00:00:00 2001 From: Sofus Addington <sofus@addington.dk> Date: Thu, 6 Mar 2025 16:55:19 +0100 Subject: [PATCH 8/9] Retry on cancelled request --- helix-term/src/handlers/diagnostics.rs | 30 ++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/helix-term/src/handlers/diagnostics.rs b/helix-term/src/handlers/diagnostics.rs index 39799c5f..4f537afb 100644 --- a/helix-term/src/handlers/diagnostics.rs +++ b/helix-term/src/handlers/diagnostics.rs @@ -177,7 +177,8 @@ pub fn pull_diagnostics_for_document( } }); - let provider = DiagnosticProvider::from_server_and_identifier(language_server.id(), identifier); + let language_server_id = language_server.id(); + let provider = DiagnosticProvider::from_server_and_identifier(language_server_id, identifier); let document_id = doc.id(); tokio::spawn(async move { @@ -193,7 +194,32 @@ pub fn pull_diagnostics_for_document( }) .await } - Err(err) => log::error!("Pull diagnostic request failed: {err}"), + Err(err) => { + let parsed_cancellation_data = if let helix_lsp::Error::Rpc(error) = err { + error.data.and_then(|data| { + serde_json::from_value::<lsp::DiagnosticServerCancellationData>(data).ok() + }) + } else { + log::error!("Pull diagnostic request failed: {err}"); + return; + }; + + if let Some(parsed_cancellation_data) = parsed_cancellation_data { + if parsed_cancellation_data.retrigger_request { + tokio::time::sleep(Duration::from_millis(500)).await; + + job::dispatch(move |editor, _| { + if let (Some(doc), Some(language_server)) = ( + editor.document(document_id), + editor.language_server_by_id(language_server_id), + ) { + pull_diagnostics_for_document(doc, language_server); + } + }) + .await; + } + } + } } }); } From e54021fe0abdcf87d4606bc868150d5270269c0e Mon Sep 17 00:00:00 2001 From: Sofus Addington <sofus@addington.dk> Date: Wed, 12 Mar 2025 11:07:13 +0100 Subject: [PATCH 9/9] Filter un-initialized language servers --- helix-term/src/handlers/diagnostics.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/helix-term/src/handlers/diagnostics.rs b/helix-term/src/handlers/diagnostics.rs index 4f537afb..96bac8a4 100644 --- a/helix-term/src/handlers/diagnostics.rs +++ b/helix-term/src/handlers/diagnostics.rs @@ -114,6 +114,7 @@ fn dispatch_pull_diagnostic_for_document( let language_servers = doc .language_servers_with_feature(LanguageServerFeature::PullDiagnostics) + .filter(|ls| ls.is_initialized()) .filter(|ls| { if !exclude_language_servers_without_inter_file_dependency { return true; @@ -142,8 +143,9 @@ fn dispatch_pull_diagnostic_for_open_documents() { let documents = editor.documents.values(); for document in documents { - let language_servers = - document.language_servers_with_feature(LanguageServerFeature::PullDiagnostics); + let language_servers = document + .language_servers_with_feature(LanguageServerFeature::PullDiagnostics) + .filter(|ls| ls.is_initialized()); for language_server in language_servers { pull_diagnostics_for_document(document, language_server);