bug#41531: 27.0.91; Better handle asynchronous eldoc backends

[João Távora]

Hi Stefan, Dmitry, Andrii and maintainers,

Moving the discussion that started in
https://github.com/joaotavora/eglot/pull/459 to the bug tracker, and
attaching the two patches that contain what I think is a decent
short-term solution to the eldoc/async problems.

It makes eldoc-diagnostic-functions have a very similar interface to
flymake-diagnostic-functions.  Flymake's handling of the multiple
backends and async is more sophisticated, and we could extend eldoc to
try similarly heroic stuff, if we do find there's a demand for it.

The main thing you probably want to read if
`eldoc-documentation-functions`'s new docstring.

This was tested summarily with Eglot, by the way, and seems to work OK.

Enjoy!
João

PS: How do I mark that the bug report contains a patch in the mail
message itself?

>From 9ca84e5482b2e7ef40f80679ec508afb008293ae Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
Date: Mon, 25 May 2020 16:39:40 +0100
Subject: [PATCH 1/2] Better handle asynchronously produced eldoc docstrings

No longer do clients of eldoc need to call eldoc-message (an internal
function) directly.  They may return any non-nil, non-string value and
call a callback afterwards.  This enables eldoc.el to exert control
over how (and crucially also when) to display the docstrings to the
user.

* lisp/emacs-lisp/eldoc.el (eldoc-documentation-functions):
Overhaul docstring.
(eldoc-documentation-compose, eldoc-documentation-default): Handle
non-nil, non-string values of elements of
eldoc-documentation-functions.  Use eldoc--handle-multiline.
(eldoc-print-current-symbol-info): Honour non-nil, non-string
values returned by eldoc-documentation-callback.
(eldoc--handle-multiline): New helper.
(Version): Bump to 1.1.0.
---
 lisp/emacs-lisp/eldoc.el | 73 ++++++++++++++++++++++++++++++----------
 1 file changed, 56 insertions(+), 17 deletions(-)

diff --git a/lisp/emacs-lisp/eldoc.el b/lisp/emacs-lisp/eldoc.el
index ef5dbf8103..f5dcdb4ea0 100644
--- a/lisp/emacs-lisp/eldoc.el
+++ b/lisp/emacs-lisp/eldoc.el
@@ -5,7 +5,7 @@
 ;; Author: Noah Friedman <friedman@splode.com>
 ;; Keywords: extensions
 ;; Created: 1995-10-06
-;; Version: 1.0.0
+;; Version: 1.1.0
 ;; Package-Requires: ((emacs "26.3"))
 
 ;; This is a GNU ELPA :core package.  Avoid functionality that is not
