search

minad / corfu

:desert_island: corfu.el - COmpletion in Region FUnction
GNU General Public License v3.0
1.15k stars 43 forks source link

Completions while using LSP Mode aren't filtered properly #41

Closed lucminah closed 3 years ago

lucminah commented 3 years ago

Hello! I'm really excited about corfu and am happy to see it grow. Hope I can help in its improvement. The issue I've found is as follows:

When completing and using LSP Mode, filtering seems to apply to the already inserted text, but breaks with what comes after. The order of the candidates seems to be affected while typing, showing first the ones that actually match what's inserted, but non-matching candidates are also shown and not filtered away. Non-LSP completion works fine, and without corfu (with the default *Completions*), the filtering works as expected.

I've reproduced the problem in emacs -q. Apart from the default configuration of straight.el, I've only evaluated what's shown in the scratch buffer in the following demonstration (in which I used Python with pylsp, but the same behavior occurs in other modes such as C/C++ with clangd). Here's a replication of that code:

(straight-use-package 'corfu)
(setq tab-indent-always 'complete)
(corfu-global-mode 1)

(straight-use-package 'lsp-mode)

2021-07-19 12-24-37

minad commented 3 years ago

Lsp-mode does not implement a correct completion table, see https://github.com/emacs-lsp/lsp-mode/issues/2970. Maybe that's what leads to issues here.

minad commented 3 years ago

Closing. Should be fixed in lsp-mode, see https://github.com/emacs-lsp/lsp-mode/issues/2970 and https://github.com/emacs-lsp/lsp-mode/issues/2975.

jdtsmith commented 3 years ago

What is the state of using corfu with lsp-mode? Any advice? Orderless filtering/highlighting does not seem to pass through (unlike eglot).

Update — Aha, I see why:

completion-category-defaults is a variable defined in ‘minibuffer.el’.
Its value is ((lsp-capf (styles lsp-passthrough)))

which I suppose I could defeat. But if I just let the lsp server give me whatever it thinks in whatever order, it works OK. Honestly I really miss orderless filtering then.

One thing I notice company does that's great is after a . (in python-mode) it auto-summons completion immediately. I don't think corfu has a predicate that can be setup for reducing auto-prefix length.

minad commented 3 years ago

@jdtsmith LSP should work probably since the fix got merged. Unfortunately it overrides the completion styles (as you figured out) in order to not interfere with LSP filtering. But if you override this setting with orderless, orderless should work too. Please give it a check - this also depends on if LSP caches the candidates within the completion table!

jdtsmith commented 3 years ago

Good thoughts. I ripped out lsp-passthrough:

  (add-hook 'lsp-mode-hook (lambda ()
                 ;; Switch back to corfu and orderless
                 (company-mode 0)
                 (setcdr (cadr (assq 'lsp-capf completion-category-defaults))
                     '(orderless))
                 (setf (caadr ;; Pad before lsp modeline error info
                    (assq 'global-mode-string mode-line-misc-info))
                   " ")))

and found lsp-mode+corfu+orderless does indeed work well in a short test! This of course does the dreaded "post-LSP-contact" filtering, but that is what I want. I use filtering rather than arrowing down almost exclusively: just so much faster especially for lots of completions.

I do really wish I could configure auto popup to happen immediately after a ".", e.g. for deeply nested foo.bar.car.dee attributes. This is company default behavior. Perhaps a special corfu-auto-complete-predicate, or if that's too heavy, simply a list of chars like corfu-auto-complete-immediately-after-chars would do the trick?

BTW, like eglot, lsp-mode also does not "dump its completion cache" when the original string is altered/deleted. I think I'm actually convinced that requesting a new table function is the job of the completion UI in this very specific situation: if the entity at point in the buffer is changed beyond recognition, the completion table should no longer be considered valid and a new one should be requested. But we've already gone around on that a few times :). Note that this is still less dumping (by far) than company does (which his basically on every keystroke).

BTW, if you do hope to attract a good number of lsp users (which seem to be growing in number rapidly), I'd suggest:

  1. Getting the lsp-mode authors not to hard-code in company, or at least provide an option to go without it.
  2. Implementing a few visual niceties.

Comparing default corfu:

image

vs. default company:

image

Main improvement of the latter is the distinctive text styling on the annotation (easy), the right alignment (enabled with company-tooltip-align-annotations) and of course icons (less easy/but also probably less important). For the coloring, perhaps just mentioning a config option like:

(set-face-foreground 'completions-annotations "CornFlowerBlue")

would give people a bit of the company vibe (if they want it). I'd also suggest providing an option to right-align annotations. I mocked these both up and corfu then looks quite nice:

image

(though less space between annotation and bar would be better). Enjoying corfu very much thanks for all your efforts.

minad commented 3 years ago

So it seems we agree on most of the points. I would like to encourage you and other users to make PRs regarding these features. Corfu's main issue is that it is young and not tested widely against different setups (capfs, programming languages, lsp-servers, themes, ...). Ideally this testing against all the different configurations and different programming languages is crowd sourced - I lack the time for doing this. Furthermore the second issue is that the entire Emacs ecosystem is developed against Company and Company added many extensions, which makes it hard for an alternative which is not entirely following the Company implementation. As I pointed out above, Corfu follows the capf/completion-in-region API quite closely, instead of adapting/retrofitting capf to the frontend as Company is doing.

You may have seen that Doom Emacs adopted Vertico/Consult/Embark/Orderless/Marginalia as their default completion UI. I have some hope that they will also add a Corfu module to their configuration, which will then help us sort out many more issues due to their well made preconfiguration of many packages. However this is more likely to happen if Corfu works well overall and ideally these visual issues should be sorted out first.

jdtsmith commented 3 years ago

Very good thanks for the detailed response. I'll see about PRs for right alignment and trigger characters. Since completions-annotations is the default face, probably just giving people info on how to set it suffices. I see modus does (completions-annotations ((,class :inherit modus-theme-slant :foreground ,cyan-faint))) so already pretty good.

If corfu does get adopted in Doom, that presumably would push the LSP/eglot's of the world to adapt, but I see your point; a bit of chicken and egg problem. I'll see if I can stimulate some movement there.

minad commented 3 years ago

I'll see about PRs for right alignment and trigger characters.

Looking forward to that.

Since completions-annotations is the default face, probably just giving people info on how to set it suffices.

I introduced a corfu-annotations face in 9249404cdc83fe4a827d0ba909de263e3f860577 to allow more flexible styling. This is useful since the popup face does not necessarily have to match the minibuffer/completions-buffer face. I agree that adding a small section to the README documenting these style tweaks would be good. We should probably describe all the faces used by Corfu.

If corfu does get adopted in Doom, that presumably would push the LSP/eglot's of the world to adapt, but I see your point; a bit of chicken and egg problem. I'll see if I can stimulate some movement there.

As I see it, the best way to help is really to document and fix all the issue that occur on a case by case basis. As I said help is very welcome there, since testing all the different setups needs a lot of effort. Furthermore it is different if I only try something quickly to fix an issue in contrast to really working a while with a configuration. So the goal should be that new users don't immediately leave again after finding some issues. If everything works reasonably well, they may stick with it. For me Corfu works quite well, but I am only using it in a tiny subset of the possible scenarios. As it has turned out, due to the variety it is much harder to get Corfu right in comparison to getting something like Vertico right. But I guess the polyculture of minibuffer completion UIs has helped there in contrast to the de-facto Company monopoly. The name fits I guess :-P

hmelman commented 3 years ago

I'm not an lsp user and I only use company rarely (in python via elpy), but looking at the styling above I'll throw out this thought. If the annotation is just the type of the candidate (variable, method, property), rather than seeing those words right-aligned or not, I think I'd rather just see the candidate fontified via the font-lock properties for variable-names and function-names. Is this a config choice? Or append "()" to method candidate names.

minad commented 3 years ago

@hmelman The annotation function is provided by the backend. Corfu currently avoids applying fancy styling on its own. However Company has the extension company-kind which is used by Company to show these ugly icons. One could as well use company-kind to style the candidates accordingly. But for now I'd like not to focus on such superficial polishing.

jdtsmith commented 3 years ago

@hmelman See #38 for some other text-based prefix concepts, which does color-code based on "flavor". I had coded something up and forgotten it. In general aligning things which may have arbitrary display properties as well as including their own attempts at alignment is a challenge. What's needed is an agreement between the supply side of CAPF backends and the presentation side about who's in charge of what. Sticking with "everything is a fixed width character" radically simplifies things, but could be constraining.

minad commented 3 years ago

The idea of company-kind is to have a semantic symbol and the UI is in charge of styling. This is a good idea, but a deviation from how styling is done in completing-read, where for example the annotations are formatted by the backend.

gagbo commented 3 years ago

@jdtsmith In my case (lsp-mode, using Doom Emacs), to get "proper" orderless corfu completion with lsp-mode I had to :

(add-hook 'doom-init-modules-hook      ; This hook runs after shipped Doom code is ran and means "let me override anything that Doom did"
          (lambda ()
            (after! lsp-mode           ; Basically with-eval-after-load
              (setq lsp-completion-provider :none))))

(add-hook 'lsp-mode-hook
          (lambda ()
            (setf (caadr ;; Pad before lsp modeline error info
                   (assq 'global-mode-string mode-line-misc-info))
                  " ")))

(add-hook 'lsp-completion-mode-hook
          (lambda ()
            (setf (alist-get 'lsp-capf completion-category-defaults) '((styles . (orderless))))))

With the :none provider, almost nothing is done in lsp-completion-mode-hook : https://github.com/emacs-lsp/lsp-mode/blob/9865d315123bb0482de358e6905838ba1acb5b45/lsp-completion.el#L754 so I don't need to turn off company-mode, and if I want to avoid nil errors I directly setf the completion-category-defaults

jdtsmith commented 3 years ago

Thanks, right you are. I had opened an issue and got the same advice. I found the name quite counter-intuitive, since completions are certainly still provided by CAPF, just company is not setup if :none. I've suggested some simplifying language. Here's what I'm currently using for orderless, after setting lsp-completion-provider to :none.

  (defun my/lsp-mode-use-orderless ()
    (setf (alist-get 'styles
             (alist-get 'lsp-capf completion-category-defaults))
      '(orderless)))