From c65b0d9dfdd0c2c6ee9bc3d6e3144dde2e44125c Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Sun, 26 Mar 2023 18:10:09 +0200 Subject: [PATCH 1/3] Add config to mark diagnostic sources as persistent --- book/src/languages.md | 1 + helix-core/src/syntax.rs | 2 + helix-term/src/application.rs | 105 ++++++++++++++++++++++------------ helix-view/src/document.rs | 21 ++++++- languages.toml | 1 + 5 files changed, 90 insertions(+), 40 deletions(-) diff --git a/book/src/languages.md b/book/src/languages.md index 5e56a332fd65..ce615ce61b01 100644 --- a/book/src/languages.md +++ b/book/src/languages.md @@ -68,6 +68,7 @@ These configuration keys are available: | `formatter` | The formatter for the language, it will take precedence over the lsp when defined. The formatter must be able to take the original file as input from stdin and write the formatted file to stdout | | `text-width` | Maximum line length. Used for the `:reflow` command and soft-wrapping if `soft-wrap.wrap-at-text-width` is set, defaults to `editor.text-width` | | `workspace-lsp-roots` | Directories relative to the workspace root that are treated as LSP roots. Should only be set in `.helix/config.toml`. Overwrites the setting of the same name in `config.toml` if set. | +| `persistent-diagnostic-sources` | An array of LSP diagnostic sources assumed unchanged when the language server resends the same set of diagnostics. Helix can track the position for these diagnostics internally instead. Useful for diagnostics that are recomputed on save. ### File-type detection and the `file-types` key diff --git a/helix-core/src/syntax.rs b/helix-core/src/syntax.rs index f43b03ade7ac..fda1a2a7a5e5 100644 --- a/helix-core/src/syntax.rs +++ b/helix-core/src/syntax.rs @@ -154,6 +154,8 @@ pub struct LanguageConfiguration { /// Hardcoded LSP root directories relative to the workspace root, like `examples` or `tools/fuzz`. /// Falling back to the current working directory if none are configured. pub workspace_lsp_roots: Option>, + #[serde(default)] + pub persistent_diagnostic_sources: Vec, } #[derive(Debug, PartialEq, Eq, Hash)] diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index e1a622f9d06b..3baf1ce4b7c3 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -738,7 +738,7 @@ impl Application { )); } } - Notification::PublishDiagnostics(params) => { + Notification::PublishDiagnostics(mut params) => { let path = match params.uri.to_file_path() { Ok(path) => path, Err(_) => { @@ -752,31 +752,69 @@ impl Application { return; } let offset_encoding = language_server.offset_encoding(); - let doc = self.editor.document_by_path_mut(&path).filter(|doc| { - if let Some(version) = params.version { - if version != doc.version() { - log::info!("Version ({version}) is out of date for {path:?} (expected ({}), dropping PublishDiagnostic notification", doc.version()); - return false; + // have to inline the function because of borrow checking... + let doc = self.editor.documents.values_mut() + .find(|doc| doc.path().map(|p| p == &path).unwrap_or(false)) + .filter(|doc| { + if let Some(version) = params.version { + if version != doc.version() { + log::info!("Version ({version}) is out of date for {path:?} (expected ({}), dropping PublishDiagnostic notification", doc.version()); + return false; + } } - } - - true - }); + true + }); if let Some(doc) = doc { - let lang_conf = doc.language_config(); - let text = doc.text(); + let lang_conf = doc.language.clone(); + let text = doc.text().clone(); + + let mut unchaged_diag_sources_ = Vec::new(); + if let Some(lang_conf) = &lang_conf { + if let Some(old_diagnostics) = + self.editor.diagnostics.get(¶ms.uri) + { + if !lang_conf.persistent_diagnostic_sources.is_empty() { + // Sort diagnostics first by severity and then by line numbers. + // Note: The `lsp::DiagnosticSeverity` enum is already defined in decreasing order + params + .diagnostics + .sort_unstable_by_key(|d| (d.severity, d.range.start)); + } + for source in &lang_conf.persistent_diagnostic_sources { + let new_diagnostics = params + .diagnostics + .iter() + .filter(|d| d.source.as_ref() == Some(source)); + let old_diagnostics = old_diagnostics + .iter() + .filter(|(d, d_server)| { + *d_server == server_id + && d.source.as_ref() == Some(source) + }) + .map(|(d, _)| d); + if new_diagnostics.eq(old_diagnostics) { + unchaged_diag_sources_.push(source.clone()) + } + } + } + } - let diagnostics = params - .diagnostics - .iter() - .filter_map(|diagnostic| { + let unchaged_diag_sources = &unchaged_diag_sources_; + let diagnostics = + params.diagnostics.iter().filter_map(move |diagnostic| { use helix_core::diagnostic::{Diagnostic, Range, Severity::*}; use lsp::DiagnosticSeverity; + if diagnostic.source.as_ref().map_or(false, |source| { + unchaged_diag_sources.contains(source) + }) { + return None; + } + // TODO: convert inside server let start = if let Some(start) = lsp_pos_to_pos( - text, + &text, diagnostic.range.start, offset_encoding, ) { @@ -787,7 +825,7 @@ impl Application { }; let end = if let Some(end) = - lsp_pos_to_pos(text, diagnostic.range.end, offset_encoding) + lsp_pos_to_pos(&text, diagnostic.range.end, offset_encoding) { end } else { @@ -807,7 +845,7 @@ impl Application { ), }); - if let Some(lang_conf) = lang_conf { + if let Some(lang_conf) = &lang_conf { if let Some(severity) = severity { if severity < lang_conf.diagnostic_severity { return None; @@ -857,38 +895,31 @@ impl Application { data: diagnostic.data.clone(), language_server_id: server_id, }) - }) - .collect(); + }); - doc.replace_diagnostics(diagnostics, server_id); + doc.replace_diagnostics(diagnostics, unchaged_diag_sources, server_id); } - let mut diagnostics = params - .diagnostics - .into_iter() - .map(|d| (d, server_id)) - .collect(); + let diagnostics = params.diagnostics.into_iter().map(|d| (d, server_id)); // Insert the original lsp::Diagnostics here because we may have no open document // for diagnosic message and so we can't calculate the exact position. // When using them later in the diagnostics picker, we calculate them on-demand. - match self.editor.diagnostics.entry(params.uri) { + let diagnostics = match self.editor.diagnostics.entry(params.uri) { 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.append(&mut diagnostics); - // Sort diagnostics first by severity and then by line numbers. - // Note: The `lsp::DiagnosticSeverity` enum is already defined in decreasing order + current_diagnostics.extend(diagnostics); current_diagnostics - .sort_unstable_by_key(|(d, _)| (d.severity, d.range.start)); - } - Entry::Vacant(v) => { - diagnostics - .sort_unstable_by_key(|(d, _)| (d.severity, d.range.start)); - v.insert(diagnostics); + // Sort diagnostics first by severity and then by line numbers. } + Entry::Vacant(v) => v.insert(diagnostics.collect()), }; + + // Sort diagnostics first by severity and then by line numbers. + // Note: The `lsp::DiagnosticSeverity` enum is already defined in decreasing order + diagnostics.sort_unstable_by_key(|(d, _)| (d.severity, d.range.start)); } Notification::ShowMessage(params) => { log::warn!("unhandled window/showMessage: {:?}", params); diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index b08370f9f371..ebd621d2a3b7 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -1667,11 +1667,26 @@ impl Document { pub fn replace_diagnostics( &mut self, - mut diagnostics: Vec, + diagnostics: impl IntoIterator, + unchanged_sources: &[String], language_server_id: usize, ) { - self.clear_diagnostics(language_server_id); - self.diagnostics.append(&mut diagnostics); + if unchanged_sources.is_empty() { + self.clear_diagnostics(language_server_id); + } else { + self.diagnostics.retain(|d| { + if d.language_server_id != language_server_id { + return true; + } + + if let Some(source) = &d.source { + unchanged_sources.contains(source) + } else { + false + } + }); + } + self.diagnostics.extend(diagnostics); self.diagnostics .sort_unstable_by_key(|diagnostic| diagnostic.range); } diff --git a/languages.toml b/languages.toml index 0c92df4e7300..9d3acc5a5ebf 100644 --- a/languages.toml +++ b/languages.toml @@ -151,6 +151,7 @@ auto-format = true comment-token = "//" language-servers = [ "rust-analyzer" ] indent = { tab-width = 4, unit = " " } +persistent-diagnostic-sources = ["rustc", "clippy"] [language.auto-pairs] '(' = ')' From caeeaea95242336c4864151f161b54769a228931 Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Sun, 26 Mar 2023 18:14:42 +0200 Subject: [PATCH 2/3] make diagnostics stick to word boundaries Diagnostics are currently extended if text is inserted at their end. This is desirable when inserting text after an identifier. For example consider: let foo = 2; --- unused variable Renaming the identifier should extend the diagnostic: let foobar = 2; ------ unused variable This is currently implemented in helix but as a consequence adding whitespaces or a type hint also extends the diagnostic: let foo = 2; -------- unused variable let foo: Bar = 2; -------- unused variable In these cases the diagnostic should remain unchanged: let foo = 2; --- unused variable let foo: Bar = 2; --- unused variable As a heuristic helix will now only extend diagnostics that end on a word char if new chars are appended to the word (so not for punctuation/ whitespace). The idea for this mapping was inspired for the word level tracking vscode uses for many positions. While VSCode doesn't currently update diagnostics after receiving publishDiagnostic it does use this system for inlay hints for example. Similarly, the new association mechanism implemented here can be used for word level tracking of inlay hints. A similar mapping function is implemented for word starts. Together these can be used to make a diagnostic stick to a word. If that word is removed that diagnostic is automatically removed too. This is the exact same behavior VSCode inlay hints eixibit. --- helix-core/src/diagnostic.rs | 4 ++ helix-core/src/transaction.rs | 87 +++++++++++++++++++++++++++++------ helix-term/src/application.rs | 11 ++++- helix-view/src/document.rs | 38 +++++++++------ 4 files changed, 112 insertions(+), 28 deletions(-) diff --git a/helix-core/src/diagnostic.rs b/helix-core/src/diagnostic.rs index 0b75d2a586f1..52d77a9aace5 100644 --- a/helix-core/src/diagnostic.rs +++ b/helix-core/src/diagnostic.rs @@ -39,6 +39,10 @@ pub enum DiagnosticTag { #[derive(Debug, Clone)] pub struct Diagnostic { pub range: Range, + // whether this diagnostic ends at the end of(or inside) a word + pub ends_at_word: bool, + pub starts_at_word: bool, + pub zero_width: bool, pub line: usize, pub message: String, pub severity: Option, diff --git a/helix-core/src/transaction.rs b/helix-core/src/transaction.rs index 9d2a3e5c43a5..263ba9ccc770 100644 --- a/helix-core/src/transaction.rs +++ b/helix-core/src/transaction.rs @@ -1,6 +1,6 @@ use smallvec::SmallVec; -use crate::{Range, Rope, Selection, Tendril}; +use crate::{chars::char_is_word, Range, Rope, Selection, Tendril}; use std::{borrow::Cow, iter::once}; /// (from, to, replacement) @@ -22,6 +22,30 @@ pub enum Operation { pub enum Assoc { Before, After, + /// Acts like `After` if a word character is inserted + /// after the position, otherwise acts like `Before` + AfterWord, + /// Acts like `Before` if a word character is inserted + /// before the position, otherwise acts like `After` + BeforeWord, +} + +impl Assoc { + /// Whether to stick to gaps + fn stay_at_gaps(self) -> bool { + !matches!(self, Self::BeforeWord | Self::AfterWord) + } + + fn insert_offset(self, s: &str) -> usize { + let chars = s.chars().count(); + match self { + Assoc::After => chars, + Assoc::AfterWord => s.chars().take_while(|&c| char_is_word(c)).count(), + // return position before inserted text + Assoc::Before => 0, + Assoc::BeforeWord => chars - s.chars().rev().take_while(|&c| char_is_word(c)).count(), + } + } } #[derive(Debug, Default, Clone, PartialEq, Eq)] @@ -411,8 +435,6 @@ impl ChangeSet { map!(|pos, _| (old_end > pos).then_some(new_pos), i); } Insert(s) => { - let ins = s.chars().count(); - // a subsequent delete means a replace, consume it if let Some((_, Delete(len))) = iter.peek() { iter.next(); @@ -420,13 +442,13 @@ impl ChangeSet { old_end = old_pos + len; // in range of replaced text map!( - |pos, assoc| (old_end > pos).then(|| { + |pos, assoc: Assoc| (old_end > pos).then(|| { // at point or tracking before - if pos == old_pos || assoc == Assoc::Before { + if pos == old_pos && assoc.stay_at_gaps() { new_pos } else { // place to end of insert - new_pos + ins + new_pos + assoc.insert_offset(s) } }), i @@ -434,20 +456,15 @@ impl ChangeSet { } else { // at insert point map!( - |pos, assoc| (old_pos == pos).then(|| { + |pos, assoc: Assoc| (old_pos == pos).then(|| { // return position before inserted text - if assoc == Assoc::Before { - new_pos - } else { - // after text - new_pos + ins - } + new_pos + assoc.insert_offset(s) }), i ); } - new_pos += ins; + new_pos += s.chars().count(); } } old_pos = old_end; @@ -880,6 +897,48 @@ mod test { let mut positions = [4, 2]; cs.update_positions(positions.iter_mut().map(|pos| (pos, Assoc::After))); assert_eq!(positions, [4, 2]); + // stays at word boundary + let cs = ChangeSet { + changes: vec![ + Retain(2), // + Insert(" ab".into()), + Retain(2), // cd + Insert("de ".into()), + ], + len: 4, + len_after: 10, + }; + assert_eq!(cs.map_pos(2, Assoc::BeforeWord), 3); + assert_eq!(cs.map_pos(4, Assoc::AfterWord), 9); + let cs = ChangeSet { + changes: vec![ + Retain(1), // + Insert(" b".into()), + Delete(1), // c + Retain(1), // d + Insert("e ".into()), + Delete(1), // + ], + len: 5, + len_after: 7, + }; + assert_eq!(cs.map_pos(1, Assoc::BeforeWord), 2); + assert_eq!(cs.map_pos(3, Assoc::AfterWord), 5); + let cs = ChangeSet { + changes: vec![ + Retain(1), // + Insert("a".into()), + Delete(2), // b + Retain(1), // d + Insert("e".into()), + Delete(1), // f + Retain(1), // + ], + len: 5, + len_after: 7, + }; + assert_eq!(cs.map_pos(2, Assoc::BeforeWord), 1); + assert_eq!(cs.map_pos(4, Assoc::AfterWord), 4); } #[test] diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index 3baf1ce4b7c3..f8140f986cdd 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -1,6 +1,7 @@ use arc_swap::{access::Map, ArcSwap}; use futures_util::Stream; use helix_core::{ + chars::char_is_word, diagnostic::{DiagnosticTag, NumberOrString}, path::get_relative_path, pos_at_coords, syntax, Selection, @@ -832,7 +833,6 @@ impl Application { log::warn!("lsp position out of bounds - {:?}", diagnostic); return None; }; - let severity = diagnostic.severity.map(|severity| match severity { DiagnosticSeverity::ERROR => Error, @@ -884,8 +884,17 @@ impl Application { Vec::new() }; + let ends_at_word = start != end + && end != 0 + && text.get_char(end - 1).map_or(false, char_is_word); + let starts_at_word = start != end + && text.get_char(start).map_or(false, char_is_word); + Some(Diagnostic { range: Range { start, end }, + ends_at_word, + starts_at_word, + zero_width: start == end, line: diagnostic.range.start.line as usize, message: diagnostic.message.clone(), severity, diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index ebd621d2a3b7..654e726d3161 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -1174,20 +1174,32 @@ impl Document { let changes = transaction.changes(); - changes.update_positions( - self.diagnostics - .iter_mut() - .map(|diagnostic| (&mut diagnostic.range.start, Assoc::After)), - ); - changes.update_positions( - self.diagnostics - .iter_mut() - .map(|diagnostic| (&mut diagnostic.range.end, Assoc::After)), - ); - // map state.diagnostics over changes::map_pos too - for diagnostic in &mut self.diagnostics { + // map diagnostics over changes too + changes.update_positions(self.diagnostics.iter_mut().map(|diagnostic| { + let assoc = if diagnostic.starts_at_word { + Assoc::BeforeWord + } else { + Assoc::After + }; + (&mut diagnostic.range.start, assoc) + })); + changes.update_positions(self.diagnostics.iter_mut().map(|diagnostic| { + let assoc = if diagnostic.ends_at_word { + Assoc::AfterWord + } else { + Assoc::Before + }; + (&mut diagnostic.range.end, assoc) + })); + self.diagnostics.retain_mut(|diagnostic| { + if diagnostic.range.start > diagnostic.range.end + || (!diagnostic.zero_width && diagnostic.range.start == diagnostic.range.end) + { + return false; + } diagnostic.line = self.text.char_to_line(diagnostic.range.start); - } + true + }); self.diagnostics .sort_unstable_by_key(|diagnostic| diagnostic.range); From 6b6d96f533655fdd8a5efbc739e683fb6168034c Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Sat, 20 May 2023 21:29:23 +0200 Subject: [PATCH 3/3] consistent diagnostic sorting --- helix-term/src/application.rs | 4 +++- helix-view/src/document.rs | 18 ++++++++++++++---- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index f8140f986cdd..1aec7fa2ebbd 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -928,7 +928,9 @@ impl Application { // Sort diagnostics first by severity and then by line numbers. // Note: The `lsp::DiagnosticSeverity` enum is already defined in decreasing order - diagnostics.sort_unstable_by_key(|(d, _)| (d.severity, d.range.start)); + diagnostics.sort_unstable_by_key(|(d, server_id)| { + (d.severity, d.range.start, *server_id) + }); } Notification::ShowMessage(params) => { log::warn!("unhandled window/showMessage: {:?}", params); diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 654e726d3161..a2f142ded6ec 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -1201,8 +1201,13 @@ impl Document { true }); - self.diagnostics - .sort_unstable_by_key(|diagnostic| diagnostic.range); + self.diagnostics.sort_unstable_by_key(|diagnostic| { + ( + diagnostic.range, + diagnostic.severity, + diagnostic.language_server_id, + ) + }); // Update the inlay hint annotations' positions, helping ensure they are displayed in the proper place let apply_inlay_hint_changes = |annotations: &mut Rc<[InlineAnnotation]>| { @@ -1699,8 +1704,13 @@ impl Document { }); } self.diagnostics.extend(diagnostics); - self.diagnostics - .sort_unstable_by_key(|diagnostic| diagnostic.range); + self.diagnostics.sort_unstable_by_key(|diagnostic| { + ( + diagnostic.range, + diagnostic.severity, + diagnostic.language_server_id, + ) + }); } pub fn clear_diagnostics(&mut self, language_server_id: usize) {