Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace uses of lsp::Location with a custom Location type #11486

Merged
merged 3 commits into from
Oct 4, 2024

Conversation

the-mikedavis
Copy link
Member

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:

pub struct Location {
    uri: helix_core::Uri,
    range: helix_core::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 enables some refactors to make some things cheaper:

  • It removes the only use of the deprecated PathOrId::from_path_buf so we can drop the owned variant of that type. Now the picker preview callback never allocates.
  • We can then use an Arc<Path> as the internal representation of helix_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

Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great overall, just one nit from my side

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.
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)]
#[non_exhaustive]
pub enum Uri {
File(PathBuf),
File(Arc<Path>),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm do we clone that often though? Some pickers return large lists so we could be dealing with thousands of Arcs

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 pascalkuthe merged commit 162028d into master Oct 4, 2024
6 checks passed
@pascalkuthe pascalkuthe deleted the lsp-location-refactor branch October 4, 2024 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-command Area: Commands A-language-server Area: Language server client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants