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

LSP: Discard publishDiagnostic from uninitialized servers #7467

Merged
merged 1 commit into from
Jun 28, 2023

Conversation

the-mikedavis
Copy link
Member

The spec explicitly disallows publishDiagnostic to be sent before the initialize response:

... the server is not allowed to send any requests or notifications to the client until it has responded with an InitializeResult ...

(https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#initialize)

But if a non-compliant server sends this we currently panic because we '.expect()' the server capabilities to be known to fetch the position encoding. Instead of panicking we can discard the notification and log the non-compliant behavior.

See #7466

The spec explicitly disallows publishDiagnostic to be sent before
the initialize response:

> ... the server is not allowed to send any requests or notifications to
> the client until it has responded with an InitializeResult ...

(https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#initialize)

But if a non-compliant server sends this we currently panic because we
'.expect()' the server capabilities to be known to fetch the position
encoding. Instead of panicking we can discard the notification and log
the non-compliant behavior.
@the-mikedavis the-mikedavis added C-enhancement Category: Improvements E-easy Call for participation: Experience needed to fix: Easy / not much A-language-server Area: Language server client S-waiting-on-review Status: Awaiting review from a maintainer. labels Jun 27, 2023
@pascalkuthe
Copy link
Member

pascalkuthe commented Jun 27, 2023

Are there any other cases where we expect the capabilities? I think I saw that a few times and that could cause simillar bugs (diagnostics is likely just the most common)

@the-mikedavis
Copy link
Member Author

It looks like this is actually the only place in handle_language_server_message that we assume we have the capabilities. Though it looks like we should be checking the capabilities when applying workspace edits. Currently it's hard-coded:

let res = apply_workspace_edit(
&mut self.editor,
helix_lsp::OffsetEncoding::Utf8,
&params.edit,
);

I'll make a PR for that too

@pascalkuthe
Copy link
Member

oh yeah, that should definitely be fixed. I remember fixing this elsewhere too but I must have missed that. Good catch!

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.

thanks!

@archseer archseer merged commit d3f8e05 into master Jun 28, 2023
@archseer archseer deleted the lsp-discard-uninitialized-notifications branch June 28, 2023 20:59
wes-adams pushed a commit to wes-adams/helix that referenced this pull request Jul 4, 2023
…or#7467)

The spec explicitly disallows publishDiagnostic to be sent before
the initialize response:

> ... the server is not allowed to send any requests or notifications to
> the client until it has responded with an InitializeResult ...

(https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#initialize)

But if a non-compliant server sends this we currently panic because we
'.expect()' the server capabilities to be known to fetch the position
encoding. Instead of panicking we can discard the notification and log
the non-compliant behavior.
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
…or#7467)

The spec explicitly disallows publishDiagnostic to be sent before
the initialize response:

> ... the server is not allowed to send any requests or notifications to
> the client until it has responded with an InitializeResult ...

(https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#initialize)

But if a non-compliant server sends this we currently panic because we
'.expect()' the server capabilities to be known to fetch the position
encoding. Instead of panicking we can discard the notification and log
the non-compliant behavior.
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
…or#7467)

The spec explicitly disallows publishDiagnostic to be sent before
the initialize response:

> ... the server is not allowed to send any requests or notifications to
> the client until it has responded with an InitializeResult ...

(https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#initialize)

But if a non-compliant server sends this we currently panic because we
'.expect()' the server capabilities to be known to fetch the position
encoding. Instead of panicking we can discard the notification and log
the non-compliant behavior.
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-enhancement Category: Improvements E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants