Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement simplified capf mode #3825

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Implement simplified capf mode #3825

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
62 changes: 37 additions & 25 deletions lsp-completion.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
"The completion backend provider."
:type '(choice
(const :tag "Use company-capf" :capf)
(const :tag "Use simplified capf" :corfu-capf)
(const :tag "None" :none))
Copy link

@minad minad Nov 24, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this option should be replaced with a new option lsp-completion-frontend which can take these values:

  • default-passthrough: Setup passthrough style
  • default-filtered: Setup flex style (or orderless if available)
  • company: Auto configure Company, like default-passthrough
  • corfu: Auto configure Corfu, like default-filtered

Note that default-filtered is equivalent to the behavior of Eglot.

:group 'lsp-completion
:package-version '(lsp-mode . "7.0.1"))
Expand Down Expand Up @@ -159,10 +160,18 @@ This will help minimize popup flickering issue in `company-mode'."
(cl-defun lsp-completion--make-item (item &key markers prefix)
"Make completion item from lsp ITEM and with MARKERS and PREFIX."
(-let (((&CompletionItem :label
:filter-text?
:sort-text?
:_emacsStartPoint start-point)
item))
(propertize label
;; In :corfu-capf mode, the filtering is done by completion styles
;; which work only with the candidate text. As some LSP servers
;; return candidates with simplified labels ("var" for "$var",
;; "Schema" for "\mysql_xdevapi\Schema"), prefer filter text as
;; label. As completion is filtering, this does makes sense.
(propertize (or (unless (or (equal lsp-completion-provider :corfu-capf)
(lsp-falsy? filter-text?)) filter-text?)
label)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also use (propertize filter-text 'display label) but then we wouldn't get completion style highlighting of the candidates. Therefore this is probably not a good option.

Copy link

@minad minad Nov 24, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took another look at the lsp capf. We should better change this, such that filter-text is used for filtering. Instead of calling (complete-with-action ...) here:

lsp-mode/lsp-completion.el

Lines 513 to 514 in e5a6274

(t
(complete-with-action action (funcall candidates) probe pred))))

We should handle the actions separately (try-completion, all-completions, test-completion), like Eglot does it. Then we can display the label and still use filter-text for filtering.

https://github.com/emacs-mirror/emacs/blob/b0fa3b1a1f31c158131325f0f451c960ec54d938/lisp/progmodes/eglot.el#L2811-L2820

Iirc I've suggested this a while ago, but then we went with the simpler solution of (complete-with-action ...) which unfortunately led to the PHP issue.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, but if we use the Eglot approach we will still get the wrong results with completion styles like flex/substring/orderless due to completion-regexp-list which is respected by all-completions. This means there is also a bug in Eglot here.

The correct approach is this:

  1. Use all-completions against filter-text strings propertized with the label
  2. Map over the result, looking up the label text property

Copy link
Member

@kiennq kiennq Nov 25, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The filter-text is already used for filtering (see lsp-completion--to-internal), however, it's filtered before passing that back to Emacs's completion framework. Thus, skipping Emacs's filtering stuff later with lsp-passthrough is the better choice as we don't want the filtered candidates to be filtered again and lower the performance.

The reason why the completion candidates are filtered before is because the prefix of each candidate can be different and can be completely different from what Emacs expect.
Currently, to conform with the Emacs completion framework, we've reported a phony prefix based on Emacs's major mode expectation of a symbol for a language. However, when we send the completion request to the language server, it has a different interpretation of what's a language symbol and creates the completion candidates based on that.

An example is Java decoration operator or JavaScript's member - see the attached example. The filterText for ["so so"] is .so so and it will not match with prefix match without the correct prefix guessing.
image

Now if we blindly apply the candidate based on Emacs's symbol expectation, that will lead to some bugs (we have bug reports for them already). I would say, this is a quirk of integrating LSP completion with Emacs completion.

To resolve that problem, we need to do the filtering with the prefix that the language server expected, which means doing it before passing back to Emacs filtering, via lsp-completion--filter-candidates.

Now if you want to support all completion styles, what you can do is to override lsp-completion--filter-candidates to call all-completions in place of the built-in lsp's flex filtering.
We have code like that before (pre Emacs 27 with no major completion styles like flex or orderless around) but decide to just go ahead with the built-in lsp flex filtering as it provides less hassle for the user (@yyoncho for confirm or to correct me if I'm wrong somewhere).
Also, displaying the candidate using its label is to stay as faithfully as possible to LSP specs (vscode also does that).
@minad @xendk

As for corfu, I think it would be better to configure the corfu to bust the cache here and call the capf on every keystroke. The lsp-mode also does cache already so it will not call the language server on every keystroke.
Also, depends on the language server, we have to call the language server (for example gopls or clangd) to refresh the candidate list if the server explicitly tells us that the candidates they provided are incomplete.
Busting the cache for corfu will not necessarily lower the perf (or it might even improve the perf as corfu doesn't have to do the caching logic anymore)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for corfu, I think it would be better to configure the corfu to bust the cache here and call the capf on every keystroke.

I'll just grab this first. How does one set this up? I've tried

  (defun my/lsp-mode-setup-completion ()
    (setf (elt (cl-member 'lsp-completion-at-point completion-at-point-functions) 0)
          (cape-capf-buster #'lsp-completion-at-point))
    )

Which as far as I can see wraps lsp-completion-at-point, but doesn't really work:

image

(after the initial popup, no further filtering is done). Typing $ar and invoking completion-at-point just gives "No match".

Copy link
Member

@yyoncho yyoncho Nov 26, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xendk yes. IOW there is no guarantee that the server uses one single point for starting the completion. Note that there might be more than one servers running in the buffer too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@minad

Again, completion style double filtering already works perfectly fine, except for the minor edge case where the server provides a filter-text. This is all we would want to have fixed. What do you think?

FTR initially we were performing the filtering in emacs and we did no filtering on lsp-mode side. Later the bug reports came and we had to move the filtering on lsp-mode side - we decided to work correctly instead of being a good citizen and returning the wrong set of results. If we do second filtering it might cut off the items with miscalculated prefixes. IMHO in order for the whole thing to work the filtering strategy should be passed to the provider instead (not sure that such a thing can be achieved). But that I guess is not in the scope.

less communication with the server

Not sure if we will do better than what we currently have because we already have a cache locally in lsp-mode.

In general, I am fine with putting a less precise implementation of the completion for the sake of being integrated better with emacs tools but the limitations should be well documented.

Copy link

@minad minad Nov 26, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yyoncho Good to know the history of the developments. Likely, I would have made the same design choices as you did.

Not sure if we will do better than what we currently have because we already have a cache locally in lsp-mode.

In fact, we can. If you use Corfu this is already the case. Note that the caching happens on the side of Corfu.

In general, I am fine with putting a less precise implementation of the completion for the sake of being integrated better with emacs tools but the limitations should be well documented.

Okay, I will prepare a PR when I find time. To be 100% clear and to avoid any misunderstandings: The patch I am going to create will preserve the existing filtering if lsp-passthrough is configured. There won't be a regression, no loss in precision. Everything will just behave exactly as before in the default lsp-completion configuration.

The difference will only be observable if you configure a different completion style and if the server attaches filter-text to the candidates.

Also the patch shouldn't add significant complexity to the existing completion function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference will only be observable if you configure a different completion style and if the server attaches filter-text to the candidates.

Any progress? I don't understand enough of this to take a stab at it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@minad Could you spare some time for this one? This PR might halfway work, but your idea seemed better.

'lsp-completion-item item
'lsp-sort-text sort-text?
'lsp-completion-start-point start-point
Expand Down Expand Up @@ -757,40 +766,43 @@ TABLE PRED"
;; Ensure that `lsp-completion-at-point' the first CAPF to be tried,
;; unless user has put it elsewhere in the list by their own
(add-to-list 'completion-at-point-functions #'lsp-completion-at-point)
(make-local-variable 'completion-category-defaults)
(setf (alist-get 'lsp-capf completion-category-defaults) '((styles . (lsp-passthrough))))
(make-local-variable 'completion-styles-alist)
(setf (alist-get 'lsp-passthrough completion-styles-alist)
'(completion-basic-try-completion
lsp-completion-passthrough-all-completions
"Passthrough completion."))

(unless (equal lsp-completion-provider :corfu-capf)
(make-local-variable 'completion-category-defaults)
(setf (alist-get 'lsp-capf completion-category-defaults) '((styles . (lsp-passthrough))))
(make-local-variable 'completion-styles-alist)
(setf (alist-get 'lsp-passthrough completion-styles-alist)
'(completion-basic-try-completion
lsp-completion-passthrough-all-completions
"Passthrough completion.")))

(cond
((equal lsp-completion-provider :none))
((and (not (equal lsp-completion-provider :none))
((or (equal lsp-completion-provider :none)
(equal lsp-completion-provider :corfu-capf)))
((and (equal lsp-completion-provider :capf)
(fboundp 'company-mode))
(setq-local company-abort-on-unique-match nil)
(company-mode 1)
(setq-local company-backends (cl-adjoin 'company-capf company-backends :test #'equal)))
(setq-local company-backends (cl-adjoin 'company-capf company-backends :test #'equal))
(when (bound-and-true-p company-mode)
(add-hook 'company-completion-started-hook
completion-started-fn
nil
t)
(add-hook 'company-after-completion-hook
after-completion-fn
nil
t)))
(t
(lsp--warn "Unable to autoconfigure company-mode.")))

(when (bound-and-true-p company-mode)
(add-hook 'company-completion-started-hook
completion-started-fn
nil
t)
(add-hook 'company-after-completion-hook
after-completion-fn
nil
t))
(add-hook 'lsp-unconfigure-hook #'lsp-completion--disable nil t))
(t
(remove-hook 'completion-at-point-functions #'lsp-completion-at-point t)
(setq-local completion-category-defaults
(cl-remove 'lsp-capf completion-category-defaults :key #'cl-first))
(setq-local completion-styles-alist
(cl-remove 'lsp-passthrough completion-styles-alist :key #'cl-first))
(unless (equal lsp-completion-provider :corfu-capf)
(setq-local completion-category-defaults
(cl-remove 'lsp-capf completion-category-defaults :key #'cl-first))
(setq-local completion-styles-alist
(cl-remove 'lsp-passthrough completion-styles-alist :key #'cl-first)))
(remove-hook 'lsp-unconfigure-hook #'lsp-completion--disable t)
(when (featurep 'company)
(remove-hook 'company-completion-started-hook
Expand Down