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

Ignore invalid file URIs from LSP #6000

Merged
merged 1 commit into from
Feb 16, 2023

Conversation

xJonathanLEI
Copy link
Contributor

This PR changes a call to unwrap() to match. Currently the editor crashes when the LSP publishes diagnostics with an unsupported URI. With this PR it simply ignores them.

Submitting this patch because the Cairo language server crashes Helix when it sends a file URI like vfs://xxxxxx where vfs refers to the internal in-memory file system of the Cairo language server.

I'm not sure if this is considered a bug of the Cairo lang server as I don't know for sure if schemes other than file is allowed. But in any case I think we shouldn't panic here.

@the-mikedavis the-mikedavis added C-bug Category: This is a bug A-language-server Area: Language server client S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 15, 2023
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.

I do think this is a bug in the Cairo LS as the uri is expected to refer to a valid file (otherwise the notification is useless). Howber the LSP standard is pretty vaque and only states that the uri fieldust.ne parsed as a valid URI but not as a valid FILE uri:

Many of the interfaces contain fields that correspond to the URI of a document. For clarity, the type of such a field is declared as a DocumentUri. Over the wire, it will still be transferred as a string, but this guarantees that the contents of that string can be parsed as a valid URI.

I think being defensive is good here (easy fix for a panic) especially as buggy and non standard compliant LSPs are sadly not that rare.

LGTM 👍

@archseer archseer merged commit 9368ac7 into helix-editor:master Feb 16, 2023
@xJonathanLEI xJonathanLEI deleted the dev/panic_on_lsp_path branch February 16, 2023 05:31
wes-adams pushed a commit to wes-adams/helix that referenced this pull request Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-language-server Area: Language server client C-bug Category: This is a bug S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants