lsp-capf: add pass-through completion style + support prefix filtering on completion table

[kiennq]

Fix #2970.

[yyoncho]

@kiennq CI is red. I will do manual testing once @minad is done with his review.

[kiennq]

@kiennq CI is red. I will do manual testing once @minad is done with his review.

Fixed PR. @minad can you help taking a look to see if this works for you?

[minad]

It is wrong to call completion-all-completions here. You should either use complete-with-action/all-completions or the method from Eglot.

As I mentioned before completion-all-completions is part of the completion style machinery. It is not part of the backend/completion table API and must not be used here.

[kiennq]

Ah, I got it now. Thanks

[kiennq]

I think this part is puzzling me the most here. In most cases when completion-styles doesn't contain style like basic and emacs..., the prefix will be ignored anyway and you will get the result filtered later.

[minad]

Yes, only a few completion styles make use of the prefix filtering. It is an optimization basically.

But note that even complex completion styles like Orderless could in principle make use of it. The first word could be passed as prefix and the subsequent words could be used for filtering outside the table.

[minad]

Using completion-basic-all-completions won't result in highlighting here since you pass the empty string. This needs a bit more effort. One has to apply the highlight manually.

[kiennq]

Yes, please do apply the highlighting manually from the front end package side.
We only provide the highlighting cue via :company-match extension and let the frontend package do the highlighting itself when needed.
I think that's more robust and much less resource-consuming than have to provide highlighting for all the items.
Some server returns a few thousand items, doing the highlighting for all of them guaranteed in a degrade perf.

[minad]

Could you clarify where the highlighting data comes from? Is the highlighting data in the company-match function provided by the server?

But I agree with the performance issue. In my frontend Vertico and Corfu I am implementing deferred highlighting for the completion styles. I have also proposed upstream to extend completion styles with an ability to defer highlighting which hopefully would obsolete :company-match.

[kiennq]

The :company-match provides a function that you can call on the candidates to get the data where's the highlight should be.

[minad]

Yes of course. Company unnecessarily forces its extensions onto every frontend. I am not fond of this.

[minad]

Checking for an empty probe is an unnecessary optimization here. It may lead to problems later on since the returned list is mutated (assuming the list is cached).

[kiennq]

We don't cache that list, rather we cache the list that's the evaluation of (funcall candidates).
But yes, this optimization might not need.

[minad]

If you cache the list returned by funcall candidates the optimization is even incorrect, since the frontend may mutate the returned list. But maybe I misunderstood you.

[minad]

Why not use complete-with-action? This will be shorter and equivalent to what you have now. I think you had that in the version of this PR after my first review.

[minad]

Furthermore you did not update the null action (try-completion). I would simply go with complete-with-action and be done. Is there a point in the current implementation?

[minad]

(lambda (str pred action) ;; compliant completion table
  (if (eq action 'metadata)
      '(metadata (category . lsp-capf)
                 (cycle-sort-function . identity)
                 (display-sort-function . identity))
    (complete-with-action action (funcall candidates) str pred)))

[kiennq]

There are, you can see that in the comment for each case there.

[kiennq]

I would rather not risk changing all of them to use the complete-with-action, if you think the implementation of some certain part is not working with your case, please state that

[minad]

Okay fair enough. (My reason would be: I don't trust the implementation. So I have to review it point by point. In comparison complete-with-action is already provided and does the right thing. Furthermore why not go with the simpler code, why make things complicated?) EDIT: I still think complete-with-action is the way to go. Please compare with Eglot. See below.

[minad]

Okay, I took a look now at the whole function. I add some annotations.

       (lambda (probe pred action)
         (cond
          ;; OK: metadata
          ((equal action 'metadata)
           `(metadata (category . lsp-capf)
                      (display-sort-function . identity)
                      (cycle-sort-function . identity)))
          ;; OK: boundaries
          ((equal (car-safe action) 'boundaries) nil)
          ;; WRONG: try-completion
          ;; try-completion should return the common prefix of all the candidates.
          ;; Take a look at the emacs21 completion style which makes very direct use of this.
          ;; The basic completion style makes use of this too, but it is a bit more involved.
          ((null action)
           (when-let ((cands (funcall candidates)))
             (if (cl-rest cands) probe (cl-first cands))))
          ;; WRONG: test-completion: not return exact match so that the selection will always be shown
          ;; Your table is specifying that the completion is never a valid completion.
          ;; The argument given here, that the selection should always be shown is dubious.
          ;; The completion table (backend!) should not take such decisions for the frontend.
          ((equal action 'lambda) nil)
          ;; OK: retrieve candidates
          ((equal action t)
           (all-completions probe (funcall candidates) pred))))

After having seen this, I stand by my point that complete-with-action would be the better solution here.

Please consider the eglot implementation, which implements every single action correctly. It handles each case separately (instead of using complete-with-action) since it does not want to use all-completions in order to take the filterText into account. This is okay, but probably unnecessary given the fact that most completion styles will pass the empty string to the backend, effectively disabling the filtering (compare your lsp-passthrough style which also deliberately passes the empty string).

       (lambda (probe pred action)
         (cond
          ((eq action 'metadata) metadata)               ; metadata
          ((eq action 'lambda)                           ; test-completion
           (test-completion probe (funcall proxies)))
          ((eq (car-safe action) 'boundaries) nil)       ; boundaries
          ((null action)                                 ; try-completion
           (try-completion probe (funcall proxies)))
          ((eq action t)                                 ; all-completions
           (cl-remove-if-not
            (lambda (proxy)
              (let* ((item (get-text-property 0 'eglot--lsp-item proxy))
                     (filterText (plist-get item :filterText)))
                (and (or (null pred) (funcall pred proxy))
                     (string-prefix-p
                      probe (or filterText proxy) completion-ignore-case))))
            (funcall proxies)))))

[kiennq]

;; WRONG: try-completion
          ;; try-completion should return the common prefix of all the candidates.
          ;; Take a look at the emacs21 completion style which makes very direct use of this.
          ;; The basic completion style makes use of this too, but it is a bit more involved.

No, the lsp-mode capf is not designed to work with any other completion styles other than the passthrough.
Unless something is broken for other packages because of this, I would just keep this.

          ;; WRONG: test-completion: not return exact match so that the selection will always be shown
          ;; Your table is specifying that the completion is never a valid completion.
          ;; The argument given here, that the selection should always be shown is dubious.
          ;; The completion table (backend!) should not take such decisions for the frontend.

This is intentional, in the past there's case when the test-completion returns an exact match, thus make the post-completion function not invoked. The problem may have been resolved (I haven't test it) but some older Emacs version may still suffer from it. We need to run the post-completion function to cleanup/expand snippets and do various actions specified by the language server.
I would rather make the completion is never a valid completion and force the user to choose from selection (even with only one item) than making completion stop working after doing a valid completion.

[kiennq]

@minad Can you check if this change is working for you (except the highlighting part)?

[minad]

@kiennq

Can you check if this change is working for you (except the highlighting part)?

I didn't find time to test this yet. From what I see, #2970 is fixed. But by inspection, and as I already mentioned, the completion table is still not fully correct.

...but some older Emacs version may still suffer from it...

Why not drop support for very old Emacsen which suffer from these issues? Which versions are affected?