Replace uses of lsp::Location with a custom Location type
#11486
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
the-mikedavis
added
A-language-server
pascalkuthe
reviewed
Sep 2, 2024
pascalkuthe
reviewed
Sep 2, 2024
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.
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.
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.
the-mikedavis
force-pushed
the
lsp-location-refactor
branch
from
50bd90d
to
da2b0a7
Compare
archseer
approved these changes
Sep 30, 2024
archseer
reviewed
Sep 30, 2024
| #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] | ||
| #[non_exhaustive] | ||
| pub enum Uri { | ||
| File(PathBuf), | ||
| File(Arc<Path>), |
There was a problem hiding this comment.
Hmm do we clone that often though? Some pickers return large lists so we could be dealing with thousands of Arcs
There was a problem hiding this comment.
For document diagnostic and symbol pickers we clone the doc's URI for each entry and for workspace diagnostics we clone the entire BTreeMap<Uri, ..>. This change shouldn't affect most other pickers (for example buffer picker) since many use the DocumentId enum member of PathOrId. It might not be ideal for global search and some others though that are using paths exclusively. It could be better anyway for memory usage since by switching from PathBuf to Path we cut excess capacity.
pascalkuthe
approved these changes
Oct 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Note that we can't create a helix-core Location type instead...
That type would look like:
But we can't convert every
lsp::Locationto 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 anlsp::Range(line+col) to ahelix_core::Range(char indexing).This cleans up the picker definitions in this file so that they can all use helpers like
jump_to_locationandlocation_to_file_locationfor the picker preview. It also enables some refactors to make some things cheaper:PathOrId::from_path_bufso we can drop the owned variant of that type. Now the picker preview callback never allocates.Arc<Path>as the internal representation ofhelix_core::Uri, making it cheap to clone. We clone URIs potentially often in diagnostics and symbol pickers.With this change we also validate URIs for LSP based pickers and goto references/definition/etc. so we don't open ones with unsupported schemes, for example #11484