From 652e31692560f25513e866190c67452e0cd2e601 Mon Sep 17 00:00:00 2001 From: Michael Davis Date: Thu, 15 Aug 2024 16:26:46 -0400 Subject: [PATCH 1/4] LSP: Use PathBufs for workspace folders Internally the LSP client should hold workspace folders as paths. Using URLs for this type is inconvenient (since we compare it to paths) and might cause mismatches because of URLs not being normalized. The URLs must be paths anyways so we can convert these types lazily when we need to send them to a server. --- helix-lsp/src/client.rs | 86 +++++++++++++++++++---------------------- helix-lsp/src/lib.rs | 10 ++--- 2 files changed, 43 insertions(+), 53 deletions(-) diff --git a/helix-lsp/src/client.rs b/helix-lsp/src/client.rs index cc1c4ce8fe67..48b7a8798747 100644 --- a/helix-lsp/src/client.rs +++ b/helix-lsp/src/client.rs @@ -32,14 +32,17 @@ use tokio::{ }, }; -fn workspace_for_uri(uri: lsp::Url) -> WorkspaceFolder { +fn workspace_for_path(path: &Path) -> WorkspaceFolder { + let name = path + .iter() + .last() + .expect("workspace paths should be non-empty") + .to_string_lossy() + .to_string(); + lsp::WorkspaceFolder { - name: uri - .path_segments() - .and_then(|segments| segments.last()) - .map(|basename| basename.to_string()) - .unwrap_or_default(), - uri, + name, + uri: lsp::Url::from_file_path(path).expect("absolute paths can be converted to `Url`s"), } } @@ -55,7 +58,7 @@ pub struct Client { config: Option, root_path: std::path::PathBuf, root_uri: Option, - workspace_folders: Mutex>, + workspace_folders: Mutex>, initialize_notify: Arc, /// workspace folders added while the server is still initializing req_timeout: u64, @@ -80,16 +83,13 @@ impl Client { &workspace, workspace_is_cwd, ); - let root_uri = root - .as_ref() - .and_then(|root| lsp::Url::from_file_path(root).ok()); - if self.root_path == root.unwrap_or(workspace) - || root_uri.as_ref().map_or(false, |root_uri| { + if &self.root_path == root.as_ref().unwrap_or(&workspace) + || root.as_ref().is_some_and(|root| { self.workspace_folders .lock() .iter() - .any(|workspace| &workspace.uri == root_uri) + .any(|workspace| workspace == root) }) { // workspace URI is already registered so we can use this client @@ -113,15 +113,16 @@ impl Client { // wait and see if anyone ever runs into it. tokio::spawn(async move { client.initialize_notify.notified().await; - if let Some(workspace_folders_caps) = client + if let Some((workspace_folders_caps, root)) = client .capabilities() .workspace .as_ref() .and_then(|cap| cap.workspace_folders.as_ref()) .filter(|cap| cap.supported.unwrap_or(false)) + .zip(root) { client.add_workspace_folder( - root_uri, + root, workspace_folders_caps.change_notifications.as_ref(), ); } @@ -129,16 +130,14 @@ impl Client { return true; }; - if let Some(workspace_folders_caps) = capabilities + if let Some((workspace_folders_caps, root)) = capabilities .workspace .as_ref() .and_then(|cap| cap.workspace_folders.as_ref()) .filter(|cap| cap.supported.unwrap_or(false)) + .zip(root) { - self.add_workspace_folder( - root_uri, - workspace_folders_caps.change_notifications.as_ref(), - ); + self.add_workspace_folder(root, workspace_folders_caps.change_notifications.as_ref()); true } else { // the server doesn't support multi workspaces, we need a new client @@ -148,29 +147,19 @@ impl Client { fn add_workspace_folder( &self, - root_uri: Option, + root: PathBuf, change_notifications: Option<&OneOf>, ) { - // root_uri is None just means that there isn't really any LSP workspace - // associated with this file. For servers that support multiple workspaces - // there is just one server so we can always just use that shared instance. - // No need to add a new workspace root here as there is no logical root for this file - // let the server deal with this - let Some(root_uri) = root_uri else { - return; - }; - + let workspace = workspace_for_path(&root); // server supports workspace folders, let's add the new root to the list - self.workspace_folders - .lock() - .push(workspace_for_uri(root_uri.clone())); + self.workspace_folders.lock().push(root); if Some(&OneOf::Left(false)) == change_notifications { // server specifically opted out of DidWorkspaceChange notifications // let's assume the server will request the workspace folders itself // and that we can therefore reuse the client (but are done now) return; } - tokio::spawn(self.did_change_workspace(vec![workspace_for_uri(root_uri)], Vec::new())); + tokio::spawn(self.did_change_workspace(vec![workspace], Vec::new())); } #[allow(clippy::type_complexity, clippy::too_many_arguments)] @@ -179,8 +168,8 @@ impl Client { args: &[String], config: Option, server_environment: HashMap, - root_path: PathBuf, - root_uri: Option, + root: Option, + workspace: PathBuf, id: LanguageServerId, name: String, req_timeout: u64, @@ -212,10 +201,13 @@ impl Client { let (server_rx, server_tx, initialize_notify) = Transport::start(reader, writer, stderr, id, name.clone()); - let workspace_folders = root_uri - .clone() - .map(|root| vec![workspace_for_uri(root)]) - .unwrap_or_default(); + let workspace_folders = root.clone().into_iter().collect(); + let root_uri = root.clone().map(|root| { + lsp::Url::from_file_path(root).expect("absolute paths can be converted to `Url`s") + }); + // `root_uri` and `workspace_folder` can be empty in case there is no workspace + // `root_url` can not, use `workspace` as a fallback + let root_path = root.unwrap_or(workspace); let client = Self { id, @@ -376,10 +368,12 @@ impl Client { self.config.as_ref() } - pub async fn workspace_folders( - &self, - ) -> parking_lot::MutexGuard<'_, Vec> { - self.workspace_folders.lock() + pub async fn workspace_folders(&self) -> Vec { + self.workspace_folders + .lock() + .iter() + .map(|path| workspace_for_path(path)) + .collect() } /// Execute a RPC request on the language server. @@ -526,7 +520,7 @@ impl Client { #[allow(deprecated)] let params = lsp::InitializeParams { process_id: Some(std::process::id()), - workspace_folders: Some(self.workspace_folders.lock().clone()), + workspace_folders: Some(self.workspace_folders().await), // root_path is obsolete, but some clients like pyright still use it so we specify both. // clients will prefer _uri if possible root_path: self.root_path.to_str().map(|path| path.to_owned()), diff --git a/helix-lsp/src/lib.rs b/helix-lsp/src/lib.rs index 134cb74fbee3..fac6bf5884d3 100644 --- a/helix-lsp/src/lib.rs +++ b/helix-lsp/src/lib.rs @@ -853,12 +853,8 @@ fn start_client( workspace_is_cwd, ); - // `root_uri` and `workspace_folder` can be empty in case there is no workspace - // `root_url` can not, use `workspace` as a fallback - let root_path = root.clone().unwrap_or_else(|| workspace.clone()); - let root_uri = root.and_then(|root| lsp::Url::from_file_path(root).ok()); - if let Some(globset) = &ls_config.required_root_patterns { + let root_path = root.as_ref().unwrap_or(&workspace); if !root_path .read_dir()? .flatten() @@ -874,8 +870,8 @@ fn start_client( &ls_config.args, ls_config.config.clone(), ls_config.environment.clone(), - root_path, - root_uri, + root, + workspace, id, name, ls_config.timeout, From b84c9a893c51b178210c2a92dc34b91603e5d442 Mon Sep 17 00:00:00 2001 From: Michael Davis Date: Thu, 15 Aug 2024 17:48:28 -0400 Subject: [PATCH 2/4] Replace `url::Url` with a String wrapper --- helix-lsp-types/Cargo.toml | 2 +- helix-lsp-types/README.md | 4 +- helix-lsp-types/src/lib.rs | 156 +++++++++++++++++++++++++++++++++---- 3 files changed, 146 insertions(+), 16 deletions(-) diff --git a/helix-lsp-types/Cargo.toml b/helix-lsp-types/Cargo.toml index 308edad698c2..535cefba5e15 100644 --- a/helix-lsp-types/Cargo.toml +++ b/helix-lsp-types/Cargo.toml @@ -25,7 +25,7 @@ bitflags = "2.6.0" serde = { version = "1.0.216", features = ["derive"] } serde_json = "1.0.133" serde_repr = "0.1" -url = {version = "2.5.4", features = ["serde"]} +percent-encoding.workspace = true [features] default = [] diff --git a/helix-lsp-types/README.md b/helix-lsp-types/README.md index 01803be17092..de19757fda8a 100644 --- a/helix-lsp-types/README.md +++ b/helix-lsp-types/README.md @@ -1,3 +1,5 @@ # Helix's `lsp-types` -This is a fork of the [`lsp-types`](https://crates.io/crates/lsp-types) crate ([`gluon-lang/lsp-types`](https://github.com/gluon-lang/lsp-types)) taken at version v0.95.1 (commit [3e6daee](https://github.com/gluon-lang/lsp-types/commit/3e6daee771d14db4094a554b8d03e29c310dfcbe)). This fork focuses usability improvements that make the types easier to work with for the Helix codebase. For example the URL type - the `uri` crate at this version of `lsp-types` - will be replaced with a wrapper around a string. +This is a fork of the [`lsp-types`](https://crates.io/crates/lsp-types) crate ([`gluon-lang/lsp-types`](https://github.com/gluon-lang/lsp-types)) taken at version v0.95.1 (commit [3e6daee](https://github.com/gluon-lang/lsp-types/commit/3e6daee771d14db4094a554b8d03e29c310dfcbe)). This fork focuses on usability improvements that make the types easier to work with for the Helix codebase. + +The URL type has been replaced with a newtype wrapper of a `String`. The `lsp-types` crate at the forked version used [`url::Url`](https://docs.rs/url/2.5.0/url/struct.Url.html) which provides conveniences for using URLs according to [the WHATWG URL spec](https://url.spec.whatwg.org). Helix supports a subset of valid URLs, namely the `file://` scheme, so a wrapper around a normal `String` is sufficient. Plus the LSP spec requires URLs to be in [RFC3986](https://tools.ietf.org/html/rfc3986) format instead. diff --git a/helix-lsp-types/src/lib.rs b/helix-lsp-types/src/lib.rs index 41c483f42359..5024f6e7cbb3 100644 --- a/helix-lsp-types/src/lib.rs +++ b/helix-lsp-types/src/lib.rs @@ -3,27 +3,155 @@ Language Server Protocol types for Rust. Based on: - -This library uses the URL crate for parsing URIs. Note that there is -some confusion on the meaning of URLs vs URIs: -. According to that -information, on the classical sense of "URLs", "URLs" are a subset of -URIs, But on the modern/new meaning of URLs, they are the same as -URIs. The important take-away aspect is that the URL crate should be -able to parse any URI, such as `urn:isbn:0451450523`. - - */ #![allow(non_upper_case_globals)] #![forbid(unsafe_code)] use bitflags::bitflags; -use std::{collections::HashMap, fmt::Debug}; +use std::{collections::HashMap, fmt::Debug, path::Path}; use serde::{de, de::Error as Error_, Deserialize, Serialize}; use serde_json::Value; -pub use url::Url; + +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Deserialize, Serialize)] +pub struct Url(String); + +// , also see +// +const RESERVED: &percent_encoding::AsciiSet = &percent_encoding::CONTROLS + // GEN_DELIMS + .add(b':') + .add(b'/') + .add(b'?') + .add(b'#') + .add(b'[') + .add(b']') + .add(b'@') + // SUB_DELIMS + .add(b'!') + .add(b'$') + .add(b'&') + .add(b'\'') + .add(b'(') + .add(b')') + .add(b'*') + .add(b'+') + .add(b',') + .add(b';') + .add(b'='); + +impl Url { + #[allow(clippy::result_unit_err)] + #[cfg(any(unix, target_os = "redox", target_os = "wasi"))] + pub fn from_file_path>(path: P) -> Result { + #[cfg(any(unix, target_os = "redox"))] + use std::os::unix::prelude::OsStrExt; + #[cfg(target_os = "wasi")] + use std::os::wasi::prelude::OsStrExt; + + let mut serialization = String::from("file://"); + // skip the root component + for component in path.as_ref().components().skip(1) { + serialization.push('/'); + serialization.extend(percent_encoding::percent_encode( + component.as_os_str().as_bytes(), + RESERVED, + )); + } + if &serialization == "file://" { + // An URL's path must not be empty. + serialization.push('/'); + } + Ok(Self(serialization)) + } + + #[allow(clippy::result_unit_err)] + #[cfg(windows)] + pub fn from_file_path>(path: P) -> Result { + from_file_path_windows(path.as_ref()) + } + + #[allow(clippy::result_unit_err)] + #[cfg_attr(not(windows), allow(dead_code))] + fn from_file_path_windows(path: &Path) -> Result { + use std::path::{Component, Prefix}; + + fn is_windows_drive_letter(segment: &str) -> bool { + segment.len() == 2 + && (segment.as_bytes()[0] as char).is_ascii_alphabetic() + && matches!(segment.as_bytes()[1], b':' | b'|') + } + + assert!(path.is_absolute()); + let mut serialization = String::from("file://"); + let mut components = path.components(); + let host_start = serialization.len() + 1; + + match components.next() { + Some(Component::Prefix(ref p)) => match p.kind() { + Prefix::Disk(letter) | Prefix::VerbatimDisk(letter) => { + serialization.push('/'); + serialization.push(letter as char); + serialization.push(':'); + } + // TODO: Prefix::UNC | Prefix::VerbatimUNC + _ => todo!("support UNC drives"), + }, + _ => unreachable!("absolute windows paths must start with a prefix"), + } + + let mut path_only_has_prefix = true; + for component in components { + if component == Component::RootDir { + continue; + } + + path_only_has_prefix = false; + + serialization.push('/'); + serialization.extend(percent_encoding::percent_encode( + component.as_os_str().as_encoded_bytes(), + RESERVED, + )); + } + + if serialization.len() > host_start + && is_windows_drive_letter(&serialization[host_start..]) + && path_only_has_prefix + { + serialization.push('/'); + } + + Ok(Self(serialization)) + } + + #[allow(clippy::result_unit_err)] + pub fn from_directory_path>(path: P) -> Result { + let Self(mut serialization) = Self::from_file_path(path)?; + if !serialization.ends_with('/') { + serialization.push('/'); + } + Ok(Self(serialization)) + } + + /// Returns the serialized representation of the URL as a `&str` + pub fn as_str(&self) -> &str { + &self.0 + } + + /// Consumes the URL, converting into a `String`. + /// Note that the string is the serialized representation of the URL. + pub fn into_string(self) -> String { + self.0 + } +} + +impl From<&str> for Url { + fn from(value: &str) -> Self { + Self(value.to_string()) + } +} // Large enough to contain any enumeration name defined in this crate type PascalCaseBuf = [u8; 32]; @@ -2843,14 +2971,14 @@ mod tests { test_serialization( &WorkspaceEdit { changes: Some( - vec![(Url::parse("file://test").unwrap(), vec![])] + vec![(Url::from("file://test"), vec![])] .into_iter() .collect(), ), document_changes: None, ..Default::default() }, - r#"{"changes":{"file://test/":[]}}"#, + r#"{"changes":{"file://test":[]}}"#, ); } From a36806e32630f80c48e7e84b5dd459eba17c73de Mon Sep 17 00:00:00 2001 From: Michael Davis Date: Thu, 15 Aug 2024 17:49:00 -0400 Subject: [PATCH 3/4] Handle conversion to/from new LSP URL type --- Cargo.lock | 6 +- Cargo.toml | 1 + helix-core/Cargo.toml | 2 +- helix-core/src/uri.rs | 116 ++++++++++++++++++--------------- helix-lsp/src/client.rs | 7 +- helix-term/src/application.rs | 7 +- helix-term/src/commands.rs | 10 +-- helix-term/src/commands/lsp.rs | 6 +- helix-term/src/lib.rs | 7 +- helix-view/Cargo.toml | 2 - helix-view/src/document.rs | 7 +- helix-view/src/handlers/lsp.rs | 16 ++--- 12 files changed, 97 insertions(+), 90 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 559e9eb8c444..679b87f39507 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1237,6 +1237,7 @@ dependencies = [ "nucleo", "once_cell", "parking_lot", + "percent-encoding", "quickcheck", "regex", "regex-cursor", @@ -1252,7 +1253,6 @@ dependencies = [ "unicode-general-category", "unicode-segmentation", "unicode-width", - "url", ] [[package]] @@ -1332,10 +1332,10 @@ name = "helix-lsp-types" version = "0.95.1" dependencies = [ "bitflags", + "percent-encoding", "serde", "serde_json", "serde_repr", - "url", ] [[package]] @@ -1468,7 +1468,6 @@ dependencies = [ "tokio", "tokio-stream", "toml", - "url", ] [[package]] @@ -2622,7 +2621,6 @@ dependencies = [ "form_urlencoded", "idna", "percent-encoding", - "serde", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 753be4b462c4..35bba77a43d8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -42,6 +42,7 @@ tree-sitter = { version = "0.22" } nucleo = "0.5.0" slotmap = "1.0.7" thiserror = "2.0" +percent-encoding = "2.3" [workspace.package] version = "24.7.0" diff --git a/helix-core/Cargo.toml b/helix-core/Cargo.toml index d245ec13a23f..485a415512ef 100644 --- a/helix-core/Cargo.toml +++ b/helix-core/Cargo.toml @@ -40,7 +40,7 @@ bitflags = "2.6" ahash = "0.8.11" hashbrown = { version = "0.14.5", features = ["raw"] } dunce = "1.0" -url = "2.5.4" +percent-encoding.workspace = true log = "0.4" anyhow = "1.0" diff --git a/helix-core/src/uri.rs b/helix-core/src/uri.rs index cbe0fadda67d..0b6f4e0b2b8d 100644 --- a/helix-core/src/uri.rs +++ b/helix-core/src/uri.rs @@ -1,6 +1,7 @@ use std::{ fmt, path::{Path, PathBuf}, + str::FromStr, sync::Arc, }; @@ -16,14 +17,6 @@ pub enum Uri { } impl Uri { - // This clippy allow mirrors url::Url::from_file_path - #[allow(clippy::result_unit_err)] - pub fn to_url(&self) -> Result { - match self { - Uri::File(path) => url::Url::from_file_path(path), - } - } - pub fn as_path(&self) -> Option<&Path> { match self { Self::File(path) => Some(path), @@ -45,81 +38,96 @@ impl fmt::Display for Uri { } } -#[derive(Debug)] -pub struct UrlConversionError { - source: url::Url, - kind: UrlConversionErrorKind, +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct UriParseError { + source: String, + kind: UriParseErrorKind, } -#[derive(Debug)] -pub enum UrlConversionErrorKind { - UnsupportedScheme, - UnableToConvert, +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum UriParseErrorKind { + UnsupportedScheme(String), + MalformedUri, } -impl fmt::Display for UrlConversionError { +impl fmt::Display for UriParseError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self.kind { - UrlConversionErrorKind::UnsupportedScheme => { + match &self.kind { + UriParseErrorKind::UnsupportedScheme(scheme) => { + write!(f, "unsupported scheme '{scheme}' in URI {}", self.source) + } + UriParseErrorKind::MalformedUri => { write!( f, - "unsupported scheme '{}' in URL {}", - self.source.scheme(), + "unable to convert malformed URI to file path: {}", self.source ) } - UrlConversionErrorKind::UnableToConvert => { - write!(f, "unable to convert URL to file path: {}", self.source) - } } } } -impl std::error::Error for UrlConversionError {} - -fn convert_url_to_uri(url: &url::Url) -> Result { - if url.scheme() == "file" { - url.to_file_path() - .map(|path| Uri::File(helix_stdx::path::normalize(path).into())) - .map_err(|_| UrlConversionErrorKind::UnableToConvert) - } else { - Err(UrlConversionErrorKind::UnsupportedScheme) - } -} +impl std::error::Error for UriParseError {} + +impl FromStr for Uri { + type Err = UriParseError; + + fn from_str(s: &str) -> Result { + use std::ffi::OsStr; + #[cfg(any(unix, target_os = "redox"))] + use std::os::unix::prelude::OsStrExt; + #[cfg(target_os = "wasi")] + use std::os::wasi::prelude::OsStrExt; + + let Some((scheme, rest)) = s.split_once("://") else { + return Err(Self::Err { + source: s.to_string(), + kind: UriParseErrorKind::MalformedUri, + }); + }; + + if scheme != "file" { + return Err(Self::Err { + source: s.to_string(), + kind: UriParseErrorKind::UnsupportedScheme(scheme.to_string()), + }); + } -impl TryFrom for Uri { - type Error = UrlConversionError; + // Assert there is no query or fragment in the URI. + if s.find(['?', '#']).is_some() { + return Err(Self::Err { + source: s.to_string(), + kind: UriParseErrorKind::MalformedUri, + }); + } - fn try_from(url: url::Url) -> Result { - convert_url_to_uri(&url).map_err(|kind| Self::Error { source: url, kind }) + let mut bytes = Vec::new(); + bytes.extend(percent_encoding::percent_decode(rest.as_bytes())); + Ok(PathBuf::from(OsStr::from_bytes(&bytes)).into()) } } -impl TryFrom<&url::Url> for Uri { - type Error = UrlConversionError; +impl TryFrom<&str> for Uri { + type Error = UriParseError; - fn try_from(url: &url::Url) -> Result { - convert_url_to_uri(url).map_err(|kind| Self::Error { - source: url.clone(), - kind, - }) + fn try_from(s: &str) -> Result { + s.parse() } } #[cfg(test)] mod test { use super::*; - use url::Url; #[test] fn unknown_scheme() { - let url = Url::parse("csharp:/metadata/foo/bar/Baz.cs").unwrap(); - assert!(matches!( - Uri::try_from(url), - Err(UrlConversionError { - kind: UrlConversionErrorKind::UnsupportedScheme, - .. + let uri = "csharp://metadata/foo/barBaz.cs"; + assert_eq!( + uri.parse::(), + Err(UriParseError { + source: uri.to_string(), + kind: UriParseErrorKind::UnsupportedScheme("csharp".to_string()), }) - )); + ); } } diff --git a/helix-lsp/src/client.rs b/helix-lsp/src/client.rs index 48b7a8798747..e8efa613f563 100644 --- a/helix-lsp/src/client.rs +++ b/helix-lsp/src/client.rs @@ -42,7 +42,8 @@ fn workspace_for_path(path: &Path) -> WorkspaceFolder { lsp::WorkspaceFolder { name, - uri: lsp::Url::from_file_path(path).expect("absolute paths can be converted to `Url`s"), + uri: lsp::Url::from_directory_path(path) + .expect("absolute paths can be converted to `Url`s"), } } @@ -742,7 +743,7 @@ impl Client { } else { Url::from_file_path(path) }; - Some(url.ok()?.to_string()) + Some(url.ok()?.into_string()) }; let files = vec![lsp::FileRename { old_uri: url_from_path(old_path)?, @@ -776,7 +777,7 @@ impl Client { } else { Url::from_file_path(path) }; - Some(url.ok()?.to_string()) + Some(url.ok()?.into_string()) }; let files = vec![lsp::FileRename { diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index 36cb295cea4c..28fcd8fcb2f9 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -744,7 +744,7 @@ impl Application { } } Notification::PublishDiagnostics(mut params) => { - let uri = match helix_core::Uri::try_from(params.uri) { + let uri = match helix_core::Uri::try_from(params.uri.as_str()) { Ok(uri) => uri, Err(err) => { log::error!("{err}"); @@ -1143,7 +1143,8 @@ impl Application { .. } = params { - self.jobs.callback(crate::open_external_url_callback(uri)); + self.jobs + .callback(crate::open_external_url_callback(uri.as_str())); return lsp::ShowDocumentResult { success: true }; }; @@ -1154,7 +1155,7 @@ impl Application { .. } = params; - let uri = match helix_core::Uri::try_from(uri) { + let uri = match helix_core::Uri::try_from(uri.as_str()) { Ok(uri) => uri, Err(err) => { log::error!("{err}"); diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index 0bfb12ad2e9a..30eb37988a9d 100644 --- a/helix-term/src/commands.rs +++ b/helix-term/src/commands.rs @@ -1347,7 +1347,9 @@ fn open_url(cx: &mut Context, url: Url, action: Action) { .unwrap_or_default(); if url.scheme() != "file" { - return cx.jobs.callback(crate::open_external_url_callback(url)); + return cx + .jobs + .callback(crate::open_external_url_callback(url.as_str())); } let content_type = std::fs::File::open(url.path()).and_then(|file| { @@ -1360,9 +1362,9 @@ fn open_url(cx: &mut Context, url: Url, action: Action) { // we attempt to open binary files - files that can't be open in helix - using external // program as well, e.g. pdf files or images match content_type { - Ok(content_inspector::ContentType::BINARY) => { - cx.jobs.callback(crate::open_external_url_callback(url)) - } + Ok(content_inspector::ContentType::BINARY) => cx + .jobs + .callback(crate::open_external_url_callback(url.as_str())), Ok(_) | Err(_) => { let path = &rel_path.join(url.path()); if path.is_dir() { diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs index fcc0333e8cd8..4fd957c872ad 100644 --- a/helix-term/src/commands/lsp.rs +++ b/helix-term/src/commands/lsp.rs @@ -69,7 +69,7 @@ struct Location { } fn lsp_location_to_location(location: lsp::Location) -> Option { - let uri = match location.uri.try_into() { + let uri = match location.uri.as_str().try_into() { Ok(uri) => uri, Err(err) => { log::warn!("discarding invalid or unsupported URI: {err}"); @@ -456,7 +456,7 @@ pub fn workspace_symbol_picker(cx: &mut Context) { .unwrap_or_default() .into_iter() .filter_map(|symbol| { - let uri = match Uri::try_from(&symbol.location.uri) { + let uri = match Uri::try_from(symbol.location.uri.as_str()) { Ok(uri) => uri, Err(err) => { log::warn!("discarding symbol with invalid URI: {err}"); @@ -510,7 +510,7 @@ pub fn workspace_symbol_picker(cx: &mut Context) { .to_string() .into() } else { - item.symbol.location.uri.to_string().into() + item.symbol.location.uri.as_str().into() } }), ]; diff --git a/helix-term/src/lib.rs b/helix-term/src/lib.rs index cf4fbd9fa7ae..71fb3b4f5c0c 100644 --- a/helix-term/src/lib.rs +++ b/helix-term/src/lib.rs @@ -18,7 +18,6 @@ use futures_util::Future; mod handlers; use ignore::DirEntry; -use url::Url; #[cfg(windows)] fn true_color() -> bool { @@ -70,10 +69,10 @@ fn filter_picker_entry(entry: &DirEntry, root: &Path, dedup_symlinks: bool) -> b } /// Opens URL in external program. -fn open_external_url_callback( - url: Url, +fn open_external_url_callback>( + url: U, ) -> impl Future> + Send + 'static { - let commands = open::commands(url.as_str()); + let commands = open::commands(url); async { for cmd in commands { let mut command = tokio::process::Command::new(cmd.get_program()); diff --git a/helix-view/Cargo.toml b/helix-view/Cargo.toml index 6f71fa05204f..0b58a86bbfa5 100644 --- a/helix-view/Cargo.toml +++ b/helix-view/Cargo.toml @@ -30,9 +30,7 @@ crossterm = { version = "0.28", optional = true } tempfile = "3.14" -# Conversion traits once_cell = "1.20" -url = "2.5.4" arc-swap = { version = "1.7.1" } diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index dcdc8dc2f114..5a17c5b73f61 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -642,7 +642,6 @@ where } use helix_lsp::{lsp, Client, LanguageServerId, LanguageServerName}; -use url::Url; impl Document { pub fn from( @@ -1822,8 +1821,8 @@ impl Document { } /// File path as a URL. - pub fn url(&self) -> Option { - Url::from_file_path(self.path()?).ok() + pub fn url(&self) -> Option { + lsp::Url::from_file_path(self.path()?).ok() } pub fn uri(&self) -> Option { @@ -1909,7 +1908,7 @@ impl Document { pub fn lsp_diagnostic_to_diagnostic( text: &Rope, language_config: Option<&LanguageConfiguration>, - diagnostic: &helix_lsp::lsp::Diagnostic, + diagnostic: &lsp::Diagnostic, language_server_id: LanguageServerId, offset_encoding: helix_lsp::OffsetEncoding, ) -> Option { diff --git a/helix-view/src/handlers/lsp.rs b/helix-view/src/handlers/lsp.rs index 1fd2289db5d8..77f8769651f8 100644 --- a/helix-view/src/handlers/lsp.rs +++ b/helix-view/src/handlers/lsp.rs @@ -57,7 +57,7 @@ pub struct ApplyEditError { pub enum ApplyEditErrorKind { DocumentChanged, FileNotFound, - InvalidUrl(helix_core::uri::UrlConversionError), + InvalidUrl(helix_core::uri::UriParseError), IoError(std::io::Error), // TODO: check edits before applying and propagate failure // InvalidEdit, @@ -69,8 +69,8 @@ impl From for ApplyEditErrorKind { } } -impl From for ApplyEditErrorKind { - fn from(err: helix_core::uri::UrlConversionError) -> Self { +impl From for ApplyEditErrorKind { + fn from(err: helix_core::uri::UriParseError) -> Self { ApplyEditErrorKind::InvalidUrl(err) } } @@ -94,7 +94,7 @@ impl Editor { text_edits: Vec, offset_encoding: OffsetEncoding, ) -> Result<(), ApplyEditErrorKind> { - let uri = match Uri::try_from(url) { + let uri = match Uri::try_from(url.as_str()) { Ok(uri) => uri, Err(err) => { log::error!("{err}"); @@ -242,7 +242,7 @@ impl Editor { // may no longer be valid. match op { ResourceOp::Create(op) => { - let uri = Uri::try_from(&op.uri)?; + let uri = Uri::try_from(op.uri.as_str())?; let path = 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) @@ -262,7 +262,7 @@ impl Editor { } } ResourceOp::Delete(op) => { - let uri = Uri::try_from(&op.uri)?; + let uri = Uri::try_from(op.uri.as_str())?; let path = uri.as_path().expect("URIs are valid paths"); if path.is_dir() { let recursive = op @@ -284,9 +284,9 @@ impl Editor { } } ResourceOp::Rename(op) => { - let from_uri = Uri::try_from(&op.old_uri)?; + let from_uri = Uri::try_from(op.old_uri.as_str())?; let from = from_uri.as_path().expect("URIs are valid paths"); - let to_uri = Uri::try_from(&op.new_uri)?; + let to_uri = Uri::try_from(op.new_uri.as_str())?; 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) From 61491af15ef74c32a1eb7b8e203974bd3780373b Mon Sep 17 00:00:00 2001 From: Michael Davis Date: Fri, 20 Dec 2024 12:04:36 -0500 Subject: [PATCH 4/4] Remove unused Result wrapper for Path->Url conversion --- helix-lsp-types/src/lib.rs | 20 ++++++++------------ helix-lsp/src/client.rs | 19 ++++++++----------- helix-lsp/src/file_event.rs | 4 +--- helix-view/src/document.rs | 2 +- 4 files changed, 18 insertions(+), 27 deletions(-) diff --git a/helix-lsp-types/src/lib.rs b/helix-lsp-types/src/lib.rs index 5024f6e7cbb3..c7b4f0f13285 100644 --- a/helix-lsp-types/src/lib.rs +++ b/helix-lsp-types/src/lib.rs @@ -42,9 +42,8 @@ const RESERVED: &percent_encoding::AsciiSet = &percent_encoding::CONTROLS .add(b'='); impl Url { - #[allow(clippy::result_unit_err)] #[cfg(any(unix, target_os = "redox", target_os = "wasi"))] - pub fn from_file_path>(path: P) -> Result { + pub fn from_file_path>(path: P) -> Self { #[cfg(any(unix, target_os = "redox"))] use std::os::unix::prelude::OsStrExt; #[cfg(target_os = "wasi")] @@ -63,18 +62,16 @@ impl Url { // An URL's path must not be empty. serialization.push('/'); } - Ok(Self(serialization)) + Self(serialization) } - #[allow(clippy::result_unit_err)] #[cfg(windows)] - pub fn from_file_path>(path: P) -> Result { + pub fn from_file_path>(path: P) -> Self { from_file_path_windows(path.as_ref()) } - #[allow(clippy::result_unit_err)] #[cfg_attr(not(windows), allow(dead_code))] - fn from_file_path_windows(path: &Path) -> Result { + fn from_file_path_windows(path: &Path) -> Self { use std::path::{Component, Prefix}; fn is_windows_drive_letter(segment: &str) -> bool { @@ -123,16 +120,15 @@ impl Url { serialization.push('/'); } - Ok(Self(serialization)) + Self(serialization) } - #[allow(clippy::result_unit_err)] - pub fn from_directory_path>(path: P) -> Result { - let Self(mut serialization) = Self::from_file_path(path)?; + pub fn from_directory_path>(path: P) -> Self { + let Self(mut serialization) = Self::from_file_path(path); if !serialization.ends_with('/') { serialization.push('/'); } - Ok(Self(serialization)) + Self(serialization) } /// Returns the serialized representation of the URL as a `&str` diff --git a/helix-lsp/src/client.rs b/helix-lsp/src/client.rs index e8efa613f563..6fc4f38ead4e 100644 --- a/helix-lsp/src/client.rs +++ b/helix-lsp/src/client.rs @@ -42,8 +42,7 @@ fn workspace_for_path(path: &Path) -> WorkspaceFolder { lsp::WorkspaceFolder { name, - uri: lsp::Url::from_directory_path(path) - .expect("absolute paths can be converted to `Url`s"), + uri: lsp::Url::from_directory_path(path), } } @@ -203,9 +202,7 @@ impl Client { Transport::start(reader, writer, stderr, id, name.clone()); let workspace_folders = root.clone().into_iter().collect(); - let root_uri = root.clone().map(|root| { - lsp::Url::from_file_path(root).expect("absolute paths can be converted to `Url`s") - }); + let root_uri = root.clone().map(lsp::Url::from_file_path); // `root_uri` and `workspace_folder` can be empty in case there is no workspace // `root_url` can not, use `workspace` as a fallback let root_path = root.unwrap_or(workspace); @@ -743,11 +740,11 @@ impl Client { } else { Url::from_file_path(path) }; - Some(url.ok()?.into_string()) + url.into_string() }; let files = vec![lsp::FileRename { - old_uri: url_from_path(old_path)?, - new_uri: url_from_path(new_path)?, + old_uri: url_from_path(old_path), + new_uri: url_from_path(new_path), }]; let request = self.call_with_timeout::( &lsp::RenameFilesParams { files }, @@ -777,12 +774,12 @@ impl Client { } else { Url::from_file_path(path) }; - Some(url.ok()?.into_string()) + url.into_string() }; let files = vec![lsp::FileRename { - old_uri: url_from_path(old_path)?, - new_uri: url_from_path(new_path)?, + old_uri: url_from_path(old_path), + new_uri: url_from_path(new_path), }]; Some(self.notify::(lsp::RenameFilesParams { files })) } diff --git a/helix-lsp/src/file_event.rs b/helix-lsp/src/file_event.rs index c7297d67fc11..1d60f1370d4c 100644 --- a/helix-lsp/src/file_event.rs +++ b/helix-lsp/src/file_event.rs @@ -106,9 +106,7 @@ impl Handler { log::warn!("LSP client was dropped: {id}"); return false; }; - let Ok(uri) = lsp::Url::from_file_path(&path) else { - return true; - }; + let uri = lsp::Url::from_file_path(&path); log::debug!( "Sending didChangeWatchedFiles notification to client '{}'", client.name() diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 5a17c5b73f61..965190299c53 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -1822,7 +1822,7 @@ impl Document { /// File path as a URL. pub fn url(&self) -> Option { - lsp::Url::from_file_path(self.path()?).ok() + self.path().map(lsp::Url::from_file_path) } pub fn uri(&self) -> Option {