refactor(lsp): debounce timer per buf and unify with non-debounce (#17016)

Part of the `pending_change` closure in the `changetracking.prepare` was
a bit confusing because it has access to `bufnr` and `uri` but it could
actually contain pending changes batched for multiple buffers.

(We accounted for that by grouping `pending_changes` by a `uri`, but
it's not obvious what's going on)

This commit changes the approach to do everything per buffer to avoid
any ambiguity.

It also brings the debounce/no-debounce a bit closer together: The
only difference is now whether a timer is used or if it is triggered
immediately

runtime/lua/vim/lsp.lua

@@ -312,56 +312,72 @@ do
   --- client_id → state
   ---
   ---   state
+  ---     use_incremental_sync: bool
+  ---     buffers: bufnr -> buffer_state
+  ---
+  ---   buffer_state
   ---     pending_change?: function that the timer starts to trigger didChange
   ---     pending_changes: table (uri -> list of pending changeset tables));
-  --                       Only set if incremental_sync is used
+  ---                      Only set if incremental_sync is used
-  ---     use_incremental_sync: bool
+  ---
-  ---     buffers?: table (bufnr → lines); for incremental sync only
   ---     timer?: uv_timer
+  ---     lines: table
   local state_by_client = {}
 
   ---@private
   function changetracking.init(client, bufnr)
+    local use_incremental_sync = (
+      if_nil(client.config.flags.allow_incremental_sync, true)
+      and client.resolved_capabilities.text_document_did_change == protocol.TextDocumentSyncKind.Incremental
+    )
     local state = state_by_client[client.id]
     if not state then
       state = {
-        pending_changes = {};
+        buffers = {};
-        last_flush = {};
+        debounce = client.config.flags.debounce_text_changes or 150,
-        use_incremental_sync = (
+        use_incremental_sync = use_incremental_sync;
-          if_nil(client.config.flags.allow_incremental_sync, true)
-          and client.resolved_capabilities.text_document_did_change == protocol.TextDocumentSyncKind.Incremental
-        );
       }
       state_by_client[client.id] = state
     end
-    if not state.use_incremental_sync then
+    if not state.buffers[bufnr] then
-      return
+      local buf_state = {}
+      state.buffers[bufnr] = buf_state
+      if use_incremental_sync then
+        buf_state.lines = nvim_buf_get_lines(bufnr, 0, -1, true)
+        buf_state.pending_changes = {}
+      end
     end
-    if not state.buffers then
-      state.buffers = {}
-    end
-    state.buffers[bufnr] = nvim_buf_get_lines(bufnr, 0, -1, true)
   end
 
   ---@private
   function changetracking.reset_buf(client, bufnr)
-    changetracking.flush(client)
+    changetracking.flush(client, bufnr)
     local state = state_by_client[client.id]
-    if state then
+    if state and state.buffers then
-      if state.buffers then
+      local buf_state = state.buffers[bufnr]
-        state.buffers[bufnr] = nil
+      state.buffers[bufnr] = nil
+      if buf_state and buf_state.timer then
+        buf_state.timer:stop()
+        buf_state.timer:close()
+        buf_state.timer = nil
       end
-      state.last_flush = {}
     end
   end
 
   ---@private
   function changetracking.reset(client_id)
     local state = state_by_client[client_id]
-    if state then
+    if not state then
-      state_by_client[client_id] = nil
+      return
-      changetracking._reset_timer(state)
     end
+    for _, buf_state in pairs(state.buffers) do
+      if buf_state.timer then
+        buf_state.timer:stop()
+        buf_state.timer:close()
+        buf_state.timer = nil
+      end
+    end
+    state.buffers = {}
   end
 
   ---@private
@@ -371,35 +387,27 @@ do
   -- debounce can be skipped and otherwise maybe reduced.
   --
   -- This turns the debounce into a kind of client rate limiting
-  local function next_debounce(debounce, state, bufnr)
+  local function next_debounce(debounce, buf_state)
     if debounce == 0 then
       return 0
     end
     local ns_to_ms = 0.000001
-    local last_flush = state.last_flush[bufnr]
+    if not buf_state.last_flush then
-    if not last_flush then
       return debounce
     end
     local now = uv.hrtime()
-    local ms_since_last_flush = (now - last_flush) * ns_to_ms
+    local ms_since_last_flush = (now - buf_state.last_flush) * ns_to_ms
-    local remaining_debounce = debounce - ms_since_last_flush
+    return math.max(debounce - ms_since_last_flush, 0)
-    if remaining_debounce > 0 then
-      return remaining_debounce
-    else
-      state.last_flush[bufnr] = now
-      return 0
-    end
   end
 
   ---@private
   function changetracking.prepare(bufnr, firstline, lastline, new_lastline)
