From 8141a4a1ab78084df94c19e6225fc3c64a05b88f Mon Sep 17 00:00:00 2001 From: Michael Davis Date: Sun, 27 Aug 2023 13:32:17 -0500 Subject: [PATCH] LSP: Key diagnostics off file path instead of URI 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. --- helix-term/src/application.rs | 6 +-- helix-term/src/commands/lsp.rs | 72 ++++++++++++++++------------------ helix-view/src/editor.rs | 9 ++--- 3 files changed, 39 insertions(+), 48 deletions(-) diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index 30df3981c896..0ef200c2f8cd 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -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(¶ms.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 @@ -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 diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs index a1f7bf17dc88..a3168dc2d585 100644 --- a/helix-term/src/commands/lsp.rs +++ b/helix-term/src/commands/lsp.rs @@ -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. @@ -134,7 +134,7 @@ struct DiagnosticStyles { } struct PickerDiagnostic { - url: lsp::Url, + path: PathBuf, diag: lsp::Diagnostic, offset_encoding: OffsetEncoding, } @@ -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()) } }; @@ -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)); @@ -258,21 +266,20 @@ enum DiagnosticsFormat { fn diag_picker( cx: &Context, - diagnostics: BTreeMap>, - _current_path: Option, + diagnostics: BTreeMap>, format: DiagnosticsFormat, ) -> Picker { // 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(), }); @@ -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) } @@ -470,17 +472,16 @@ 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(¤t_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))); @@ -488,16 +489,9 @@ pub fn diagnostics_picker(cx: &mut Context) { } 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))); } diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs index fffbe6207f06..f46a0d6a6cac 100644 --- a/helix-view/src/editor.rs +++ b/helix-view/src/editor.rs @@ -914,7 +914,7 @@ pub struct Editor { pub macro_recording: Option<(char, Vec)>, pub macro_replaying: Vec, pub language_servers: helix_lsp::Registry, - pub diagnostics: BTreeMap>, + pub diagnostics: BTreeMap>, pub diff_providers: DiffProviderRegistry, pub debugger: Option, @@ -1815,7 +1815,7 @@ impl Editor { /// Returns all supported diagnostics for the document pub fn doc_diagnostics<'a>( language_servers: &'a helix_lsp::Registry, - diagnostics: &'a BTreeMap>, + diagnostics: &'a BTreeMap>, document: &Document, ) -> impl Iterator + 'a { Editor::doc_diagnostics_with_filter(language_servers, diagnostics, document, |_, _| true) @@ -1825,7 +1825,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>, + diagnostics: &'a BTreeMap>, document: &Document, filter: impl Fn(&lsp::Diagnostic, usize) -> bool + 'a, @@ -1834,8 +1834,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)?;