fix(lsp): send textDocument/didChange for each buffer

[mjlbach]

Closes #16424

[mfussenegger]

Shouldn't this flush instead of throw it away?

[dmitmel]

client.notify doesn't cause a flush

[dmitmel]

That can be fixed too, but I think that explicitly performing a reset here won't hurt

[mfussenegger]

I'm aware that notify doesn't flush. It can't flush as long as the debounce itself calls client.notify to do the debouncing.

My question is whether this here should be calling flush instead of reset_buf.

[dmitmel]

Well, currently calling flush makes more sense because reset_buf currently doesn't delete the entry in pending_changes for the buffer, but that is beside the point. The thing is that we are sending new buffer contents immediately afterwards, and with the pending didChange events meant to be applied to the version of the buffer before reload, they are basically useless because their results will be discarded as soon as the client processes didOpen. Thus I think that it is better to throw them away.

[mjlbach]

There's no point to flushing as we send the full document here https://github.com/mjlbach/neovim/blob/988fd1d825c99e6212f25c10236eda528a89dda1/runtime/lua/vim/lsp.lua#L478-L486

I'll modify reset_buf to flush pending changes (I thought we overwriting state entirely in change_tracking.init)

[mfussenegger]

I think debounce should only debounce - not additionally optimize away change sets, even if they're most likely unnecessary. I'm pretty sure the optimization wouldn't have any practical impact.

And I think keeping the semantics of debounce on/off close to each other helps reasoning about potential issues or with trouble shooting. Both should result in the same set of changes getting eventually reported to the server (although in a different format due the batching)

[yorickpeterse]

I'm giving the changes in this PR a spin. So far they seem to solve the issues outlined in #16424 nicely.

[dmitmel]

Will this not result in the reported issue if incremental sync is disabled? Doesn't the if above which adds to pending_changes be rewritten as

      if state.use_incremental_sync then
        if not state.pending_changes[uri] then
          state.pending_changes[uri] = {}
        end
        table.insert(state.pending_changes[uri], incremental_changes(client))
      else
        state.pending_changes[uri] = true
      end

or something like that, and this one to

        -- drop the outer `if state.use_incremental_sync`
        for change_uri, content_changes in pairs(state.pending_changes) do
          if state.use_incremental_sync then
            content_changes = full_changes()
          end
          client.notify("textDocument/didChange", {
            textDocument = {
              uri = change_uri;
              version = util.buf_versions[vim.uri_to_bufnr(change_uri)];
            };
            contentChanges = content_changes,
          })
        end
        state.pending_changes = nil

[dmitmel]

@mjlbach Could you please take a look?

[mjlbach]

I don't understand what you are saying above. Mathias took over this PR, but I'm pretty sure it's fine because prepare is called with each buffer, and full changes is computed per buffer (there is no pending changes for non-incremental sync as that does not make sense) and sent as such. It's a little weird how I wrote it because the prepare that is called on a given buffer for inc sync flushes all buffer changes.

[mfussenegger]

If you can provide an example that breaks (or even better, produce a test case) I can have a closer look.

[dmitmel]

Nevermind, I checked the repro posted in the original instance with the exact same settings, but allow_incremental_sync disabled, and, apparently, it doesn't show up.

[mjlbach]

I agree the way I chose to implement it is kinda messy and probably could be cleaned up to make more explicit