clojure-emacs / cider

The Clojure Interactive Development Environment that Rocks for Emacs
https://cider.mx
GNU General Public License v3.0
3.52k stars 643 forks source link

Completion can trigger unwanted user prompts #3699

Closed yuhan0 closed 1 month ago

yuhan0 commented 1 month ago

Expected behavior

Completion popups which display docstrings of the candidates should not trigger unexpected disambiguation prompts, which can interfere with regular input and navigation.

Actual behavior

Cider's company backend relies on the var-info op, which can fall through to cider--var-choice and trigger a completing-read prompt when there are multiple ambiguous candidates (usually related to Java interop calls). https://github.com/clojure-emacs/cider/blob/5f79b02fda70179349ba34a4fed1436708c669c3/cider-completion.el#L220

When used with corfu-mode, this results in unwanted prompts showing up while trying to navigate the candidate list:

corfu-popup

Steps to reproduce the problem

Additional Context

The company-box frontend has a workaround for this specific issue, essentially by overriding completing-read while it looks up documentation:

https://github.com/sebastiencs/company-box/blob/c4f2e243fba03c11e46b1600b124e036f2be7691/company-box-doc.el#L160

We could treat it as an upstream issue and ask Corfu to implement a similar workaround, but this is probably unintended behaviour on Cider's end. i.e. Calling cider-complete should not cause any user-facing effects besides returning a docstring-annotated list of candidates.

Environment & Version information

CIDER version information

;; CIDER 1.14.0-snapshot (package: 1.14.0-snapshot), nREPL 1.2.0-beta1
;; Clojure 1.12.0-alpha9, Java 21.0.2

Lein / Clojure CLI version

Clojure CLI version 1.11.3.1463

Emacs version

29.3

Operating system

macOS 14.5

JDK distribution

openjdk version "21.0.2" 2024-01-16 LTS

vemv commented 1 month ago

Thanks for the report!

This problem is solved in a Company branch.

(Worth noting that Corfu uses Company-specific properties, so it's the only API we have to care about)

You can track the whole discussion and the branches (company, cider) here: https://github.com/dgutov/robe/issues/144

I can push my branch again and then if you were interested you could drive it to the finish line.

...Unfortunately this year I don't quite have the availability ship a lot myself, especially for dense/delicate changes like this one.

yuhan0 commented 1 month ago

Ahh, that looks like quite a complicated change indeed.. I was thinking of a much less sophisticated fix on the part of the language backend implementations, something like an informal API contract: "Ensure that the function passed to :company-doc-buffer has no side-effects".

Don't think I'd personally have the time / interest to tackle the changes in the linked discussion, but thanks for the context :)

(I treat these popup docs as nice-to-have extras anyway, so having it display incomplete info isn't really a big deal when the candidates are ambiguous - aka. what the company-box workaround does by simply selecting the first candidate)

vemv commented 1 month ago

Yeah well, for the feature-complete state of things that we should aim for, sophistication is a necessary part of the formula.

The prompt has always been there and it makes even more sense with enrich-classpath and other cider-nrepl, Compliment improvements over the last year.

The bright side is that the resulting UI can be very lightweight and intuitive, e.g. just use all 4 arrows, that's it.

yuhan0 commented 1 month ago

Alright, in the meantime I hacked together a fix locally: https://github.com/yuhan0/cider/tree/bypass-prompt

Probably doesn't belong in the main repo if a more comprehensive solution is on the way, feel free to close this issue :)

vemv commented 1 month ago

Alright! Happy that we left some written trail.