Skip to content

Commit

Permalink
LSP: Key diagnostics off file path instead of URI
Browse files Browse the repository at this point in the history 
URIs need to be normalized to be comparable. For example a language
server could send a URI for a path containing '+' as '%2B' but we might
encode this in something like 'Document::url' as just '+'. We can
normalize the URI straight into a PathBuf though since this is the only
value we compare these diagnostics URIs against. This also covers
edge-cases like windows drive letter capitalization.
  • Loading branch information
@the-mikedavis @postsolar
the-mikedavis authored and postsolar committed Apr 4, 2024
1 parent 6ff7178 commit fa8f721
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 48 deletions.
6 changes: 2 additions & 4 deletions helix-term/src/application.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -753,9 +753,7 @@ impl Application {
let lang_conf = doc.language.clone();

if let Some(lang_conf) = &lang_conf {
if let Some(old_diagnostics) =
self.editor.diagnostics.get(&params.uri)
{
if let Some(old_diagnostics) = self.editor.diagnostics.get(&path) {
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
Expand Down Expand Up @@ -788,7 +786,7 @@ impl Application {
// 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.
let diagnostics = match self.editor.diagnostics.entry(params.uri) {
let diagnostics = match self.editor.diagnostics.entry(path) {
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
Expand Down
72 changes: 33 additions & 39 deletions helix-term/src/commands/lsp.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ use std::{
collections::{BTreeMap, HashSet},
fmt::Write,
future::Future,
path::PathBuf,
path::{Path, PathBuf},
};

/// Gets the first language server that is attached to a document which supports a specific feature.
Expand Down Expand Up @@ -134,7 +134,7 @@ struct DiagnosticStyles {
}

struct PickerDiagnostic {
url: lsp::Url,
path: PathBuf,
diag: lsp::Diagnostic,
offset_encoding: OffsetEncoding,
}
Expand Down Expand Up @@ -167,8 +167,7 @@ impl ui::menu::Item for PickerDiagnostic {
let path = match format {
DiagnosticsFormat::HideSourcePath => String::new(),
DiagnosticsFormat::ShowSourcePath => {
let file_path = self.url.to_file_path().unwrap();
let path = path::get_truncated_path(file_path);
let path = path::get_truncated_path(&self.path);
format!("{}: ", path.to_string_lossy())
}
};
Expand Down Expand Up @@ -208,24 +207,33 @@ fn jump_to_location(
return;
}
};
jump_to_position(editor, &path, location.range, offset_encoding, action);
}

let doc = match editor.open(&path, action) {
fn jump_to_position(
editor: &mut Editor,
path: &Path,
range: lsp::Range,
offset_encoding: OffsetEncoding,
action: Action,
) {
let doc = match editor.open(path, action) {
Ok(id) => doc_mut!(editor, &id),
Err(err) => {
let err = format!("failed to open path: {:?}: {:?}", location.uri, err);
let err = format!("failed to open path: {:?}: {:?}", path, err);
editor.set_error(err);
return;
}
};
let view = view_mut!(editor);
// TODO: convert inside server
let new_range =
if let Some(new_range) = lsp_range_to_range(doc.text(), location.range, offset_encoding) {
new_range
} else {
log::warn!("lsp position out of bounds - {:?}", location.range);
return;
};
let new_range = if let Some(new_range) = lsp_range_to_range(doc.text(), range, offset_encoding)
{
new_range
} else {
log::warn!("lsp position out of bounds - {:?}", range);
return;
};
// we flip the range so that the cursor sits on the start of the symbol
// (for example start of the function).
doc.set_selection(view.id, Selection::single(new_range.head, new_range.anchor));
Expand Down Expand Up @@ -258,21 +266,20 @@ enum DiagnosticsFormat {

fn diag_picker(
cx: &Context,
diagnostics: BTreeMap<lsp::Url, Vec<(lsp::Diagnostic, usize)>>,
_current_path: Option<lsp::Url>,
diagnostics: BTreeMap<PathBuf, Vec<(lsp::Diagnostic, usize)>>,
format: DiagnosticsFormat,
) -> Picker<PickerDiagnostic> {
// TODO: drop current_path comparison and instead use workspace: bool flag?

// flatten the map to a vec of (url, diag) pairs
let mut flat_diag = Vec::new();
for (url, diags) in diagnostics {
for (path, diags) in diagnostics {
flat_diag.reserve(diags.len());

for (diag, ls) in diags {
if let Some(ls) = cx.editor.language_server_by_id(ls) {
flat_diag.push(PickerDiagnostic {
url: url.clone(),
path: path.clone(),
diag,
offset_encoding: ls.offset_encoding(),
});
Expand All @@ -292,22 +299,17 @@ fn diag_picker(
(styles, format),
move |cx,
PickerDiagnostic {
url,
path,
diag,
offset_encoding,
},
action| {
jump_to_location(
cx.editor,
&lsp::Location::new(url.clone(), diag.range),
*offset_encoding,
action,
)
jump_to_position(cx.editor, path, diag.range, *offset_encoding, action)
},
)
.with_preview(move |_editor, PickerDiagnostic { url, diag, .. }| {
let location = lsp::Location::new(url.clone(), diag.range);
Some(location_to_file_location(&location))
.with_preview(move |_editor, PickerDiagnostic { path, diag, .. }| {
let line = Some((diag.range.start.line as usize, diag.range.end.line as usize));
Some((path.clone().into(), line))
})
.truncate_start(false)
}
Expand Down Expand Up @@ -470,34 +472,26 @@ pub fn workspace_symbol_picker(cx: &mut Context) {

pub fn diagnostics_picker(cx: &mut Context) {
let doc = doc!(cx.editor);
if let Some(current_url) = doc.url() {
if let Some(current_path) = doc.path() {
let diagnostics = cx
.editor
.diagnostics
.get(&current_url)
.get(current_path)
.cloned()
.unwrap_or_default();
let picker = diag_picker(
cx,
[(current_url.clone(), diagnostics)].into(),
Some(current_url),
[(current_path.clone(), diagnostics)].into(),
DiagnosticsFormat::HideSourcePath,
);
cx.push_layer(Box::new(overlaid(picker)));
}
}

pub fn workspace_diagnostics_picker(cx: &mut Context) {
let doc = doc!(cx.editor);
let current_url = doc.url();
// TODO not yet filtered by LanguageServerFeature, need to do something similar as Document::shown_diagnostics here for all open documents
let diagnostics = cx.editor.diagnostics.clone();
let picker = diag_picker(
cx,
diagnostics,
current_url,
DiagnosticsFormat::ShowSourcePath,
);
let picker = diag_picker(cx, diagnostics, DiagnosticsFormat::ShowSourcePath);
cx.push_layer(Box::new(overlaid(picker)));
}

Expand Down
9 changes: 4 additions & 5 deletions helix-view/src/editor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -917,7 +917,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<lsp::Url, Vec<(lsp::Diagnostic, usize)>>,
pub diagnostics: BTreeMap<PathBuf, Vec<(lsp::Diagnostic, usize)>>,
pub diff_providers: DiffProviderRegistry,

pub debugger: Option<dap::Client>,
Expand Down Expand Up @@ -1818,7 +1818,7 @@ impl Editor {
/// Returns all supported diagnostics for the document
pub fn doc_diagnostics<'a>(
language_servers: &'a helix_lsp::Registry,
diagnostics: &'a BTreeMap<lsp::Url, Vec<(lsp::Diagnostic, usize)>>,
diagnostics: &'a BTreeMap<PathBuf, Vec<(lsp::Diagnostic, usize)>>,
document: &Document,
) -> impl Iterator<Item = helix_core::Diagnostic> + 'a {
Editor::doc_diagnostics_with_filter(language_servers, diagnostics, document, |_, _| true)
Expand All @@ -1828,7 +1828,7 @@ 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<lsp::Url, Vec<(lsp::Diagnostic, usize)>>,
diagnostics: &'a BTreeMap<PathBuf, Vec<(lsp::Diagnostic, usize)>>,

document: &Document,
filter: impl Fn(&lsp::Diagnostic, usize) -> bool + 'a,
Expand All @@ -1837,8 +1837,7 @@ impl Editor {
let language_config = document.language.clone();
document
.path()
.and_then(|path| url::Url::from_file_path(path).ok()) // TODO log error?
.and_then(|uri| diagnostics.get(&uri))
.and_then(|path| diagnostics.get(path))
.map(|diags| {
diags.iter().filter_map(move |(diagnostic, lsp_id)| {
let ls = language_servers.get_by_id(*lsp_id)?;
Expand Down

0 comments on commit fa8f721

Please sign in to comment.