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.

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

Implement simplified capf mode #3825

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

Conversation

xendk
Copy link
Contributor

@xendk xendk commented Nov 24, 2022

This adds another lsp-completion-provider option, :corfu-capf,
which makes auto-completion work better with standard completion
front-ends and language servers that prettify completion labels.

In essence, it uses the filterText property from the language server
completion reply in preference to label. This makes sense as
completion is basically filtering, the drawback is that what was
previously shown as "Schema" in the popup now shows up as
"\mysql_xdevapi\Schema", and "my_var" as "$my_var".

It also disables the lsp-passthrough completion style, so
completions are again fed through the standard completion functions
(or whatever the user has set up). Non-company-capf front-ends expect
the completion style to filter the completions, and lsp-passthrough
doesn't.

This does mean that prefix filtering is back in effect with the
default Emacs completion styles, which might filter out some of the
more outlandish suggestions that the language server provides
(serenata, for instance, suggests "dummy()" as completion for "my"),
which is a bit sad, but completion styles like orderless can remedy
this.

For me, it's an acceptable compromise, but for someone not coding PHP
it might not be, so that's why it's a new option. I'll argue that it
should be the :none option as even the built-in
completion-at-point wont work with serenata for variables, but I'm
not entirely sure it's OK to change the behaviour of :none?

Closes #3817

@xendk
Copy link
Contributor Author

xendk commented Nov 24, 2022

...and this might need some work, but input would be appreciated.

;; 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

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

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

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

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.

@@ -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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

corfu for variables not working in php-mode with serenata ls
4 participants