From 5419101de392ff5cc6a559a45e0d39bdc22e1573 Mon Sep 17 00:00:00 2001 From: Michael Davis Date: Sat, 17 Feb 2024 14:25:29 -0500 Subject: [PATCH] Convert LSP URL types into core URIs Co-authored-by: soqb --- helix-term/src/application.rs | 28 ++++---- helix-term/src/commands/lsp.rs | 122 ++++++++++++++++++--------------- helix-view/src/document.rs | 4 ++ helix-view/src/editor.rs | 11 ++- helix-view/src/handlers/lsp.rs | 69 +++++++++++++------ 5 files changed, 136 insertions(+), 98 deletions(-) diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index 30df3981c896..624d82ad2db3 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -723,10 +723,10 @@ impl Application { } } Notification::PublishDiagnostics(mut params) => { - let path = match params.uri.to_file_path() { - Ok(path) => path, - Err(_) => { - log::error!("Unsupported file URI: {}", params.uri); + let uri = match helix_core::Uri::try_from(params.uri) { + Ok(uri) => uri, + Err(err) => { + log::error!("{err}"); return; } }; @@ -737,11 +737,11 @@ impl Application { } // 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)) + .find(|doc| doc.uri().is_some_and(|u| u == uri)) .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()); + log::info!("Version ({version}) is out of date for {uri:?} (expected ({}), dropping PublishDiagnostic notification", doc.version()); return false; } } @@ -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(&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 @@ -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(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 @@ -1127,20 +1125,22 @@ impl Application { .. } = params; - let path = match uri.to_file_path() { - Ok(path) => path, + let uri = match helix_core::Uri::try_from(uri) { + Ok(uri) => uri, Err(err) => { - log::error!("unsupported file URI: {}: {:?}", uri, err); + log::error!("{err}"); return lsp::ShowDocumentResult { success: false }; } }; + // If `Uri` gets another variant other than `Path` this may not be valid. + let path = uri.as_path().expect("URIs are valid paths"); let action = match take_focus { Some(true) => helix_view::editor::Action::Replace, _ => helix_view::editor::Action::VerticalSplit, }; - let doc_id = match self.editor.open(&path, action) { + let doc_id = match self.editor.open(path, action) { Ok(id) => id, Err(err) => { log::error!("failed to open path: {:?}: {:?}", uri, err); diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs index a1f7bf17dc88..c05681ac0d22 100644 --- a/helix-term/src/commands/lsp.rs +++ b/helix-term/src/commands/lsp.rs @@ -16,7 +16,9 @@ use tui::{ use super::{align_view, push_jump, Align, Context, Editor}; -use helix_core::{syntax::LanguageServerFeature, text_annotations::InlineAnnotation, Selection}; +use helix_core::{ + syntax::LanguageServerFeature, text_annotations::InlineAnnotation, Selection, Uri, +}; use helix_stdx::path; use helix_view::{ document::{DocumentInlayHints, DocumentInlayHintsId}, @@ -79,6 +81,8 @@ impl ui::menu::Item for lsp::Location { // With the preallocation above and UTF-8 paths already, this closure will do one (1) // allocation, for `to_file_path`, else there will be two (2), with `to_string_lossy`. let mut write_path_to_res = || -> Option<()> { + // We don't convert to a `helix_core::Uri` here because we've already checked the scheme. + // This path won't be normalized but it's only used for display. let path = self.uri.to_file_path().ok()?; res.push_str(&path.strip_prefix(cwdir).unwrap_or(&path).to_string_lossy()); Some(()) @@ -104,25 +108,28 @@ struct SymbolInformationItem { impl ui::menu::Item for SymbolInformationItem { /// Path to currently focussed document - type Data = Option; - - fn format(&self, current_doc_path: &Self::Data) -> Row { - if current_doc_path.as_ref() == Some(&self.symbol.location.uri) { - self.symbol.name.as_str().into() - } else { - match self.symbol.location.uri.to_file_path() { - Ok(path) => { - let get_relative_path = path::get_relative_path(path.as_path()); - format!( - "{} ({})", - &self.symbol.name, - get_relative_path.to_string_lossy() - ) - .into() - } - Err(_) => format!("{} ({})", &self.symbol.name, &self.symbol.location.uri).into(), + type Data = Option; + + fn format(&self, current_doc_uri: &Self::Data) -> Row { + if let Ok(uri) = Uri::try_from(&self.symbol.location.uri) { + if current_doc_uri + .as_ref() + .is_some_and(|doc_uri| doc_uri == &uri) + { + return self.symbol.name.as_str().into(); + } + if let Some(path) = uri.as_path() { + let relative_path = path::get_relative_path(path); + return format!( + "{} ({})", + &self.symbol.name, + relative_path.to_string_lossy() + ) + .into(); } } + + format!("{} ({})", &self.symbol.name, &self.symbol.location.uri).into() } } @@ -134,7 +141,7 @@ struct DiagnosticStyles { } struct PickerDiagnostic { - url: lsp::Url, + uri: Uri, diag: lsp::Diagnostic, offset_encoding: OffsetEncoding, } @@ -164,13 +171,12 @@ impl ui::menu::Item for PickerDiagnostic { None => String::new(), }; - 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 = match (format, self.uri.as_path()) { + (DiagnosticsFormat::ShowSourcePath, Some(path)) => { + let path = path::get_truncated_path(path); format!("{}: ", path.to_string_lossy()) } + _ => String::new(), }; Spans::from(vec![ @@ -182,13 +188,13 @@ impl ui::menu::Item for PickerDiagnostic { } } -fn location_to_file_location(location: &lsp::Location) -> FileLocation { - let path = location.uri.to_file_path().unwrap(); +fn location_to_file_location(location: &lsp::Location) -> Option { + let path = Uri::try_from(&location.uri).ok()?.as_path_buf()?.into(); let line = Some(( location.range.start.line as usize, location.range.end.line as usize, )); - (path.into(), line) + Some((path, line)) } fn jump_to_location( @@ -200,16 +206,17 @@ fn jump_to_location( let (view, doc) = current!(editor); push_jump(view, doc); - let path = match location.uri.to_file_path() { - Ok(path) => path, - Err(_) => { - let err = format!("unable to convert URI to filepath: {}", location.uri); - editor.set_error(err); + let uri = match Uri::try_from(&location.uri) { + Ok(uri) => uri, + Err(err) => { + editor.set_error(err.to_string()); return; } }; + // If `Uri` gets another variant other than `Path` this may not be valid. + let path = uri.as_path().expect("URIs are valid paths"); - let doc = match editor.open(&path, 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); @@ -236,9 +243,9 @@ fn jump_to_location( type SymbolPicker = Picker; -fn sym_picker(symbols: Vec, current_path: Option) -> SymbolPicker { +fn sym_picker(symbols: Vec, current_uri: Option) -> SymbolPicker { // TODO: drop current_path comparison and instead use workspace: bool flag? - Picker::new(symbols, current_path, move |cx, item, action| { + Picker::new(symbols, current_uri, move |cx, item, action| { jump_to_location( cx.editor, &item.symbol.location, @@ -246,7 +253,7 @@ fn sym_picker(symbols: Vec, current_path: Option>, - _current_path: Option, + diagnostics: BTreeMap>, + _current_path: Option, 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 (uri, 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(), + uri: uri.clone(), diag, offset_encoding: ls.offset_encoding(), }); @@ -292,22 +299,25 @@ fn diag_picker( (styles, format), move |cx, PickerDiagnostic { - url, + uri, diag, offset_encoding, }, action| { + let Ok(url) = uri.to_url() else { + return; + }; jump_to_location( cx.editor, - &lsp::Location::new(url.clone(), diag.range), + &lsp::Location::new(url, 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 { uri, diag, .. }| { + let line = Some((diag.range.start.line as usize, diag.range.end.line as usize)); + Some((uri.clone().as_path_buf()?.into(), line)) }) .truncate_start(false) } @@ -376,7 +386,7 @@ pub fn symbol_picker(cx: &mut Context) { } }) .collect(); - let current_url = doc.url(); + let current_uri = doc.uri(); if futures.is_empty() { cx.editor @@ -391,7 +401,7 @@ pub fn symbol_picker(cx: &mut Context) { symbols.append(&mut lsp_items); } let call = move |_editor: &mut Editor, compositor: &mut Compositor| { - let picker = sym_picker(symbols, current_url); + let picker = sym_picker(symbols, current_uri); compositor.push(Box::new(overlaid(picker))) }; @@ -453,13 +463,13 @@ pub fn workspace_symbol_picker(cx: &mut Context) { .boxed() }; - let current_url = doc.url(); + let current_uri = doc.uri(); let initial_symbols = get_symbols("".to_owned(), cx.editor); cx.jobs.callback(async move { let symbols = initial_symbols.await?; let call = move |_editor: &mut Editor, compositor: &mut Compositor| { - let picker = sym_picker(symbols, current_url); + let picker = sym_picker(symbols, current_uri); let dyn_picker = DynamicPicker::new(picker, Box::new(get_symbols)); compositor.push(Box::new(overlaid(dyn_picker))) }; @@ -470,17 +480,17 @@ 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_uri) = doc.uri() { let diagnostics = cx .editor .diagnostics - .get(¤t_url) + .get(¤t_uri) .cloned() .unwrap_or_default(); let picker = diag_picker( cx, - [(current_url.clone(), diagnostics)].into(), - Some(current_url), + [(current_uri.clone(), diagnostics)].into(), + Some(current_uri), DiagnosticsFormat::HideSourcePath, ); cx.push_layer(Box::new(overlaid(picker))); @@ -489,13 +499,13 @@ 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(); + let current_uri = doc.uri(); // 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, + current_uri, DiagnosticsFormat::ShowSourcePath, ); cx.push_layer(Box::new(overlaid(picker))); @@ -831,7 +841,7 @@ fn goto_impl( let picker = Picker::new(locations, cwdir, move |cx, location, action| { jump_to_location(cx.editor, location, offset_encoding, action) }) - .with_preview(move |_editor, location| Some(location_to_file_location(location))); + .with_preview(move |_editor, location| location_to_file_location(location)); compositor.push(Box::new(overlaid(picker))); } } diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 4e7b1de9f0cd..5d2d4146d1db 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -1670,6 +1670,10 @@ impl Document { Url::from_file_path(self.path()?).ok() } + pub fn uri(&self) -> Option { + Some(self.path()?.clone().into()) + } + #[inline] pub fn text(&self) -> &Rope { &self.text diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs index 68b74cf00eba..64809924527a 100644 --- a/helix-view/src/editor.rs +++ b/helix-view/src/editor.rs @@ -42,7 +42,7 @@ pub use helix_core::diagnostic::Severity; use helix_core::{ auto_pairs::AutoPairs, syntax::{self, AutoPairConfig, IndentationHeuristic, LanguageServerFeature, SoftWrap}, - Change, LineEnding, Position, Selection, NATIVE_LINE_ENDING, + Change, LineEnding, Position, Selection, Uri, NATIVE_LINE_ENDING, }; use helix_dap as dap; use helix_lsp::lsp; @@ -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, @@ -1812,7 +1812,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) @@ -1822,7 +1822,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, @@ -1830,8 +1830,7 @@ impl Editor { let text = document.text().clone(); let language_config = document.language.clone(); document - .path() - .and_then(|path| url::Url::from_file_path(path).ok()) // TODO log error? + .uri() .and_then(|uri| diagnostics.get(&uri)) .map(|diags| { diags.iter().filter_map(move |(diagnostic, lsp_id)| { diff --git a/helix-view/src/handlers/lsp.rs b/helix-view/src/handlers/lsp.rs index beb106b2bf83..ef26baa97798 100644 --- a/helix-view/src/handlers/lsp.rs +++ b/helix-view/src/handlers/lsp.rs @@ -1,6 +1,7 @@ use crate::editor::Action; use crate::Editor; use crate::{DocumentId, ViewId}; +use helix_core::Uri; use helix_lsp::util::generate_transaction_from_edits; use helix_lsp::{lsp, OffsetEncoding}; @@ -54,18 +55,30 @@ pub struct ApplyEditError { pub enum ApplyEditErrorKind { DocumentChanged, FileNotFound, - UnknownURISchema, + InvalidUrl(helix_core::uri::UrlConversionError), IoError(std::io::Error), // TODO: check edits before applying and propagate failure // InvalidEdit, } +impl From for ApplyEditErrorKind { + fn from(err: std::io::Error) -> Self { + ApplyEditErrorKind::IoError(err) + } +} + +impl From for ApplyEditErrorKind { + fn from(err: helix_core::uri::UrlConversionError) -> Self { + ApplyEditErrorKind::InvalidUrl(err) + } +} + impl ToString for ApplyEditErrorKind { fn to_string(&self) -> String { match self { ApplyEditErrorKind::DocumentChanged => "document has changed".to_string(), ApplyEditErrorKind::FileNotFound => "file not found".to_string(), - ApplyEditErrorKind::UnknownURISchema => "URI schema not supported".to_string(), + ApplyEditErrorKind::InvalidUrl(err) => err.to_string(), ApplyEditErrorKind::IoError(err) => err.to_string(), } } @@ -74,25 +87,28 @@ impl ToString for ApplyEditErrorKind { impl Editor { fn apply_text_edits( &mut self, - uri: &helix_lsp::Url, + url: &helix_lsp::Url, version: Option, text_edits: Vec, offset_encoding: OffsetEncoding, ) -> Result<(), ApplyEditErrorKind> { - let path = match uri.to_file_path() { - Ok(path) => path, - Err(_) => { - let err = format!("unable to convert URI to filepath: {}", uri); - log::error!("{}", err); - self.set_error(err); - return Err(ApplyEditErrorKind::UnknownURISchema); + let uri = match Uri::try_from(url) { + Ok(uri) => uri, + Err(err) => { + log::error!("{err}"); + return Err(err.into()); } }; + let path = uri.as_path().expect("URIs are valid paths"); - let doc_id = match self.open(&path, Action::Load) { + let doc_id = match self.open(path, Action::Load) { Ok(doc_id) => doc_id, Err(err) => { - let err = format!("failed to open document: {}: {}", uri, err); + let err = format!( + "failed to open document: {}: {}", + path.to_string_lossy(), + err + ); log::error!("{}", err); self.set_error(err); return Err(ApplyEditErrorKind::FileNotFound); @@ -102,7 +118,7 @@ impl Editor { let doc = doc_mut!(self, &doc_id); if let Some(version) = version { if version != doc.version() { - let err = format!("outdated workspace edit for {path:?}"); + let err = format!("outdated workspace edit for {:?}", path); log::error!("{err}, expected {} but got {version}", doc.version()); self.set_error(err); return Err(ApplyEditErrorKind::DocumentChanged); @@ -158,9 +174,9 @@ impl Editor { for (i, operation) in operations.iter().enumerate() { match operation { lsp::DocumentChangeOperation::Op(op) => { - self.apply_document_resource_op(op).map_err(|io| { + self.apply_document_resource_op(op).map_err(|err| { ApplyEditError { - kind: ApplyEditErrorKind::IoError(io), + kind: err, failed_change_idx: i, } })?; @@ -214,12 +230,18 @@ impl Editor { Ok(()) } - fn apply_document_resource_op(&mut self, op: &lsp::ResourceOp) -> std::io::Result<()> { + fn apply_document_resource_op( + &mut self, + op: &lsp::ResourceOp, + ) -> Result<(), ApplyEditErrorKind> { use lsp::ResourceOp; use std::fs; + // NOTE: If `Uri` gets another variant other than `Path` the below `expect`s + // may no longer be valid. match op { ResourceOp::Create(op) => { - let path = op.uri.to_file_path().unwrap(); + let uri = Uri::try_from(&op.uri)?; + let path = uri.as_path_buf().expect("URIs are valid paths"); let ignore_if_exists = op.options.as_ref().map_or(false, |options| { !options.overwrite.unwrap_or(false) && options.ignore_if_exists.unwrap_or(false) }); @@ -236,7 +258,8 @@ impl Editor { } } ResourceOp::Delete(op) => { - let path = op.uri.to_file_path().unwrap(); + let uri = Uri::try_from(&op.uri)?; + let path = uri.as_path_buf().expect("URIs are valid paths"); if path.is_dir() { let recursive = op .options @@ -251,17 +274,19 @@ impl Editor { } self.language_servers.file_event_handler.file_changed(path); } else if path.is_file() { - fs::remove_file(&path)?; + fs::remove_file(path)?; } } ResourceOp::Rename(op) => { - let from = op.old_uri.to_file_path().unwrap(); - let to = op.new_uri.to_file_path().unwrap(); + let from_uri = Uri::try_from(&op.old_uri)?; + let from = from_uri.as_path().expect("URIs are valid paths"); + let to_uri = Uri::try_from(&op.new_uri)?; + let to = to_uri.as_path().expect("URIs are valid paths"); let ignore_if_exists = op.options.as_ref().map_or(false, |options| { !options.overwrite.unwrap_or(false) && options.ignore_if_exists.unwrap_or(false) }); if !ignore_if_exists || !to.exists() { - self.move_path(&from, &to)?; + self.move_path(from, to)?; } } }