[vim/vim] Change the LSP HTTP header Content-Type field (PR #12295)
Yegappan Lakshmanan
Based on the LSP specification, change the HTTP header Content-Type field from vim-jsonrpc to vscode-jsonrpc.
Some LSP servers (e.g. https://github.com/hashicorp/terraform-ls), compare the value of the Content-Type field against
vscode-jsonrpc and reject the LSP message if it is set to some other value.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/12295
Commit Summary
- 270e7c0 Based on the LSP specification, change the HTTP header Content-Type field from vim-jsonrpc to vscode-jsonrpc
File Changes
(3 files)
- M src/json.c (2)
- M src/testdir/test_channel.vim (2)
- M src/testdir/test_channel_lsp.py (10)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
codecov[bot]
Codecov Report
Merging #12295 (270e7c0) into master (cfc788c) will increase coverage by
0.67%.
The diff coverage isn/a.
@@ Coverage Diff @@ ## master #12295 +/- ## ========================================== + Coverage 82.05% 82.72% +0.67% ========================================== Files 160 150 -10 Lines 193344 180174 -13170 Branches 43406 40489 -2917 ========================================== - Hits 158640 149048 -9592 + Misses 21823 18176 -3647 - Partials 12881 12950 +69
| Flag | Coverage Δ | |
|---|---|---|
| huge-clang-none | 82.72% <ø> (+<0.01%) |
⬆️ |
| linux | 82.72% <ø> (+<0.01%) |
⬆️ |
| mingw-x64-HUGE | ? |
|
| mingw-x86-HUGE | ? |
|
| windows | ? |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/json.c | 81.81% <ø> (-1.62%) |
⬇️ |
... and 142 files with indirect coverage changes
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Shane-XB-Qian
seems it just says Defaults to application/vscode-jsonrpc; charset=utf-8, but did not say Must be to ?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Shane-XB-Qian
the specification just default to vscode-jsonrpc, so did not mean vim-jsonrpc was incorrect, IMO.
// so this modification perhaps is unnecessary, which server side perhaps should to compliance it better.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
We are not modifying the language server specification and adding anything Vim specific to the protocol or the headers.
So it is better to follow the specification and use the default value for the Content-Type field (to gain maximum compatibility
with the existing language servers).
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Shane-XB-Qian
IMO: the specification just tried to identify it is jsonrpc,that should be the maximum compatibility, (vs 'vscode-' or 'vim-' should be just a prefix), if the lsp server hard code to vscode-jsonrpc only, looks it was its flaw :-)
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Linwei
I belief that the responsible course of action is to raise issues with LSP servers in order to ensure that they are capable of accommodating non-VSCode clients, rather than modifying the Content-type ourselves.
LSP servers should be cognizant of the fact that they are not exclusively serving VSCode, and should strive to maintain maximum compatibility with other clients.
The Content-type serves as a means of identifying client type, and therefore should not be altered to vscode-jsonapi, as this is merely a default value in the specification and not a mandatory requirement.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Linwei
at least, make it default to vim-jsonapi and provide a way to change it like:
set lsptype=vscode-jsonapi
or something.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
Yegappan wrote:
> We are not modifying the language server specification and adding
> anything Vim specific to the protocol or the headers.
> So it is better to follow the specification and use the default value
> for the Content-Type field (to gain maximum compatibility
> with the existing language servers).
specification. I don't know what VScode-specific things there are, but
it's probably best to do it that way, we can expect it's widely
accepted.
--
Kiss me twice. I'm schizophrenic.
/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
D. Ben Knoble
Don’t we now run the risk of all editors pretending to be VSCode, much like how all the browsers pretend to be each other?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
> Don’t we now run the risk of all editors pretending to be VSCode, much
> like how [all the browsers pretend to be each
Yes. I have no idea why they allow for variations anyway. It is a new
standard, there should be no variations. If some editor wants data in a
slightly different format, it should specify just that, not with some
generic name. You can bring it up to the people who create the
specification, if you like.
--
CART DRIVER: Bring out your dead!
We follow the cart through a wretched, impoverished plague-ridden village.
A few starved mongrels run about in the mud scavenging. In the open
doorway of one house perhaps we jug glimpse a pair of legs dangling from
the ceiling. In another doorway an OLD WOMAN is beating a cat against a
wall rather like one does with a mat. The cart passes round a dead donkey
or cow in the mud. And a MAN tied to a cart is being hammered to death by
four NUNS with huge mallets.
"Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD
/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Magnus Groß
@yegappan Is there any reason why the Content-Type header is set in the first place? It causes problems with some LSP servers such as haskell-language-server, which doesn't support that header and crashes with the following error when it receives that header (when using your vim9 LSP client):
Error | Failed to parse message header:
: string
Obviously we could fix that particular server, but I think there is incentive to not send that header at all, because:
- it is an optional header
- the value that we set (
Content-Type: application/vscode-jsonrpc; charset=utf-8) is already the default value - other popular LSP clients also omit this header
I have tested the following LSP clients and not a single one of them sets the Content-Type header, only the Content-Length header is set for all of them:
- coc.nvim
- Neovim LSP
- VSCode
- Ale
- YouCompleteMe
Therefore in order to improve compatibility with slightly misbehaving LSP clients, I propose to drop this header:
diff --git a/src/json.c b/src/json.c index acf7ac57c..e2a011309 100644 --- a/src/json.c +++ b/src/json.c @@ -105,8 +105,7 @@ json_encode_lsp_msg(typval_T *val) ga_init2(&lspga, 1, 4000); // Header according to LSP specification. vim_snprintf((char *)IObuff, IOSIZE, - "Content-Length: %u\r\n" - "Content-Type: application/vscode-jsonrpc; charset=utf-8\r\n\r\n", + "Content-Length: %u\r\n\r\n", ga.ga_len - 1); ga_concat(&lspga, IObuff); ga_concat_len(&lspga, ga.ga_data, ga.ga_len);
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan Is there any reason why the
Content-Typeheader is set in the first place? It causes problems with some LSP servers such as haskell-language-server, which doesn't support that header and crashes with the following error when it receives that header (when using your vim9 LSP client):
vim-dev ML
On Fri, Aug 25, 2023 at 4:26 PM Magnus Groß ***@***.***>
wrote:
> @yegappan <https://github.com/yegappan> Is there any reason why the
> some LSP servers such as haskell-language-server
> that header (when using your vim9 LSP client
I originally set it to follow the LSP specification. But as we learned
over a period of time, some of
the LSP servers are not able to properly handle/ignore this header.
> Error | Failed to parse message header:
> : string
>
> Obviously we could fix that particular server, but I think there is
> incentive to not send that header at all, because:
>
> - the value that we set (Content-Type: application/vscode-jsonrpc;
> - other popular LSP clients also omit this header
> I have tested the following LSP clients and not a single one of them sets
> the Content-Type header, only the Content-Length header is set for all of
> them:
>
> - Neovim LSP
> <https://github.com/neovim/neovim/blob/5d8ab32f3871b0232972cac1116ac7cba98389e5/runtime/lua/vim/lsp/rpc.lua#L22>
> - VSCode
> - Ale
> <https://github.com/dense-analysis/ale/blob/115ad17ace047cab20ccc67f79c943aaf3f0f291/autoload/ale/lsp.vim#L149>
> - YouCompleteMe
> Therefore in order to improve compatibility with slightly misbehaving LSP
> clients, I propose to drop this header:
>
> diff --git a/src/json.c b/src/json.c
> // Header according to LSP specification.
> ga_concat(&lspga, IObuff);
> ga_concat_len(&lspga, ga.ga_data, ga.ga_len);
>
>
I don't see a problem in removing the Content-Type header. Can you create
a PR with this change?
Thanks,
Yegappan
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.