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

[completion] Use ignore instead of try-completion-function in CIDER completion style #3750

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

alexander-yakushev
Copy link
Member

@alexander-yakushev alexander-yakushev commented Oct 21, 2024

Should fix #3749.

Turns out the API of a custom completion style's "try-completion function" doesn't match Emacs' own try-completion because why the hell should it, right? The API expected from that function is also not documented anywhere.

However, I've seen that eglot simply uses ignore instead of implementing that function and everything still works. I've verified locally that the completion is still operational with ignore there in both company-mode and M-x complete-symbol.

Asking @rrudakov to kindly verify the fix locally (you can replace the body of cider-completion-try-completion with just returning nil to verify).

@rrudakov
Copy link
Contributor

Thanks @alexander-yakushev! This fixes the error.

One thing I noticed with the cider completion style, the matched characters are not highlighted in the candidates list.

With built-in flex style it looks like this:
image

With cider completion style:
image

Another issue I noticed is that "fuzzy" matching doesn't really work, if I type Instnt with cider completion style I'm not getting any completion suggestions, which is not the case with flex style.

@alexander-yakushev
Copy link
Member Author

Wait, did you really typed Instnt, triggered the completion, and got all of the candidates on the first screenshot?

@rrudakov
Copy link
Contributor

Wait, did you really typed Instnt, triggered the completion, and got all of the candidates on the first screenshot?

Yes, it works with flex completion style.

@rrudakov
Copy link
Contributor

I'm not sure if it works with company though, but it definitely works with built-in completion and with corfu.

@alexander-yakushev
Copy link
Member Author

alexander-yakushev commented Oct 21, 2024

Are you sure you haven't triggered completion at Inst and then typed in the additional characters?

Otherwise I don't understand how this is possible. Compliment for Instnt returns 0 candidates.

@rrudakov
Copy link
Contributor

Are you sure you haven't triggered completion at Inst and then typed in the additional characters?

Ah, sorry, looks like this is it. I didn't realize it actually makes a difference.

@rrudakov
Copy link
Contributor

So I guess with flex styles the list is fetched once and then filtered on the client's side and with cider style it's fetched on every typed character, right?

@alexander-yakushev
Copy link
Member Author

alexander-yakushev commented Oct 21, 2024

I didn't realize it actually makes a difference.

Yes, it does. The initial completion candidates for the prefix at the moment of the completion triggering are generated by the backend (cider-nrepl, compliment). Characters that are typed after that filter the candidates on the frontend (Emacs). Compliment doesn't support the completion as fuzzy as flex offers (for performance and practicality reasons). I'd rather make the frontend and backend matching algorithms consistent to avoid the confusion like the one just now.

@alexander-yakushev
Copy link
Member Author

and with cider style it's fetched on every typed character, right?

I don't think this should happen. I think it still happens on the frontend but with stricter rules than flex matching.

@alexander-yakushev
Copy link
Member Author

And the whole ordeal of moving from flex happened because it doesn't work correctly with completing Persistent, for example.

@rrudakov
Copy link
Contributor

So, with cider style, if I type Persistent I'm getting the following:
image

With flex:
image

Am I missing something?

P.S: Sorry if I'm asking dumb questions :)

@alexander-yakushev
Copy link
Member Author

Regarding the lack of highlighting matched characters – yeah, I noticed that as well. It still highlights correctly if the completion prefix is the actual prefix of the candidate:
image
But there is no highlight for non-prefix candidates like you've showed. I currently don't know if there is a way to fix this.

@alexander-yakushev
Copy link
Member Author

alexander-yakushev commented Oct 21, 2024

Am I missing something?

Can you please try the following with flex style:

  1. Enter Persistent into the editor.
  2. Do M-x completion-at-point.

What does it do?

@rrudakov
Copy link
Contributor

It still highlights correctly if the completion prefix is the actual prefix of the candidate

I'm pretty sure it's because basic completion style is used for that.

@rrudakov
Copy link
Contributor

Am I missing something?

Can you please try the following with flex style:

  1. Enter Persistent into the editor.
  2. Do M-x completion-at-point.

What does it do?

Screen.Recording.2024-10-21.at.16.01.11.mov

@alexander-yakushev
Copy link
Member Author

Interesting. On my machine, a different thing happens. Let me try to reproduce with emacs -q.

@rrudakov
Copy link
Contributor

I've just tried to reproduce with emacs -q and I think I was able to reproduce your issue. (Persistent) becomes (.Persistent) and then I cannot get any more completion candidates. I'm curious what do I have in my config to make it actually work.

@rrudakov
Copy link
Contributor

So, after evaluating (setopt completion-category-overrides '((cider (styles . (flex))))) completion starts working as expected.

Funny that setting (setopt completion-styles '(flex)) doesn't fix that.

@alexander-yakushev
Copy link
Member Author

I'm curious what do I have in my config to make it actually work.

That would be great to know indeed 😄.

@rrudakov
Copy link
Contributor

So, after evaluating (setopt completion-category-overrides '((cider (styles . (flex))))) completion starts working as expected.

Funny that setting (setopt completion-styles '(flex)) doesn't fix that.

Too soon, actually it's (setopt completion-ignore-case t). @alexander-yakushev could you please confirm?

@rrudakov
Copy link
Contributor

Full snippet to make it work in emacs -q:

(package-initialize)
(require 'cider)
(setopt completion-styles '(flex))
(setopt completion-ignore-case t)

@alexander-yakushev
Copy link
Member Author

Thanks, I could reproduce that indeed. Still, this looks like a side effect. The problem comes from the fact that completion-at-point machinery looks through all candidates returned by the backend and tries to grow a common substring from them. It happens so for Persistent and other similar queries, there is a common prefix . in all of them, that . is prepended, completion is triggered again, and Compliment returns nothing for such query. I can imagine that enabling completion-ignore-case makes it so there is some candidate that no longer matches .Persistent??? Now that I typed it out it sounds even more confusing.

@rrudakov
Copy link
Contributor

Thanks, I could reproduce that indeed. Still, this looks like a side effect. The problem comes from the fact that completion-at-point machinery looks through all candidates returned by the backend and tries to grow a common substring from them. It happens so for Persistent and other similar queries, there is a common prefix . in all of them, that . is prepended, completion is triggered again, and Compliment returns nothing for such query. I can imagine that enabling completion-ignore-case makes it so there is some candidate that no longer matches .Persistent??? Now that I typed it out it sounds even more confusing.

I agree it weird and confusing. I was able to find it just by enabling options from my config one by one. Maybe some emacs experts could shine some light on this.

@rrudakov
Copy link
Contributor

rrudakov commented Oct 21, 2024

image

There are candidates in the list that has lower case p as a first match, so I think you're right. I think Persistent prefix is sent to the backend and the list of candidates is fetched, but then because of this ignore-case settings, there is no more common prefix, for some candidates it's . for others it's p, so we see the unfiltered list.

@alexander-yakushev
Copy link
Member Author

I'll take the liberty of merging this ASAP since the original issue is quite nasty.

@alexander-yakushev alexander-yakushev merged commit 7b051c4 into master Oct 21, 2024
39 checks passed
@alexander-yakushev alexander-yakushev deleted the try-completion-ignore branch October 21, 2024 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error after enabling cider completion style
2 participants