Grab the completion prefix correctly

[toniz4]

This PR makes the CIDER completions behave correctly. Reworking the method to grab the prefix in cider-complete-at-point. Making it compatible with "fuzzy" completion styles.

Tested with the flex and orderless completion styles.

What should we do with cider-company-enable-fuzzy-completion? With
this we can remove the CIDER completion style. Should
cider-company-enable-fuzzy-completion just mirror the behavior of cider-enable-flex-completion?

This also makes CIDER use the default completion style, instead of setting completion-category-overrides to make CIDER use the basic completion style.

---

Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

• The commits are consistent with our contribution guidelines
• You've added tests (if possible) to cover your change(s)
• All tests are passing (eldev test)
• All code passes the linter (eldev lint) which is based on elisp-lint and includes
  • byte-compilation, checkdoc, check-declare, packaging metadata, indentation, and trailing whitespace checks.
• You've updated the changelog (if adding/changing user-visible functionality)
• You've updated the user manual (if adding/changing user-visible functionality)

Thanks!

If you're just starting out to hack on CIDER you might find this section of its
manual extremely useful.

[vemv]

Hi @toniz4 !

Thanks much for your efforts.

Caching / redundant trip avoidance was recently tackled in https://github.com/clojure-emacs/cider/pull/3655/files . Had you noticed?

From my side I don't quite have the time to go through it however I'd constructively suggest:

• Please try to sync up with the most relevant folks for the discussions, in this case from Cider completion style is invalid #3006 it can be appreciated that @alexander-yakushev made a series of analyses (and at least one subsequent PR)
  • most of all I'm trying to avoid having competing ideas - not the most efficient outcome!
• When presenting a PR, just as valuable as the code changes, is an analysis of what was wrong and how you've addressed those problems.
  • without that we'd have to take that it works at face value - not always reassuring, especially for one of our most important features

Once that's all clear, probably we'd appreciate unit tests for this area - it's a dense one and very few of us have the capability to maintain it in detail over the years.

None of this is to discourage you - I'm simply trying to set up us all for success.

Cheers - V

[toniz4]

Hi @toniz4 !

Thanks much for your efforts.

Caching / redundant trip avoidance was recently tackled in https://github.com/clojure-emacs/cider/pull/3655/files . Had you noticed?

From my side I don't quite have the time to go through it however I'd constructively suggest:

