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.

Sign up for GitHub

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

Jump to bottom

Convert LSP URLs into custom URIs #9652

Closed
wants to merge 2 commits into from
Closed

Convert LSP URLs into custom URIs #9652

wants to merge 2 commits into from

Conversation

the-mikedavis
Copy link
Member

@the-mikedavis the-mikedavis commented Feb 17, 2024 •
edited by pascalkuthe
Loading

This introduces a custom URI type for core meant to be extended later if we support other schemes. For now it's just a wrapper over a PathBuf. We use this new Uri type to firewall lsp::Url so that:

  • we normalize URLs consistently (for example URL-encoded characters like + or Windows drive letters)
  • we handle language servers sending valid URLs that we can't understand (for example C#'s LS sending csharp:/metadata/foo/bar/Baz.cs URLs)

Closes #2109
Closes #3267
Closes #7367

@the-mikedavis the-mikedavis added A-language-server Area: Language server client A-core Area: Helix core improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 17, 2024
@the-mikedavis the-mikedavis mentioned this pull request Feb 17, 2024
Closed
soqb
soqb reviewed Feb 17, 2024
View reviewed changes
Copy link
Contributor

@soqb soqb left a comment

Choose a reason for hiding this comment

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

i'm fully on board with this direction as an iteration on the one i proposed.

(note: i approve of this PR, but think it a bit too mischievous to approve a PR with some of my own commits in it)

helix-core/src/uri.rs Outdated Show resolved Hide resolved
helix-core/src/uri.rs Outdated Show resolved Hide resolved
helix-core/src/uri.rs Outdated Show resolved Hide resolved
helix-core/src/uri.rs Outdated Show resolved Hide resolved
helix-term/src/application.rs Outdated
let language_server = language_server!();
if !language_server.is_initialized() {
log::error!("Discarding publishDiagnostic notification sent by an uninitialized server: {}", language_server.name());
return;
}
// 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.path().map(|p| p == path).unwrap_or(false))
Copy link
Contributor

Choose a reason for hiding this comment

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

out of scope for this PR (of course) but it's making more and more sense to switch to indexing documents by URI...

helix-term/src/commands/lsp.rs Show resolved Hide resolved
@the-mikedavis
Copy link
Member Author

Thanks for the feedback @soqb!

I've left most of the path conversions as expects for now. Ideally we would use Uri nearly everywhere we use a Path/PathBuf currently (for example Editor::open or replacing Document's path field) and move the expects into the places where the paths are actually used. I briefly looked into making that change but Path/PathBuf usage is really pervasive and it would end up being a really really large change. IMO we should delay a big refactor like that until we find a concrete use-case for other Uris

pascalkuthe
pascalkuthe reviewed Feb 18, 2024
View reviewed changes
helix-core/src/uri.rs Show resolved Hide resolved
helix-core/src/uri.rs Outdated Show resolved Hide resolved
helix-term/src/application.rs Outdated Show resolved Hide resolved
helix-term/src/commands/lsp.rs Outdated Show resolved Hide resolved
helix-term/src/commands/lsp.rs Outdated Show resolved Hide resolved
@nkitsaini
Copy link
Contributor

nkitsaini commented Feb 20, 2024 •
edited
Loading

Shouldn't this be provided by lsp-types? Currently lsp-types just uses url::Url but I think it will be better if lsp-types has a custom type with accurate serde parsing. Atleast for the normalization part.

Unaware of this pull request, I had raised an issue on lsp-types for the normalization of Uri as I came across the normalization bug while trying to make svelteserver work correctly: gluon-lang/lsp-types#276

@the-mikedavis
Copy link
Member Author

It would definitely be convenient if lsp_types took care of normalization. We would still want a Helix-specific URI though for future expansion.

Calling it a spec violation seems like editorializing though. The spec only says that clients and servers should be careful to handle different encodings consistently: it's convenient if that's taken care of at deserialization time but not mandatory. Converting all URIs to PathBuf is also a viable approach for Rust clients/servers - that's what #7367 does

@nkitsaini
Copy link
Contributor

nkitsaini commented Feb 20, 2024 •
edited
Loading

We would still want a Helix-specific URI though for future expansion.

Yes, I think we will need something helix specific sooner or later.

Calling it a spec violation seems like editorializing though

On a second re-read, I agree it is not exactly a violation of the spec. I had mistakenly assumed that this is what it meant while looking at the reference implementation. Sorry for that. So it seems we can't rely on lsp-types implementing this.

@pascalkuthe
Copy link
Member

this branch has a conflict otherwise lgtm

@the-mikedavis
Copy link
Member Author

A version of this was merged with #9647 (added there since it enabled a refactor for picker previews to eliminate allocations: 3906f66)

@the-mikedavis the-mikedavis deleted the core-uri-type branch July 15, 2024 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soqb soqb soqb left review comments

@pascalkuthe pascalkuthe pascalkuthe approved these changes

Assignees
No one assigned
Labels
A-core Area: Helix core improvements A-language-server Area: Language server client S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Milestone
No milestone
4 participants
@the-mikedavis @nkitsaini @pascalkuthe @soqb