GNU bug report logs - #78530
31.0.50; [PATCH] eglot-code-action-suggestion doesn't call the callback function in the original buffer

Previous Next

To reply to this bug, email your comments to 78530 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Yuan Fu <casouri <at> gmail.com>
To: Bug Report Emacs <bug-gnu-emacs <at> gnu.org>
Subject: 31.0.50; [PATCH] eglot-code-action-suggestion doesn't call the 
 callback function in the original buffer
Date: Wed, 21 May 2025 00:42:21 -0700

[Message part 1 (text/plain, inline)]

X-Debbugs-CC: joaotavora <at> gmail.com

Copying from the commit message:

Some minor modes adds their own eldoc display function to
'eldoc-display-functions' hook buffer-locally.  So when eldoc
uses 'eldoc-display-functions' to display docs, it should use
the buffer-local value of the hook.

In 'eldoc--invoke-strategy', the code that runs
'eldoc-display-functions' hook is wrapped in a callback function
that eventually gets passed to each documentation function in
'eldoc-documentation-functions'. So now it's the documentation
functions' responsibility to call the eldoc callback in the
original buffer.

All the eglot documentation functions indeed do that, using
'eglot--when-buffer-window' to switch to the original buffer when
calling the eldoc callback. Except for
'eglot-code-action-suggestion'.

This patch fixes that.

This bug was originally reported on eldoc-box [1]. The user
found that eldoc-box's display function are rarely used, while
the minor mode is turned on. This patch fixes the issue.

[1] https://github.com/casouri/eldoc-box/issues/126#issuecomment-2896611278

I think we should fix the eldoc callback function so it always uses the
hook value of the hook in the original buffer; at the very least we
should document the pitfall in the docstring of
eldoc-documentation-functions. João, WDYT?

Yuan

[eglot-code-action.patch (application/octet-stream, attachment)]

Message #8 received at 78530 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Yuan Fu <casouri <at> gmail.com>
Cc: 78530 <at> debbugs.gnu.org
Subject: Re: bug#78530: 31.0.50; [PATCH] eglot-code-action-suggestion doesn't
 call the callback function in the original buffer
Date: Wed, 21 May 2025 12:20:39 +0100

Good catch, good analysis and great commit message.

Just please rename the subject line of the commit message to
start with "Eglot: " if you can.  Makes it easier to identify.  For example:

Eglot: always call ElDoc callbacks in correct buffer

Then add a reference to this bug#78530 and push at will.

João

Message #11 received at 78530 <at> debbugs.gnu.org (full text, mbox):

From: Yuan Fu <casouri <at> gmail.com>
To: João Távora <joaotavora <at> gmail.com>
Cc: 78530 <at> debbugs.gnu.org
Subject: Re: bug#78530: 31.0.50; [PATCH] eglot-code-action-suggestion doesn't
 call the callback function in the original buffer
Date: Wed, 21 May 2025 22:48:15 -0700

> On May 21, 2025, at 4:20 AM, João Távora <joaotavora <at> gmail.com> wrote:
> 
> Good catch, good analysis and great commit message.
> 
> Just please rename the subject line of the commit message to
> start with "Eglot: " if you can.  Makes it easier to identify.  For example:
> 
> Eglot: always call ElDoc callbacks in correct buffer
> 
> Then add a reference to this bug#78530 and push at will.

Cool. And thanks for the commit title, I was struggling to come up with a good one :) I pushed the fix. But we should also think about fixing the root cause. For example, switching to the original buffer temporarily when running ‘eldoc-display-functions’, like what eglot documentation functions do already. WDYT?

Yuan

Message #14 received at 78530 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Yuan Fu <casouri <at> gmail.com>
Cc: 78530 <at> debbugs.gnu.org
Subject: Re: bug#78530: 31.0.50; [PATCH] eglot-code-action-suggestion doesn't
 call the callback function in the original buffer
Date: Thu, 22 May 2025 10:28:38 +0100

[Message part 1 (text/plain, inline)]

Ok,

To understand exactly what you mean, just show a patch to eldoc.el. Even
though I'm not directly maintaining it anymore lately, I can look at it.

João Távora

