[vim/vim] WIP: Add support for processing language server protocol messages (PR #10025)
Yegappan Lakshmanan
Add basic support for processing language server protocol messages. The protocol is
described in: https://microsoft.github.io/language-server-protocol/specification.
The existing support in Vim for processing JSON encoded messages doesn't work
for language server protocol. The LSP messages start with a HTTP header followed
by a JSON encoded dictionary. This format is currently not supported by the Vim
channel "json" mode.
This PR adds a new "lsp" channel mode to process the LSP header. A LSP client
can use ch_sendexpr() to send a Vim expression to the LSP server and
received messages will be decoded and the callback function will be invoked
with a Vim type.
I have tested this with the Vim9 LSP client and it works. But this needs more
extensive testing.
The RPC semantic is not yet supported.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/10025
Commit Summary
- 3ce9a45 Add support for processing language server protocol messages
File Changes
(6 files)
- M runtime/doc/channel.txt (28)
- M src/channel.c (143)
- M src/job.c (2)
- M src/json.c (25)
- M src/proto/json.pro (1)
- M src/structs.h (1)
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 #10025 (3ce9a45) into master (bf269ed) will decrease coverage by
1.04%.
The diff coverage is32.87%.
@@ Coverage Diff @@ ## master #10025 +/- ## ========================================== - Coverage 81.91% 80.86% -1.05% ========================================== Files 167 152 -15 Lines 187261 174462 -12799 Branches 42217 39570 -2647 ========================================== - Hits 153401 141087 -12314 + Misses 21521 20740 -781 - Partials 12339 12635 +296
| Flag | Coverage Δ | |
|---|---|---|
| huge-clang-none | 82.33% <32.87%> (-0.02%) |
⬇️ |
| huge-gcc-none | ? |
|
| huge-gcc-testgui | ? |
|
| huge-gcc-unittests | 2.01% <0.00%> (-0.01%) |
⬇️ |
| linux | 80.86% <32.87%> (-3.05%) |
⬇️ |
| mingw-x64-HUGE | ? |
|
| mingw-x64-HUGE-gui | ? |
|
| windows | ? |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/json.c | 78.36% <0.00%> (-5.85%) |
⬇️ |
| src/channel.c | 81.43% <38.33%> (-2.34%) |
⬇️ |
| src/job.c | 89.22% <50.00%> (-1.23%) |
⬇️ |
| src/libvterm/src/rect.h | 0.00% <0.00%> (-96.78%) |
⬇️ |
| src/libvterm/src/state.c | 34.80% <0.00%> (-54.77%) |
⬇️ |
| src/libvterm/src/keyboard.c | 40.00% <0.00%> (-47.63%) |
⬇️ |
| src/libvterm/include/vterm.h | 0.00% <0.00%> (-44.45%) |
⬇️ |
| src/libvterm/src/parser.c | 55.41% <0.00%> (-40.49%) |
⬇️ |
| src/libvterm/src/pen.c | 44.37% <0.00%> (-39.90%) |
⬇️ |
| src/libvterm/src/encoding.c | 37.37% <0.00%> (-36.16%) |
⬇️ |
| ... and 149 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered by Codecov. Last update bf269ed...3ce9a45. Read the comment docs.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan pushed 1 commit.
- 6205790 Add support for LSP JSON RPC
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
LemonBoy
@LemonBoy commented on this pull request.
In src/channel.c:
> + * It has the following two fields:
+ *
+ * Content-Length: ...
+ * Content-Type: application/vscode-jsonrpc; charset=utf-8
+ *
+ * Each field ends with "\r\n". The header ends with an additional "\r\n".
+ */
+ static int
+channel_skip_lsp_http_hdr(js_read_T *reader)
+{
+ char_u *p;
+
+ // We find the end once, to avoid calling strlen() many times.
+ reader->js_end = reader->js_buf + STRLEN(reader->js_buf);
+
+ // skip the HTTP header
The logic here looks a bit fragile as it assumes a fixed order of header fields (that's irrelevant according to RFC7320 3.2.2) and a fixed number of them (the LSP specification only states there's at least one entry "and that at least one header is mandatory").
A better solution would be to iterate line by line (terminated by \r\n) until an empty one is found and parsing/validating each header entry that's found.
Some validation of the parsed data is also needed as:
- The
Content-Lengthmust match the effective payload length according to the HTTP spec and it can be easily used to check if the given request has been fully received, delaying the parsing otherwise.
When a Content-Length is given in a message where a message-body is allowed, its field value MUST exactly match the number of OCTETs in the message-body. HTTP/1.1 user agents MUST notify the user when an invalid length is received and detected.
Content-Typemust containutf8orutf-8to make the code more robust wrt encoding changes.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
Yegappan Lakshmanan
vim-dev ML
On Mon, Mar 28, 2022 at 1:48 AM LemonBoy ***@***.***> wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In src/channel.c
> <https://github.com/vim/vim/pull/10025#discussion_r836191546>:
>
> > + * It has the following two fields:
> + *
> + * Content-Length: ...
> + * Content-Type: application/vscode-jsonrpc; charset=utf-8
> + *
> + * Each field ends with "\r\n". The header ends with an additional "\r\n".
> + */
> + static int
> +channel_skip_lsp_http_hdr(js_read_T *reader)
> +{
> + char_u *p;
> +
> + // We find the end once, to avoid calling strlen() many times.
> + reader->js_end = reader->js_buf + STRLEN(reader->js_buf);
> +
> + // skip the HTTP header
>
> The logic here looks a bit fragile as it assumes a fixed order of header
> fields (that's irrelevant according to RFC7320 3.2.2) and a fixed number of
> them (the LSP specification only states there's at least one entry "and
> that at least one header is mandatory").
> A better solution would be to iterate line by line (terminated by \r\n)
> until an empty one is found and parsing/validating each header entry that's
> found.
> Some validation of the parsed data is also needed as:
>
> according to the HTTP spec and it can be easily used to check if the given
> request has been fully received, delaying the parsing otherwise.
>
>
>
> When a Content-Length is given in a message where a message-body is allowed, its field value MUST exactly match the number of OCTETs in the message-body. HTTP/1.1 user agents MUST notify the user when an invalid length is received and detected.
>
>
Thanks for the feedback. I have updated the PR to process the HTTP header
correctly
and check for these conditions. Let me know if any other cases need to be
handled.
Regards,
Yegappan
>
> wrt encoding changes.
>
>
>
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
LemonBoy
@LemonBoy commented on this pull request.
In src/channel.c:
> +
+ // Process each line in the header till an empty line is read (header
+ // separator).
+ while (TRUE)
+ {
+ line_start = p;
+ while (*p != NUL && *p != '\n')
+ p++;
+ if (*p == NUL) // partial header
+ return MAYBE;
+ p++;
+
+ // process the content length field (if present)
+ if ((p - line_start > 16)
+ && STRNICMP(line_start, "Content-Length: ", 16) == 0)
+ payload_len = atoi((char *)line_start + 16);
This line is quite dangerous, a negative content length can and will wreak havock.
The use of atoi is also problematic, in case of overflow you may be invoking undefined behaviour, plus the error checking is quite lacking. This call can be replaced with the use of strtol (and proper error checking, don't forget the check for ERANGE) and a check to make sure the value is not negative.
Using unsigned variables for everything that represents a length is also a good idea.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
LemonBoy
> + // separator).
+ while (TRUE)
+ {
+ line_start = p;
+ while (*p != NUL && *p != '\n')
+ p++;
+ if (*p == NUL) // partial header
+ return MAYBE;
+ p++;
+
+ // process the content length field (if present)
+ if ((p - line_start > 16)
+ && STRNICMP(line_start, "Content-Length: ", 16) == 0)
+ payload_len = atoi((char *)line_start + 16);
+
+ if ((line_start + 2) == p && line_start[0] == '\r' &&
Minor nit, make this consistent with the check above.
⬇️ Suggested change- if ((line_start + 2) == p && line_start[0] == '\r' && + if ((p - line_start) == 2 && line_start[0] == '\r' &&
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
LemonBoy
> + payload_len = atoi((char *)line_start + 16); + + if ((line_start + 2) == p && line_start[0] == '\r' && + line_start[1] == '\n') + // reached the empty line + break; + } + + if (payload_len == -1) + // Content-Length field is not present in the header + return FAIL; + + hdr_len = p - reader->js_buf; + + // if the entire payload is not received, wait for more data to arrive + if (jsbuf_len < hdr_len + payload_len)
The parser should only consume payload_len bytes from the request, the remaining data shall be interpreted as the beginning of a new one.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
LemonBoy
@LemonBoy commented on this pull request.
In src/json.c:
> @@ -86,6 +86,31 @@ json_encode_nr_expr(int nr, typval_T *val, int options)
ga_append(&ga, NUL);
return ga.ga_data;
}
+
+/*
+ * Encode "val" into a JSON format string prefixed by the LSP HTTP header.
+ * Returns NULL when out of memory.
+ */
+ char_u *
+json_encode_lsp_msg(typval_T *val)
+{
+ garray_T ga;
+ garray_T lspga;
+
+ ga_init2(&ga, 1, 4000);
+ json_encode_gap(&ga, val, 0);
If json_encode_gap fails an empty payload is sent, the error should be relayed to the caller.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Yegappan Lakshmanan
@LemonBoy commented on this pull request.
In src/channel.c:
> + + // Process each line in the header till an empty line is read (header + // separator). + while (TRUE) + { + line_start = p; + while (*p != NUL && *p != '\n') + p++; + if (*p == NUL) // partial header + return MAYBE; + p++; + + // process the content length field (if present) + if ((p - line_start > 16) + && STRNICMP(line_start, "Content-Length: ", 16) == 0) + payload_len = atoi((char *)line_start + 16);This line is quite dangerous, a negative content length can and will wreak havock.
The use ofatoiis also problematic, in case of overflow you may be invoking undefined behaviour, plus the error checking is quite lacking. This call can be replaced with the use ofstrtol(and proper error checking, don't forget the check forERANGE) and a check to make sure the value is not negative.
vim-dev ML
On Tue, Mar 29, 2022 at 12:52 AM LemonBoy ***@***.***> wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In src/channel.c
> <https://github.com/vim/vim/pull/10025#discussion_r837163064>:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
On Tue, Mar 29, 2022 at 12:55 AM LemonBoy <vim-dev...@256bit.org> wrote:
>
> @LemonBoy commented on this pull request.
>
> ________________________________
>
> In src/channel.c:
>
> > + payload_len = atoi((char *)line_start + 16);
> +
> + if ((line_start + 2) == p && line_start[0] == '\r' &&
> + line_start[1] == '\n')
> + // reached the empty line
> + break;
> + }
> +
> + if (payload_len == -1)
> + // Content-Length field is not present in the header
> + return FAIL;
> +
> + hdr_len = p - reader->js_buf;
> +
> + // if the entire payload is not received, wait for more data to arrive
> + if (jsbuf_len < hdr_len + payload_len)
>
> The parser should only consume payload_len bytes from the request, the remaining data shall be interpreted as the beginning of a new one.
>
so that only the
payload_len bytes are consumed from the request.
- Yegappan
vim-dev ML
—
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 commented.
Prabir Shrestha
Was about to play around and noticed that no release has been created for the past 7 days at https://github.com/vim/vim-win32-installer/releases.
https://ci.appveyor.com/project/chrisbra/vim-win32-installer/builds/43143548/job/okn2ow3fvro3kxvjFailures:
From test_channel.vim:
Found errors in Test_channel_lsp_mode():
Run 1:
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[459]..function RunTheTest[44]..Test_channel_lsp_mode[1]..RunServer[21]..LspTests line 104: Expected [{'jsonrpc': '2.0', 'result': {'method': 'echo', 'jsonrpc': '2.0', 'id': 110, 'params': {'s': 'msg-with-id'}}}] but got []
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[459]..function RunTheTest[44]..Test_channel_lsp_mode[1]..RunServer[21]..LspTests line 110: Expected {'id': 14, 'jsonrpc': '2.0', 'result': 'extra-hdr-fields'} but got ''
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[459]..function RunTheTest[44]..Test_channel_lsp_mode[1]..RunServer[21]..LspTests line 119: Expected {'id': 16, 'jsonrpc': '2.0', 'result': 'alive'} but got ''
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[459]..function RunTheTest[44]..Test_channel_lsp_mode[1]..RunServer[21]..LspTests line 127: Expected {'id': 18, 'jsonrpc': '2.0', 'result': 'alive'} but got ''
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[459]..function RunTheTest[44]..Test_channel_lsp_mode[1]..RunServer[21]..LspTests line 135: Expected {'id': 20, 'jsonrpc': '2.0', 'result': 'alive'} but got ''
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[459]..function RunTheTest[44]..Test_channel_lsp_mode[1]..RunServer[21]..LspTests line 143: Expected {'id': 22, 'jsonrpc': '2.0', 'result': 'alive'} but got ''
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[459]..function RunTheTest[44]..Test_channel_lsp_mode[1]..RunServer[21]..LspTests line 151: Expected {'id': 24, 'jsonrpc': '2.0', 'result': 'alive'} but got ''
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[459]..function RunTheTest[44]..Test_channel_lsp_mode[1]..RunServer[21]..LspTests line 171: Expected [{'jsonrpc': '2.0', 'result': {'method': 'echo', 'jsonrpc': '2.0', 'params': {'s': 'no-callback'}}}] but got []
Run 2:
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[492]..function RunTheTest[44]..Test_channel_lsp_mode[1]..RunServer[21]..LspTests line 104: Expected [{'jsonrpc': '2.0', 'result': {'method': 'echo', 'jsonrpc': '2.0', 'id': 110, 'params': {'s': 'msg-with-id'}}}] but got []
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[492]..function RunTheTest[44]..Test_channel_lsp_mode[1]..RunServer[21]..LspTests line 110: Expected {'id': 14, 'jsonrpc': '2.0', 'result': 'extra-hdr-fields'} but got ''
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[492]..function RunTheTest[44]..Test_channel_lsp_mode[1]..RunServer[21]..LspTests line 119: Expected {'id': 16, 'jsonrpc': '2.0', 'result': 'alive'} but got ''
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[492]..function RunTheTest[44]..Test_channel_lsp_mode[1]..RunServer[21]..LspTests line 127: Expected {'id': 18, 'jsonrpc': '2.0', 'result': 'alive'} but got ''
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[492]..function RunTheTest[44]..Test_channel_lsp_mode[1]..RunServer[21]..LspTests line 135: Expected {'id': 20, 'jsonrpc': '2.0', 'result': 'alive'} but got ''
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[492]..function RunTheTest[44]..Test_channel_lsp_mode[1]..RunServer[21]..LspTests line 143: Expected {'id': 22, 'jsonrpc': '2.0', 'result': 'alive'} but got ''
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[492]..function RunTheTest[44]..Test_channel_lsp_mode[1]..RunServer[21]..LspTests line 151: Expected {'id': 24, 'jsonrpc': '2.0', 'result': 'alive'} but got ''
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[492]..function RunTheTest[44]..Test_channel_lsp_mode[1]..RunServer[21]..LspTests line 171: Expected [{'jsonrpc': '2.0', 'result': {'method': 'echo', 'jsonrpc': '2.0', 'params': {'s': 'no-callback'}}}] but got []
Run 3:
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[492]..function RunTheTest[44]..Test_channel_lsp_mode[1]..RunServer[21]..LspTests line 104: Expected [{'jsonrpc': '2.0', 'result': {'method': 'echo', 'jsonrpc': '2.0', 'id': 110, 'params': {'s': 'msg-with-id'}}}] but got []
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[492]..function RunTheTest[44]..Test_channel_lsp_mode[1]..RunServer[21]..LspTests line 110: Expected {'id': 14, 'jsonrpc': '2.0', 'result': 'extra-hdr-fields'} but got ''
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[492]..function RunTheTest[44]..Test_channel_lsp_mode[1]..RunServer[21]..LspTests line 119: Expected {'id': 16, 'jsonrpc': '2.0', 'result': 'alive'} but got ''
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[492]..function RunTheTest[44]..Test_channel_lsp_mode[1]..RunServer[21]..LspTests line 127: Expected {'id': 18, 'jsonrpc': '2.0', 'result': 'alive'} but got ''
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[492]..function RunTheTest[44]..Test_channel_lsp_mode[1]..RunServer[21]..LspTests line 135: Expected {'id': 20, 'jsonrpc': '2.0', 'result': 'alive'} but got ''
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[492]..function RunTheTest[44]..Test_channel_lsp_mode[1]..RunServer[21]..LspTests line 143: Expected {'id': 22, 'jsonrpc': '2.0', 'result': 'alive'} but got ''
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[492]..function RunTheTest[44]..Test_channel_lsp_mode[1]..RunServer[21]..LspTests line 151: Expected {'id': 24, 'jsonrpc': '2.0', 'result': 'alive'} but got ''
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[492]..function RunTheTest[44]..Test_channel_lsp_mode[1]..RunServer[21]..LspTests line 171: Expected [{'jsonrpc': '2.0', 'result': {'method': 'echo', 'jsonrpc': '2.0', 'params': {'s': 'no-callback'}}}] but got []
Flaky test failed too often, giving up
TEST FAILURE
NMAKE : fatal error U1077: 'if' : return code '0x1'
Stop.
Command exited with code 1
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Yegappan Lakshmanan
Was about to play around and noticed that no release has been created for the past 7 days at https://github.com/vim/vim-win32-installer/releases.
vim-dev ML
On Tue, Apr 5, 2022 at 5:44 PM Prabir Shrestha ***@***.***>
wrote:
> Was about to play around and noticed that no release has been created for
> the past 7 days at https://github.com/vim/vim-win32-installer/releases.
>
>
> The latest MS-Windows Vim build (v8.2.4710) is available now. You should
be able to try out
the LSP support.
Regards,
Yegappan
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Prabir Shrestha
I'm able to download the latest version of vim with lsp support on Windows. Thanks for the quick fix. ArchLinux vim also seems to be updated to include this patch. Looking forward to seeing plugin authors use this feature.
I have started adding support for native lsp client in vim-lsp and I'm very happy with the simplicity and ease of use of the api.
I have just got basic request/response working so this feedback is a bit limited while playing with the api for several mins.
- LSP support $/cancelRequest which requires sending the request id. This is heavily used by vim-lsp in order to cancel previous requests such as completion results that could had changed, highlighting current keyword that could had changed due to the cursor movements. Doc explicitly says
Note that in the request message the 'id' field should not be specified. Is it possible to have an api to either set the id or get the id of the request? - Is it possible to document what
respis in the example doc.let resp = ch_evalexpr(ch, req, #{timeout: 100}). I also see this being used in the test and it checks for empty object. I thought this was the request id but always seem to be empty during my limited test. Probably this only makes sense for synchronous request? It would still be good to document that it will always be empty for async as it created a bit confusion to me. - Minor: Should the doc include the full raw request?
{'jsonrpc': '2.0', 'id': 1, 'result': { ...}}. I see this as useful doc when writing remote LSP/Json RPC plugins. - It took be a bit to figure out the callback args for
ch_evalexprbut seems like this is already documented at the end where it says to refer to |channel-callback|. One thing that wasn't clear is if there is a JSON RPC Error Message would it still call the callback function? Clarifying that caller needs to handle the message whether it is success or error rpc messages would be better here. In the future I can see havingon_successandon_errorcallbacks. I don't see me using this in vim-lsp but do see it using in other plugins. https://www.jsonrpc.org/specification#response_object - Can the sync and async method be used on the same channel? Because vim doesn't support async BufWritePre we recommend users to use the following code to auto format on save. While 99% of the code will be using async function there will be sync methods for writing.
let g:lsp_format_sync_timeout = 1000 autocmd! BufWritePre *.rs,*.go call execute('LspDocumentFormatSync')
- Is it possible to document the behavior of how to handle timeouts? When using
let resp = ch_evalexpr(ch, req, #{timeout: 100})what error is thrown and what error should I handle?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Prabir Shrestha
I figured out how to get the id, but seems like the request and response id are not the same. req id seems to always be +1 of response id.
let req = { 'method': 'initialize', 'params': {...} } call ch_sendexpr(ch, req, { 'callback': function('s:cb', [req] }) call s:log(req) " req => { "method": "initialize", "jsonrpc": "2.0", "id": 2, "params": {} } function! s:cb(req, ch, res) abort " req => { "method": "initialize", "jsonrpc": "2.0", "id": 2, "params": {} } " res => { "id": 1, "result": {}, "jsonrpc": "2.0" } call s:log(req, res) endfunction
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Prabir Shrestha
Seems like bug in my code. Ids are generated correctly but would be good to document that id in request will be added.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Yegappan Lakshmanan
I'm able to download the latest version of vim with lsp support on Windows. Thanks for the quick fix. ArchLinux vim also seems to be updated to include this patch. Looking forward to seeing plugin authors use this feature.
I have started adding support for native lsp client in vim-lsp and I'm very happy with the simplicity and ease of use of the api.
I have just got basic request/response working so this feedback is a bit limited while playing with the api for several mins.
- LSP support $/cancelRequest which requires sending the request id. This is heavily used by vim-lsp in order to cancel previous requests such as completion results that could had changed, highlighting current keyword that could had changed due to the cursor movements. Doc explicitly says
Note that in the request message the 'id' field should not be specified. Is it possible to have an api to either set the id or get the id of the request?
- Is it possible to document what
respis in the example doc.let resp = ch_evalexpr(ch, req, #{timeout: 100}). I also see this being used in the test and it checks for empty object. I thought this was the request id but always seem to be empty during my limited test. Probably this only makes sense for synchronous request? It would still be good to document that it will always be empty for async as it created a bit confusion to me.
- Minor: Should the doc include the full raw request?
{'jsonrpc': '2.0', 'id': 1, 'result': { ...}}. I see this as useful doc when writing remote LSP/Json RPC plugins.
- It took be a bit to figure out the callback args for
ch_evalexprbut seems like this is already documented at the end where it says to refer to |channel-callback|. One thing that wasn't clear is if there is a JSON RPC Error Message would it still call the callback function? Clarifying that caller needs to handle the message whether it is success or error rpc messages would be better here. In the future I can see havingon_successandon_errorcallbacks. I don't see me using this in vim-lsp but do see it using in other plugins. https://www.jsonrpc.org/specification#response_object
- Can the sync and async method be used on the same channel?
- Because vim doesn't support async BufWritePre we recommend users to use the following code to auto format on save. While 99% of the code will be using async function there will be sync methods for writing.
let g:lsp_format_sync_timeout = 1000 autocmd! BufWritePre *.rs,*.go call execute('LspDocumentFormatSync')
- Is it possible to document the behavior of how to handle timeouts? When using
let resp = ch_evalexpr(ch, req, #{timeout: 100})what error is thrown and what error should I handle?
vim-dev ML
On Thu, Apr 14, 2022 at 10:34 PM Prabir Shrestha ***@***.***>
wrote:
> I'm able to download the latest version of vim with lsp support on
> Windows. Thanks for the quick fix. ArchLinux vim also seems to be updated
> to include this patch. Looking forward to seeing plugin authors use this
> feature.
>
> I have started adding support for native lsp client in vim-lsp
> simplicity and ease of use of the api.
>
Thanks for the feedback.
> I have just got basic request/response working so this feedback is a bit
> limited while playing with the api for several mins.
>
> <https://microsoft.github.io/language-server-protocol/specifications/specification-current/#cancelRequest>
> which requires sending the request id. This is heavily used by vim-lsp in
> order to cancel previous requests such as completion results that could had
> changed, highlighting current keyword that could had changed due to the
> cursor movements. Doc explicitly says Note that in the request message
> the 'id' field should not be specified. Is it possible to have an api
> to either set the id or get the id of the request?
>
>
Are you using the ch_evalexpr() function or the ch_sendexpr() function to
send the original request?
If you are using the ch_sendexpr() function, then you can specify an
identifier in the message. Vim will
not modify the identifier. If you are using the ch_evalexpr() function,
then Vim will return only when the
response message is received or the request times out. So you cannot cancel
a request sent
using the ch_evalexpr() function.
>
> resp = ch_evalexpr(ch, req, #{timeout: 100}). I also see this being
> used in the test and it checks for empty object. I thought this was the
> request id but always seem to be empty during my limited test. Probably
> this only makes sense for synchronous request? It would still be good to
> document that it will always be empty for async as it created a bit
> confusion to me.
>
> The ch_evalexpr() function returns the decoded response message returned
by the server
for the request message. In the test_channel.vim file, you will see a few
examples for
this (e.g.around line 2540). The ch_sendexpr() function always returns an
empty string.
I will update the help text to describe this.
> '2.0', 'id': 1, 'result': { ...}}. I see this as useful doc when
> writing remote LSP/Json RPC plugins.
>
>
Yes.
>
> but seems like this is already documented at the end where it says to refer
> to |channel-callback|. One thing that wasn't clear is if there is a JSON
> RPC Error Message would it still call the callback function? Clarifying
> that caller needs to handle the message whether it is success or error rpc
> messages would be better here. In the future I can see having
> on_success and on_error callbacks. I don't see me using this in
> vim-lsp but do see it using in other plugins.
> https://www.jsonrpc.org/specification#response_object
>
>
The callback function will be called for a JSON RPC error also. So it will
be called for both success and error.
I will update the doc to clarify this.
>
>
>
Yes. You can use the ch_evalexpr(), ch_sendexpr() and ch_sendraw()
functions on the same
channel.
>
> use the following code to auto format on save. While 99% of the code will
> be using async function there will be sync methods for writing.
>
>
>
> 1. Is it possible to document the behavior of how to handle timeouts?
> When using let resp = ch_evalexpr(ch, req, #{timeout: 100}) what error
> is thrown and what error should I handle?
>
>
> In the case of a timeout, an empty string will be returned. There won't be
any error messages.
I will update the doc.
Thanks,
Yegappan
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
One correction below.
On Fri, Apr 15, 2022 at 8:12 AM Yegappan Lakshmanan <yega...@gmail.com> wrote:
>
> Hi,
>
> On Thu, Apr 14, 2022 at 10:34 PM Prabir Shrestha <vim-dev...@256bit.org> wrote:
>>
>> I'm able to download the latest version of vim with lsp support on Windows. Thanks for the quick fix. ArchLinux vim also seems to be updated to include this patch. Looking forward to seeing plugin authors use this feature.
>>
>> I have started adding support for native lsp client in vim-lsp and I'm very happy with the simplicity and ease of use of the api.
>
>
> Thanks for the feedback.
>
>>
>> I have just got basic request/response working so this feedback is a bit limited while playing with the api for several mins.
>>
>> LSP support $/cancelRequest which requires sending the request id. This is heavily used by vim-lsp in order to cancel previous requests such as completion results that could had changed, highlighting current keyword that could had changed due to the cursor movements. Doc explicitly says Note that in the request message the 'id' field should not be specified. Is it possible to have an api to either set the id or get the id of the request?
>
>
> Are you using the ch_evalexpr() function or the ch_sendexpr() function to send the original request?
> If you are using the ch_sendexpr() function, then you can specify an identifier in the message. Vim will
> not modify the identifier. If you are using the ch_evalexpr() function, then Vim will return only when the
>
then Vim will use an
internally generated 'id' value and will overwrite the caller supplied
'id'. Currently there is
no support for getting the 'id' value used by the ch_sendexpr()
function. I think we can
return the 'id' value used if a callback function is specified.
- Yegappan
vim-dev ML
One correction below.
On Fri, Apr 15, 2022 at 8:12 AM Yegappan Lakshmanan ***@***.***> wrote:
>
> Hi,
>
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
I'm able to download the latest version of vim with lsp support on Windows. Thanks for the quick fix. ArchLinux vim also seems to be updated to include this patch. Looking forward to seeing plugin authors use this feature.
I have started adding support for native lsp client in vim-lsp and I'm very happy with the simplicity and ease of use of the api.
I have just got basic request/response working so this feedback is a bit limited while playing with the api for several mins.
- LSP support $/cancelRequest which requires sending the request id. This is heavily used by vim-lsp in order to cancel previous requests such as completion results that could had changed, highlighting current keyword that could had changed due to the cursor movements. Doc explicitly says
Note that in the request message the 'id' field should not be specified. Is it possible to have an api to either set the id or get the id of the request?
vim-dev ML
On Thu, Apr 14, 2022 at 10:34 PM Prabir Shrestha ***@***.***>
wrote:
> I'm able to download the latest version of vim with lsp support on
> Windows. Thanks for the quick fix. ArchLinux vim also seems to be updated
> to include this patch. Looking forward to seeing plugin authors use this
> feature.
>
> I have started adding support for native lsp client in vim-lsp
> simplicity and ease of use of the api.
>
> I have just got basic request/response working so this feedback is a bit
> limited while playing with the api for several mins.
>
> <https://microsoft.github.io/language-server-protocol/specifications/specification-current/#cancelRequest>
> order to cancel previous requests such as completion results that could had
> changed, highlighting current keyword that could had changed due to the
> cursor movements. Doc explicitly says Note that in the request message
> the 'id' field should not be specified. Is it possible to have an api
> to either set the id or get the id of the request?
>
>
Patch 8.2.4758 (
https://github.com/vim/vim/commit/3b470ae88f034d3741832ab1cc51a5bb8edaf4c6)
added
the support for returning the message ID for a request message sent using
ch_sendexpr(). The help text
is also updated to describe how to use these functions. Let me know if you
have any additional comments.
Regards,
Yegappan
>
> resp = ch_evalexpr(ch, req, #{timeout: 100}). I also see this being
> used in the test and it checks for empty object. I thought this was the
> request id but always seem to be empty during my limited test. Probably
> this only makes sense for synchronous request? It would still be good to
> document that it will always be empty for async as it created a bit
> confusion to me.
> '2.0', 'id': 1, 'result': { ...}}. I see this as useful doc when
> writing remote LSP/Json RPC plugins.
> but seems like this is already documented at the end where it says to refer
> to |channel-callback|. One thing that wasn't clear is if there is a JSON
> RPC Error Message would it still call the callback function? Clarifying
> that caller needs to handle the message whether it is success or error rpc
> messages would be better here. In the future I can see having
> on_success and on_error callbacks. I don't see me using this in
> vim-lsp but do see it using in other plugins.
> https://www.jsonrpc.org/specification#response_object
> vim doesn't support async BufWritePre we recommend users to use the
> following code to auto format on save. While 99% of the code will be using
> async function there will be sync methods for writing.
>
>
>
> 1. Is it possible to document the behavior of how to handle timeouts?
> When using let resp = ch_evalexpr(ch, req, #{timeout: 100}) what error
> is thrown and what error should I handle?
>
>
>
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Prabir Shrestha
Thanks for the quick updates on docs.
I'm working on a native-lsp-client branch in vim-lsp but not able to get completion items working. There could be a potential parsing bug.
Here is the link to the full channel log: https://gist.github.com/prabirshrestha/74d69fe9df0a85126727e99d09ddecbb
I see a bunch of decoding failed in the messages.
25.979927 ERR on 0: Decoding failed - discarding input
26.147193 : looking for messages on channels
26.147276 on 0: Incomplete message (126 bytes) - wait 100 msec for more
26.147304 : SafeState: back to waiting, triggering SafeStateAgain
26.147323 : looking for messages on channels
26.147340 on 0: still waiting on incomplete message
26.147359 : SafeState: back to waiting, triggering SafeStateAgain
26.147378 on 0: still waiting on incomplete message
26.147396 on 0: still waiting on incomplete message
27.438976 : raw key input: "�[O"
27.439098 : looking for messages on channels
27.439125 on 0: timed out
27.439140 ERR on 0: Decoding failed - discarding input
The language server is returning the completion items, but since the parsing fails I can never get the message.
The repository I'm trying is a simple hello world rust program created via cargo new helloworld and try to type and see the completion items.
I also see timed out messages, Do we need to configure some timeout options here?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Yegappan Lakshmanan
Thanks for the quick updates on docs.
I'm working on a native-lsp-client branch in vim-lsp but not able to get completion items working. There could be a potential parsing bug.
Here is the link to the full channel log: https://gist.github.com/prabirshrestha/74d69fe9df0a85126727e99d09ddecbb
vim-dev ML
On Sat, Apr 16, 2022 at 2:10 PM Prabir Shrestha ***@***.***>
wrote:
> Thanks for the quick updates on docs.
>
> I'm working on a native-lsp-client branch
> vim-lsp but not able to get completion items working. There could be a
> potential parsing bug.
>
> Here is the link to the full channel log:
> https://gist.github.com/prabirshrestha/74d69fe9df0a85126727e99d09ddecbb
>
>
>
Thanks for reporting the issue. It looks like if the HTTP header is
received separately
from the JSON payload, then there is a problem in processing the response
properly.
I am able to reproduce this issue with the test program. I need to debug
this more.
Regards,
Yegappan
> The language server is returning the completion items, but since the
> parsing fails I can never get the message.
>
> The repository I'm trying is a simple hello world rust program created via cargo
> new helloworld and try to type and see the completion items.
>
> I also see timed out messages, Do we need to configure some timeout
> options here?
>
> In one of the logs I see partial messages.
> https://gist.github.com/prabirshrestha/74d69fe9df0a85126727e99d09ddecbb#file-channel-log-L252
>
>
>
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Prabir Shrestha
Glad that you have a repro. Do watch out for perf issues though this can be fixed along the way in the near future while the parsing bug is fixed. In vim-lsp we have a pretty bad perf due to parsing where we concat strings and this causes lot of memory allocation.
This threads might be relevant:
You might also be interested in how vscode lang server implements it. They use a MessageBuffer and MessageReader and directly reading as bytes with allocNative.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Yegappan Lakshmanan
Glad that you have a repro. Do watch out for perf issues though this can be fixed along the way in the near future while the parsing bug is fixed. In vim-lsp we have a pretty bad perf due to parsing where we concat strings and this causes lot of memory allocation.
This threads might be relevant:
You might also be interested in how vscode lang server implements it. They use a MessageBuffer and MessageReader and directly reading as bytes with
allocNative.
Thanks,
vim-dev ML
On Sat, Apr 16, 2022 at 11:06 PM Prabir Shrestha ***@***.***>
wrote:
> Glad that you have a repro. Do watch out for perf issues though this can
> be fixed along the way in the near future while the parsing bug is fixed.
> In vim-lsp we have a pretty bad perf due to parsing where we concat strings
> and this causes lot of memory allocation.
>
> This threads might be relevant:
>
> <https://github.com/prabirshrestha/vim-lsp/issues/1073>
> - ***@***.***
> <https://github.com/hrsh7th/vim-vital-vs/commit/6af4d3483e5e33d881f9d6da8c766d4bc9b43bf8>
>
> You might also be interested in how vscode lang server implements it. They
> use a MessageBuffer
> and MessageReader
> <https://github.com/microsoft/vscode-languageserver-node/blob/main/jsonrpc/src/common/messageReader.ts>
> and directly reading as bytes with allocNative.
>
>
>
I have created PR https://github.com/vim/vim/pull/10215 with a fix for
this. Can you try
it out and let me know if it works with your tests?
Thanks,
Yegappan
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Prabir Shrestha
I have tried the PR and it correctly parses the LSP message. Thanks for the quick fix!
Found another bug. Doc says that channel callback will return decoded json rpc message as vim dict. https://github.com/yegappan/vim/blob/aea71ca1f496c89dbfd127376368bb8c7c693782/runtime/doc/channel.txt#L1415-L1422
But from my experience seems like it may return a string if the server writes to stderr. I did a quickfix in vim-lsp to always check the type to be dict. prabirshrestha/vim-lsp@71efb2d. I think the better option would be to never call the callback for stderr and ask the caller to attach out_err instead.
Here is when a callback response is string.
Sun Apr 17 14:58:54 2022:["s:native_notification_callback","2022/04/17 14:58:54 efm-langserver: reading on stdin, writing on stdout"]
Here is when callback response is a dict
Sun Apr 17 14:59:25 2022:["s:native_notification_callback",{"id":0,"jsonrpc":"2.0","method":"window/workDoneProgress/create","params":{"token":"rustAnalyzer/Fetching"}}]
let l:jobopt = { 'in_mode': 'lsp', 'out_mode': 'lsp', 'noblock': 1, 'callback': function('s:native_notification_callback') } let l:job = job_start(a:opts['cmd'], l:jobopt) function! s:native_notification_callback(channel, response) abort call s:log('s:s:native_notification_callback', a:response) if type(a:response) == type({}) " a:response is json decoded lsp message else " a:respone is string from stderr endif endfunction
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Yegappan Lakshmanan
I have tried the PR and it correctly parses the LSP message. Thanks for the quick fix!
Found another bug. Doc says that
channel callbackwill return decoded json rpc message as vim dict. https://github.com/yegappan/vim/blob/aea71ca1f496c89dbfd127376368bb8c7c693782/runtime/doc/channel.txt#L1415-L1422
vim-dev ML
On Sun, Apr 17, 2022 at 3:04 PM Prabir Shrestha ***@***.***>
wrote:
> I have tried the PR and it correctly parses the LSP message. Thanks for
> the quick fix!
>
Thanks for verifying the fix.
> Found another bug. Doc says that channel callback will return decoded
> json rpc message as vim dict.
> https://github.com/yegappan/vim/blob/aea71ca1f496c89dbfd127376368bb8c7c693782/runtime/doc/channel.txt#L1415-L1422
>
We should call this a doc bug. Depending on the mode specified for stderr,
the callback
function can be called with either a string or a dict. To handle LSP
messages on stdout and
strings on stderr, it is better to specify separate callback functions for
"out_cb" and
"err_cb" (as you have described below). I will update the help to mention
this.
Regards,
Yegappan
> But from my experience seems like it may return a string if the server
> writes to stderr. I did a quickfix in vim-lsp to always check the type to
> <https://github.com/prabirshrestha/vim-lsp/commit/71efb2d265369cae0a7e3c492457289359a542db>.
> I think the better option would be to never call the callback for stderr
> and ask the caller to attach out_err instead.
>
> Here is when a callback response is string.
>
> Sun Apr 17 14:58:54 2022:["s:native_notification_callback","2022/04/17 14:58:54 efm-langserver: reading on stdin, writing on stdout"]
>
> Here is when callback response is a dict
>
> Sun Apr 17 14:59:25 2022:["s:native_notification_callback",{"id":0,"jsonrpc":"2.0","method":"window/workDoneProgress/create","params":{"token":"rustAnalyzer/Fetching"}}]
>
> let l:jobopt = { 'in_mode': 'lsp', 'out_mode': 'lsp', 'noblock': 1, 'callback': function('s:native_notification_callback') }let l:job = job_start(a:opts['cmd'], l:jobopt)
> function! s:native_notification_callback(channel, response) abort
> call s:log('s:s:native_notification_callback', a:response)
> if type(a:response) == type({}) " a:response is json decoded lsp message
> else " a:respone is string from stderr
> endifendfunction
>
>
>
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Prabir Shrestha
I'm unblocked now that I can use out_cb and err_cb. Makes sense to clarify this in the doc.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Yegappan Lakshmanan
I have tried the PR and it correctly parses the LSP message. Thanks for the quick fix!
vim-dev ML
On Sun, Apr 17, 2022 at 3:04 PM Prabir Shrestha ***@***.***>
wrote:
> I have tried the PR and it correctly parses the LSP message. Thanks for
> the quick fix!
>
>
> Do we need any further changes to the LSP APIs or the doc? Are you able
to convert
the vim-lsp plugin (
https://github.com/prabirshrestha/vim-lsp/tree/native-lsp-client) to use
the new APIs?
Thanks,
Yegappan
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Prabir Shrestha
I have been using native-lsp-client in vim-lsp for several days and haven't seen any issues with it. I should be all good. I need to rector code on my side and add support for tcp to be on par with the master branch before I can merge it.
My next would then be to profile and see the native client's perf improvements but this probably few weeks later. I usually use the typescript emiter.ts file for perf benchmarks on lsp as it is ~6k of real code in case you want to ahead and try it out.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Bram Moolenaar
> Add basic support for processing language server protocol messages.
> The protocol is described in:
> https://microsoft.github.io/language-server-protocol/specification.
>
> The existing support in Vim for processing JSON encoded messages
> doesn't work for language server protocol. The LSP messages start
> with a HTTP header followed by a JSON encoded dictionary. This format
> is currently not supported by the Vim channel "json" mode.
>
> This PR adds a new "lsp" channel mode to process the LSP header. A LSP client
> can use ch_sendexpr() to send a Vim expression to the LSP server and
> received messages will be decoded and the callback function will be invoked
> with a Vim type.
>
> I have tested this with the Vim9 LSP client and it works. But this needs more
> extensive testing.
>
> The RPC semantic is not yet supported.
Should be OK to support this, it should make handling the messages
faster (did you measure how much?).
A detail: The header is not a "HTTP header", that would suggest many
HTTP fields would be supported. Better is "HTTP semantic". It
basically means a field name, colon and value terminated by CR NL.
I suggest that in channel_skip_lsp_http_hdr() a loop is used to skip
lines until an empty one is found. I assume a future version may add
another header field, we should ignore it, so that and older Vim works
with a newer LSP server.
--
Q: How many hardware engineers does it take to change a lightbulb?
A: None. We'll fix it in software.
/// 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 commented.