Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Track long lived diagnostics inside helix #6447

Merged
merged 3 commits into from
Dec 27, 2023
Merged

Track long lived diagnostics inside helix #6447

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c65b0d9
Add config to mark diagnostic sources as persistent
pascalkuthe Mar 26, 2023
caeeaea
make diagnostics stick to word boundaries
pascalkuthe Mar 26, 2023
6b6d96f
consistent diagnostic sorting
pascalkuthe May 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions book/src/languages.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
4 changes: 4 additions & 0 deletions helix-core/src/diagnostic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
pascalkuthe marked this conversation as resolved.
Show resolved Hide resolved
pub line: usize,
pub message: String,
pub severity: Option<Severity>,
Expand Down
2 changes: 2 additions & 0 deletions helix-core/src/syntax.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Vec<PathBuf>>,
#[serde(default)]
pub persistent_diagnostic_sources: Vec<String>,
the-mikedavis marked this conversation as resolved.
Show resolved Hide resolved
}

#[derive(Debug, PartialEq, Eq, Hash)]
Expand Down
87 changes: 73 additions & 14 deletions helix-core/src/transaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)
Expand All @@ -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)]
Expand Down Expand Up @@ -411,43 +435,36 @@ 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();

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
);
} 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;
Expand Down Expand Up @@ -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), // <space><space>
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), // <space>
Insert(" b".into()),
Delete(1), // c
Retain(1), // d
Insert("e ".into()),
Delete(1), // <space>
],
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), // <space>
Insert("a".into()),
Delete(2), // <space>b
Retain(1), // d
Insert("e".into()),
Delete(1), // f
Retain(1), // <space>
],
len: 5,
len_after: 7,
};
assert_eq!(cs.map_pos(2, Assoc::BeforeWord), 1);
assert_eq!(cs.map_pos(4, Assoc::AfterWord), 4);
}

#[test]
Expand Down
118 changes: 80 additions & 38 deletions helix-term/src/application.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -738,7 +739,7 @@ impl Application {
));
}
}
Notification::PublishDiagnostics(params) => {
Notification::PublishDiagnostics(mut params) => {
let path = match params.uri.to_file_path() {
Ok(path) => path,
Err(_) => {
Expand All @@ -752,31 +753,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(&params.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())
}
}
}
}
pascalkuthe marked this conversation as resolved.
Show resolved Hide resolved

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,
) {
Expand All @@ -787,14 +826,13 @@ 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 {
log::warn!("lsp position out of bounds - {:?}", diagnostic);
return None;
};

let severity =
diagnostic.severity.map(|severity| match severity {
DiagnosticSeverity::ERROR => Error,
Expand All @@ -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;
Expand Down Expand Up @@ -846,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,
Expand All @@ -857,38 +904,33 @@ 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, server_id)| {
(d.severity, d.range.start, *server_id)
});
}
Notification::ShowMessage(params) => {
log::warn!("unhandled window/showMessage: {:?}", params);
Expand Down
Loading
Loading