-
-
Notifications
You must be signed in to change notification settings - Fork 860
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
The completion table does not perform prefix filtering #2970
Comments
Eglot has an eglot-stay-out-of mechanism. Would it make sense to provide something like this also for lsp? In this particular case the user may want to opt out from the completion-category-defaults modification. Line 734 in fc30baf
However it may be easier to just overwrite the completion-category-defaults after activating the lsp-mode. So I think I would not add a separate stay out mechanism. |
The issue is that the emacs completion tables are broken abstraction and do not work with lsp protocol. Emacs completion framework has several assumptions that do not hold in lsp case.
There are probably much more than that. To solve this issue we might introduce a variable which will force lsp-mode to behave more like the traditional backends and generally recommend against enabling that feature because it will give you an incorrect set of completion items. |
Yes, I am aware of this impedance mismatch. My proposal is to use a different (lambda (str pred action)
(if (eq action 'metadata)
'(metadata (category . lsp-capf)
(cycle-sort-function . identity)
(display-sort-function . identity))
(complete-with-action action (funcall candidates) str pred))) In order to prevent double filtering a This change will then give the user more freedom, since the double filtering Unfortunately there will still be issues with this approach, in particular in |
Above I outlined my proposal again. I also want to discuss your points a bit more specifically:
As I see it, the lsp server returns a list of items which fit at point. The server may use the current prefix or something else. The prefix filtering I am talking about is a pure Emacs thing and will always lead to double filtering.
I am not sure I understand what you mean by that.
The
I would avoid this. My proposal involves adding a custom |
Language server can have a different understanding of prefix compare to Emacs, thus each candidate item returned by the language server can have different prefixes and replace a different range in Emacs's buffer. As you see, that's why we cannot use the predicate in the completion table function, since that predicate is not so correct (we just try to return the symbol at point, which can be a very different concept between Emacs and LSP). Note that the
I agree with this, Also, since we can only perform secondary filtering at this point, why bothering calling Alternatively, perhaps you can simulate the inserting from the minibuffer to the current window buffer so the filter will not be secondary anymore. That would make the current |
You can use the predicate here. But it does not matter anyways since the predicate will be nil except if the user overwrites it via some advices.
This is not correct. The completion styles basic, emacs21 and emacs22 use the prefix for filtering. Only the combination of basic with your completion table, which ignores the prefix, leads to the prefix being ignored. Other completion styles like substring, partial-completion, initials, flex and orderless cannot make use of the prefix and will always pass the empty string to the completion table. You can see it like this - by calling the completion table with a prefix string, basic takes advantages of the filtering inside the table as an optimization. See also the comment in minibuffer.el: https://github.com/emacs-mirror/emacs/blob/1dba0ca278f8185912e8d39b2af05fc6739b65f8/lisp/minibuffer.el#L3455-L3456
This is how completion tables are supposed to work. The completion table must be called again with each new input. You may want to add some caching in the lsp completion table (There is also completion-table-with-cache). However in practice it only matters for consult-completion-in-region and similar completion-in-region functions which transfer the completion to the minibuffer. If the completion is really performed inside the buffer this is not an issue.
Of course it could, but this is incorrect. It won't work for file table completion then for example. The correct approach is to always recall the completion table. The dynamic completion table should then take care of caching itself such that this is not more costly than calling it once. My Vertico package implements this correctly, as does Icomplete.
Yes, I've considered this. But this is a very lsp specific workaround. I think in that case it would be better to provide a separate |
You're right.
You miss my response, I specifically stated that if we can only do the secondary filtering there.
Yes, this can work. I will take a look at this |
What do you mean exactly? The list returned by the completion table can be mutated. For example the default completion UI mutates the list by sorting. But the candidate strings itself are not mutated, the highlighting is applied to copies of the strings.
Yes. The solution I am proposing is equivalent, only that the completion table is compliant. basic won't be passthrough as it is now. If you use an explicit lsp-passthrough completion style the user will have the choice.
I am sorry, but I don't understand exactly what you are trying to tell here. It is good that you are doing caching and I agree that it is difficult since the server may not expect that you are doing caching. I am also aware that only a part of the candidates may be returned, which will is the first problem with minibuffer-driven completion as in There is one thing I would like to understand better - you say that the lsp server may return different results even with the same input. I don't think that's the case. If |
It's not guaranteed and entirely depends on the language server implementation. |
Yes, of course the language server could show whatever non-deterministic behavior it likes. But this is what I call a bug then. |
#2975 looks mostly like what I had in mind, but not quite. I added a few comments. |
Not really, the language server can indexing the code slowly and only returns part of completion items at first. After that at a later time, it can return more accurate items that may not include the previous ones. |
Okay, fair enough. It is a bit of an ugly behavior. Are there language servers which do that? |
We used to use flex in the past and it does not work well - it will either return more results than needed or it will return zero results(the same is true for other completion categories). Everyone applying a different(not the one implemented in lsp-mode) filtering strategy should be warned that the completion result might not be the real one. |
You are right that it may return fewer results than not performing double filtering. However it cannot be that it shows more results than with the current implementation which does not perform double filtering. Overall I agree with you that flex is not an ideal solution. I am also not proposing it here.
Agree, with a custom filtering strategy you may see fewer candidates than with the lsp-passthrough strategy I am proposing or the current strategy (basic plus completion table, which ignores the prefix). |
Maybe I misunderstood your suggestion. I thought that inserting the text in the buffer is problematic for you. If it is not, as long as the buffer is up to date and the completion table filtering is performed by lsp-mode it will work fine. |
Yes, this is problematic for me. I won't implement this in |
I got confused:
I am not sure what your proposal for complying with Emacs completion specification and follow lsp protocol specification is. Without having the text inserted we cannot handle partial results and we cannot perform filtering. |
The PR by @kiennq goes in the right direction and only has a few minor issues. The point is that the completion table should follow the Emacs completion specification. (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))) In comparison the current completion table: Lines 488 to 504 in 4f23138
By providing a custom completion style (called |
For comparison - Eglot filters the candidate by checking for the |
Ok, what I was just saying is that we should make that visible to avoid misleading the users. In certain cases, I find broken completion extremely irritating. |
I agree. I am just trying to get the best out of it with what we have. With my proposal we should get a solution which is as good as it gets regarding compliance with lsp and Emacs completion specification (except for the highlighting, but this is currently also imperfect).
It makes sense to point out that minibuffer completion like |
Eglot is not respecting prefix defined by language server and is using the prefix from Emacs only, that's why in some cases, you will see that it put much less item after filtering compare to Good thing is that with the custom completion style defined, we separated the case between calling by the completion table via
The highlighting is working for |
Correct.
Correct. The prefix filter is the second-level Emacs filtering to conform with the Emacs specification. This is what I am asking for here.
No,
All my completion packages (and also Emacs default completion and Icomplete) invoke the completion table via
Of course it does. But this is a best effort approach. The highlighting is applied by the |
The highlighting is determined via the
Yes, and it does not respect the user's setting |
Ah I see, you are right. So this makes use of the highlighting data as returned by lsp? The downside is that this is a company specific extension, which is incompatible with other completion UIs. It would be better to apply the highlighting on the level of the completion style and removing
No, it respects the completion styles or rather you are confusing the different levels here. The backend/completion-table implementation sits one level below the completion styles. |
If you ask me, better to throw away this completion implementation and create a simpler async specification(e. g. following what company-mode or other editors have) instead of having the current cryptic spec/implementation. |
I think you missed my point, all it does is doing prefix matching, then certainly it's not respecting the user's |
Well, then propose it on emacs-devel. I agree with you that the current spec is baroque and complicated (and confusing as this discussion shows). But one can work with it and so far it went well enough in my use cases. Note that completion tables serve a dual purpose since they can complete candidates and filter so this complexity is hard to argue away. Then there is the big issue with backward compatibility. There are hundreds of Regarding async - you are right that there is some potential for improvement. In the case of |
At some point I will, this is not my top priority right now.
IMHO it all can be done without breaking the existing code.
lsp-mode was criticized for the need to install one more package and also the company-lsp maintainer was inactive and we didn't have write permissions for their repo. Also, it has the issues that eglot currently has - it was calculating the prefixes incorrectly and we started implementing the capf to correct those. Later that turned into a saga. |
Oh, and forgot the most important part - move that completion framework in elpa. |
IMO, capf is not that bad, it's only missing good documentations and async callback. We work around the async requirement by doing canceling on user typing already, the left part is for good documentation. |
What is the status here and in #2975? |
@minad - are you fine with the changes in that PR? I was planning to merge it once you approve it? |
@yyoncho Yes, please merge the PR. It fixes this issue. |
I approved it, I will give some time to @kiennq in case smt else is missing and then I will proceed wth the merge. |
Thank you for the bug report
lsp-mode
related packages.Bug description
The lsp completion table is non conforming to the specification. In particular for the all-completions action (t) the returned candidates must be filtered with the given prefix string.
The problem is here:
lsp-mode/lsp-completion.el
Line 504 in fc30baf
Original issue:
minad/consult#346
Steps to reproduce
See minad/consult#346
Expected behavior
The completion table should be comforming to the specification.
Which Language Server did you use?
Any
OS
Linux
Anything else?
The issue here is that lsp wants to avoid double filtering. My proposal is to not use basic but a custom completion style which does not make use of prefix filtering for the lsp-capf category. Given such a completion style allows to use a fully compliant completion table. In most scenarios the end result will be equivalent. However not in all scenarios, e.g., not in the case when consult-completion-in-region is used, where the completion is transferred to the minibuffer. I can help out with a patch.
Cc @kiennq
The text was updated successfully, but these errors were encountered: