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

Support LSP diagnostic tags #9767

Closed
wants to merge 1 commit into from
Closed

Conversation

the-mikedavis
Copy link
Member

These are defined as DiagnosticTag in the spec. Tags are an optional part of a diagnostic that can give some extra metadata. Currently the spec only contains Unnecessary and Deprecated and it gives some hints on how to theme those keys. We can introduce some new theme keys for these tags and fall back to the diagnostic's severity highlight.

These are defined as [`DiagnosticTag`] in the spec. Tags are an optional
part of a diagnostic that can give some extra metadata. Currently the
spec only contains `Unnecessary` and `Deprecated` and it gives some
hints on how to theme those keys. We can introduce some new theme keys
for these tags and fall back to the diagnostic's severity highlight.

[`DiagnosticTag`]: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#diagnosticTag
@the-mikedavis the-mikedavis added 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 Feb 29, 2024
@pascalkuthe
Copy link
Member

Sort of semiproductove input:

This came up for. Either in an issue or somekind of PR. I know I also locked atcode like this. I can't remember more than that fir the life of me but maybe searching trough github may turn something up

@pascalkuthe
Copy link
Member

I think one more thing I remebrr: What pyroght is doing may or may not be non standard compliant. I think that is the direction the discussion around these tags went last time

@the-mikedavis
Copy link
Member Author

(For context this is related to #9765)

Yeah I believe pyright should still be sending these diagnostics regardless of if the client supports tags, though I'm not sure if the spec says it explicitly. I might send a PR upstream to fix that. Regardless though I think it would be nice to take advantage of the distinct highlights we get from tags when a language server sends them

@the-mikedavis
Copy link
Member Author

I submitted that change upstream: microsoft/pyright#7380

@the-mikedavis
Copy link
Member Author

the-mikedavis commented Mar 1, 2024

I submitted this only for pyright and I'm not aware of any other language server that takes advantage of this feature. Pyright is insistent on their interpretation of the spec despite the considerable discussion against it: microsoft/pyright#1118

So I'm closing this in protest of pyright's IMO incorrect interpretation and IMO bad-faith discussion. If we can find another language server that uses this feature in the spirit the spec suggests then we can re-open but I don't see a point currently. We can do as the pyright maintainer suggests and guide people away from pyright and towards pylance instead

@the-mikedavis the-mikedavis deleted the lsp-diagnostic-tags branch March 1, 2024 06:48
@kas2020-commits
Copy link
Contributor

It's a shame to see the outcome of microsoft/pyright#1118. Thanks for jumping on this though @the-mikedavis I appreciated the effort.

A silver lining is that I kinda solved my issue by adding reportDeprecated = "warning" to my pyright config, so I finally get all the diagnostics I was getting on Neovim but in helix too.

I will say tangentially that I really love how neovim/neovide can dim out entire blocks of unreachable code. It makes the diagnostic a lot more visual and illustrative where it makes sense. I'm not sure if killing this PR prevents helix from being able to do this or not, but if helix can do this it makes sense to me to include tags in the clientInfo to convey that to the server.

Here's a cool example using Pyright on neovim/neovide:

Capture d’écran, le 2024-03-01 à 15 19 49

@the-mikedavis
Copy link
Member Author

Yeah maybe closing this is a little hasty. I suppose we can also "protest" pyright's behavior by just implementing the spec correctly :P (as Neovim does)

I totally overlooked that rust-analyzer uses this part of the spec (correctly): https://github.com/rust-lang/rust-analyzer/blob/79e0fee6a30a5f563e9b709cc5959694709e19c4/crates/rust-analyzer/src/diagnostics/to_proto.rs#L339-L357

deprecated-strikethrough

@the-mikedavis the-mikedavis restored the lsp-diagnostic-tags branch March 1, 2024 21:20
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 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