* Please try to sync up with the most relevant folks for the discussions, in this case from [Cider completion style is invalid #3006](https://github.com/clojure-emacs/cider/issues/3006) it can be appreciated that @alexander-yakushev made a series of analyses (and at least one subsequent PR)
  
  * most of all I'm trying to avoid having competing ideas - not the most efficient outcome!

* When presenting a PR, just as valuable as the code changes, is an analysis of what was wrong and how you've addressed those problems.
  
  * without that we'd have to take that it works at face value - not always reassuring, especially for one of our most important features

Once that's all clear, probably we'd appreciate unit tests for this area - it's a dense one and very few of us have the capability to maintain it in detail over the years.

None of this is to discourage you - I'm simply trying to set up us all for success.

Cheers - V

Of course, I saw the commits made by @alexander-yakushev after I started working on this, but we kinda followed the same course.

But in this PR I'm trying to address another issue, the way we are getting the prefix are insufficient. So I tried to take the string inside bounds

(defun cider--complete-with-cache (bounds)
  "Return completions to the symbol at `BOUNDS' with caching.
If the completion of `bounds' is cached, return the cached completions,
otherwise, call `cider-complete', set the cache, and return the completions."
  (cdr-safe
   (if (and (consp cider--completion-cache)
            (eq bounds (car cider--completion-cache)))
       cider--completion-cache
     (setq cider--completion-cache
           `(,bounds . ,(cider-complete (buffer-substring (car bounds) (cdr bounds))))))))

But after some testing, this is also insufficient. The bounds is set when the completion starts.

We can see in robe, that the prefix gathering is much more complex:

https://github.com/dgutov/robe/blob/6bc8a07fc483407971de0966d367a11006b3ab80/robe.el#L949-L952

[bbatsov]

Should cider-company-enable-fuzzy-completion just mirror the behavior of cider-enable-flex-completion?

If we're confident in the solution - I think yes. I don't want to kill cider-enable-flex-completion right away, as it might break some user setups. But we can mark it officially as deprecated and scheduled for removal.

I'll have to ponder a bit more on your PR, as I'm not sure I quite follow the issue with the bounds that it's trying to solve. Perhaps @alexander-yakushev will understand this better and have an easier time evaluating it.

[alexander-yakushev]

Should
cider-company-enable-fuzzy-completion just mirror the behavior of cider-enable-flex-completion?

It also depends on how important the support for Emacs <27 is, I had an impression that there was also a compatibility dimension to this.

[toniz4]

In the latest commit, it passes all the "tests" @alexander-yakushev presented here #3653 (comment)

Completing cji -> clojure.java.io

Completing InputSt -> java.io.InputStream

Completing str/sl is a bit different. When I'm in company-mode, I can type str/sl as expected and it will return the completions you would expect. But corfu works differently, I have to type "str", select "str" in the completions, and after that press "/". Probably because str returns multiple candidates, so corfu doesn't make assumptions. This doesn't happens if the namespace is the sole completion candidate.

[alexander-yakushev]

@toniz4 thanks for picking this up!

You should take a look at #3655 for the context. It is crucial that the completion is as fresh as possible and correct too. We figured out there that being a closed over variable inside cider-complete-at-point is the best place for the cache. With your current implementation, if I understand correctly, the cache transcends even different CIDER sessions which is quite dangerous.

[toniz4]

@alexander-yakushev Yeah, I think that's a better way to go. The way I implemented the functions looks cleaner, but I was not thinking about multiple sessions. But it would be really improbable to "hit" the cache even on another buffer, but it's possible.

Either way, putting this stuff in a closure is the right way

[bbatsov]

The CI is failing with:

In cider-complete-at-point:
             cider-completion.el:222:34: Error: Unused lexical argument `pred'

You can ignore the integration tests, as something's wrong with their container images since last week. We'll have to address this separately.

[bbatsov]

We figured out there that being a closed over variable inside cider-complete-at-point is the best place for the cache.

We should also probably add a comment in the code for this, so it won't get forgotten down the road. :-)

[toniz4]

Updated the function to keep basically the same caching behavior that @alexander-yakushev introduced. But it differs in the way we are getting the prefix for the completions. Kinda confusing having 2 things that serve as a prefix, but we are working with emacs. Upon reviewing other packages capf's, I observed that commonly they had a specific mechanism to retrieve the prefix, rather than directly using the prefix from the lambda.

Another thing, I removed the hard coded style override, that were setting the basic completion style for CIDER completions. Is this ok? Currently it will use the completion style configured by the user (by default is (basic partial-completion emacs22))

[bbatsov]

Another thing, I removed the hard coded style override, that were setting the basic completion style for CIDER completions. Is this ok? Currently it will use the completion style configured by the user (by default is (basic partial-completion emacs22))

That's fine by me (from what I got everything should work now, so those are completely redundant), but I'll leave it to @alexander-yakushev to decide how to proceed here.

[alexander-yakushev]

LGTM too! Once this is merged, I'll try using enable-flex again and see if there is any disparity with a custom completion style.

[toniz4]

About tests, @vemv do you have any idea how can we make unit tests for cider-complete-at-point?

[vemv]

Looking reasonable!

wrt unit tests, I don't have ideas at the moment. Might get back at it later today.

Either way I'd appreciate much if after the last round of feedback, you can try this branch over some 24-48 hours.

That QA-ing tends to catch most issues.

[bbatsov]

Yeah, using add-to-list is better - it also adds only new elements and can add new elements to the beginning or end of a list.

[vemv]

LGTM!

Some final suggestions:

• I think the PR doesn't introduce caching anymore? If so please update PR title and description
• Please add a changelog entry

Cheers - V

[toniz4]

Before merging, I would like to QA it more over the weekend. This week I was off work, so I couldn't test it in the real world.

[bbatsov]

@toniz4 Sure. No rush.

[toniz4]

I've been testing this since Sunday, it seems to be working great! If there's no more reviews to be made, I think it's ready to merge.

[vemv]

Awesome! Please merge master in first.

[vemv]

Great work, kudos!

I'll be sure to use Cider master just now to give it some extra QA.

[toniz4]

Great work, kudos!

I'll be sure to use Cider master just now to give it some extra QA.

Thanks! Feel free to ping me if any problem appears

[alexander-yakushev]

Installed the latest MELPA Cider and switched to (cider-enable-flex-completion) from cider-company-enable-fuzzy-completion. Works like a charm!

[bbatsov]

Excellent! Seems we can hard-deprecate the company-specific code then.

[toniz4]

Excellent! Seems we can hard-deprecate the company-specific code then.

By removing completely the completion style? And make the old function just echo the deprecation notice

[vemv]

I'd pursue the path of least obstrusiveness, i.e. try to keep things functional, and echo things once at most (and not on every completion).

Assuming that removing the cider completion style is a good idea, I still would keep it around assuming that it has no real cost. At least during the course of one stable release.

Users rarely enjoy starting a work day to broken setups :)

[vemv]

the company-specific code also includes stuff that most certainly shouldn't be removed at all.

This code doesn't have a pure-Emacs equivalent and powers Company and Corfu:

              :company-kind #'cider-company-symbol-kind
              :company-doc-buffer #'cider-create-compact-doc-buffer
              :company-location #'cider-company-location
              :company-docsig #'cider-company-docsig

[toniz4]

the company-specific code also includes stuff that most certainly shouldn't be removed at all.

This code doesn't have a pure-Emacs equivalent and powers Company and Corfu:

              :company-kind #'cider-company-symbol-kind
              :company-doc-buffer #'cider-create-compact-doc-buffer
              :company-location #'cider-company-location
              :company-docsig #'cider-company-docsig

yup, this is a valid implementation and should not be removed.

I would just remove the competition style. But I agree on waiting a couple of stable releases first.

[vemv]

We're talking about the cider completion style, correct?

It's not immediately obvious to me why it became redundant, mind to generously state here for posterity?

Last but not least - great work! No surprises since I adopted this. Between this and @alexander-yakushev's improvements I'm pretty positive that it feels snappier (especially for the doc part).

[toniz4]

By my understanding and @bbatsov's confirmation, the completion style was made to fix flex completions in company. That was necessary because the complete-at-point function was not grabbing the prefix in a way that would be compatible with "flex-ish" competition styles.

And by the analysis made by @minad in #3006, the completion style by itself does not follow the specifications of a completion style. But somehow it appears to work with company.

The flex completion style has been on emacs for some years too.

[toniz4]

Another option is to fix the completion style making it compliant. But since I don't use it and testing anything involving completions in emacs is a PITA, I think I would let someone pickup that

[vemv]

Thanks for the summary!

A PR that marked all the now-redundant bits with comments would seem a good start.

In that PR itself we can iterate over the preferred soft/hard deprecation mechanism.

[bbatsov]

By my understanding and @bbatsov's confirmation, the completion style was made to fix flex completions in company. That was necessary because the complete-at-point function was not grabbing the prefix in a way that would be compatible with "flex-ish" competition styles.

Yeah, that is what I was referring to. We can keep around other company-specific code, but I'm not sure whether the old completion style adds any value. Down the road we can move the company logic outside of cider-completion.el (e.g. to cider-company.el), so we can load it only when someone actually uses company as a small additional optimization and to clarify better what's core completion logic vs what's specific to company.

Anyways, none of this is particularly important.

[toniz4]

Down the road we can move the company logic outside of cider-completion.el (e.g. to cider-company.el), so we can load it only when someone actually uses company as a small additional optimization and to clarify better what's core completion logic vs what's specific to company.

I think it's fine having the :company-* stuff there, corfu still uses some of those functions and maybe other completion frameworks use it too. Basically is a non standard standard.