feat(lsp)!: change handler signature

[mjlbach]

Previously, the handler signature was:

function(err, method, params, client_id, bufnr, config)

In order to better support external plugins that wish to extend the
protocol, there is other information which would be advantageous to
forward to the client, such as the original params of the request that
generated the callback.

In order to do this, we would need to break symmetry of the handlers, to
add an additional "params" as the 7th argument.

Instead, this PR changes the signature of the handlers to:

function(err, result, context, config)

where context includes params, client_id, and bufnr. This also leaves
flexibility for future use-cases.

Todos:

• fix tests
• make sure I didn't break anything
• update docs
• warn plugin authors
• Decide what to do about compatibility question

[jose-elias-alvarez]

Looks great! Do you think it would make sense to handle #15174 here, too, since the PR modifies the interaction between buf_request and buf_request_all? As far as I can tell, the implementation here will run into the same nil index issue, since context.client_id will be nil if no client supports a given method. (My PR to fix that issue seems to have stalled and is incompatible with the changes here, anyways.)

[mjlbach]

Looks great! Do you think it would make sense to handle #15174 here, too, since the PR modifies the interaction between buf_request and buf_request_all? As far as I can tell, the implementation here will run into the same nil index issue, since context.client_id will be nil if no client supports a given method. (My PR to fix that issue seems to have stalled and is incompatible with the changes here, anyways.)

I think that is out of scope of this PR, but I will rebase and review your PR after this.

[mjlbach]

A couple notes, I changed result to params in a few places for "consistency" but maybe I should not have done this, as params is codified in the spec. LMK what you think.

[mjlbach]

@ray-x @jose-elias-alvarez @hrsh7th @glepnir @akinsho @simrat39 this is a warning that (although we will not be merging this yet) it will break your plugins that rely on overriding handlers (also lmk if there were any obvious plugin authors I missed)

[ms-jpq]

this breaks existing buf_request handlers right?

[mjlbach]

@ms-jpq I knew there was someone I forgot :)

The function signature for buf_request itself is the same, but the handler passed to the handler argument follows the new pattern.

[ms-jpq]

thanks :)

so config is always a table and never nil right? then we can just check how many elements are in the function argument?, and do it that way.

no version detection required

[mjlbach]

@ms-jpq config can be nil, but if nil is set to an empty table within the handler. I'd recommend using the wrapper I added in vim.lsp.compat and vim.fn.has('nvim-0.5.1') to gate which handler paradigm you use, we'll push this change to 0.5.1 so it shouldn't be too disruptive (hopefully...)

[ms-jpq]

haha yeah, but it would break all the things for people who havn't updated the plugins though.

I think Ill just use bufnr on arg position 5 as a gate, then you are free to do whatever with the version numbers 👍

[ms-jpq]

anyways, thank you for letting us know early 💯

[justinmk]

this looks like something that can be copy-pasted into any project. So we can document it, but we don't need to introduce this compat module into the API and then later deal with removing it (thus duplicating the migration issue we're already trying to deal with now)

[mfussenegger]

I was just thinking how to adapt the handlers in my plugins so that they work with all three: 0.5, master prior this change and master with this change.

And that would require some kind of feature or version check.
With the current PR state I'd probably create something like this:

function M.mk_handler(fn)
  if vim.lsp.compat then
    return vim.lsp.compat.wrap_05_handler(fn)
  end
  return fn
end

Implementing the wrap_05_handler isn't the issue, but some kind of marker that can be checked at runtime to see which signature format is used would be good. nvim-0.6 won't be granular enough, as it would also return true for versions after 0.5, but before this change.

[justinmk]

nvim-0.6 won't be granular enough, as it would also return true for versions after 0.5, but before this change.

:echo has('nvim-0.5.1') is granular. Your function would be:

function M.mk_handler(fn)
  if vim.fn.has('nvim-0.5.1') then
    return function(err, result, context, config)
      return fn(err, context.method, result, context.client_id, context.bufnr, config)
    end
  end
  return fn
end

[mfussenegger]

vim.fn.has('nvim-0.5.1') would also return true for the current master (without this change, using the old signature)

I guess it is acceptable that master/plugin combinations are broken for a bit, but if there were some check for patch-level or signature-format level, plugins could make the transition seamless. (At least if they adapt the code before this PR is merged)

[mfussenegger]

Something like the following might work to keep compatiblity with all 3 variants:

function M.mk_handler(fn)
  return function(...)
    local is_new = type(select(4, ...)) ~= 'number'
    if is_new then
      fn(...)
    else
      local err = select(1, ...)
      local method = select(2, ...)
      local result = select(3, ...)
      local client_id = select(4, ...)
      local bufnr = select(5, ...)
      local config = select(6, ...)
      fn(err, result, { method = method, client_id = client_id, bufnr = bufnr }, config)
    end
  end
end

So +1 from my side to remove vim.lsp.compat from this PR

[mfussenegger]

Might be worth logging the context with vim.inspect, to ensure this automatically logs everything if the context gets extended.

[mjlbach]

vim.inspect has a runtime cost though right? I guess it's fine since this is only done if the user is using debug

[mfussenegger]

I'd think the overhead is acceptable given that it is logged on debug level and that is not enabled by default.

[mjlbach]

I think I have addressed all of the comments.

The main remaining to-do is to figure out the compatibility question:

1. Do we include a compat module (originally suggested by TJ) or just document here/in breaking changes issue how to work around this in plugins?
2. Do we do a runtime compatibility check somehow for testing if users are using old argument style (possible runtime cost)
3. How long do we wait after this warning to plugin authors to merge this PR? Certain plugins (lspsaga) are not likely to be updated. I propose 1 week from today.

I think we can release 0.5.1 after this (from an LSP perspective at least), however, we either need to fix, or not backport the URI changes that caused #15261