Unknown json-rpc requests should get an error response, not be ignored

[rymdbar]

Information

VIM version: 2:8.2.2434-3+deb11u1 Operating System: Debian

What went wrong

For full description, and instructions on how to reproduce, please see: https://github.com/LuaLS/lua-language-server/issues/2318

While lua-language-server shouldn't need to, it does attempt to call workspace/configuration:

{
  "id":1,
  "jsonrpc":"2.0",
  "method":"workspace/configuration",
  "params":{
    "items":[
      {
        "scopeUri":"file:///tmp/luals",
        "section":"Lua"
      },
      {
        "scopeUri":"file:///tmp/luals",
        "section":"files.associations"
      },
      {
        "scopeUri":"file:///tmp/luals",
        "section":"files.exclude"
      },
      {
        "scopeUri":"file:///tmp/luals",
        "section":"editor.semanticHighlighting.enabled"
      },
      {
        "scopeUri":"file:///tmp/luals",
        "section":"editor.acceptSuggestionOnEnter"
      }
    ]
  }
}

{
  "id":2,
  "jsonrpc":"2.0",
  "method":"workspace/configuration",
  "params":{
    "items":[
      {
        "scopeUri":"file:///tmp/luals",
        "section":"Lua"
      },
      {
        "scopeUri":"file:///tmp/luals",
        "section":"files.associations"
      },
      {
        "scopeUri":"file:///tmp/luals",
        "section":"files.exclude"
      },
      {
        "scopeUri":"file:///tmp/luals",
        "section":"editor.semanticHighlighting.enabled"
      },
      {
        "scopeUri":"file:///tmp/luals",
        "section":"editor.acceptSuggestionOnEnter"
      }
    ]
  }
}

{
  "id":3,
  "jsonrpc":"2.0",
  "method":"workspace/configuration",
  "params":{
    "items":[
      {
        "scopeUri":"file:///tmp/luals",
        "section":"Lua"
      },
      {
        "scopeUri":"file:///tmp/luals",
        "section":"files.associations"
      },
      {
        "scopeUri":"file:///tmp/luals",
        "section":"files.exclude"
      },
      {
        "scopeUri":"file:///tmp/luals",
        "section":"editor.semanticHighlighting.enabled"
      },
      {
        "scopeUri":"file:///tmp/luals",
        "section":"editor.acceptSuggestionOnEnter"
      }
    ]
  }
}

There are no responses from ale. While it is silly to attempt the call to a client lacking the capability, the Language Server Protocol Specification mandates that:

Every processed request must send a response back to the sender of the request.

From context it appears this must is to be considered a MUST as defined by rfc2119.

[rymdbar]

To my understanding, all which should be needed to solve this issue is to detect whenever an unsupported method call is received, and communicating that it'll be denied. I've been reading and stepping through the ale source code without obtaining sufficient understanding of how to do that. There is a METHOD_NOT_FOUND constant, but calling ALEFindReferences on it returns No references found. Not that it means much, as surrounding constants seem to be used for parsing rather than generating messages.

In an attempt to intercept the code path where an error response could be added, I added the following tracing:

diff --git a/autoload/ale/job.vim b/autoload/ale/job.vim
index 0fc43a8..00c4e55 100644
--- a/autoload/ale/job.vim
+++ b/autoload/ale/job.vim
@@ -72,6 +72,8 @@ function! s:VimOutputCallback(channel, data) abort
     " Only call the callbacks for jobs which are valid.
     if l:job_id > 0 && has_key(s:job_map, l:job_id)
         call ale#util#GetFunction(s:job_map[l:job_id].out_cb)(l:job_id, a:data)
+    else
+        call ale#lsp_linter#TraceLSPMessage(a:channel, a:data)
     endif
 endfunction
 
diff --git a/autoload/ale/lsp.vim b/autoload/ale/lsp.vim
index 0519c79..39aa97a 100644
--- a/autoload/ale/lsp.vim
+++ b/autoload/ale/lsp.vim
@@ -340,6 +340,7 @@ function! ale#lsp#HandleInitResponse(conn, response) abort
 endfunction
 
 function! ale#lsp#HandleMessage(conn_id, message) abort
+    call ale#lsp_linter#TraceLSPMessage(a:conn_id, a:message)
     let l:conn = get(s:connections, a:conn_id, {})
 
     if empty(l:conn)
@@ -371,6 +372,7 @@ function! ale#lsp#HandleMessage(conn_id, message) abort
             for l:Callback in l:conn.callback_list
                 call ale#util#GetFunction(l:Callback)(a:conn_id, l:response)
             endfor
