LSP: Don't advertise support for Pull Diagnostics #627
Conversation
Standard doesn't actually support the Pull Diagnostics that were added in LSP 3.17. It supports Push Diagnostics just fine. Pull Diagnostics: Advertise support via diagnosticProvider in the server capabilities. Respond to textDocument/diagnostic with diagnostics. Note that the textDocument/diagnostic message does not include the text to be diagnosed. Push Diagnostics: At some point after receiving a textDocument/didOpen or a textDocument/didChange, send a textDocument/publishDiagnostics to the client. Pull Diagnostics give the client more control over when diagnostics are computed and received and are preferred going forward. Since Standard doesn't actually support them (yet?) we shouldn't advertise that we do.
|
I think this should be tested in the VS Code extension before merge. (Just in case VS Code fails to successfully add diagnostics provided by Standard in the Problems view if we don't purport to provide diagnostics) |
|
@searls I can fire up VS Code and do said testing. I'll report back. |
|
@searls VS Code seems to work just fine with this change. I'm going to have our team run with this change to make sure. |
|
Thanks for checking on this
|
|
@searls We haven't had any issues with either vscode or neovim (release or nightly). |
| def test_diagnotic_route | ||
| msgs, err = run_server_on_requests({ | ||
| method: "textDocument/diagnostic", | ||
| jsonrpc: "2.0", | ||
| params: { | ||
| textDocument: { | ||
| uri: "file:///path/to/file.rb" | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| assert_equal "", err.string | ||
| assert_equal 0, msgs.count | ||
| end |
There was a problem hiding this comment.
Is there any intentions to remove rather than update the test case as follows?
def test_diagnotic_route
msgs, err = run_server_on_requests({
method: "textDocument/diagnostic",
jsonrpc: "2.0",
params: {
textDocument: {
uri: "file:///path/to/file.rb"
}
}
})
- assert_equal ", err.string
- assert_equal 0, msgs.count
+ assert_equal "[server] Unsupported Method: textDocument/diagnostic\n", err.string
+ assert_equal 1, msgs.count
endThere was a problem hiding this comment.
I'm not sure that spec makes sense like that. Since we don't advertise support for it, a client won't ever send us that message.
|
This all seems reasonable and worth merging. If it causes any regression in the VS Code extension, we're sure to hear about it |
Standard doesn't actually support the Pull Diagnostics that were added in LSP 3.17. It supports Push Diagnostics just fine.
Pull Diagnostics: Advertise support via
diagnosticProviderin the server capabilities. Respond totextDocument/diagnosticwith diagnostics. Note that thetextDocument/diagnosticmessage does not include the text to be diagnosed.Push Diagnostics: At some point after receiving a
textDocument/didOpenor atextDocument/didChange, send atextDocument/publishDiagnosticsto the client.Pull Diagnostics give the client more control over when diagnostics are computed and received and are preferred going forward. Since Standard doesn't actually support them (yet?) we shouldn't advertise that we do.
Context
I figured all of this out while trying to figure out why neovim 0.10 nightly wouldn't update Standard's diagnostics until I forced a buffer reload. It seems that neovim is getting confused when Standard doesn't give a response to
textDocument/diagnostic.Turning off the advertisement for Pull Diagnostics caused neovim to stop sending the
textDocument/diagnosticmessages entirely and restored support for the existingtextDocument/publishDiagnosticsmessages.References
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_pullDiagnostics
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#serverCapabilities
https://atlee.ca/posts/pull-diagnostic-support-for-neovim/