eldoc recursion error

eldoc recursion error

[Matt Price]

[-- Attachment #1: Type: text/plain, Size: 339 bytes --]

I know there have been a couple of updates to org-eldoc lately.  After
updating to current master, I get this error in source blocks if eldoc mode
is turned on:

eldoc error: (error Lisp nesting exceeds ‘max-lisp-eval-depth’)

Is there an easy fix for this? is it a generic eldoc problem or specific to
org?

Thanks!

Matt

[-- Attachment #2: Type: text/html, Size: 461 bytes --]

Re: eldoc recursion error

[Bastien]

Hi Matt,

can you provide a recipe to reproduce the problem?

Thanks,

-- 
 Bastien

Re: eldoc recursion error

[Matt Price]

[-- Attachment #1: Type: text/plain, Size: 295 bytes --]

On Tue, Sep 8, 2020 at 10:25 AM Bastien <bzg@gnu.org> wrote:

> Hi Matt,
>
> can you provide a recipe to reproduce the problem?
>
> oops, sorry, that was stupid.

In a new org file, add these lines:

#+begin_src python
print
#+end_src

position cursor inside block and the error message occurs.

[-- Attachment #2: Type: text/html, Size: 723 bytes --]

Re: eldoc recursion error

[Bastien]

Hi Matt,

Matt Price <moptop99@gmail.com> writes:

> In a new org file, add these lines:
>
> #+begin_src python
> print
> #+end_src
>
> position cursor inside block and the error message occurs.

I can't reproduce the bug.  What version of Emacs are you using?

Can you give a recipe starting with emacs -q?

Thanks!

-- 
 Bastien

Re: eldoc recursion error

[Matt Price]

[-- Attachment #1: Type: text/plain, Size: 780 bytes --]

On Tue, Sep 8, 2020 at 10:53 AM Bastien <bzg@gnu.org> wrote:

> Hi Matt,
>
> Matt Price <moptop99@gmail.com> writes:
>
> > In a new org file, add these lines:
> >
> > #+begin_src python
> > print
> > #+end_src
> >
> > position cursor inside block and the error message occurs.
>
> I can't reproduce the bug.  What version of Emacs are you using?
>
> Can you give a recipe starting with emacs -q?
>

I htink I have it now.

$ emacs -q

(require 'org-eldoc) ;;(or navigate to lisp/org-eldoc.el and eval-buffer)
create new org file, add this content:

#+begin_src python
 print
 #+end_src

ensure eldoc-mode is turned on. place cursor inside block.

If for some reason that doesn' twork try manually running `org-eldoc-load`
and repositioning cursor.


> Thanks!
>
> --
>  Bastien
>

[-- Attachment #2: Type: text/html, Size: 1566 bytes --]

Re: eldoc recursion error

[Matt Price]

[-- Attachment #1: Type: text/plain, Size: 1005 bytes --]

On Tue, Sep 8, 2020 at 11:27 AM Matt Price <moptop99@gmail.com> wrote:

>
>
> On Tue, Sep 8, 2020 at 10:53 AM Bastien <bzg@gnu.org> wrote:
>
>> Hi Matt,
>>
>> Matt Price <moptop99@gmail.com> writes:
>>
>> > In a new org file, add these lines:
>> >
>> > #+begin_src python
>> > print
>> > #+end_src
>> >
>> > position cursor inside block and the error message occurs.
>>
>> I can't reproduce the bug.  What version of Emacs are you using?
>>
>> Can you give a recipe starting with emacs -q?
>>
>
> I htink I have it now.
>
> $ emacs -q
>
I forgot to say, this is emacs git (2.8.0.50 from yesterday) and org-mode
master (current)

>
> (require 'org-eldoc) ;;(or navigate to lisp/org-eldoc.el and eval-buffer)
> create new org file, add this content:
>
> #+begin_src python
>  print
>  #+end_src
>
> ensure eldoc-mode is turned on. place cursor inside block.
>
> If for some reason that doesn' t work try manually running
> `org-eldoc-load` and repositioning cursor.
>
>
>> Thanks!
>>
>> --
>>  Bastien
>>
>

[-- Attachment #2: Type: text/html, Size: 2252 bytes --]

Re: eldoc recursion error

[Kyle Meyer]

Matt Price writes:

> On Tue, Sep 8, 2020 at 11:27 AM Matt Price <moptop99@gmail.com> wrote:

>> On Tue, Sep 8, 2020 at 10:53 AM Bastien <bzg@gnu.org> wrote:
>>
>>> Matt Price <moptop99@gmail.com> writes:
>>>
>>> > In a new org file, add these lines:
>>> >
>>> > #+begin_src python
>>> > print
>>> > #+end_src
>>> >
>>> > position cursor inside block and the error message occurs.
>>>
>>> I can't reproduce the bug.  What version of Emacs are you using?
>>>
>>> Can you give a recipe starting with emacs -q?
>>
>> I htink I have it now.
>>
>> $ emacs -q
>>
> I forgot to say, this is emacs git (2.8.0.50 from yesterday) and org-mode
> master (current)

I can trigger it with Emacs's master.  Rather than being an issue with
the two recent compatibility kludges (47f26b1e7 and b2b587387), I think
this comes down to an additional (and hopefully last) spot,
org-eldoc-get-mode-local-documentation-function, needing to be adjusted,
(as suggested in <https://orgmode.org/list/87v9hlak7y.fsf@kyleam.com>).

Taking a quick look, I think it will need to be a bit more involved than
the other kludges.

Re: eldoc recursion error

[James N V Cash]

[-- Attachment #1: Type: text/plain, Size: 210 bytes --]


I've attached a patch that addresses the recursion issue with Emacs 28
and shows eldoc properly with example python. It presumably should act
the same with older versions of Emacs, although I haven't tested.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Org eldoc recursion patch --]
[-- Type: text/x-diff, Size: 819 bytes --]

diff --git a/contrib/lisp/org-eldoc.el b/contrib/lisp/org-eldoc.el
index 3b0999340..ccc23b523 100644
--- a/contrib/lisp/org-eldoc.el
+++ b/contrib/lisp/org-eldoc.el
@@ -116,9 +116,12 @@
         (when (fboundp mode-func)
           (with-temp-buffer
             (funcall mode-func)
-            (setq doc-func (and eldoc-documentation-function
-                                (symbol-value 'eldoc-documentation-function)))
-            (puthash lang doc-func org-eldoc-local-functions-cache))
+	    (setq doc-func (if (boundp 'eldoc-documentation-functions)
+			       (car eldoc-documentation-functions)
+			       (and eldoc-documentation-function
+				    (symbol-value 'eldoc-documentation-function))))
+
+	    (puthash lang doc-func org-eldoc-local-functions-cache))
           doc-func)
       cached-func)))
 

Re: eldoc recursion error

[Kyle Meyer]

James N V Cash writes:

> I've attached a patch that addresses the recursion issue with Emacs 28
> and shows eldoc properly with example python. It presumably should act
> the same with older versions of Emacs, although I haven't tested.

Thanks for the patch!  For information about the expected commit message
format, please see <https://orgmode.org/worg/org-contribute.html>.

> diff --git a/contrib/lisp/org-eldoc.el b/contrib/lisp/org-eldoc.el
> index 3b0999340..ccc23b523 100644
> --- a/contrib/lisp/org-eldoc.el
> +++ b/contrib/lisp/org-eldoc.el
> @@ -116,9 +116,12 @@
>          (when (fboundp mode-func)
>            (with-temp-buffer
>              (funcall mode-func)
> -            (setq doc-func (and eldoc-documentation-function
> -                                (symbol-value 'eldoc-documentation-function)))
> -            (puthash lang doc-func org-eldoc-local-functions-cache))
> +	    (setq doc-func (if (boundp 'eldoc-documentation-functions)
> +			       (car eldoc-documentation-functions)
> +			       (and eldoc-documentation-function
> +				    (symbol-value 'eldoc-documentation-function))))
> +
> +	    (puthash lang doc-func org-eldoc-local-functions-cache))
>            doc-func)
>        cached-func)))

Okay, so when eldoc-documentation-functions is defined (Emacs >=28), we
take the first function and go with it.  That might not be exactly what
you'd see in the native buffer, depending on whether there are other
members of eldoc-documentation-functions and how they interact.  (I'm
being vague, because I don't really know anything about eldoc, but it
seems like that must be the case.)  Anyway, I'd guess it will be good
enough in most cases, and it's certainly better than the recursion
error.

However, the docstring of eldoc-documentation-functions says

    Each hook function is called with at least one argument CALLBACK,
    a function, and decides whether to display a doc short string
    about the context around point.

But downstream in org-eldoc-documentation-function, the function is
called with no arguments:

    (t (let ((doc-fun (org-eldoc-get-mode-local-documentation-function lang)))
         (when (functionp doc-fun) (funcall doc-fun))))

That happens to work with python-eldoc-function, whose signature is
(&rest _ignored), but it may error with other valid eldoc functions.
So, at the very least, I think we should wrap the call in a
condition-case with a wrong-number-of-arguments handler.

Thoughts?

Re: eldoc recursion error

[James N. V. Cash]

[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]


Kyle Meyer <kyle@kyleam.com> writes:
> Thanks for the patch!  For information about the expected commit message
> format, please see <https://orgmode.org/worg/org-contribute.html>.

Ah will do, thanks! I'm still learning how to work with the email &
patch-based workflow.

> Okay, so when eldoc-documentation-functions is defined (Emacs >=28), we
> take the first function and go with it.  That might not be exactly what
> you'd see in the native buffer, depending on whether there are other
> members of eldoc-documentation-functions and how they interact.  (I'm
> being vague, because I don't really know anything about eldoc, but it
> seems like that must be the case.)  Anyway, I'd guess it will be good
> enough in most cases, and it's certainly better than the recursion
> error.

Ah yes, very true. I've attached another patch, which tries to better
preserve how the new eldoc strategy works, by passing through the
callback to the mode-local eldoc function if available, which will be a
closure over the configured documentation strategy with
eldoc-documentation-functions bound to the appropriate mode-local value.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Revised org eldoc patch --]
[-- Type: text/x-diff, Size: 2520 bytes --]

From 4c2042c9e3820d88cd655cf01d572b9686a40d31 Mon Sep 17 00:00:00 2001
From: "James N. V. Cash" <james.nvc@gmail.com>
Date: Thu, 17 Sep 2020 10:51:13 -0400
Subject: [PATCH] Address org-eldoc-recursion issue

* org-eldoc.el (org-eldoc-get-mode-local-documentation-function,
org-eldoc-documentation-function): Support Emacs 28-style eldoc, where
instead of a single function, the eldoc-documentation-functions hook
has a list of functions, which may optionally take a callback.
---
 contrib/lisp/org-eldoc.el | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/contrib/lisp/org-eldoc.el b/contrib/lisp/org-eldoc.el
index 3b0999340..85e238e64 100644
--- a/contrib/lisp/org-eldoc.el
+++ b/contrib/lisp/org-eldoc.el
@@ -116,8 +116,13 @@
         (when (fboundp mode-func)
           (with-temp-buffer
             (funcall mode-func)
-            (setq doc-func (and eldoc-documentation-function
-                                (symbol-value 'eldoc-documentation-function)))
+	    (setq doc-func (if (boundp 'eldoc-documentation-functions)
+			       (lexical-let ((doc-funs (symbol-value 'eldoc-documentation-functions)))
+				 (lambda ()
+				   (let ((eldoc-documentation-functions doc-funs))
+				     (funcall eldoc-documentation-strategy))))
+			       (and eldoc-documentation-function
+				    (symbol-value 'eldoc-documentation-function))))
             (puthash lang doc-func org-eldoc-local-functions-cache))
           doc-func)
       cached-func)))
@@ -127,7 +132,7 @@
 (declare-function php-eldoc-function "php-eldoc" ())
 (declare-function go-eldoc--documentation-function "go-eldoc" ())
 
-(defun org-eldoc-documentation-function (&rest _ignored)
+(defun org-eldoc-documentation-function (&optional callback)
   "Return breadcrumbs when on a headline, args for src block header-line,
   calls other documentation functions depending on lang when inside src body."
   (or
@@ -161,7 +166,12 @@
              (string= lang "golang")) (when (require 'go-eldoc nil t)
                                         (go-eldoc--documentation-function)))
            (t (let ((doc-fun (org-eldoc-get-mode-local-documentation-function lang)))
-                (when (functionp doc-fun) (funcall doc-fun))))))))
+                (when (functionp doc-fun)
+		  (if (functionp callback)
+		      (condition-case nil
+			  (funcall doc-fun callback)
+			(wrong-number-of-arguments (funcall doc-fun)))
+		    (funcall doc-fun)))))))))
 
 ;;;###autoload
 (defun org-eldoc-load ()
-- 
2.25.1

Re: eldoc recursion error

[Kyle Meyer]

James N. V. Cash writes:

> Kyle Meyer <kyle@kyleam.com> writes:

>> Okay, so when eldoc-documentation-functions is defined (Emacs >=28), we
>> take the first function and go with it.  That might not be exactly what
>> you'd see in the native buffer, depending on whether there are other
>> members of eldoc-documentation-functions and how they interact.  (I'm
>> being vague, because I don't really know anything about eldoc, but it
>> seems like that must be the case.)  Anyway, I'd guess it will be good
>> enough in most cases, and it's certainly better than the recursion
>> error.
>
> Ah yes, very true. I've attached another patch, which tries to better
> preserve how the new eldoc strategy works, by passing through the
> callback to the mode-local eldoc function if available, which will be a
> closure over the configured documentation strategy with
> eldoc-documentation-functions bound to the appropriate mode-local value.

Thanks, sounds good.

> Subject: [PATCH] Address org-eldoc-recursion issue
[...]
> @@ -116,8 +116,13 @@
>          (when (fboundp mode-func)
>            (with-temp-buffer
>              (funcall mode-func)
> -            (setq doc-func (and eldoc-documentation-function
> -                                (symbol-value 'eldoc-documentation-function)))
> +	    (setq doc-func (if (boundp 'eldoc-documentation-functions)
> +			       (lexical-let ((doc-funs (symbol-value 'eldoc-documentation-functions)))

Using lexical-let here is problematic because it's obsolete since Emacs
24.  Taking a quick glance, I don't see any issues with switching this
file over to lexical binding by adding " -*- lexical-binding: t; -*-" to
the first line.

Also, why use

    (doc-funs (symbol-value 'eldoc-documentation-functions))

rather than

    (doc-funs eldoc-documentation-functions)

?

> +				 (lambda ()
> +				   (let ((eldoc-documentation-functions doc-funs))
> +				     (funcall eldoc-documentation-strategy))))
> +			       (and eldoc-documentation-function
> +				    (symbol-value 'eldoc-documentation-function))))

nit: Please switch this to the Elisp style of indenting the `else' arm
less than the `then' arm.

>              (puthash lang doc-func org-eldoc-local-functions-cache))
>            doc-func)
>        cached-func)))
> @@ -127,7 +132,7 @@
>  (declare-function php-eldoc-function "php-eldoc" ())
>  (declare-function go-eldoc--documentation-function "go-eldoc" ())
>  
> -(defun org-eldoc-documentation-function (&rest _ignored)
> +(defun org-eldoc-documentation-function (&optional callback)

Perhaps even with the callback parameter the &rest should stay around.
The docstring of eldoc-documentation-functions makes me nervous because
it says "each hook function is called with _at least_ one argument" (my
emphasis).

>    "Return breadcrumbs when on a headline, args for src block header-line,
>    calls other documentation functions depending on lang when inside src body."
>    (or
> @@ -161,7 +166,12 @@
>               (string= lang "golang")) (when (require 'go-eldoc nil t)
>                                          (go-eldoc--documentation-function)))
>             (t (let ((doc-fun (org-eldoc-get-mode-local-documentation-function lang)))
> -                (when (functionp doc-fun) (funcall doc-fun))))))))
> +                (when (functionp doc-fun)
> +		  (if (functionp callback)
> +		      (condition-case nil
> +			  (funcall doc-fun callback)
> +			(wrong-number-of-arguments (funcall doc-fun)))
> +		    (funcall doc-fun)))))))))

Hmm, I think the more complete approach you put together for
org-eldoc-get-mode-local-documentation-function, along with your change
to consider the callback parameter here, means we don't need to bother
with the condition-case/wrong-number-of-arguments dance.  The callback
alone should be a reliable indication we're on Emacs 28, in which case
we can expect the function to accept a callback argument (even if they
ignore it like python-eldoc-function does).

Thanks again for working on this.

Re: eldoc recursion error

[James N. V. Cash]

Kyle Meyer <kyle@kyleam.com> writes:

> Using lexical-let here is problematic because it's obsolete since Emacs
> 24.  Taking a quick glance, I don't see any issues with switching this
> file over to lexical binding by adding " -*- lexical-binding: t; -*-" to
> the first line.

Cool, will do. I was concerned that the lexical-binding stanza was
missing for a reason, but I'll give it a shot.

> Also, why use
>
>     (doc-funs (symbol-value 'eldoc-documentation-functions))
>
> rather than
>
>     (doc-funs eldoc-documentation-functions)
>
> ?

Good question...I was doing that just because the original code does
~(symbol-value 'eldoc-documentation-function)~ and I assumed there was
some reason I didn't understand for doing so.

>> +         (lambda ()
>> +           (let ((eldoc-documentation-functions doc-funs))
>> +             (funcall eldoc-documentation-strategy))))
>> +             (and eldoc-documentation-function
>> +            (symbol-value 'eldoc-documentation-function))))
>
> nit: Please switch this to the Elisp style of indenting the `else' arm
> less than the `then' arm.

Oops, will do.

>> -(defun org-eldoc-documentation-function (&rest _ignored)
>> +(defun org-eldoc-documentation-function (&optional callback)
>
> Perhaps even with the callback parameter the &rest should stay around.
> The docstring of eldoc-documentation-functions makes me nervous because
> it says "each hook function is called with _at least_ one argument" (my
> emphasis).

Good call, will do.

> Hmm, I think the more complete approach you put together for
> org-eldoc-get-mode-local-documentation-function, along with your change
> to consider the callback parameter here, means we don't need to bother
> with the condition-case/wrong-number-of-arguments dance.  The callback
> alone should be a reliable indication we're on Emacs 28, in which case
> we can expect the function to accept a callback argument (even if they
> ignore it like python-eldoc-function does).

I was just thinking more about this, and I'm concerned I might need to
change things around a little bit more. The closure that
org-eldoc-get-mode-local-documentation-function now returns under Emacs
28 doesn't take any arguments and instead lets the
eldoc-documentation-strategy function create its own new callback.

I think the current approach will appear to work when the documentation
function returns a value directly, but if it uses the callback, then I
think that it won't, since it invokes a new, separate callback.

I will take another crack at addressing this and send another patch shortly.

> Thanks again for working on this.

My pleasure!

Re: eldoc recursion error

[James N. V. Cash]

[-- Attachment #1: Type: text/plain, Size: 943 bytes --]

James N. V. Cash <james.cash@occasionallycogent.com> writes:

> I was just thinking more about this, and I'm concerned I might need to
> change things around a little bit more. The closure that
> org-eldoc-get-mode-local-documentation-function now returns under Emacs
> 28 doesn't take any arguments and instead lets the
> eldoc-documentation-strategy function create its own new callback.
>
> I think the current approach will appear to work when the documentation
> function returns a value directly, but if it uses the callback, then I
> think that it won't, since it invokes a new, separate callback.
>
> I will take another crack at addressing this and send another patch shortly.

Indeed, testing with an eldoc backend (a personally hacked-up version of
clojure's cider) that actually uses the callback, it wasn't working with
the previous approach. I've attached a patch that addresses the
previously-mentioned issues as well as this.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: org eldoc recursion patch --]
[-- Type: text/x-diff, Size: 3051 bytes --]

From 028675e460d12711ba4b91bf576b35e307a0e640 Mon Sep 17 00:00:00 2001
From: "James N. V. Cash" <james.nvc@gmail.com>
Date: Thu, 17 Sep 2020 10:51:13 -0400
Subject: [PATCH] Address org-eldoc-recursion issue

* org-eldoc.el (org-eldoc-get-mode-local-documentation-function,
org-eldoc-documentation-function): Support Emacs 28-style eldoc, where
instead of a single function, the eldoc-documentation-functions hook
has a list of functions, which may optionally take a callback.
---
 contrib/lisp/org-eldoc.el | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/contrib/lisp/org-eldoc.el b/contrib/lisp/org-eldoc.el
index 3b0999340..b86ad1f39 100644
--- a/contrib/lisp/org-eldoc.el
+++ b/contrib/lisp/org-eldoc.el
@@ -1,4 +1,4 @@
-;;; org-eldoc.el --- display org header and src block info using eldoc
+;;; org-eldoc.el --- display org header and src block info using eldoc -*- lexical-binding: t; -*-
 
 ;; Copyright (c) 2014-2020 Free Software Foundation, Inc.
 
@@ -114,11 +114,17 @@
         doc-func)
     (if (eq 'empty cached-func)
         (when (fboundp mode-func)
-          (with-temp-buffer
-            (funcall mode-func)
-            (setq doc-func (and eldoc-documentation-function
-                                (symbol-value 'eldoc-documentation-function)))
-            (puthash lang doc-func org-eldoc-local-functions-cache))
+	  (with-temp-buffer
+	    (funcall mode-func)
+	    (setq doc-func (if (boundp 'eldoc-documentation-functions)
+			       (let ((doc-funs eldoc-documentation-functions))
+				 (lambda (callback)
+				   (let ((eldoc-documentation-functions doc-funs)
+					 (eldoc--make-callback (lambda (_ignored) callback)))
+				     (funcall eldoc-documentation-strategy))))
+			     (and eldoc-documentation-function
+				  (symbol-value 'eldoc-documentation-function))))
+	    (puthash lang doc-func org-eldoc-local-functions-cache))
           doc-func)
       cached-func)))
 
@@ -127,7 +133,7 @@
 (declare-function php-eldoc-function "php-eldoc" ())
 (declare-function go-eldoc--documentation-function "go-eldoc" ())
 
-(defun org-eldoc-documentation-function (&rest _ignored)
+(defun org-eldoc-documentation-function (&rest args)
   "Return breadcrumbs when on a headline, args for src block header-line,
   calls other documentation functions depending on lang when inside src body."
   (or
@@ -160,8 +166,12 @@
              (string= lang "go")
              (string= lang "golang")) (when (require 'go-eldoc nil t)
                                         (go-eldoc--documentation-function)))
-           (t (let ((doc-fun (org-eldoc-get-mode-local-documentation-function lang)))
-                (when (functionp doc-fun) (funcall doc-fun))))))))
+           (t (let ((doc-fun (org-eldoc-get-mode-local-documentation-function lang))
+		    (callback (car args)))
+                (when (functionp doc-fun)
+		  (if (functionp callback)
+		      (funcall doc-fun callback)
+		    (funcall doc-fun)))))))))
 
 ;;;###autoload
 (defun org-eldoc-load ()
-- 
2.25.1

Re: eldoc recursion error

[Kyle Meyer]

James N. V. Cash writes:

> Indeed, testing with an eldoc backend (a personally hacked-up version of
> clojure's cider) that actually uses the callback, it wasn't working with
> the previous approach. I've attached a patch that addresses the
> previously-mentioned issues as well as this.
[...]
> @@ -114,11 +114,17 @@
>          doc-func)
>      (if (eq 'empty cached-func)
>          (when (fboundp mode-func)
> -          (with-temp-buffer
> -            (funcall mode-func)
> -            (setq doc-func (and eldoc-documentation-function
> -                                (symbol-value 'eldoc-documentation-function)))
> -            (puthash lang doc-func org-eldoc-local-functions-cache))
> +	  (with-temp-buffer
> +	    (funcall mode-func)
> +	    (setq doc-func (if (boundp 'eldoc-documentation-functions)
> +			       (let ((doc-funs eldoc-documentation-functions))
> +				 (lambda (callback)
> +				   (let ((eldoc-documentation-functions doc-funs)
> +					 (eldoc--make-callback (lambda (_ignored) callback)))
> +				     (funcall eldoc-documentation-strategy))))
> +			     (and eldoc-documentation-function
> +				  (symbol-value 'eldoc-documentation-function))))
> +	    (puthash lang doc-func org-eldoc-local-functions-cache))

All right, so we can't get by without using eldoc--make-callback here?
Relying on a symbol marked with "--" makes me uneasy, and I'd like to
avoid it if possible.  Does your cider test case above break if we use
eldoc-print-current-symbol-info without relaying the callback?  That is,
this squashed into your patch:

diff --git a/contrib/lisp/org-eldoc.el b/contrib/lisp/org-eldoc.el
index b86ad1f39..06dcd6fe2 100644
--- a/contrib/lisp/org-eldoc.el
+++ b/contrib/lisp/org-eldoc.el
@@ -118,10 +118,9 @@ (defun org-eldoc-get-mode-local-documentation-function (lang)
 	    (funcall mode-func)
 	    (setq doc-func (if (boundp 'eldoc-documentation-functions)
 			       (let ((doc-funs eldoc-documentation-functions))
-				 (lambda (callback)
-				   (let ((eldoc-documentation-functions doc-funs)
-					 (eldoc--make-callback (lambda (_ignored) callback)))
-				     (funcall eldoc-documentation-strategy))))
+				 (lambda ()
+				   (let ((eldoc-documentation-functions doc-funs))
+				     (eldoc-print-current-symbol-info))))
 			     (and eldoc-documentation-function
 				  (symbol-value 'eldoc-documentation-function))))
 	    (puthash lang doc-func org-eldoc-local-functions-cache))
@@ -133,7 +132,7 @@ (declare-function css-eldoc-function "css-eldoc" ())
 (declare-function php-eldoc-function "php-eldoc" ())
 (declare-function go-eldoc--documentation-function "go-eldoc" ())
 
-(defun org-eldoc-documentation-function (&rest args)
+(defun org-eldoc-documentation-function (&rest _ignored)
   "Return breadcrumbs when on a headline, args for src block header-line,
   calls other documentation functions depending on lang when inside src body."
   (or
@@ -166,12 +165,9 @@ (defun org-eldoc-documentation-function (&rest args)
              (string= lang "go")
              (string= lang "golang")) (when (require 'go-eldoc nil t)
                                         (go-eldoc--documentation-function)))
-           (t (let ((doc-fun (org-eldoc-get-mode-local-documentation-function lang))
-		    (callback (car args)))
+           (t (let ((doc-fun (org-eldoc-get-mode-local-documentation-function lang)))
                 (when (functionp doc-fun)
-		  (if (functionp callback)
-		      (funcall doc-fun callback)
-		    (funcall doc-fun)))))))))
+		  (funcall doc-fun))))))))
 
 ;;;###autoload
 (defun org-eldoc-load ()

Re: eldoc recursion error

[James N. V. Cash]

[-- Attachment #1: Type: text/plain, Size: 1275 bytes --]

Kyle Meyer <kyle@kyleam.com> writes:

> All right, so we can't get by without using eldoc--make-callback here?
> Relying on a symbol marked with "--" makes me uneasy, and I'd like to
> avoid it if possible.

Yeah, good point. I was doing that so I could honour the setting of
eldoc-documentation-strategy. I don't know if that's going to be a
concern in practice though.

> Does your cider test case above break if we use
> eldoc-print-current-symbol-info without relaying the callback?  That is,
> this squashed into your patch:

My concern with using the eldoc-print-current-symbol-info is that it's
now somewhat subverting the actual eldoc documentation function -- i.e.
the invocation of org-eldoc-documentation-function now "fails" and
instead it prints out the actual documentation as a side-effect. Indeed,
applying that patch makes the eldoc for python code blocks not work
correctly.

The below patch which essentially just inlines the definition of
eldoc-documentation-default, so it's not messing around with any private
variables in eldoc, although it now won't honour the documentation
strategy. It remains to be seen if that will be an issue in practice,
but if necessary we could just check the value of
eldoc-documentation-strategy and behave appropriately.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: org eldoc recursion patch --]
[-- Type: text/x-diff, Size: 3051 bytes --]

From 7d59ecadbea429626bae90464d76f01b60c8d67f Mon Sep 17 00:00:00 2001
From: "James N. V. Cash" <james.nvc@gmail.com>
Date: Thu, 17 Sep 2020 10:51:13 -0400
Subject: [PATCH] Address org-eldoc-recursion issue

* org-eldoc.el (org-eldoc-get-mode-local-documentation-function,
org-eldoc-documentation-function): Support Emacs 28-style eldoc, where
instead of a single function, the eldoc-documentation-functions hook
has a list of functions, which may optionally take a callback.
---
 contrib/lisp/org-eldoc.el | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/contrib/lisp/org-eldoc.el b/contrib/lisp/org-eldoc.el
index 3b0999340..946a57273 100644
--- a/contrib/lisp/org-eldoc.el
+++ b/contrib/lisp/org-eldoc.el
@@ -1,4 +1,4 @@
-;;; org-eldoc.el --- display org header and src block info using eldoc
+;;; org-eldoc.el --- display org header and src block info using eldoc -*- lexical-binding: t; -*-
 
 ;; Copyright (c) 2014-2020 Free Software Foundation, Inc.
 
@@ -114,11 +114,18 @@
         doc-func)
     (if (eq 'empty cached-func)
         (when (fboundp mode-func)
-          (with-temp-buffer
-            (funcall mode-func)
-            (setq doc-func (and eldoc-documentation-function
-                                (symbol-value 'eldoc-documentation-function)))
-            (puthash lang doc-func org-eldoc-local-functions-cache))
+	  (with-temp-buffer
+	    (funcall mode-func)
+	    (setq doc-func (if (boundp 'eldoc-documentation-functions)
+			       (let ((doc-funs eldoc-documentation-functions))
+				 (lambda (callback)
+				   (let ((eldoc-documentation-functions doc-funs))
+				     (run-hook-with-args-until-success
+				      'eldoc-documentation-functions
+				      callback))))
+			     (and eldoc-documentation-function
+				  (symbol-value 'eldoc-documentation-function))))
+	    (puthash lang doc-func org-eldoc-local-functions-cache))
           doc-func)
       cached-func)))
 
@@ -127,7 +134,7 @@
 (declare-function php-eldoc-function "php-eldoc" ())
 (declare-function go-eldoc--documentation-function "go-eldoc" ())
 
-(defun org-eldoc-documentation-function (&rest _ignored)
+(defun org-eldoc-documentation-function (&rest args)
   "Return breadcrumbs when on a headline, args for src block header-line,
   calls other documentation functions depending on lang when inside src body."
   (or
@@ -160,8 +167,12 @@
              (string= lang "go")
              (string= lang "golang")) (when (require 'go-eldoc nil t)
                                         (go-eldoc--documentation-function)))
-           (t (let ((doc-fun (org-eldoc-get-mode-local-documentation-function lang)))
-                (when (functionp doc-fun) (funcall doc-fun))))))))
+           (t (let ((doc-fun (org-eldoc-get-mode-local-documentation-function lang))
+		    (callback (car args)))
+                (when (functionp doc-fun)
+		  (if (functionp callback)
+		      (funcall doc-fun callback)
+		    (funcall doc-fun)))))))))
 
 ;;;###autoload
 (defun org-eldoc-load ()
-- 
2.25.1

Re: eldoc recursion error

[Kyle Meyer]

James N. V. Cash writes:

> Kyle Meyer <kyle@kyleam.com> writes:
>
>> Does your cider test case above break if we use
>> eldoc-print-current-symbol-info without relaying the callback?  That is,
>> this squashed into your patch:
>
> My concern with using the eldoc-print-current-symbol-info is that it's
> now somewhat subverting the actual eldoc documentation function -- i.e.
> the invocation of org-eldoc-documentation-function now "fails" and
> instead it prints out the actual documentation as a side-effect. Indeed,
> applying that patch makes the eldoc for python code blocks not work
> correctly.

Okay.  Testing with the current Emacs master branch, I saw what I
thought were the expected messages, but perhaps I wasn't testing
complicated enough python blocks on my end:

#+begin_src python
  print("ok")
  int(1)
#+end_src

Either way ...

> The below patch which essentially just inlines the definition of
> eldoc-documentation-default, so it's not messing around with any private
> variables in eldoc, although it now won't honour the documentation
> strategy. It remains to be seen if that will be an issue in practice,
> but if necessary we could just check the value of
> eldoc-documentation-strategy and behave appropriately.

... this sounds fine to me.  Lightly testing your latest patch with
Emacs 27 and 28, things work on my end.

Pushed (c20cb0993).  Thanks.

Re: eldoc recursion error

[Kyle Meyer]

This was resolved by the thread at
<https://orgmode.org/list/87imcealr7.fsf@gmail.com>.  Although the first
message in that thread had an In-Reply-To header pointing to a message
in this thread, it didn't have a proper References header, so I don't
think <https://updates.orgmode.org/> will pick up on the "X-Woof-Bug:
fixed" header in <https://orgmode.org/list/87h7rrx64v.fsf@kyleam.com>.
Hence this message...