On Thu, May 22, 2025, 06:48 Yuan Fu <casouri <at> gmail.com> wrote:

>
>
> > On May 21, 2025, at 4:20 AM, João Távora <joaotavora <at> gmail.com> wrote:
> >
> > Good catch, good analysis and great commit message.
> >
> > Just please rename the subject line of the commit message to
> > start with "Eglot: " if you can.  Makes it easier to identify.  For
> example:
> >
> > Eglot: always call ElDoc callbacks in correct buffer
> >
> > Then add a reference to this bug#78530 and push at will.
>
> Cool. And thanks for the commit title, I was struggling to come up with a
> good one :) I pushed the fix. But we should also think about fixing the
> root cause. For example, switching to the original buffer temporarily when
> running ‘eldoc-display-functions’, like what eglot documentation functions
> do already. WDYT?
>
> Yuan

[Message part 2 (text/html, inline)]

Message #17 received at 78530 <at> debbugs.gnu.org (full text, mbox):

From: Yuan Fu <casouri <at> gmail.com>
To: João Távora <joaotavora <at> gmail.com>
Cc: 78530 <at> debbugs.gnu.org
Subject: Re: bug#78530: 31.0.50; [PATCH] eglot-code-action-suggestion doesn't
 call the callback function in the original buffer
Date: Thu, 22 May 2025 18:00:09 -0700

[Message part 1 (text/plain, inline)]

> On May 22, 2025, at 2:28 AM, João Távora <joaotavora <at> gmail.com> wrote:
> 
> Ok, 
> 
> To understand exactly what you mean, just show a patch to eldoc.el. Even though I'm not directly maintaining it anymore lately, I can look at it. 

Something like this. This way, documentation functions don’t need to worry about running the callback in the original buffer.

Yuan

[eldoc-invoke-strategy.patch (application/octet-stream, attachment)]

Message #20 received at 78530 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: joaotavora <at> gmail.com, Yuan Fu <casouri <at> gmail.com>
Cc: 78530 <at> debbugs.gnu.org
Subject: Re: bug#78530: 31.0.50;
 [PATCH] eglot-code-action-suggestion doesn't call the callback
 function in the original buffer
Date: Sat, 07 Jun 2025 11:08:51 +0300

Ping!  João, any comments or suggestions?

