Move completions cycling to RPC [DRAFT]

[svilupp]

This fixes #18

This is an initial draft to move the getCompletionsCycling from a buffer call to an RPC

Draft:

• created a mirror of the panelSolution method with RPC call and dedicated create_handler

Gaps:

• misses any Githu Copilot consumption/feeding logic, as I cannot get into some sensible debug mode and am not familiar with how to interact with copilot
• no tests have been added or conducted

Considerations:

• If I got it right, methods.id is a simple way to track the sequence of calls. Do we need to have different ID for panelSolution vs CompletionsCycling? While only one should be ever initiated, what happens if the user switches over the mode during runtime - would that nuke the methods.id counter anyway?
• It was unclear whether we need CyclingCompletionsDone equivalent to PanelCompletionsDone

This is probably a zero value-add! Please feel free to scrap it and do it yourself properly.

[svilupp]

Scratch my previous comments. I re-wrote it to take advantage of the existing methods and infrastructure.

Current status: Copilot seems to work and no Julia LS crashes yet.

Questions:

• do we need the "methods.id" tracker of function calls when not dealing with panels? (ie, use req_params(), incrementing it, etc)... I don't see any benefits and since we're not creating handlers all the time, I think it can be removed?

• What is the need/logic for the final callback(completions) in the method getCompletionsCycling (in addition to being called in the request response_handler / buf_request in the previous case)? Isn't it called unnecessarily even after a successful completion request? (not sure if it matters/collides) I see several failure modes (request-err, request-sent, response empty, response.completion empty), but can we not simply ignore them and just return callback() just at the end of the getCompletionCycling? (empty completions in failure cases) It seems that we don't really care if the request failed, because we don't have to clean up any handlers in this completion mode

[zbirenbaum]

First off, sorry about the delay, I've been pretty busy, but I am really impressed that you were able to write this without having any lua experience :+1: PanelID is only required for getPanelCompletions, as it is a field the lsp checks for. The reason I have the dynamic registration for panel handlers is because unlike getCompletionsCycling, the response to the request for getPanelCompletions is not actually the completions. The real completions come from a notification, and a second notification (PanelCompletionsDone) comes in to let you know that there are no more expected. Cycling doesn't have such a complex system, and the completions are provided directly in the response callback.

That's not something that anyone could really be expected to know without a lot of knowledge of the copilot internals, so don't feel too bad about it. You actually got pretty close, and I'm sure given that info you could figure it out, but I figured I would help a bit.

Here's the full body for a functioning rpc version of the function:

methods.getCompletionsCycling = function (self, params, callback)
  local request = self.client.rpc.request
  local bufnr = params.context.bufnr
  local row = params.context.cursor.row

  methods.existing_matches[bufnr] = methods.existing_matches[bufnr] or {}
  methods.existing_matches[bufnr][row] = methods.existing_matches[bufnr][row] or {}

  local respond_callback = function(err, response)
    if err then return err end
    if not response or vim.tbl_isempty(response.completions) then return end --j
    methods.existing_matches[bufnr][row] = add_results(response.completions, params)
    local existing_matches = methods.existing_matches[bufnr][row]
    local completions = formatter.format_completions(vim.tbl_values(existing_matches or {}), params)
    callback(completions)
  end

  request("getCompletionsCycling", util.get_completion_params(), respond_callback)
  local completions = formatter.format_completions(vim.tbl_values(methods.existing_matches[bufnr][row] or {}), params)
  callback(completions)
end

You can open a new PR or do a hard reset and just paste that in over the current method, or you can manually remove the changes from outside that function and replace it. Since you put a good amount of work into this I'd like to give some credit

[svilupp]

Thank you. I've made the change.

Out of curiosity, what is the need for having the callback() inside of respond_callback() function? Isn't it called twice upon a successful request (once in the respond_callback() and once at the end of the method? methods.existing_matches seems non-local within respond_callback() so it would hold the results of completions in both of those callback calls, wouldn't it?

[zbirenbaum]

Thank you. I've made the change.

Out of curiosity, what is the need for having the callback() inside of respond_callback() function? Isn't it called twice upon a successful request (once in the respond_callback() and once at the end of the method? methods.existing_matches seems non-local within respond_callback() so it would hold the results of completions in both of those callback calls, wouldn't it?

Looks good!

And good question, callback is a function passed by nvim-cmp to the source's completion function, and takes a table with IsIncomplete and a table of the completions as the argument. respond_callback is the function triggered when a response is received from the LSP for the request submitted. Response callback receives the completion, which is then passed to cmp.

cmp will wait until it receives a response from the source (via callback), so I return the cached completions immediately to avoid freezing cmp (callback at the end), and then provide cmp the updated completions once they are received from copilot (callback inside response_callback).