Cider completion style is invalid

[minad]

Hi Bozhidar!

Cider adds a custom cider completion style to the completion-styles-alist. However the entry that is being added does not follow the required specification of completion-styles. This issue has been found by @manuel-uberti (see minad/corfu#8 for the original issue). I wonder how cider gets away with the current implementation. Does the default completion-at-point function work with Cider?

Daniel

---

(defun cider-company-unfiltered-candidates (string &rest _)
  "Return CIDER completion candidates for STRING as is, unfiltered."
  (cider-complete string))

(add-to-list 'completion-styles-alist
             '(cider
               cider-company-unfiltered-candidates
               cider-company-unfiltered-candidates
               "CIDER backend-driven completion style."))

• The first function should follow the calling convention of completion-try-completion, it should return t, nil or a pair (newstr.newpoint)
• The second function should follow completion-all-completions (I think it follows the spec, but you are not returning a base size in the last cdr which is okay I guess?)

[bbatsov]

Does the default completion-at-point function work with Cider?

Yeah, it does. This code is one of the oldest parts of CIDER and has seen minimal changes in the past 9 years, so I'm assuming that some APIs might have changed in Emacs. Is there some documentation on the topic that I can peruse?

[minad]

The docstrings of completion-try-completion and completion-all-completions should have all the necessary information.

[bbatsov]

I see. So basically we need some cider-completion-try-completion function alongside the existing one. I guess I'll also have to check what are the benefits/use-cases of all the params we've suppressed for cider-company-unfiltered-candidates, as I see the actual definition has a lot of params:

(completion-all-completions STRING TABLE PRED POINT &optional METADATA)

[bbatsov]

Btw, I've also forgotten what's the difference between completion-at-point-functions and completion-styles-alist. Perhaps @dgutov can help by suggesting what'd be the appropriate changes.

[dgutov]

@bbatsov Completions functions return some data from their own set, completion styles filter the data provided by the former.

I haven't written any completion styles myself, though, so I'm not going to help with the exact diff.

[bbatsov]

I was mostly wondering if those completion styles have anything to do with company-mode, but I guess I'll figure this out myself when I find a bit of time.

[bbatsov]

Looking at the surrounding code - this was some hack to enable fuzzy completion in company-mode:

(defun cider-company-unfiltered-candidates (string &rest _)
  "Return CIDER completion candidates for STRING as is, unfiltered."
  (cider-complete string))

(add-to-list 'completion-styles-alist
             '(cider
               cider-company-unfiltered-candidates
               cider-company-unfiltered-candidates
               "CIDER backend-driven completion style."))

