bug involving eglot + corfu with clojure-lsp or rustic (likely others too)

[mpenet]

Hi,

First of all thanks for your work on this mode, it's really excellent.

I just updated to 1.14 and corfu popup-info feature broke in the process (everything works fine with 1.13). corfu works fine everywhere else, as soon as eglot is enabled and doc info is available it breaks.

I am not sure how to debug this, I tried a few things already and I cannot get a stacktrace so far.

You can follow the discussion that gives more details here : minad/corfu#314

The symptoms are that instead of getting the doc popup I see this in Messages

Error running timer ‘corfu-popupinfo--show’: (buffer-read-only #)

and I get a similar error with another feature from the same mode to get doc for a completion candidate:

Buffer is read-only: #<killed buffer>

The issue on corfu mentions a few ways to reproduce the issue. I would gladly try to help figure this out if I can be of any help.

Thanks

[joaotavora]

At a first sight, it seems that this is a timer that Corfu uses: corfu-popupinfo--show. You could step or use trace-values at various steps in that function.

[mpenet]

  corfu-popupinfo--show(#("cond" 0 1 (completion-score 0.75 face completions-common-part eglot--lsp-item (:label #("cond" 0 1 (eglot--lsp-item #8)) :kind 3 :detail "clojure.core/cond" :data (:unresolved [["documentation" (:uri "file:///clojure.core.clj" :name "cond" :ns "clojure.core")]]))) 1 3 (face completions-common-part) 3 4 (face completions-first-difference)))
  apply(corfu-popupinfo--show #("cond" 0 1 (completion-score 0.75 face completions-common-part eglot--lsp-item (:label #("cond" 0 1 (eglot--lsp-item #9)) :kind 3 :detail "clojure.core/cond" :data (:unresolved [["documentation" (:uri "file:///clojure.core.clj" :name "cond" :ns "clojure.core")]]))) 1 3 (face completions-common-part) 3 4 (face completions-first-difference)))
  timer-event-handler([t 25646 32164 891630 nil corfu-popupinfo--show (#("cond" 0 1 (completion-score 0.75 face completions-common-part eglot--lsp-item (:label #("cond" 0 1 (eglot--lsp-item #10)) :kind 3 :detail "clojure.core/cond" :data (:unresolved [...]))) 1 3 (face completions-common-part) 3 4 (face completions-first-difference))) nil 0 nil])

  (if corfu-popupinfo--timer (progn (cancel-timer corfu-popupinfo--timer) (setq corfu-popupinfo--timer nil)))
  corfu-popupinfo--show(#("cond" 0 1 (completion-score 0.75 face completions-common-part eglot--lsp-item (:label #("cond" 0 1 (eglot--lsp-item #8)) :kind 3 :detail "clojure.core/cond" :data (:unresolved [["documentation" (:uri "file:///clojure.core.clj" :name "cond" :ns "clojure.core")]]))) 1 3 (face completions-common-part) 3 4 (face completions-first-difference)))
  apply(corfu-popupinfo--show #("cond" 0 1 (completion-score 0.75 face completions-common-part eglot--lsp-item (:label #("cond" 0 1 (eglot--lsp-item #9)) :kind 3 :detail "clojure.core/cond" :data (:unresolved [["documentation" (:uri "file:///clojure.core.clj" :name "cond" :ns "clojure.core")]]))) 1 3 (face completions-common-part) 3 4 (face completions-first-difference)))
  timer-event-handler([t 25646 32164 891630 nil corfu-popupinfo--show (#("cond" 0 1 (completion-score 0.75 face completions-common-part eglot--lsp-item (:label #("cond" 0 1 (eglot--lsp-item #10)) :kind 3 :detail "clojure.core/cond" :data (:unresolved [...]))) 1 3 (face completions-common-part) 3 4 (face completions-first-difference))) nil 0 nil])

  (cancel-timer corfu-popupinfo--timer)
  (progn (cancel-timer corfu-popupinfo--timer) (setq corfu-popupinfo--timer nil))
  (if corfu-popupinfo--timer (progn (cancel-timer corfu-popupinfo--timer) (setq corfu-popupinfo--timer nil)))
  corfu-popupinfo--show(#("cond" 0 1 (completion-score 0.75 face completions-common-part eglot--lsp-item (:label #("cond" 0 1 (eglot--lsp-item #8)) :kind 3 :detail "clojure.core/cond" :data (:unresolved [["documentation" (:uri "file:///clojure.core.clj" :name "cond" :ns "clojure.core")]]))) 1 3 (face completions-common-part) 3 4 (face completions-first-difference)))
  apply(corfu-popupinfo--show #("cond" 0 1 (completion-score 0.75 face completions-common-part eglot--lsp-item (:label #("cond" 0 1 (eglot--lsp-item #9)) :kind 3 :detail "clojure.core/cond" :data (:unresolved [["documentation" (:uri "file:///clojure.core.clj" :name "cond" :ns "clojure.core")]]))) 1 3 (face completions-common-part) 3 4 (face completions-first-difference)))
  timer-event-handler([t 25646 32164 891630 nil corfu-popupinfo--show (#("cond" 0 1 (completion-score 0.75 face completions-common-part eglot--lsp-item (:label #("cond" 0 1 (eglot--lsp-item #10)) :kind 3 :detail "clojure.core/cond" :data (:unresolved [...]))) 1 3 (face completions-common-part) 3 4 (face completions-first-difference))) nil 0 nil])

  (progn (cancel-timer corfu-popupinfo--timer) (setq corfu-popupinfo--timer nil))
  (if corfu-popupinfo--timer (progn (cancel-timer corfu-popupinfo--timer) (setq corfu-popupinfo--timer nil)))
  corfu-popupinfo--show(#("cond" 0 1 (completion-score 0.75 face completions-common-part eglot--lsp-item (:label #("cond" 0 1 (eglot--lsp-item #8)) :kind 3 :detail "clojure.core/cond" :data (:unresolved [["documentation" (:uri "file:///clojure.core.clj" :name "cond" :ns "clojure.core")]]))) 1 3 (face completions-common-part) 3 4 (face completions-first-difference)))
  apply(corfu-popupinfo--show #("cond" 0 1 (completion-score 0.75 face completions-common-part eglot--lsp-item (:label #("cond" 0 1 (eglot--lsp-item #9)) :kind 3 :detail "clojure.core/cond" :data (:unresolved [["documentation" (:uri "file:///clojure.core.clj" :name "cond" :ns "clojure.core")]]))) 1 3 (face completions-common-part) 3 4 (face completions-first-difference)))
  timer-event-handler([t 25646 32164 891630 nil corfu-popupinfo--show (#("cond" 0 1 (completion-score 0.75 face completions-common-part eglot--lsp-item (:label #("cond" 0 1 (eglot--lsp-item #10)) :kind 3 :detail "clojure.core/cond" :data (:unresolved [...]))) 1 3 (face completions-common-part) 3 4 (face completions-first-difference))) nil 0 nil])

content of eglot buffer:

(:jsonrpc "2.0" :id 6 :method "completionItem/resolve" :params
          (:label
           #("cond" 0 1 ...)
           :kind 3 :detail "clojure.core/cond" :data
           (:unresolved
            [["documentation" ...]])))
[server-reply] (id:6) Thu Apr  6 10:20:44 2023:
(:jsonrpc "2.0" :id 6 :result
          (:label "cond" :kind 3 :detail "clojure.core/cond" :documentation
                  (:kind "markdown" :value "```clojure\nclojure.core/cond\n[& clauses]\n```\n\nTakes a set of test/expr pairs. It evaluates each test one at a\ntime.  If a test returns logical true, cond evaluates and returns\nthe value of the corresponding expr and doesn't evaluate any of the\nother tests or exprs. (cond) returns nil.\n\n__Examples:__\n\n```clojure\n(defn pos-neg-or-zero\n  \"Determines whether or not n is positive, negative, or zero\"\n  [n]\n  (cond\n    (< n 0) \"negative\"\n    (> n 0) \"positive\"\n    :else \"zero\"))\n\nuser=> (pos-neg-or-zero 5)\n\"positive\"\nuser=> (pos-neg-or-zero -1)\n\"negative\"\nuser=> (pos-neg-or-zero 0)\n\"zero\"\n\n```\n---\n```clojure\nuser=> (let [grade 85]\n         (cond\n           (>= grade 90) \"A\"\n           (>= grade 80) \"B\"\n           (>= grade 70) \"C\"\n           (>= grade 60) \"D\"\n           :else \"F\"))\n\"B\"\n```\n---\n```clojure\n;; See examples for \"if\" explaining Clojure's idea of logical true\n;; and logical false.\n```\n---\n```clojure\n;; Generates a random number compares it to user input\n(let [rnd (rand-int 10)\n      guess (Integer/parseInt (read-line))]\n  (cond\n    (= rnd guess) (println \"You got my guess right!\")\n    :else (println \"Sorry... guess again!\")))\n```\n---\n```clojure\n;; Simple Condition Example \n\n(defn test-x [x]\n	(cond\n		(< x 10) \"less than\"\n		(> x 20) \"greater than\"\n	)\n)\n\n============test============\n(test-x 9)\n\n=> \"less than\"\n```\n---\n```clojure\n(defn print-cond [xx] \n  (cond \n    (< xx 6) \"less than 6\"\n    (< xx 8) \"less than 8\"\n    :else \"Greater than 8\"))\n=> #'aurora.system/print-cond\n(print-cond 5)\n=> \"less than 6\"\n(print-cond 7)\n=> \"less than 8\"\n(print-cond 8)\n=> \"Greater than 8\"\n(print-cond 10)\n=> \"Greater than 8\"\n\n```\n---\n```clojure\n;; If a condition is not matched `nil` will be returned\n(cond\n  false \"sumfin\")\n;;=> nil\n```\n---\n```clojure\n;; Instead of use the :else keyword as the default path you can use \n;; whatever that gives meaning to the code\n\nuser=> (cond \n          :whatever \"You can use any default keyword\")\n;; You can use any default keyword\n```\n---\n```clojure\n;; cond is a macro the easiest way to understand how it works is to macroexpand it\nuser=> (clojure.walk/macroexpand-all\n       '(let [n 3] \n          (cond\n            (odd? n) \"odd\"\n            (even? n) \"even\"\n            (< n 0) \"negitive\"\n            (< 0 n) \"positive\"\n            :else \"zero\")))\n\n(let [n 3]\n (if (odd? n)\n  \"odd\"\n  (if (even? n)\n   \"even\"\n   (if (< n 0)\n    \"negitive\"\n    (if (< 0 n)\n     \"positive\"\n     (if :else\n      \"zero\"\n      nil))))))\n```\n\n__See also:__\n\n[clojure.core/condp](https://clojuredocs.org/clojure.core/condp)\n\n[clojure.core/case](https://clojuredocs.org/clojure.core/case)\n\n[clojure.core/if](https://clojuredocs.org/clojure.core/if)\n\n[clojure.core/cond->](https://clojuredocs.org/clojure.core/cond->)\n\n__Notes:__\n\nWe should add a comment in the docstring for the final usage of :else.\n---\n`:else` is not special. Any keyword will do.\n---\nActually any non-nil or true will work.  I prefer to use :default.  :else implies truthiness in evaluation rather what it really is which is a 'default' value.\n---\nIt seems the documentation is not accurate. From my experiment, it seems the function decscription should be like the following:\n\nTakes a set of test/expr pairs. It evaluates each test one at a time.  \nIf a test returns logical true, cond evaluates the corresponding expr and\ncontinue to evaluate the next test/expr, until a test fails or \nno more pair of test(:else)/expr to evaluate,\nand returns the last evaluation value of the expr \nthat its corresponding test evaluated to truthy.\nIf there is no test evaluated to truthy, \nreturn the evaluation of :else clause.\nIf there is no :else clause return nil.\n\nThe above description can be supported by the following example:\n\n<pre>\n(defn pos-neg-or-zero\n  \"Determines whether or not n is positive, negative, or zero\"\n  [n]\n  (cond\n    (odd? n) \"odd\"\n    (even? n) \"even\"\n    (< n 0) \"negitive\"\n    (< 0 n) \"positive\"\n    :else \"zero\"))\n\n(odd? 3)                                ; => true\n(pos-neg-or-zero 3)                     ;=> \"positive\"\n(even? 0)                               ; => true\n(pos-neg-or-zero 0)                     ;=> \"zero\"\n(even? 4)                               ; => true\n(pos-neg-or-zero 4)                     ;=> \"positive\"\n</>\n\n---\nyubrshen,   \n\n(pos-neg-or-zero 3) => \"odd\"\n---\n`:else` is just a convention. It works because\n\n<pre>\n=> (boolean :else)\ntrue\n</pre>\n\nIn fact anything except for `false` and `nil` will evaluate to `true` (see [boolean](https://clojuredocs.org/clojure.core/boolean)).\n\n----\n\n*[/Users/mpenet/.m2/repository/org/clojure/clojure/1.12.0-alpha1/clojure-1.12.0-alpha1.jar:clojure/core.clj](zipfile:///Users/mpenet/.m2/repository/org/clojure/clojure/1.12.0-alpha1/clojure-1.12.0-alpha1.jar::clojure/core.clj)*")))

[jdtsmith]

if it is so trivial then so should be adding an option to Corfu (and turning it on by default), that does that triviality

Because (AFAIK) only eglot's capf needs it, and for other capfs, enabling such an option would be a needless inefficiency that may undermine their own (potentially more clever) caching strategies. I agree that recommending this solution for eglot is a good approach. In my experience it works well, but you are no doubt far more familiar with various servers and corner cases, which is why I asked for your experience. If you have a chance to try sometime that would be helpful.

claims that vanilla completion at point also caches the inner table have not been confirmed

The claim was actually for completion-in-region-mode, which is easily confirmed. You can see where the collection table is saved in completion--in-region:

(setq completion-in-region--data
	      `(,(if (markerp start) start (copy-marker start))
                ,(copy-marker end t) ,collection ,predicate))

It doesn't get re-used there, but other builtin completion tools do reuse this saved collection table. For example, the built-in icomplete does this; see icomplete-exhibit and icomplete--completion-table (which is just (nth 2 completion-in-region--data)). It's fair to say such re-use is relatively rare (since icomplete-in-buffer doesn't see much use AFAIK), but it is there.

[joaotavora]

Because (AFAIK) only eglot's capf needs it,

That's narrow sighted, I'm afraid. SLY's capfs has the same problem, and so has SLiME's and so has CIDER's and a couple of my capfs i use at day job, and in general any capfs that uses an external, non-elisp-process-space source of completions.

for other capfs, enabling such an option would be a needless inefficiency that may undermine their own (potentially more clever) caching strategies.

Please start a new Emacs bug report and post and benchmarks for such capfs, comparing the two approaches side by side.

If the difference is indeed significant, then I don't see why the capf can't do that caching itself.

For example, the built-in icomplete does this; see icomplete-exhibit and icomplete--completion-table (which is just (nth 2 completion-in-region--data)). It's fair to say such re-use is relatively rare (since icomplete-in-buffer doesn't see much use AFAIK), but it is there.

You couldn't have found a more obscure example. It doesn't see much use because it's been broken until very very recently. And by your account, still is. But I maintain incomplete, so i can fix that.

[jdtsmith]

in general any capfs that uses an external, non-elisp-process-space source of completions

I don't have experience with all these you mention. But there is nothing general about such capfs I can envision that requires them to be used as a one-and-done sequence of calls. I have a CAPF that uses external process completion, and does its own caching, updating the completions as the input changes. In my case they are prefix-only completions, so aggressive caching is possible (if you've already completed foo, there is no need to recontact the process for foob); table-dumping is costly, and I have to explicitly work around it.

If the difference is indeed significant, then I don't see why the capf can't do that caching itself.

Of course it can, and should. But if it chooses to cache inside its collection table (which is quite natural), that will be defeated by frequent table-dumping. One could argue that that ship has sailed, given that company dumps the table so frequently anyway.

You couldn't have found a more obscure example.

Yes, which is why I mentioned that "such re-use is relatively rare". But the point is, it is an older builtin package which illustrates the apparent design of the 2-step CAPF.

Anyway, I did start by saying rehashing this particular issue wouldn't be productive... and should have followed my own advice! I'll leave it at "agreeing to disagree" :).

[joaotavora]

I don't have experience with all these you mention. But there is
nothing general about such capfs I can envision that requires them to be
used as a one-and-done sequence of calls.

Yes there is. In these capfs the full set of completions lives
elsewhere, not in Emacs's process memory. This theme becomes more and
more common as Emacs interacts with things beyond Elisp (which, it can
be argued, has the only table where such a non-dump strategy would make
sense -- more on that below).

In my case they are prefix-only completions, so aggressive caching is
possible

Your case is not the "general case". File name completion can't do
that, for example (you add a '/' and you might open up a whole new world
of completions). Neither can regexp completion. And even with prefix
completions many external systems simply cap the results to the first
50, 100, 150 because transmitting them "over the wire" is so slow. So
your aggresive caching is not possible (unless you plan on losing
completions).

One could argue that that ship has sailed, given that company dumps
the table so frequently anyway.

That ship never docked to being with. You're getting yourself and
everyone else wet trying to swim to it.

Yes, which is why I mentioned that "such re-use is relatively rare".
But the point is, it is an older builtin package which illustrates the
apparent design of the 2-step CAPF.

Not only rare, but non-existent until a few weeks ago. It probably
never really works and is simply buggy.

Anyway, I did start by saying rehashing this particular issue wouldn't
be productive... and should have followed my own advice! I'll leave it
at "agreeing to disagree" :).

This seems to be a recurring enough theme that it would be worth for you
and Daniel to share reproducible results of your research. So please
open the Emacs bug as I suggested, and in it include examples of these
capf's that Corfu so significantly enhances.

I've run some benchamrks with elisp-completion-at-point, and I see
absolutely no difference between the dump and non-dump strategies. I'd
be happy to discuss them.

[jdtsmith]

Yes there is. In these capfs the full set of completions lives elsewhere, not in Emacs's process memory.

This is certainly relevant for performance and caching issues. But it is not relevant to the question of whether a CAPF's table generates new completions when called with altered input. The only distinction we are mentioning is whether the UI calls the CAPF again (recontacting the server), or whether the table notices changed input and recontacts the server itself. The expensive server contact is (for something like eglot) equivalent. That's why I have mentioned cape-capf-buster as the "best we can do" in this type of case. Benchmarking should not be expected to reveal a difference.

Your case is not the "general case". File name completion can't do
that, for example (you add a '/' and you might open up a whole new world
of completions). Neither can regexp completion. And even with prefix
completions many external systems simply cap the results to the first
50, 100, 150 because transmitting them "over the wire" is so slow. So
your aggresive caching is not possible (unless you plan on losing
completions).

Sure, of course. I did not say this was a general case. But it is a perfectly sensible real-world case. One could indeed imagine a bizarre server whose completion results are stochastic and entirely unpredictable (b->penguin, ba->watermelon), in which case any form of caching is strongly counter-indicated. But "don't know anything about what the server will return" is also decidedly not the general case. One can imagine a full spectrum from "emacs can predict clearly the class of results it will get" to "emacs will know nothing at all about what results it might get". Different caching strategies can be employed along this continuum.

I've run some benchamrks with elisp-completion-at-point, and I see absolutely no difference between the dump and non-dump strategies

Well, yes (in memory and all). I fear I may not have clearly communicated, or you have not fully construed the main point. You can design ~equally performant capfs with either strategy. The main thing is agreeing on that strategy based on the API. The two strategies come with different costs:

1. Frequent table dumping: cannot reliably cache information in closures for "the duration of completion".
2. Table auto-updates: must keep track of input and regenerate completion appropriately when it changes.

If you have a collection of CAPFs that subscribe to strategy 1, they will not work well with UIs that expect strategy 2, and visa versa. This is our current situation, further research is not required to see it. If I give you an example of a CAPF that performs poorly with strategy 1, it could be re-written not to have this problem. And similarly for CAPFs which don't do well with strategy 2 (which we've already seen an example of by "rewriting" eglot's capf via a wrapper).

Having two somewhat incompatible standards is the main problem here. Sadly a single sentence in the manual could have cleared all this up, instead of leaving us to read the tea-leaves!

[mpenet]

I was curious and tried with company + company-quickhelp to compare, and I also get an error (always with clojure-lsp):

Error running timer ‘company-quickhelp--show’: (error "Company: backend company-capf error "Buffer is read-only: #" with args (doc-buffer concat)") [2 times]

I will try to provide a full repro.

[mpenet]

might be of interest, it goes away in corfu with the change suggested here minad/corfu#314 (comment)

I am not sure it's a proper fix or just some rubberband but that might give an hint of what's the issue.

Same with quickdoc line 182:

(defun company-quickhelp--show ()
  (company-quickhelp--cancel-timer)
  (when (and (company-quickhelp-pos-tip-available-p)
             company-selection)
    (while-no-input
      (let* ((selected (nth company-selection company-candidates))
             (doc (let ((inhibit-message t)
                        (inhibit-read-only t)) ; added this

[joaotavora]

Ok I have a (more) minimal repro now:

Alright. I can work with this. Will try.

[joaotavora]

Reproduced.

[joaotavora]

This is a bug in eglot.el .... and jsonrpc.el. I'll have more details in the commit that fixes it. Markdown's gfm-view-mode is a read-only-mode, and the modifications that eglot--format-markup does in that buffer have been "illegal" for a long time now. But those modifications weren't being flagged to me because jsonrpc.el blindly sets inhibit-read-only to t in the process filter, so everyone in that stack can do whatever they want to buffers.

After Eglot 1.14, a new caching mechanism came along that doesn't always contact the server, and it seems to be kicking in your case, which is really good. But that doc request doesn't come in from LSP and jsonrpc.el doesn't enter into the picture to hack in the inhibit-read-only. So you get the error.

It didn't happen in my simple tests because I didn't install markdown-mode. Pretty sure if I install it I will see the error in the minimal clangd recipe too.

I'll fix this in Emacs master, eglot.el and jsonrpc.el. In the meantime use some of these hacky workarounds. Maybe

(advice-add 'eglot--format-markup :around (lambda (orig &rest args) (let ((inhibit-read-only t)) (apply orig args))))

untested, but something like this should work.

[mpenet]

Thanks João for the explanation and your patience on that one.
That does indeed fix the issue for now. I ll keep an eye on the next commits/release for the proper fix.

[joaotavora]

Thank you for the error recipe that uncovered this complex bug ;)

[joaotavora]

JD Smith ***@***.***> writes:

This is certainly relevant for performance and caching issues. But it is not relevant to the question of whether a CAPF's table generates new completions when called with altered input.

It is. I've just told you that because of that, many external completion systems, not just LSP servers, cap the number of completions transmitted over the wire. So if the server has 10 completions for the prefix "foo" and only gives you 5, you _must_ re-contact it when you add another letter.

The only distinction we are mentioning is whether the UI calls the CAPF again (recontacting the server), or whether the table notices changed input and recontacts the server itself.

The table is completely oblivious to _where_ that input is being entered by the user. That's UI problem.

Benchmarking should not be expected to reveal a difference.

It was you who brought up "innefiency" earlier, as a reason Corfu behaves that way. So there should be some kind of gain, presumably. I asked to see numbers of that gain.

Sure, of course. I did not say this was a general case. But it is a perfectly sensible real-world case. One could indeed imagine a bizarre server whose completion results are stochastic and entirely unpredictable (b->penguin, ba->watermelon), in which case any form of caching is strongly counter-indicated. But "don't know anything about what the server will return" is also decidedly not the general case.

"general case" and "common case" are not the same thing. An uncommon/infrequent case (which this is not, but let's assume it is for a moement) is part of the "general case". You must code for the general case, not for the so-called "corner case".

I've run some benchamrks with elisp-completion-at-point, and I see absolutely no difference between the dump and non-dump strategies Well, yes (in memory and all). I fear I may not have clearly communicated, or you have not fully construed the main point. You can design ~equally performant capfs with either strategy. The main thing is agreeing on that strategy based on the API. The two strategies come with different costs:

It was the _same_ capf, not two different capfs. The same one. No difference between "dump" and "non-dump". I'm asking you to point me to some capfs, at least one, where the difference in strategy makes some kind of difference.

Having two somewhat incompatible standards is the main problem here. Sadly a single sentence in the manual could have cleared all this up, instead of leaving us to read the tea-leaves!

There are no tea leaves to read. Eglot and SLY backends existed way before Corfu was invented, and so did the M-x completion-at-point and Company. The only reason I can think of for inventing Corfu like this is for gaining some performance benefit somewhere, since I presume, functionally it would have been easy for Daniel to design it with what you call the "dump" strategy. I'm just asking to see some numbers.