-
-
Notifications
You must be signed in to change notification settings - Fork 890
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
base: master
Are you sure you want to change the base?
Conversation
...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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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.
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.
There was a problem hiding this comment.
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:
- Use all-completions against filter-text strings propertized with the label
- Map over the result, looking up the label text property
There was a problem hiding this comment.
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.
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)
There was a problem hiding this comment.
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:
(after the initial popup, no further filtering is done). Typing $ar
and invoking completion-at-point
just gives "No match".
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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 styledefault-filtered
: Setup flex style (or orderless if available)company
: Auto configure Company, likedefault-passthrough
corfu
: Auto configure Corfu, likedefault-filtered
Note that default-filtered
is equivalent to the behavior of Eglot.
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 servercompletion reply in preference to
label
. This makes sense ascompletion 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, socompletions 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-incompletion-at-point
wont work with serenata for variables, but I'mnot entirely sure it's OK to change the behaviour of
:none
?Closes #3817