From 52a2033c31e6bc78aaf8d3242296601ecb5a0618 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 | 112 ++++++++++++++++++--------------- helix-view/src/document.rs | 4 ++ helix-view/src/editor.rs | 9 ++- helix-view/src/handlers/lsp.rs | 68 +++++++++++++------- 5 files changed, 131 insertions(+), 90 deletions(-) diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index 30df3981c8961..275a2eaf44fa8 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().map(|u| u == uri).unwrap_or(false)) .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 a1f7bf17dc881..6d41f9a3ffa88 100644 --- a/helix-term/src/commands/lsp.rs +++ b/helix-term/src/commands/lsp.rs @@ -79,6 +79,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,24 +106,27 @@ 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()); + type Data = Option; + + fn format(&self, current_doc_uri: &Self::Data) -> Row { + match helix_core::Uri::try_from(&self.symbol.location.uri) { + Ok(uri) if uri.as_path().is_some() => { + if current_doc_uri + .as_ref() + .is_some_and(|doc_uri| doc_uri == &uri) + { + self.symbol.name.as_str().into() + } else { + let relative_path = path::get_relative_path(uri.as_path().unwrap()); format!( "{} ({})", &self.symbol.name, - get_relative_path.to_string_lossy() + relative_path.to_string_lossy() ) .into() } - Err(_) => format!("{} ({})", &self.symbol.name, &self.symbol.location.uri).into(), } + _ => format!("{} ({})", &self.symbol.name, &self.symbol.location.uri).into(), } } } @@ -134,7 +139,7 @@ struct DiagnosticStyles { } struct PickerDiagnostic { - url: lsp::Url, + uri: helix_core::Uri, diag: lsp::Diagnostic, offset_encoding: OffsetEncoding, } @@ -164,13 +169,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 +186,16 @@ 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 = helix_core::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 +207,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 helix_core::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 +244,12 @@ 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 +257,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 +303,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 +390,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 +405,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 +467,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 +484,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 +503,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 +845,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 4e7b1de9f0cd5..5d2d4146d1dbd 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 68b74cf00ebac..64d94bf856923 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, @@ -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 beb106b2bf83e..8755ce3911bb8 100644 --- a/helix-view/src/handlers/lsp.rs +++ b/helix-view/src/handlers/lsp.rs @@ -54,18 +54,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 +86,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 helix_core::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 +117,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 +173,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 +229,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 = helix_core::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 +257,8 @@ impl Editor { } } ResourceOp::Delete(op) => { - let path = op.uri.to_file_path().unwrap(); + let uri = helix_core::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 +273,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 = helix_core::Uri::try_from(&op.old_uri)?; + let from = from_uri.as_path().expect("URIs are valid paths"); + let to_uri = helix_core::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)?; } } }