@@ -338,12 +338,26 @@ eldoc-display-message-no-interference-p
 
 
 (defvar eldoc-documentation-functions nil
-  "Hook for functions to call to return doc string.
-Each function should accept no arguments and return a one-line
-string for displaying doc about a function etc. appropriate to
-the context around point.  It should return nil if there's no doc
-appropriate for the context.  Typically doc is returned if point
-is on a function-like name or in its arg list.
+  "Hook of functions that produce doc strings.
+Each hook function should accept no arguments and decide whether
+to display a doc short string about the context around point.  If
+the decision and the doc string can be produced quickly, the hook
+function should immediately return the doc string, or nil if
+there's no doc appropriate for the context.  Otherwise, if its
+computation is expensive or can't be performed directly, the hook
+function should save the value bound to
+`eldoc-documentation-callback', and arrange for that callback
+function to be asynchronously called at a later time, passing it
+either nil or the desired doc string.  The hook function should
+then return a non-nil, non-string value.
+
+A current limitation of the asynchronous case is that it is only
+guaranteed to work correctly if the value of
+`eldoc-documentation-function' (notice the singular) is
+`eldoc-documentation-default'.
+
+Typically doc is returned if point is on a function-like name or
+in its arg list.
 
 Major modes should modify this hook locally, for example:
   (add-hook \\='eldoc-documentation-functions #\\='foo-mode-eldoc nil t)
@@ -351,14 +365,18 @@ eldoc-documentation-functions
 taken into account if the major mode specific function does not
 return any documentation.")
 
+(defun eldoc--handle-multiline (res)
+  "Helper for handling a bit of `eldoc-echo-area-use-multiline-p'."
+  (if eldoc-echo-area-use-multiline-p res
+    (truncate-string-to-width
+     res (1- (window-width (minibuffer-window))))))
+
 (defun eldoc-documentation-default ()
   "Show first doc string for item at point.
 Default value for `eldoc-documentation-function'."
   (let ((res (run-hook-with-args-until-success 
'eldoc-documentation-functions)))
-    (when res
-      (if eldoc-echo-area-use-multiline-p res
-        (truncate-string-to-width
-         res (1- (window-width (minibuffer-window))))))))
+    (cond ((stringp res) (eldoc--handle-multiline res))
+          (t res))))
 
 (defun eldoc-documentation-compose ()
   "Show multiple doc string results at once.
@@ -368,13 +386,11 @@ eldoc-documentation-compose
      'eldoc-documentation-functions
      (lambda (f)
        (let ((str (funcall f)))
-         (when str (push str res))
+         (when (stringp str) (push str res))
          nil)))
     (when res
       (setq res (mapconcat #'identity (nreverse res) ", "))
-      (if eldoc-echo-area-use-multiline-p res
-        (truncate-string-to-width
-         res (1- (window-width (minibuffer-window))))))))
+      (eldoc--handle-multiline res))))
 
 (defcustom eldoc-documentation-function #'eldoc-documentation-default
   "Function to call to return doc string.
@@ -408,6 +424,12 @@ eldoc--supported-p
            ;; there's some eldoc support in the current buffer.
            (local-variable-p 'eldoc-documentation-function))))
 
+;; this variable should be unbound, but that confuses
+;; `describe-symbol' for some reason.
+(defvar eldoc-documentation-callback nil
+  "Dynamically bound.  Accessible to `eldoc-documentation-functions'.
+See that function for details.")
+
 (defun eldoc-print-current-symbol-info ()
   "Print the text produced by `eldoc-documentation-function'."
   ;; This is run from post-command-hook or some idle timer thing,
@@ -417,11 +439,28 @@ eldoc-print-current-symbol-info
         ;; Erase the last message if we won't display a new one.
         (when eldoc-last-message
           (eldoc-message nil))
-      (let ((non-essential t))
+      (let ((non-essential t)
+            (buffer (current-buffer)))
         ;; Only keep looking for the info as long as the user hasn't
         ;; requested our attention.  This also locally disables inhibit-quit.
         (while-no-input
-          (eldoc-message (funcall eldoc-documentation-function)))))))
+          (let*
+              ((waiting-for-callback nil)
+               (eldoc-documentation-callback
+                (lambda (string)
+                  (with-current-buffer buffer
+                    ;; JT@2020-05-25: Currently, we expect one single
+                    ;; docstring from the client, we silently swallow
+                    ;; anything the client unexpectedly gives us,
+                    ;; including updates.  This could change.
+                    (when waiting-for-callback
+                      (eldoc-message (eldoc--handle-multiline string))
+                      (setq waiting-for-callback nil)))))
+               (res
+                (funcall eldoc-documentation-function)))
+            (cond ((stringp res) (eldoc-message res))
+                  (res (setq waiting-for-callback t))
+                  (t (eldoc-message nil)))))))))
 
 ;; If the entire line cannot fit in the echo area, the symbol name may be
 ;; truncated or eliminated entirely from the output to make room for the
-- 
2.20.1

>From a6ba9972ee0e3305c7b41fd380a88dd18a6626a1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
Date: Mon, 25 May 2020 17:38:23 +0100
Subject: [PATCH 2/2] Adjust eldoc-documentation-functions protocol for better
 async

Instead of exposing a eldoc-documentation-callback variable to
clients, just pass it the callback as the first argument.