(defun cider-company-enable-fuzzy-completion ()
  "Enable backend-driven fuzzy completion in the current buffer."
  (setq-local completion-styles '(cider)))

[minad]

It is okay to effectively disable the completion style given that your backend already performs the filtering. However the functions should still follow the specifications, as I noted only the return value of the try-completion function is incorrect. cider-company-unfiltered-candidates is only valid as all-completions function.

[manuel-uberti]

Looking at the surrounding code - this was some hack to enable fuzzy completion in company-mode:

(defun cider-company-unfiltered-candidates (string &rest _)
  "Return CIDER completion candidates for STRING as is, unfiltered."
  (cider-complete string))

(add-to-list 'completion-styles-alist
             '(cider
               cider-company-unfiltered-candidates
               cider-company-unfiltered-candidates
               "CIDER backend-driven completion style."))

(defun cider-company-enable-fuzzy-completion ()
  "Enable backend-driven fuzzy completion in the current buffer."
  (setq-local completion-styles '(cider)))

It was probably depending on my settings, but without hooking cider-company-enable-fuzzy-completion into cider-mode-hook I was never able to get Company completion in my CLJ/CLJC/CLJS buffers.

[toniz4]

I've been taking a look at this, probably it would take some work to do. And so far, it appears that I will have to change somethings in nrepl, to fix #3019 correctly and make it work with orderless and other (any?) completion styles.

[vemv]

@toniz4 thanks for taking a look!

#3653 is a useful read as well.

Feel free to share your initial analysis so that we all are on the same page.

[toniz4]

@toniz4 thanks for taking a look!

#3653 is a useful read as well.

Feel free to share your initial analysis so that we all are on the same page.

I will!

From what I could already gather, the prefix that cider-complete-at-point receives it's different in other styles, if you have a ns called utils, for example, and type "uti" and try to complete, the prefix will be blank, because nothing matches uti, and nrepl will not return the namespace as a option, because it checks if something could be a namespace, and "" doesn't looks like a namespace. If you would type "utils", nrepl will return the symbols in that namespace.

In the basic style it works because everything you type is considered a prefix, since it doesn't try to match stuff fuzzy-ly, it (kinda) makes sense.

In this weekend I will try to mess with nrepl itself.

Btw, there's a easier way to debug elisp? I've been just using (message ...), tried using debug and edebug, but It didn't seem to work as I intended.

[vemv]

Btw, there's a easier way to debug elisp?

You could perhaps advice-add core related Emacs functions so that you get a bespoke log of their inputs / outputs.

We also have the nrepl log (*nrepl-messages-<repl name>*).

[vemv]

I also have a particular stance for the overall issue (and related issues) - if the cider completion style is in fact invalid, perhaps the best course of action would be to:

• Determine if we need a custom one to begin with
• If so, implement a new, compliant, one from scratch
• And replace cider with cider-v2 from the defaults

This way we can cleanly swap implementations without risks/breakage. The old one, valid or not, works well for many people and isn't a frequent source of complaints.

IMO without a clear understanding (and summary, for others) of how Emacs expect things to work, and how we tackled those requirements, we'll remain in the same messy situation for another batch of years.

It's not a small task, but probably not huge either.

cc/ @bbatsov @alexander-yakushev

[bbatsov]

This way we can cleanly swap implementations without risks/breakage. The old one, valid or not, works well for many people and isn't a frequent source of complaints.

I guess few people use advanced features, that's why few people have noticed the issues that currently exist. I don't think it'd be particularly risky to iterate on the current codebase, as it's not complex.

IMO without a clear understanding (and summary, for others) of how Emacs expect things to work, and how we tackled those requirements, we'll remain in the same messy situation for another batch of years.

That's the main issue for me - there's no clear documentation about implementing the completion APIs, so getting things right usually requires researching "reference" implementation and trying to figure out what's expected from us. I've often thought about doing a deeper dive there, but never found the time and the desire to do so. I recall I wrote the original completion code in less than 1 hour with minimal research, so it's definitely not rocket science. I also recall I barely touched it since then. 😅

Before someone really analyzes what needs to be done it'd be premature to discuss alternative implementations and so on.

[vemv]

I've often thought about doing a deeper dive there, but never found the time and the desire to do so

Yes, at the moment similarly I don't have the particular time or personal desire of iterating on this. Although I'd be happy for a variety of users to be supported.

Before someone really analyzes what needs to be done it'd be premature to discuss alternative implementations and so on.

Exactly - someone interested in taking this to the next level would have to make quite the ansaysis - namely a full-stack one:

• Emacs core APIs
• cider.el
• cider-nrepl
• Compliment

(of course, happy to help with whatever we can)

Finally, perhaps Emacs lsp implementations (especially the official one) may be a good 'reference' implementation, if docs don't suffice.

[alexander-yakushev]

This comment summarizes one of the problems really well: #3019 (comment).

Here's what I understood so far:

1. The claim that CIDER completion style is invalid is indeed correct, but it is easily fixed. We define the completion style like this:

(defun cider-company-unfiltered-candidates (string &rest _)
  "Return CIDER completion candidates for STRING as is, unfiltered."
  (cider-complete string))

(add-to-list 'completion-styles-alist
             '(cider
               cider-company-unfiltered-candidates
               cider-company-unfiltered-candidates
               "CIDER backend-driven completion style."))

So, a completion style is a list of symbol 'cider and two functions – where we provide the same function cider-company-unfiltered-candidates. That function ignores everything but the first argument (the prefix) and returns the results of calling cider-complete on the prefix. But if you look at any default completion style that comes with Emacs, e.g. on partial-completion:

(partial-completion
     completion-pcm-try-completion completion-pcm-all-completions)

(defun completion-pcm-all-completions (string table pred point)
  (pcase-let ((`(,pattern ,all ,prefix ,_suffix)
               (completion-pcm--find-all-completions string table pred point)))
    (when all
      (nconc (completion-pcm--hilit-commonality pattern all)
             (length prefix)))))

See that completion-pcm-all-completions returns a length of the prefix as the last argument. This seems to be what @minad was talking about when he said that there is no number in the cdr.

So, overall, this mismatch can be easily fixed in CIDER and we should do that.

[alexander-yakushev]

This is problem number 2:

Several Emacs completion styles (new prepackaged flex and orderless, which seems to be a third-party library) operate by not sending the completion prefix to the backend (i.e., sending an empty prefix instead) and expecting all possible completion results back so they can filter them on the client (in Emacs). Compliment doesn't operate like that, and will never do, because the full list of potential candidates that Compliment can provide is prohibitively long. Compliment reacts to an empty-prefix with a non-empty list, so it works partially – all vars in the current namespace will be offered, all namespaces, all imported classes. But it won't show non-imported classes, vars in another namespace, keywords, etc. But this is still not the preferred way of using Compliment.

[alexander-yakushev]

Problem number 3:

Other Emacs prepackaged completion styles (like partial-completion or initials) give the correct prefix to the backend – this is good! Besides, they closely match the behavior that Compliment tries to achieve – Compliment in fact acts as some fusion of partial-completion and initials. So, can we use either of them to achieve the behavior that is currently established by cider-company-enable-fuzzy-completion (that is, by overriding the completion style with our custom one)? Unforunately, no, because the client-side filtering that those completion styles perform is very Elisp-specific. That is, they only consider - as a separator but not other characters, so for example, in partial-completion:

• r-m-h completes to ref-max-history
• c.j.i doesn't complete to clojure.java.io.

[vemv]

Several Emacs completion styles (new prepackaged flex and orderless, which seems to be a third-party library) operate by not sending the completion prefix to the backend (i.e., sending an empty prefix instead)

That's where I'd try to make CIDER intervene in some way - in other words, bridge both worlds smartly.

We could try to stay compliant with flex/orderless. But when CIDER hits Compliment, it does send all the relevant info (e.g. we override the prefix from empty to actual user input, and we possibly pass the completion style as a separate k-v).

In the end, it's always CIDER that creates nrepl communication - we are in control. Whenever that happens we can inspect what the user was typing (assuming that flex/orderless hide that from us).

[vemv]

Unforunately, no, because the client-side filtering that those completion styles perform is very Elisp-specific.

If we are sure about that, I'd be happy to document that specifically (we already have documented that misc styles are currently unsupported - but we can double down on that).

[alexander-yakushev]

If we are sure about that, I'd be happy to document that specifically (we already have documented that misc styles are currently unsupported - but we can double down on that).

I gathered that from a cursory analysis I did today. I tried setting different default completion styles, observe what happens in CIDER<->cider-nrepl interaction and what gets suggested by (completion-at-point), with some extra Elisp debugging. At least out of the box, that's how those completion styles behave.

I've noticed just now that there are some ways to customize them, e.g. there is completion-pcm--delim-wild-regex. I can take a look if it is possible to reasonably make them behave for our purposes using this.

[vemv]

Thanks (and kudos) 👍

I've noticed just now that there are some ways to customize them, e.g. there is completion-pcm--delim-wild-regex. I can take a look if it is possible to reasonably make them behave for our purposes using this.

Yeah it would be odd if completions were only useful for Elisp. Emacs has always been a multi-lang editor.

[bbatsov]

I can take a look if it is possible to reasonably make them behave for our purposes using this.

That'd be great.

Fantastic analysis, btw! Seems to me it'd be best to start with 1), as it's probably the easiest item to address.

[alexander-yakushev]

Sorry, disregard what I said above. The delim regex that it generates includes multiple characters, so it indeed completes c.j.i to clojure.java.io. And if initials is selected, then cji is completable to clojure.java.io.

So, overall, I'd say that most default completion styles (basic, partial-completion, initials) partially work with CIDER right now, in the sense that the user gets some completions, but not all that CIDER can provide. E.g., str/l-i-o or str/lio won't be completed to str/last-index-of by any of those, and we'll never get InputStream => java.io.InputStream completions from them.

The ideal solution for me personally (and subjectively) would be the following

0. Keep our custom completion style. We can call it cider-all, cider-unfiltered, or keep it as cider for compatibility, however, I don't know how important the compatibility in this particular thing is.
1. Fix the API incompatibility of our custom completion style, as described in this issue.
2. Two options:
   a) Keep the default completion style for CIDER as basic and let the user override it to cider-all with a function like (cider-completion-enable-fuzzy), or:
   b) Set the default completion style for CIDER as our custom cider-all. I have a subjective and visceral preference for this, mostly because of the short classname completion. However, this also has a practical benefit that non-compatible completion styles like flex or orderless won't break CIDER completion anymore. I'd wager that users who blanketly enable those completion styles do not insist on us using them for CIDER if we can't, and would rather have any working completion here even if it doesn't match their preferred style.
3. If user still does override the completion style for CIDER to some other default style like partial-completion or initials, the completion will still work to a reasonable extent. <= This is already true.
4. (Bonus) If user overrides the completion style for CIDER to flex or orderless, the completion will still work to a reasonable extent. This will require intercepting the backend call and providing the completion prefix which those styles hide.

[vemv]

I'd only suggest to not introduce breakage in, for instance, the following forms:

• defaults
• names
• behavior (what gets completed for a given name)

i.e. the current behavior is pretty correct for practical purposes - it works for what can work and actively prevents unsupported stuff from being activated.

The next-gen solution can live under a separate name and out of the defaults, at least during one release.

(For clarity, I'd personally be happy to enable it / give feedback from day 1).

Most of all, I'd try to keep users isolated from churn and ensure that they always have a known-good "escape hatch" if there were surprises.

[toniz4]

First, I don't think CIDER should need a custom completion style, my understanding is that, the CIDER completion style was a "hack" to make fuzzy completion work (correct me if I'm wrong).

I think the main problem is in cider-complete-at-point itself, since with flex completion the prefix could be blank, we shouldn't pass that as an argument to cider-complete, the prefix is already been passed to complete-with-action.

cider/cider-completion.el

Lines 197 to 203 in 380073e

	(cond ((eq action 'metadata) `(metadata (category . cider))) ;; defines a completion category named 'cider, used later in our `completion-category-overrides` logic.
	((eq (car-safe action) 'boundaries) nil)
	(t (with-current-buffer (current-buffer)
	(complete-with-action action
	(cider-complete prefix) prefix pred)))))
	:annotation-function #'cider-annotate-symbol
	:company-kind #'cider-company-symbol-kind

See how that is implemented in eglot:

https://github.com/joaotavora/eglot/blob/db91d58374627a195b731a61bead9b4f84a7e4bc/eglot.el#L3136-L3154

It's getting the prefix from another place (lsp magic stuff probably, not that clear to me)

Same thing in haskell mode, but easier to understand:

https://github.com/haskell/haskell-mode/blob/d23ec34788286405f377ce485d2a17e302d94a4f/haskell-completions.el#L324

So, I think the way CIDER is sending the prefix to nrepl is incorrect. I'm thinking on a right way of grabbing the prefix in a decent way (maybe symbol-at-point is enough?).

Anyways, I think we should open a discussion topic.

[bbatsov]

First, I don't think CIDER should need a custom completion style, my understanding is that, the CIDER completion style was a "hack" to make fuzzy completion work (correct me if I'm wrong).

Yeah, more or less.

Anyways, I think we should open a discussion topic.

Feel free to do so.

[toniz4]

Can we close this? The CIDER completion style was soft deprecated already.

@vemv @bbatsov

[vemv]

Sure thing!

Anyone encountering this issue should check out https://docs.cider.mx/cider/usage/code_completion.html#completion-styles