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.

3 files changed, 39 insertions, 48 deletions

diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs
index 30df3981..0ef200c2 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(&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
@@ -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 a1f7bf17..a3168dc2 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<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(),
                 });
@@ -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(&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)));
@@ -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 fffbe620..f46a0d6a 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<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>,
@@ -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<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)
@@ -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<lsp::Url, Vec<(lsp::Diagnostic, usize)>>,
+        diagnostics: &'a BTreeMap<PathBuf, Vec<(lsp::Diagnostic, usize)>>,
 
         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)?;