* lisp/emacs-lisp/eldoc.el (eldoc-documentation-functions): Rewrite docstring.
(eldoc-documentation-default, eldoc-documentation-compose): Use
internal eldoc--callback.
(eldoc-documentation-callback).
(eldoc---callback): Rename from eldoc-documentation-callback.
(eldoc-print-current-symbol-info): Bind eldoc--callback.

* lisp/hexl.el (hexl-print-current-point-info): Adjust to new
eldoc-documentation-functions protocol.

* lisp/progmodes/cfengine.el (cfengine3-documentation-function):
Adjust to new eldoc-documentation-functions protocol.

* lisp/progmodes/elisp-mode.el
(elisp-eldoc-documentation-function): Adjust to new
eldoc-documentation-functions protocol.

* lisp/progmodes/octave.el (octave-eldoc-function): Adjust to new
eldoc-documentation-functions protocol.

* lisp/progmodes/python.el (python-eldoc-function): Adjust to new
eldoc-documentation-functions protocol.
---
 lisp/emacs-lisp/eldoc.el     | 45 ++++++++++++++++++------------------
 lisp/hexl.el                 |  2 +-
 lisp/progmodes/cfengine.el   |  2 +-
 lisp/progmodes/elisp-mode.el |  6 +++--
 lisp/progmodes/octave.el     |  4 ++--
 lisp/progmodes/python.el     |  2 +-
 6 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/lisp/emacs-lisp/eldoc.el b/lisp/emacs-lisp/eldoc.el
