From 606b95717211eab05ce9564cec8312b015220c3d Mon Sep 17 00:00:00 2001
From: Michael Davis <mcarsondavis@gmail.com>
Date: Thu, 8 Aug 2024 11:03:29 -0400
Subject: [PATCH 1/3] Replace uses of lsp::Location with a custom Location type
The lsp location type has the lsp's URI type and a range. We can replace
that with a custom type private to the lsp commands module that uses the
core URI type instead.
We can't entirely replace the type with a new Location type in core.
That type might look like:
pub struct Location {
uri: crate::Uri,
range: crate::Range,
}
But we can't convert every `lsp::Location` to this type because for
definitions, references and diagnostics language servers send documents
which we haven't opened yet, so we don't have the information to convert
an `lsp::Range` (line+col) to a `helix_core::Range` (char indexing).
This cleans up the picker definitions in this file so that they can all
use helpers like `jump_to_location` and `location_to_file_location` for
the picker preview. It also removes the only use of the deprecated
`PathOrId::from_path_buf` function, allowing us to drop the owned
variant of that type in the child commit.
---
helix-core/src/uri.rs | 24 +++-
helix-term/src/commands/lsp.rs | 196 ++++++++++++++-------------------
2 files changed, 105 insertions(+), 115 deletions(-)
diff --git a/helix-core/src/uri.rs b/helix-core/src/uri.rs
index 4e03c58b..ddb9fb7a 100644
--- a/helix-core/src/uri.rs
+++ b/helix-core/src/uri.rs
@@ -1,4 +1,7 @@
-use std::path::{Path, PathBuf};
+use std::{
+ fmt,
+ path::{Path, PathBuf},
+};
/// A generic pointer to a file location.
///
@@ -47,6 +50,14 @@ impl TryFrom<Uri> for PathBuf {
}
}
+impl fmt::Display for Uri {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ match self {
+ Self::File(path) => write!(f, "{}", path.display()),
+ }
+ }
+}
+
#[derive(Debug)]
pub struct UrlConversionError {
source: url::Url,
@@ -59,11 +70,16 @@ pub enum UrlConversionErrorKind {
UnableToConvert,
}
-impl std::fmt::Display for UrlConversionError {
- fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+impl fmt::Display for UrlConversionError {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self.kind {
UrlConversionErrorKind::UnsupportedScheme => {
- write!(f, "unsupported scheme in URL: {}", self.source.scheme())
+ write!(
+ f,
+ "unsupported scheme '{}' in URL {}",
+ self.source.scheme(),
+ self.source
+ )
}
UrlConversionErrorKind::UnableToConvert => {
write!(f, "unable to convert URL to file path: {}", self.source)
diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs
index 93ac2a84..fcc0333e 100644
--- a/helix-term/src/commands/lsp.rs
+++ b/helix-term/src/commands/lsp.rs
@@ -34,7 +34,7 @@ use crate::{
use std::{
cmp::Ordering,
collections::{BTreeMap, HashSet},
- fmt::{Display, Write},
+ fmt::Display,
future::Future,
path::Path,
};
@@ -61,10 +61,31 @@ macro_rules! language_server_with_feature {
}};
}
+/// A wrapper around `lsp::Location` that swaps out the LSP URI for `helix_core::Uri`.
+#[derive(Debug, Clone, PartialEq, Eq)]
+struct Location {
+ uri: Uri,
+ range: lsp::Range,
+}
+
+fn lsp_location_to_location(location: lsp::Location) -> Option<Location> {
+ let uri = match location.uri.try_into() {
+ Ok(uri) => uri,
+ Err(err) => {
+ log::warn!("discarding invalid or unsupported URI: {err}");
+ return None;
+ }
+ };
+ Some(Location {
+ uri,
+ range: location.range,
+ })
+}
+
struct SymbolInformationItem {
+ location: Location,
symbol: lsp::SymbolInformation,
offset_encoding: OffsetEncoding,
- uri: Uri,
}
struct DiagnosticStyles {
@@ -75,35 +96,35 @@ struct DiagnosticStyles {
}
struct PickerDiagnostic {
- uri: Uri,
+ location: Location,
diag: lsp::Diagnostic,
offset_encoding: OffsetEncoding,
}
-fn uri_to_file_location<'a>(uri: &'a Uri, range: &lsp::Range) -> Option<FileLocation<'a>> {
- let path = uri.as_path()?;
- let line = Some((range.start.line as usize, range.end.line as usize));
+fn location_to_file_location(location: &Location) -> Option<FileLocation> {
+ let path = location.uri.as_path()?;
+ let line = Some((
+ location.range.start.line as usize,
+ location.range.end.line as usize,
+ ));
Some((path.into(), line))
}
fn jump_to_location(
editor: &mut Editor,
- location: &lsp::Location,
+ location: &Location,
offset_encoding: OffsetEncoding,
action: Action,
) {
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);
- return;
- }
+ let Some(path) = location.uri.as_path() else {
+ let err = format!("unable to convert URI to filepath: {:?}", location.uri);
+ editor.set_error(err);
+ return;
};
- jump_to_position(editor, &path, location.range, offset_encoding, action);
+ jump_to_position(editor, path, location.range, offset_encoding, action);
}
fn jump_to_position(
@@ -196,7 +217,10 @@ fn diag_picker(
for (diag, ls) in diags {
if let Some(ls) = cx.editor.language_server_by_id(ls) {
flat_diag.push(PickerDiagnostic {
- uri: uri.clone(),
+ location: Location {
+ uri: uri.clone(),
+ range: diag.range,
+ },
diag,
offset_encoding: ls.offset_encoding(),
});
@@ -243,7 +267,7 @@ fn diag_picker(
// between message code and message
2,
ui::PickerColumn::new("path", |item: &PickerDiagnostic, _| {
- if let Some(path) = item.uri.as_path() {
+ if let Some(path) = item.location.uri.as_path() {
path::get_truncated_path(path)
.to_string_lossy()
.to_string()
@@ -261,26 +285,14 @@ fn diag_picker(
primary_column,
flat_diag,
styles,
- move |cx,
- PickerDiagnostic {
- uri,
- diag,
- offset_encoding,
- },
- action| {
- let Some(path) = uri.as_path() else {
- return;
- };
- jump_to_position(cx.editor, path, diag.range, *offset_encoding, action);
+ move |cx, diag, action| {
+ jump_to_location(cx.editor, &diag.location, diag.offset_encoding, action);
let (view, doc) = current!(cx.editor);
view.diagnostics_handler
.immediately_show_diagnostic(doc, view.id);
},
)
- .with_preview(move |_editor, PickerDiagnostic { uri, diag, .. }| {
- let line = Some((diag.range.start.line as usize, diag.range.end.line as usize));
- Some((uri.as_path()?.into(), line))
- })
+ .with_preview(move |_editor, diag| location_to_file_location(&diag.location))
.truncate_start(false)
}
@@ -303,7 +315,10 @@ pub fn symbol_picker(cx: &mut Context) {
container_name: None,
},
offset_encoding,
- uri: uri.clone(),
+ location: Location {
+ uri: uri.clone(),
+ range: symbol.selection_range,
+ },
});
for child in symbol.children.into_iter().flatten() {
nested_to_flat(list, file, uri, child, offset_encoding);
@@ -337,7 +352,10 @@ pub fn symbol_picker(cx: &mut Context) {
lsp::DocumentSymbolResponse::Flat(symbols) => symbols
.into_iter()
.map(|symbol| SymbolInformationItem {
- uri: doc_uri.clone(),
+ location: Location {
+ uri: doc_uri.clone(),
+ range: symbol.location.range,
+ },
symbol,
offset_encoding,
})
@@ -392,17 +410,10 @@ pub fn symbol_picker(cx: &mut Context) {
symbols,
(),
move |cx, item, action| {
- jump_to_location(
- cx.editor,
- &item.symbol.location,
- item.offset_encoding,
- action,
- );
+ jump_to_location(cx.editor, &item.location, item.offset_encoding, action);
},
)
- .with_preview(move |_editor, item| {
- uri_to_file_location(&item.uri, &item.symbol.location.range)
- })
+ .with_preview(move |_editor, item| location_to_file_location(&item.location))
.truncate_start(false);
compositor.push(Box::new(overlaid(picker)))
@@ -453,8 +464,11 @@ pub fn workspace_symbol_picker(cx: &mut Context) {
}
};
Some(SymbolInformationItem {
+ location: Location {
+ uri,
+ range: symbol.location.range,
+ },
symbol,
- uri,
offset_encoding,
})
})
@@ -490,7 +504,7 @@ pub fn workspace_symbol_picker(cx: &mut Context) {
})
.without_filtering(),
ui::PickerColumn::new("path", |item: &SymbolInformationItem, _| {
- if let Some(path) = item.uri.as_path() {
+ if let Some(path) = item.location.uri.as_path() {
path::get_relative_path(path)
.to_string_lossy()
.to_string()
@@ -507,15 +521,10 @@ pub fn workspace_symbol_picker(cx: &mut Context) {
[],
(),
move |cx, item, action| {
- jump_to_location(
- cx.editor,
- &item.symbol.location,
- item.offset_encoding,
- action,
- );
+ jump_to_location(cx.editor, &item.location, item.offset_encoding, action);
},
)
- .with_preview(|_editor, item| uri_to_file_location(&item.uri, &item.symbol.location.range))
+ .with_preview(|_editor, item| location_to_file_location(&item.location))
.with_dynamic_query(get_symbols, None)
.truncate_start(false);
@@ -847,7 +856,7 @@ impl Display for ApplyEditErrorKind {
fn goto_impl(
editor: &mut Editor,
compositor: &mut Compositor,
- locations: Vec<lsp::Location>,
+ locations: Vec<Location>,
offset_encoding: OffsetEncoding,
) {
let cwdir = helix_stdx::env::current_working_dir();
@@ -860,80 +869,41 @@ fn goto_impl(
_locations => {
let columns = [ui::PickerColumn::new(
"location",
- |item: &lsp::Location, cwdir: &std::path::PathBuf| {
- // The preallocation here will overallocate a few characters since it will account for the
- // URL's scheme, which is not used most of the time since that scheme will be "file://".
- // Those extra chars will be used to avoid allocating when writing the line number (in the
- // common case where it has 5 digits or less, which should be enough for a cast majority
- // of usages).
- let mut res = String::with_capacity(item.uri.as_str().len());
-
- if item.uri.scheme() == "file" {
- // 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`.
- if let Ok(path) = item.uri.to_file_path() {
- // 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.
- res.push_str(
- &path.strip_prefix(cwdir).unwrap_or(&path).to_string_lossy(),
- );
- }
+ |item: &Location, cwdir: &std::path::PathBuf| {
+ let path = if let Some(path) = item.uri.as_path() {
+ path.strip_prefix(cwdir).unwrap_or(path).to_string_lossy()
} else {
- // Never allocates since we declared the string with this capacity already.
- res.push_str(item.uri.as_str());
- }
+ item.uri.to_string().into()
+ };
- // Most commonly, this will not allocate, especially on Unix systems where the root prefix
- // is a simple `/` and not `C:\` (with whatever drive letter)
- write!(&mut res, ":{}", item.range.start.line + 1)
- .expect("Will only failed if allocating fail");
- res.into()
+ format!("{path}:{}", item.range.start.line + 1).into()
},
)];
let picker = Picker::new(columns, 0, locations, cwdir, move |cx, location, action| {
jump_to_location(cx.editor, location, offset_encoding, action)
})
- .with_preview(move |_editor, location| {
- use crate::ui::picker::PathOrId;
-
- let lines = Some((
- location.range.start.line as usize,
- location.range.end.line as usize,
- ));
-
- // TODO: we should avoid allocating by doing the Uri conversion ahead of time.
- //
- // To do this, introduce a `Location` type in `helix-core` that reuses the core
- // `Uri` type instead of the LSP `Url` type and replaces the LSP `Range` type.
- // Refactor the callers of `goto_impl` to pass iterators that translate the
- // LSP location type to the custom one in core, or have them collect and pass
- // `Vec<Location>`s. Replace the `uri_to_file_location` function with
- // `location_to_file_location` that takes only `&helix_core::Location` as
- // parameters.
- //
- // By doing this we can also eliminate the duplicated URI info in the
- // `SymbolInformationItem` type and introduce a custom Symbol type in `helix-core`
- // which will be reused in the future for tree-sitter based symbol pickers.
- let path = Uri::try_from(&location.uri).ok()?.as_path_buf()?;
- #[allow(deprecated)]
- Some((PathOrId::from_path_buf(path), lines))
- });
+ .with_preview(move |_editor, location| location_to_file_location(location));
compositor.push(Box::new(overlaid(picker)));
}
}
}
-fn to_locations(definitions: Option<lsp::GotoDefinitionResponse>) -> Vec<lsp::Location> {
+fn to_locations(definitions: Option<lsp::GotoDefinitionResponse>) -> Vec<Location> {
match definitions {
- Some(lsp::GotoDefinitionResponse::Scalar(location)) => vec![location],
- Some(lsp::GotoDefinitionResponse::Array(locations)) => locations,
+ Some(lsp::GotoDefinitionResponse::Scalar(location)) => {
+ lsp_location_to_location(location).into_iter().collect()
+ }
+ Some(lsp::GotoDefinitionResponse::Array(locations)) => locations
+ .into_iter()
+ .flat_map(lsp_location_to_location)
+ .collect(),
Some(lsp::GotoDefinitionResponse::Link(locations)) => locations
.into_iter()
- .map(|location_link| lsp::Location {
- uri: location_link.target_uri,
- range: location_link.target_range,
+ .map(|location_link| {
+ lsp::Location::new(location_link.target_uri, location_link.target_range)
})
+ .flat_map(lsp_location_to_location)
.collect(),
None => Vec::new(),
}
@@ -1018,7 +988,11 @@ pub fn goto_reference(cx: &mut Context) {
cx.callback(
future,
move |editor, compositor, response: Option<Vec<lsp::Location>>| {
- let items = response.unwrap_or_default();
+ let items: Vec<Location> = response
+ .into_iter()
+ .flatten()
+ .flat_map(lsp_location_to_location)
+ .collect();
if items.is_empty() {
editor.set_error("No references found.");
} else {
From 48e93577882e58de85d451225494efe48fe9b606 Mon Sep 17 00:00:00 2001
From: Michael Davis <mcarsondavis@gmail.com>
Date: Thu, 8 Aug 2024 11:05:12 -0400
Subject: [PATCH 2/3] picker: Removed owned variant of PathOrId
The only caller of `from_path_buf` was removed in the parent commit
allowing us to drop owned variant of path's `Cow`. With this change we
never need to allocate in the picker preview callback.
---
helix-term/src/ui/picker.rs | 21 +++------------------
1 file changed, 3 insertions(+), 18 deletions(-)
diff --git a/helix-term/src/ui/picker.rs b/helix-term/src/ui/picker.rs
index 82fe9689..ecf8111a 100644
--- a/helix-term/src/ui/picker.rs
+++ b/helix-term/src/ui/picker.rs
@@ -32,7 +32,7 @@ use std::{
borrow::Cow,
collections::HashMap,
io::Read,
- path::{Path, PathBuf},
+ path::Path,
sync::{
atomic::{self, AtomicUsize},
Arc,
@@ -63,26 +63,12 @@ pub const MAX_FILE_SIZE_FOR_PREVIEW: u64 = 10 * 1024 * 1024;
#[derive(PartialEq, Eq, Hash)]
pub enum PathOrId<'a> {
Id(DocumentId),
- // See [PathOrId::from_path_buf]: this will eventually become `Path(&Path)`.
- Path(Cow<'a, Path>),
-}
-
-impl<'a> PathOrId<'a> {
- /// Creates a [PathOrId] from a PathBuf
- ///
- /// # Deprecated
- /// The owned version of PathOrId will be removed in a future refactor
- /// and replaced with `&'a Path`. See the caller of this function for
- /// more details on its removal.
- #[deprecated]
- pub fn from_path_buf(path_buf: PathBuf) -> Self {
- Self::Path(Cow::Owned(path_buf))
- }
+ Path(&'a Path),
}
impl<'a> From<&'a Path> for PathOrId<'a> {
fn from(path: &'a Path) -> Self {
- Self::Path(Cow::Borrowed(path))
+ Self::Path(path)
}
}
@@ -581,7 +567,6 @@ impl<T: 'static + Send + Sync, D: 'static + Send + Sync> Picker<T, D> {
match path_or_id {
PathOrId::Path(path) => {
- let path = path.as_ref();
if let Some(doc) = editor.document_by_path(path) {
return Some((Preview::EditorDocument(doc), range));
}
From da2b0a74844f77ba233638c2b5a0ee2367a66871 Mon Sep 17 00:00:00 2001
From: Michael Davis <mcarsondavis@gmail.com>
Date: Thu, 8 Aug 2024 11:17:20 -0400
Subject: [PATCH 3/3] Make helix_core::Uri cheap to clone
We clone this type very often in LSP pickers, for example diagnostics
and symbols. We can use a single Arc in many cases to avoid the
unnecessary clones.
---
helix-core/src/uri.rs | 25 ++++++-------------------
helix-view/src/handlers/lsp.rs | 18 +++++++++++-------
2 files changed, 17 insertions(+), 26 deletions(-)
diff --git a/helix-core/src/uri.rs b/helix-core/src/uri.rs
index ddb9fb7a..cbe0fadd 100644
--- a/helix-core/src/uri.rs
+++ b/helix-core/src/uri.rs
@@ -1,15 +1,18 @@
use std::{
fmt,
path::{Path, PathBuf},
+ sync::Arc,
};
/// A generic pointer to a file location.
///
/// Currently this type only supports paths to local files.
+///
+/// Cloning this type is cheap: the internal representation uses an Arc.
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)]
#[non_exhaustive]
pub enum Uri {
- File(PathBuf),
+ File(Arc<Path>),
}
impl Uri {
@@ -26,27 +29,11 @@ impl Uri {
Self::File(path) => Some(path),
}
}
-
- pub fn as_path_buf(self) -> Option<PathBuf> {
- match self {
- Self::File(path) => Some(path),
- }
- }
}
impl From<PathBuf> for Uri {
fn from(path: PathBuf) -> Self {
- Self::File(path)
- }
-}
-
-impl TryFrom<Uri> for PathBuf {
- type Error = ();
-
- fn try_from(uri: Uri) -> Result<Self, Self::Error> {
- match uri {
- Uri::File(path) => Ok(path),
- }
+ Self::File(path.into())
}
}
@@ -93,7 +80,7 @@ impl std::error::Error for UrlConversionError {}
fn convert_url_to_uri(url: &url::Url) -> Result<Uri, UrlConversionErrorKind> {
if url.scheme() == "file" {
url.to_file_path()
- .map(|path| Uri::File(helix_stdx::path::normalize(path)))
+ .map(|path| Uri::File(helix_stdx::path::normalize(path).into()))
.map_err(|_| UrlConversionErrorKind::UnableToConvert)
} else {
Err(UrlConversionErrorKind::UnsupportedScheme)
diff --git a/helix-view/src/handlers/lsp.rs b/helix-view/src/handlers/lsp.rs
index 6aff2e50..1fd2289d 100644
--- a/helix-view/src/handlers/lsp.rs
+++ b/helix-view/src/handlers/lsp.rs
@@ -243,7 +243,7 @@ impl Editor {
match op {
ResourceOp::Create(op) => {
let uri = Uri::try_from(&op.uri)?;
- let path = uri.as_path_buf().expect("URIs are valid paths");
+ 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)
});
@@ -255,13 +255,15 @@ impl Editor {
}
}
- fs::write(&path, [])?;
- self.language_servers.file_event_handler.file_changed(path);
+ fs::write(path, [])?;
+ self.language_servers
+ .file_event_handler
+ .file_changed(path.to_path_buf());
}
}
ResourceOp::Delete(op) => {
let uri = Uri::try_from(&op.uri)?;
- let path = uri.as_path_buf().expect("URIs are valid paths");
+ let path = uri.as_path().expect("URIs are valid paths");
if path.is_dir() {
let recursive = op
.options
@@ -270,11 +272,13 @@ impl Editor {
.unwrap_or(false);
if recursive {
- fs::remove_dir_all(&path)?
+ fs::remove_dir_all(path)?
} else {
- fs::remove_dir(&path)?
+ fs::remove_dir(path)?
}
- self.language_servers.file_event_handler.file_changed(path);
+ self.language_servers
+ .file_event_handler
+ .file_changed(path.to_path_buf());
} else if path.is_file() {
fs::remove_file(path)?;
}