microsoft / language-server-protocol

Defines a common protocol for language servers.
https://microsoft.github.io/language-server-protocol/
Creative Commons Attribution 4.0 International
10.91k stars 764 forks source link

Specification for MarkupContent support in diagnostic messages #1905

Closed MariaSolOs closed 3 months ago

MariaSolOs commented 4 months ago

Closes https://github.com/microsoft/language-server-protocol/issues/250 by introducing a new client capability in workspace.diagnostic.markupMessageSupport, which when set allows language server to send diagnostic messages as MarkupContent.

Note that several language servers today (like LuaLS or rust-analyzer) already return diagnostics with Markdown syntax (such as backticks). For a better UI experience, formalizing this in the spec is desired.

The implementation was done in Neovim here.

MariaSolOs commented 4 months ago

Note to self: Update the meta model (unless it's autogenerated?)

MariaSolOs commented 4 months ago

That being said, @dbaeumer has the final call.

If objects only flow between the same client and server a capability is enough on the receiving side. However diagnostic can be more tricky since the flow in code actions from the client to the server. And in principal diagnostics from a plugin A can flow to a a server B. So we would need a server capability as well to let clients know that the can handle diagnostics with markup content.

Makes sense. I'll add the server capability too.

I am unsure of where exactly this capability should be defined though. I don't believe we should add it under diagnosticsProvider, since a non-provider could still receive diagnostics. Do you think we should add it at the top level @dbaeumer?

dbaeumer commented 4 months ago

I would mirror what we have on the client

ServerCapabilities {
    textDocument: {
         diagnostics: {
         }
    }
}
MariaSolOs commented 4 months ago

I would mirror what we have on the client

ServerCapabilities {
    textDocument: {
         diagnostics: {
         }
    }
}

I assume you're referring to workspace capabilities ;)

EDIT: Oh wait, I just realized that there's workspace.diagnostics and textDocument.diagnostic capabilities. I had initially added to the former one, but it makes more sense to add it to text document capabilities.

MariaSolOs commented 4 months ago

That being said, @dbaeumer has the final call.

If objects only flow between the same client and server a capability is enough on the receiving side. However diagnostic can be more tricky since the flow in code actions from the client to the server. And in principal diagnostics from a plugin A can flow to a a server B. So we would need a server capability as well to let clients know that the can handle diagnostics with markup content.

@dbaeumer I've addressed this in https://github.com/microsoft/language-server-protocol/commit/406cd2357f23f1fe9c6b66315759a252770b2a5c :)

MariaSolOs commented 3 months ago

@MariaSolOs PR looks good to me. Would you be willing to make a PR against the vscode-languageserver-node repository as well. We would need it for the meta model and we should set the capabilities to false for VS Code.

@dbaeumer sure, but now that we have an editor implementation and the meta model has been updated here as well, could we merge this first?

dbaeumer commented 3 months ago

@MariaSolOs just out of curiosity: how did you update the meta model?

MariaSolOs commented 3 months ago

@MariaSolOs just out of curiosity: how did you update the meta model?

Ehhh I did it manually, with lots of care and love hehe

mfussenegger commented 3 months ago

What should a client do if the server sends diagnostics with markup, but has markupMessageSupport set to false?

Would a client then need to filter out those diagnostics in places where the client sends them back to the server (e.g. code actions)?

Any chance to have a clarification for this in the specification? The PR against neovim currently converts the MarkupContent to a string by simply taking the value from MarkupContent unmodified, dropping the kind information.

Other possible options I see:

dbaeumer commented 3 months ago

Any chance to have a clarification for this in the specification?

Agree we should do that. And we should exclude those diagnostics from the servers that don't have support for them.