index f5dcdb4ea0..a4e1e460ac 100644
--- a/lisp/emacs-lisp/eldoc.el
+++ b/lisp/emacs-lisp/eldoc.el
@@ -339,22 +339,22 @@ eldoc-display-message-no-interference-p
 
 (defvar eldoc-documentation-functions nil
   "Hook of functions that produce doc strings.
-Each hook function should accept no arguments and decide whether
-to display a doc short string about the context around point.  If
-the decision and the doc string can be produced quickly, the hook
-function should immediately return the doc string, or nil if
-there's no doc appropriate for the context.  Otherwise, if its
-computation is expensive or can't be performed directly, the hook
-function should save the value bound to
-`eldoc-documentation-callback', and arrange for that callback
-function to be asynchronously called at a later time, passing it
-either nil or the desired doc string.  The hook function should
-then return a non-nil, non-string value.
-
-A current limitation of the asynchronous case is that it is only
-guaranteed to work correctly if the value of
-`eldoc-documentation-function' (notice the singular) is
-`eldoc-documentation-default'.
+Each hook function should accept at least one argument CALLBACK
+and decide whether to display a doc short string about the
+context around point.  If the decision and the doc string can be
+produced quickly, the hook function can ignore CALLBACK and
+immediately return the doc string, or nil if there's no doc
+appropriate for the context.  Otherwise, if its computation is
+expensive or can't be performed directly, the hook function
+should arrange for CALLBACK to be asynchronously called at a
+later time, passing it either nil or the desired doc string.  The
+hook function should then return a non-nil, non-string value.
+
+Note that this hook is only in effect if the value of
+`eldoc-documentation-function' (notice the singular) is bound to
+one of its pre-set values.  Furthermore, the asynchronous
+mechanism described above is only guaranteed to work correctly if
+that value is `eldoc-documentation-default'.
 
 Typically doc is returned if point is on a function-like name or
 in its arg list.
@@ -374,7 +374,9 @@ eldoc--handle-multiline
 (defun eldoc-documentation-default ()
   "Show first doc string for item at point.
 Default value for `eldoc-documentation-function'."
-  (let ((res (run-hook-with-args-until-success 
'eldoc-documentation-functions)))
+  (let ((res (run-hook-with-args-until-success
+              'eldoc-documentation-functions
+              eldoc--callback)))
     (cond ((stringp res) (eldoc--handle-multiline res))
           (t res))))
 
@@ -385,7 +387,7 @@ eldoc-documentation-compose
     (run-hook-wrapped
      'eldoc-documentation-functions
      (lambda (f)
-       (let ((str (funcall f)))
+       (let ((str (funcall f eldoc--callback)))
          (when (stringp str) (push str res))
          nil)))
     (when res
@@ -426,9 +428,8 @@ eldoc--supported-p
 
 ;; this variable should be unbound, but that confuses
 ;; `describe-symbol' for some reason.
-(defvar eldoc-documentation-callback nil
-  "Dynamically bound.  Accessible to `eldoc-documentation-functions'.
-See that function for details.")
+(defvar eldoc---callback nil
+  "Dynamically bound.  Passed to  `eldoc-documentation-functions'.")
 
 (defun eldoc-print-current-symbol-info ()
   "Print the text produced by `eldoc-documentation-function'."
@@ -446,7 +447,7 @@ eldoc-print-current-symbol-info
         (while-no-input
           (let*
               ((waiting-for-callback nil)
-               (eldoc-documentation-callback
+               (eldoc--callback
                 (lambda (string)
                   (with-current-buffer buffer
                     ;; JT@2020-05-25: Currently, we expect one single
diff --git a/lisp/hexl.el b/lisp/hexl.el
index cf7118f208..38eca77e26 100644
--- a/lisp/hexl.el
+++ b/lisp/hexl.el
@@ -515,7 +515,7 @@ hexl-current-address
       (message "Current address is %d/0x%08x" hexl-address hexl-address))
     hexl-address))
 
-(defun hexl-print-current-point-info ()
+(defun hexl-print-current-point-info (&rest _ignored)
   "Return current hexl-address in string.
 This function is intended to be used as eldoc callback."
   (let ((addr (hexl-current-address)))
diff --git a/lisp/progmodes/cfengine.el b/lisp/progmodes/cfengine.el
index f25b3cb9e2..9a6d81ce06 100644
--- a/lisp/progmodes/cfengine.el
+++ b/lisp/progmodes/cfengine.el
@@ -1294,7 +1294,7 @@ cfengine3-make-syntax-cache
                           'symbols))
         syntax)))
 
-(defun cfengine3-documentation-function ()
+(defun cfengine3-documentation-function (&rest _ignored)
   "Document CFengine 3 functions around point.
 Intended as the value of `eldoc-documentation-function', which see.
 Use it by enabling `eldoc-mode'."
diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index d37eb8c152..d7865a7319 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -1402,8 +1402,10 @@ elisp--eldoc-last-data
       or argument string for functions.
   2 - `function' if function args, `variable' if variable documentation.")
 
-(defun elisp-eldoc-documentation-function ()
-  "`eldoc-documentation-function' (which see) for Emacs Lisp."
+(defun elisp-eldoc-documentation-function (_ignored &rest _also-ignored)
+  "Contextual documentation function for Emacs Lisp.
+Intended to be placed in `eldoc-documentation-functions' (which
+see)."
   (let ((current-symbol (elisp--current-symbol))
        (current-fnsym  (elisp--fnsym-in-current-sexp)))
     (cond ((null current-fnsym)
diff --git a/lisp/progmodes/octave.el b/lisp/progmodes/octave.el
index 352c1810d1..2cf305c404 100644
--- a/lisp/progmodes/octave.el
+++ b/lisp/progmodes/octave.el
@@ -1639,8 +1639,8 @@ octave-eldoc-function-signatures
                   (nreverse result)))))
   (cdr octave-eldoc-cache))
 
-(defun octave-eldoc-function ()
-  "A function for `eldoc-documentation-function' (which see)."
+(defun octave-eldoc-function (&rest _ignored)
+  "A function for `eldoc-documentation-functions' (which see)."
   (when (inferior-octave-process-live-p)
     (let* ((ppss (syntax-ppss))
            (paren-pos (cadr ppss))
diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index 1ca9f01963..404a67ba9f 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -4571,7 +4571,7 @@ python-eldoc-function-timeout-permanent
   :type 'boolean
   :version "25.1")
 
-(defun python-eldoc-function ()
+(defun python-eldoc-function (&rest _ignored)
   "`eldoc-documentation-function' for Python.
 For this to work as best as possible you should call
 `python-shell-send-buffer' from time to time so context in
-- 
2.20.1