> Cc: 78530 <at> debbugs.gnu.org
> From: Yuan Fu <casouri <at> gmail.com>
> Date: Thu, 22 May 2025 18:00:09 -0700
> 
> > On May 22, 2025, at 2:28 AM, João Távora <joaotavora <at> gmail.com> wrote:
> > 
> > Ok, 
> > 
> > To understand exactly what you mean, just show a patch to eldoc.el. Even though I'm not directly maintaining it anymore lately, I can look at it. 
> 
> Something like this. This way, documentation functions don’t need to worry about running the callback in the original buffer.
> 
> Yuan
> 
> 
> From a8145feaee2491718d06f725da8624c251895e29 Mon Sep 17 00:00:00 2001
> From: Yuan Fu <casouri <at> gmail.com>
> Date: Thu, 22 May 2025 17:57:37 -0700
> Subject: [PATCH] Eldoc: Run eldoc-display-functions with original buffer
>  (bug#78530)
> 
> * lisp/emacs-lisp/eldoc.el (eldoc--invoke-strategy): Run the
> hook with the original buffer as the current buffer. This way
> we're certain that the buffer-local value of
> 'eldoc-display-functions' is used.
> ---
>  lisp/emacs-lisp/eldoc.el | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/lisp/emacs-lisp/eldoc.el b/lisp/emacs-lisp/eldoc.el
> index 9e193580106..3d8202c34fa 100644
> --- a/lisp/emacs-lisp/eldoc.el
> +++ b/lisp/emacs-lisp/eldoc.el
> @@ -896,7 +896,8 @@ eldoc--invoke-strategy
>           (want 0)
>           ;; The doc strings and corresponding options registered so
>           ;; far.
> -         (docs-registered '()))
> +         (docs-registered '())
> +         (orig-buffer (current-buffer)))
>      (cl-labels
>          ((register-doc
>            (pos string plist origin)
> @@ -905,12 +906,13 @@ eldoc--invoke-strategy
>                    docs-registered)))
>           (display-doc
>            ()
> -          (run-hook-with-args
> -           'eldoc-display-functions (mapcar #'cdr
> -                                            (setq docs-registered
> -                                                  (sort docs-registered
> -                                                        (lambda (a b) (< (car a) (car b))))))
> -           interactive))
> +          (with-current-buffer orig-buffer
> +            (run-hook-with-args
> +             'eldoc-display-functions (mapcar #'cdr
> +                                              (setq docs-registered
> +                                                    (sort docs-registered
> +                                                          (lambda (a b) (< (car a) (car b))))))
> +             interactive)))
>           (make-callback
>            (method origin)
>            (let ((pos (prog1 howmany (cl-incf howmany))))
> -- 
> 2.39.5 (Apple Git-154)
> 

Message #23 received at 78530 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78530 <at> debbugs.gnu.org, Yuan Fu <casouri <at> gmail.com>
Subject: Re: bug#78530: 31.0.50; [PATCH] eglot-code-action-suggestion doesn't
 call the callback function in the original buffer
Date: Mon, 9 Jun 2025 00:19:36 +0100

On Sat, Jun 7, 2025 at 9:08 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> Ping!  João, any comments or suggestions?

I think the patch is definitely in the right direction, but it would make
even more sense if it checked if the "original buffer" is still live by
the time the lambda is called.  If it's not, probably the callback
lambda shouldn't get called.  This is why Eglot uses its own macro
eglot--when-buffer-live.

And this new "rule" should be encoded in the documentation (docstring
and/or manual).  No need for lots of text, just a note.

And we should think a little bit if we're breaking backward
compatibility.  Though I don't think we are: not many ElDoc clients are
relying on the fact that the buffer the callbacks are run in is somewhat
arbitrary.

Finally the patch could come with some changes to Eglot as well, because
this allows for eglot.el to be simplified.  But be careful: the way to
do this is to do the eldoc.el change bumping its :core package version,
and then make the eglot.el change bumping its Package-requires: eldoc
version.  That way we guarantee that older Emacsen pulling in new eglot.el
also pull in the new suitable eldoc.el.

Thanks,
João

Message #26 received at 78530 <at> debbugs.gnu.org (full text, mbox):

From: Yuan Fu <casouri <at> gmail.com>
To: João Távora <joaotavora <at> gmail.com>
Cc: 78530 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#78530: 31.0.50; [PATCH] eglot-code-action-suggestion doesn't
 call the callback function in the original buffer
Date: Mon, 9 Jun 2025 18:30:15 -0700

> On Jun 8, 2025, at 4:19 PM, João Távora <joaotavora <at> gmail.com> wrote:
> 
> On Sat, Jun 7, 2025 at 9:08 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
>> 
>> Ping!  João, any comments or suggestions?
> 
> I think the patch is definitely in the right direction, but it would make
> even more sense if it checked if the "original buffer" is still live by
> the time the lambda is called.  If it's not, probably the callback
> lambda shouldn't get called.  This is why Eglot uses its own macro
> eglot--when-buffer-live.

Ohh yeah, good call.

> 
> And this new "rule" should be encoded in the documentation (docstring
> and/or manual).  No need for lots of text, just a note.

Will do.

> 
> And we should think a little bit if we're breaking backward
> compatibility.  Though I don't think we are: not many ElDoc clients are
> relying on the fact that the buffer the callbacks are run in is somewhat
> arbitrary.

Plus usually the callback _is_ ran in the original buffer, unless the provider of the documentation function does something weird. (In our case, eglot runs the callback in a temp buffer.)

> Finally the patch could come with some changes to Eglot as well, because
> this allows for eglot.el to be simplified.  But be careful: the way to
> do this is to do the eldoc.el change bumping its :core package version,
> and then make the eglot.el change bumping its Package-requires: eldoc
> version.  That way we guarantee that older Emacsen pulling in new eglot.el
> also pull in the new suitable eldoc.el.

Cool. Let me check if we really don’t need the eglot--when-buffer-window wrapper in the documentation functions. Or you’re certain that we don’t?

Yuan

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.