fix(lsp): more info in error msg, deduplicate test #39359

2 files changed, 61 insertions, 91 deletions

diff --git a/runtime/lua/vim/lsp/rpc.lua b/runtime/lua/vim/lsp/rpc.lua
index 234f895f2b..7d85be89c9 100644
--- a/runtime/lua/vim/lsp/rpc.lua
+++ b/runtime/lua/vim/lsp/rpc.lua
@@ -454,8 +454,16 @@ function Client:handle_body(body)
 
   -- Received a request.
   if type(decoded.method) == 'string' and decoded.id and decoded.id ~= vim.NIL then
-    local id_type = type(decoded.id)
-    assert(id_type == 'number' or id_type == 'string', 'Request id must be a number or a string')
+    if type(decoded.id) ~= 'number' and type(decoded.id) ~= 'string' then
+      log.error(
+        'Server request id must be a number or string, got ' .. type(decoded.id),
+        decoded.method,
+        decoded.id
+      )
+      self:on_error(M.client_errors.INVALID_SERVER_MESSAGE, decoded)
+      return
+    end
+
     -- Schedule here so that the users functions don't trigger an error and
     -- we can still use the result.
     vim.schedule(coroutine.wrap(function()
@@ -553,7 +561,7 @@ function Client:handle_body(body)
       log.error('No callback found for server response id ' .. result_id)
     end
   elseif decoded.id == vim.NIL then
-    log.warn('Server sent response with null id', decoded.error)
+    log.warn('Server sent response with null id', decoded.method, decoded.error)
     self:on_error(M.client_errors.INVALID_SERVER_MESSAGE, decoded)
   elseif type(decoded.method) == 'string' then
     -- Received a notification.
diff --git a/test/functional/plugin/lsp_spec.lua b/test/functional/plugin/lsp_spec.lua
index cd6629b378..adb679e393 100644
--- a/test/functional/plugin/lsp_spec.lua
+++ b/test/functional/plugin/lsp_spec.lua
@@ -3041,8 +3041,15 @@ describe('LSP', function()
       eq(expected, result)
     end)
 
-    it('does not crash on error response with null id (JSON-RPC 2.0 parse error)', function()
-      local result = exec_lua(function()
+    --- Starts a TCP server that completes initialization, then sends `null_id_payload` after the
+    --- "initialized" notification. If `notification_method` is given, registers a handler
+    --- that tracks whether it was dispatched as a notification.
+    ---
+    --- @param null_id_payload string JSON
+    --- @param notification_method? string
+    --- @return { on_error_called: table, notification_received: boolean, messages: boolean }.
+    local function test_null_id_response(null_id_payload, notification_method)
+      return exec_lua(function()
         local server = assert(vim.uv.new_tcp())
         local accepted
         local messages = {}
@@ -3068,12 +3075,12 @@ describe('LSP', function()
                   table.concat({ 'Content-Length: ', tostring(#msg), '\r\n\r\n', msg })
                 )
               elseif payload.method == 'initialized' then
-                -- Then send an error response with null id (parse error per JSON-RPC 2.0 §5)
-                local msg =
-                  '{"jsonrpc":"2.0","error":{"code":-32700,"message":"Parse error"},"id":null}'
-                accepted:write(
-                  table.concat({ 'Content-Length: ', tostring(#msg), '\r\n\r\n', msg })
-                )
+                accepted:write(table.concat({
+                  'Content-Length: ',
+                  tostring(#null_id_payload),
+                  '\r\n\r\n',
+                  null_id_payload,
+                }))
               end
             end
           end, function()
@@ -3088,110 +3095,65 @@ describe('LSP', function()
         end)
         local port = server:getsockname().port
         local on_error_called = false
+        local notification_received = false
+        local handlers = nil
+        if notification_method then
+          handlers = {
+            [notification_method] = function()
+              notification_received = true
+              return {}
+            end,
+          }
+        end
         local client_id = assert(vim.lsp.start({
           name = 'null-id-test',
           cmd = vim.lsp.rpc.connect('127.0.0.1', port),
           on_error = function(_code, _err)
             on_error_called = true
           end,
+          handlers = handlers,
         }))
         vim.lsp.get_client_by_id(client_id)
-        -- Wait for the initialized notification to be sent and the null-id error to be received
         vim.wait(1000, function()
-          return #messages >= 2 and on_error_called
+          return #messages >= 2 and (on_error_called or notification_received)
         end)
         if accepted and not accepted:is_closing() then
           accepted:close()
         end
-        server:close()
         server:shutdown()
+        server:close()
         return {
           messages = messages,
           on_error_called = on_error_called,
+          notification_received = notification_received,
         }
       end)
-      -- The key assertion: Neovim should not crash, and the error handler should be called
+    end
+
+    it('null-id in response (JSON-RPC 2.0 parse error) is handled, emits error', function()
+      local result = test_null_id_response(
+        '{"jsonrpc":"2.0","error":{"code":-32700,"message":"Parse error"},"id":null}'
+      )
       eq(true, result.on_error_called)
       eq(true, #result.messages >= 2)
     end)
 
-    it('does not misclassify server request with null id as notification', function()
-      local result = exec_lua(function()
-        local server = assert(vim.uv.new_tcp())
-        local accepted
-        local messages = {}
-        server:bind('127.0.0.1', 0)
-        server:listen(127, function(err)
-          assert(not err, err)
-          accepted = assert(vim.uv.new_tcp())
-          server:accept(accepted)
-          accepted:read_start(require('vim.lsp.rpc').create_read_loop(function(body)
-            local payload = vim.json.decode(body)
-            if payload.method then
-              table.insert(messages, payload.method)
-              if payload.method == 'initialize' then
-                local msg = vim.json.encode({
-                  id = payload.id,
-                  jsonrpc = '2.0',
-                  result = {
-                    capabilities = {},
-                  },
-                })
-                accepted:write(
-                  table.concat({ 'Content-Length: ', tostring(#msg), '\r\n\r\n', msg })
-                )
-              elseif payload.method == 'initialized' then
-                -- Send a server request with null id (invalid per JSON-RPC 2.0)
-                local msg =
-                  '{"jsonrpc":"2.0","method":"workspace/configuration","params":{"items":[]},"id":null}'
-                accepted:write(
-                  table.concat({ 'Content-Length: ', tostring(#msg), '\r\n\r\n', msg })
-                )
-              end
-            end
-          end, function()
-            if accepted and not accepted:is_closing() then
-              accepted:close()
-            end
-          end, function()
-            if accepted and not accepted:is_closing() then
-              accepted:close()
-            end
-          end))
-        end)
-        local port = server:getsockname().port
-        local on_error_called = false
-        local notification_received = false
-        local client_id = assert(vim.lsp.start({
-          name = 'null-id-request-test',
-          cmd = vim.lsp.rpc.connect('127.0.0.1', port),
-          on_error = function(_code, _err)
-            on_error_called = true
-          end,
-          handlers = {
-            ['workspace/configuration'] = function()
-              notification_received = true
-              return {}
-            end,
-          },
-        }))
-        vim.lsp.get_client_by_id(client_id)
-        vim.wait(1000, function()
-          return #messages >= 2 and (on_error_called or notification_received)
-        end)
-        if accepted and not accepted:is_closing() then
-          accepted:close()
-        end
-        server:close()
-        server:shutdown()
-        return {
-          messages = messages,
-          on_error_called = on_error_called,
-          notification_received = notification_received,
-        }
-      end)
-      -- Should be dispatched as an error, NOT silently handled as a notification
+    it('null-id in response does not misclassify as a notification', function()
+      -- Sanity check: a real notification (no id) dispatches the handler.
+      local valid = test_null_id_response(
+        '{"jsonrpc":"2.0","method":"workspace/configuration","params":{"items":[]}}',
+        'workspace/configuration'
+      )
+      eq(true, valid.notification_received)
+
+      local result = test_null_id_response(
+        -- Error response with null id (parse error per JSON-RPC 2.0 §5)
+        '{"jsonrpc":"2.0","method":"workspace/configuration","params":{"items":[]},"id":null}',
+        'workspace/configuration'
+      )
+      -- Should be dispatched as an error, NOT silently handled as a notification.
       eq(true, result.on_error_called)
+      -- Null id must NOT be dispatched as a notification.
       eq(false, result.notification_received)
     end)
   end)