-    local incremental_changes = function(client)
+    local incremental_changes = function(client, buf_state)
-      local cached_buffers = state_by_client[client.id].buffers
       local curr_lines = nvim_buf_get_lines(bufnr, 0, -1, true)
       local line_ending = buf_get_line_ending(bufnr)
       local incremental_change = sync.compute_diff(
-        cached_buffers[bufnr], curr_lines, firstline, lastline, new_lastline, client.offset_encoding or 'utf-16', line_ending)
+        buf_state.lines, curr_lines, firstline, lastline, new_lastline, client.offset_encoding or 'utf-16', line_ending)
-      cached_buffers[bufnr] = curr_lines
+      buf_state.lines = curr_lines
       return incremental_change
     end
     local full_changes = once(function()
@@ -413,79 +421,68 @@ do
         return
       end
       local state = state_by_client[client.id]
-      changetracking._reset_timer(state)
+      local buf_state = state.buffers[bufnr]
-      local debounce = next_debounce(client.config.flags.debounce_text_changes or 150, state, bufnr)
+      changetracking._reset_timer(buf_state)
-      if debounce == 0 then
+      local debounce = next_debounce(state.debounce, buf_state)
-        if state.pending_change then
-          state.pending_change()
-        end
-        local changes = state.use_incremental_sync and incremental_changes(client) or full_changes()
-        client.notify("textDocument/didChange", {
-          textDocument = {
-            uri = uri;
-            version = util.buf_versions[bufnr];
-          };
-          contentChanges = { changes, }
-        })
-        return
-      end
       if state.use_incremental_sync then
         -- This must be done immediately and cannot be delayed
         -- The contents would further change and startline/endline may no longer fit
-        if not state.pending_changes[uri] then
+        table.insert(buf_state.pending_changes, incremental_changes(client, buf_state))
-          state.pending_changes[uri] = {}
-        end
-        table.insert(state.pending_changes[uri], incremental_changes(client))
       end
-      state.pending_change = function()
+      buf_state.pending_change = function()
-        state.pending_change = nil
+        buf_state.pending_change = nil
-        state.last_flush[bufnr] = uv.hrtime()
+        buf_state.last_flush = uv.hrtime()
         if client.is_stopped() or not vim.api.nvim_buf_is_valid(bufnr) then
           return
         end
-        if state.use_incremental_sync then
+        local changes = state.use_incremental_sync and buf_state.pending_changes or { full_changes() }
-          for change_uri, content_changes in pairs(state.pending_changes) do
+        client.notify("textDocument/didChange", {
-            client.notify("textDocument/didChange", {
+          textDocument = {
-              textDocument = {
+            uri = uri,
-                uri = change_uri;
+            version = util.buf_versions[bufnr],
-                version = util.buf_versions[vim.uri_to_bufnr(change_uri)];
+          },
-              };
+          contentChanges = changes,
-              contentChanges = content_changes,
+        })
-            })
+        buf_state.pending_changes = {}
-          end
+      end
-          state.pending_changes = {}
+      if debounce == 0 then
-        else
+        buf_state.pending_change()
-          client.notify("textDocument/didChange", {
+      else
-            textDocument = {
+        local timer = vim.loop.new_timer()
-              uri = uri;
+        buf_state.timer = timer
-              version = util.buf_versions[bufnr];
+        -- Must use schedule_wrap because `full_changes()` calls nvim_buf_get_lines
-            };
+        timer:start(debounce, 0, vim.schedule_wrap(buf_state.pending_change))
-            contentChanges = { full_changes() },
-          })
-        end
       end
-      state.timer = vim.loop.new_timer()
-      -- Must use schedule_wrap because `full_changes()` calls nvim_buf_get_lines
-      state.timer:start(debounce, 0, vim.schedule_wrap(state.pending_change))
     end
   end
 
-  function changetracking._reset_timer(state)
+  function changetracking._reset_timer(buf_state)
-    if state.timer then
+    if buf_state.timer then
-      state.timer:stop()
+      buf_state.timer:stop()
-      state.timer:close()
+      buf_state.timer:close()
-      state.timer = nil
+      buf_state.timer = nil
     end
   end
 
   --- Flushes any outstanding change notification.
   ---@private
-  function changetracking.flush(client)
+  function changetracking.flush(client, bufnr)
     local state = state_by_client[client.id]
-    if state then
+    if not state then
-      changetracking._reset_timer(state)
+      return
-      if state.pending_change then
+    end
-        state.pending_change()
+    if bufnr then
+      local buf_state = state.buffers[bufnr] or {}
+      changetracking._reset_timer(buf_state)
+      if buf_state.pending_change then
+        buf_state.pending_change()
+      end
+    else
+      for _, buf_state in pairs(state.buffers) do
+        changetracking._reset_timer(buf_state)
+        if buf_state.pending_change then
+          buf_state.pending_change()
+        end
       end
     end
   end
@@ -991,7 +988,7 @@ function lsp.start_client(config)
         or error(string.format("not found: %q request handler for client %q.", method, client.name))
     end
     -- Ensure pending didChange notifications are sent so that the server doesn't operate on a stale state
-    changetracking.flush(client)
+    changetracking.flush(client, bufnr)
     bufnr = resolve_bufnr(bufnr)
     local _ = log.debug() and log.debug(log_prefix, "client.request", client_id, method, params, handler, bufnr)
     local success, request_id = rpc.request(method, params, function(err, result)
@@ -1048,14 +1045,16 @@ function lsp.start_client(config)
   ---@private
   --- Sends a notification to an LSP server.
   ---
-  ---@param method (string) LSP method name.
+  ---@param method string LSP method name.
-  ---@param params (optional, table) LSP request params.
+  ---@param params table|nil LSP request params.
-  ---@param bufnr (number) Buffer handle, or 0 for current.
   ---@returns {status} (bool) true if the notification was successful.
   ---If it is false, then it will always be false
   ---(the client has shutdown).
-  function client.notify(...)
+  function client.notify(method, params)
-    return rpc.notify(...)
+    if method ~= 'textDocument/didChange' then
+      changetracking.flush(client)
+    end
+    return rpc.notify(method, params)
   end
 
   ---@private

test/functional/plugin/lsp_spec.lua

@@ -76,7 +76,7 @@ local function fake_lsp_server_setup(test_name, timeout_ms, options)
       end;
       flags = {
         allow_incremental_sync = options.allow_incremental_sync or false;
-        debounce_text_changes = 0;
+        debounce_text_changes = options.debounce_text_changes or 0;
       };
       on_exit = function(...)
         vim.rpcnotify(1, "exit", ...)
@@ -929,7 +929,60 @@ describe('LSP', function()
       local client
       test_rpc_server {
         test_name = "basic_check_buffer_open_and_change_incremental";
-        options = { allow_incremental_sync = true };
+        options = {
+          allow_incremental_sync = true,
+        };
+        on_setup = function()
+          exec_lua [[
+            BUFFER = vim.api.nvim_create_buf(false, true)
+            vim.api.nvim_buf_set_lines(BUFFER, 0, -1, false, {
+              "testing";
+              "123";
+            })
+          ]]
+        end;
+        on_init = function(_client)
+          client = _client
+          local sync_kind = exec_lua("return require'vim.lsp.protocol'.TextDocumentSyncKind.Incremental")
+          eq(sync_kind, client.resolved_capabilities().text_document_did_change)
+          eq(true, client.resolved_capabilities().text_document_open_close)
+          exec_lua [[
+            assert(lsp.buf_attach_client(BUFFER, TEST_RPC_CLIENT_ID))
+          ]]
+        end;
+        on_exit = function(code, signal)
+          eq(0, code, "exit code", fake_lsp_logfile)
+          eq(0, signal, "exit signal", fake_lsp_logfile)
+        end;
+        on_handler = function(err, result, ctx)
+          if ctx.method == 'start' then
+            exec_lua [[
+              vim.api.nvim_buf_set_lines(BUFFER, 1, 2, false, {
+                "123boop";
+              })
+            ]]
+            client.notify('finish')
+          end
+          eq(table.remove(expected_handlers), {err, result, ctx}, "expected handler")
+          if ctx.method == 'finish' then
+            client.stop()
+          end
+        end;
+      }
+    end)
+    it('should check the body and didChange incremental with debounce', function()
+      local expected_handlers = {
+        {NIL, {}, {method="shutdown", client_id=1}};
+        {NIL, {}, {method="finish", client_id=1}};
+        {NIL, {}, {method="start", client_id=1}};
+      }
+      local client
+      test_rpc_server {
+        test_name = "basic_check_buffer_open_and_change_incremental";
+        options = {
+          allow_incremental_sync = true,
+          debounce_text_changes = 5
+        };
         on_setup = function()
           exec_lua [[
             BUFFER = vim.api.nvim_create_buf(false, true)