Specification for MarkupContent support in diagnostic messages

[MariaSolOs]

Closes #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.

[rchl]

It will be a breaking change (type-wise) if the type suddenly changes from string to string | MarkupContent. I think this might need to be a separate property.

(not sure how such cases were handled historically)

[rchl]

Realistically servers will probably have to support both plain string and markup since there is no guarantee that all clients will support markup. So it might make sense for servers to provide both variants in separate properties (when client supports markup).

[MariaSolOs]

It will be a breaking change (type-wise) if the type suddenly changes from string to string | MarkupContent. I think this might need to be a separate property.

I think this is fine given that it is behind a capability. Note how previous proposals followed the same approach; e.g. snipped edits.

[MariaSolOs]

Realistically servers will probably have to support both plain string and markup since there is no guarantee that all clients will support markup. So it might make sense for servers to provide both variants in separate properties (when client supports markup).

True, although one could argue that this is also true for other features that support markup content, yet those don't provide both formats.

[MariaSolOs]

That being said, @dbaeumer has the final call.

[MariaSolOs]

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

[dbaeumer]

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.

[MariaSolOs]

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]

I would mirror what we have on the client

ServerCapabilities {
    textDocument: {
         diagnostics: {
         }
    }
}

[MariaSolOs]

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]

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 406cd23 :)

[dbaeumer]

@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.

[mfussenegger]

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)?

[MariaSolOs]

@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]

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

[MariaSolOs]

@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]

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:

• Exclude diagnostics with MarkupContent if the server has no support for it
• Ignore the server capability but ensure only diagnostics originating from a server are sent to it on code actions requests (but then why require the extra server capability?)

[dbaeumer]

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.