+            call ale#lsp_linter#TraceLSPMessage(a:conn_id, l:response)
         endfor
     endif
 endfunction
diff --git a/autoload/ale/lsp_linter.vim b/autoload/ale/lsp_linter.vim
index 05a0294..389f972 100644
--- a/autoload/ale/lsp_linter.vim
+++ b/autoload/ale/lsp_linter.vim
@@ -195,7 +195,12 @@ function! ale#lsp_linter#AddErrorMessage(linter_name, message) abort
     call add(g:ale_lsp_error_messages[a:linter_name], a:message)
 endfunction
 
+function! ale#lsp_linter#TraceLSPMessage(conn_id, response) abort
+    echom 'ale#lsp_linter#TraceLSPMessage' . string(a:response)
+endfunction
+
 function! ale#lsp_linter#HandleLSPResponse(conn_id, response) abort
+    echom 'ale#lsp_linter#HandleLSPResponse' . string(a:response)
     let l:method = get(a:response, 'method', '')
 
     if get(a:response, 'jsonrpc', '') is# '2.0' && has_key(a:response, 'error')

However it shows that only messages received after completed initialization gets a chance to reach those callbacks. If following the call graph from the very start of execution, things related to understanding LSP interaction start to become interesting in ale#lsp_linter#CheckWithLSP. I can't find anything between that point and the trace-points of my patch above which looks obvious in catching the early messages from the language-server, but apparently I'm missing something.

[w0rp]

ALE shouldn't be sent the workspace/configuration requests from the server as we indicate we don't support that in the initialize message we send to the server. (intializationOptions.capabilities.workspace.configuration === false) I don't want to implement workspace/configuration messages because you have to respond with values for every option a server asks for, and it means you have to separately define configurations for every server for them to run at all. The configurations can change over time, so you not only have to implement specific details for each language server, but support different ranges of options for different server versions. I much prefer initializationOptions and workspace/didChangeConfiguration, where the client either tells the server once at start up or updates the configuration over time, and the server can assume default settings for those you don't specify. lua-language-server should be patched not to send the messages if the client doesn't support them.

I'd added an item to https://github.com/dense-analysis/ale/issues/3369 for showing messages received that ALE doesn't handle in :ALEInfo to make it easier to spot issues like these.

[rymdbar]

Interesting to learn about #3369. Having more easily available information about attempted unhandled requests sure would debugging easier.

Indeed the lua-language-server should know better than to call workspace/configuration. Yet it does, and it does three attempts before giving up. I've pushed a fix for this to git.netizen.se/lua-language-server/. Let us hope it gets merged!

Regardless of that, it might be worth reconsidering whether it is a good idea to break protocol and be all silent on valid method calls. Given both the wording of the specification (quoted already) and @sumneko's comment:

So the client could answering empty results (or anything else), but the client should not silent.

Please note that the suggestion is not to implement support for responding with values for every option a server asks for. The title of this issue was intended to lead thoughts to answering valid but unimplemented requests with a MethodNotFound (-32601) ErrorCode.

Maybe this is too low prioritized to actually happen, but I it seems strange to consider it a wontfix.

[w0rp]

Maybe this is too low prioritized to actually happen, but I it seems strange to consider it a wontfix.

No one said it was "wontfix." It's just not a priority. I put this in the class of issues anyone can submit a reasonable pull request for, but I won't be implementing any time soon.

[benknoble]

This just bit me with yaml-language-server: my config wasn't getting communicated to it, and debugging (by writing log files in the config-related functions of the language server, or using vim-lsp and it's log file… side note: I would love a way to get a log of LSP messages with ALE) was behaving strangely.

I finally realized that yaml-language-server (unconditionally?) sends workspace/configuration and that ALE wasn't responding, so the relevant awaits never returned. Responding with an error message might be reasonable thing to do to prevent asynchronous callbacks from hanging around… and note that the spec seems to prefer workspace/configuration as of 3.17:

This pull model replaces the old push model were the client signaled configuration change via an event. If the server still needs to react to configuration changes (since the server caches the result of workspace/configuration requests) the server should register for an empty configuration change using the following registration pattern:

Of course, as suggested, I'll go off and file an issue with yaml-language-server doing the wrong thing.

[w0rp]

I'll note that this issue will remain open and I'd like it to be handled. I just don't have the time to handle it myself. If someone has an elegant pull request, I'll merge it.

[benknoble]

I should note: I filed https://github.com/redhat-developer/yaml-language-server/issues/927 with the YLS folks.

I'm not sure what changed (either in ALE or YLS), but in the last few weeks it seems that YLS is working when I set g:ale_yaml_ls_config now.