Skip to content

Commit

Permalink
make diagnostics stick to word boundaries
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
pascalkuthe authored and archseer committed Dec 27, 2023
1 parent 8653e1b commit 515ef17
Show file tree
Hide file tree
Showing 4 changed files with 112 additions and 28 deletions.
4 changes: 4 additions & 0 deletions helix-core/src/diagnostic.rs
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,
pub line: usize,
pub message: String,
pub severity: Option<Severity>,
Expand Down
87 changes: 73 additions & 14 deletions helix-core/src/transaction.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use ropey::RopeSlice;
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 @@ -23,6 +23,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 @@ -415,43 +439,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 @@ -884,6 +901,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
11 changes: 10 additions & 1 deletion helix-term/src/application.rs
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 @@ -811,7 +812,6 @@ impl Application {
log::warn!("lsp position out of bounds - {:?}", diagnostic);
return None;
};

let severity =
diagnostic.severity.map(|severity| match severity {
DiagnosticSeverity::ERROR => Error,
Expand Down Expand Up @@ -863,8 +863,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 Down
38 changes: 25 additions & 13 deletions helix-view/src/document.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1213,20 +1213,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);
Expand Down

0 comments on commit 515ef17

Please sign in to comment.