bug#50245: 28.0.50; Instrumenting a function does not show "Edebug:" in the echo area

[Daniel Martín]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>
> I'm having some problems following the logic in
> `edebug--eval-defun'/`eval-defun' -- perhaps Stefan can help here; added
> to the CCs. 

I decided to familiarize myself with the Edebug code so that I can fix
bugs and implement features.

An automatic git bisect says that this is the commit that make the
regression evident:

commit bae2cfe63cbd11eaf348dfa7fbb2b9f7362fc747
Author: Stefan Monnier <monnier@iro.umontreal.ca>
Date:   Thu Feb 18 10:27:36 2021 -0500

    * lisp/emacs-lisp/edebug.el (eval-defun): Simplify
    
    (edebug-all-defs, edebug-all-forms): Don't autoload since the problem
    it was working around has been fixed a while back.
    (edebug--eval-defun): Rename from `edebug-eval-defun` and simplify by
    making it an `:around` advice.
    (edebug-install-read-eval-functions)
    (edebug-uninstall-read-eval-functions): Adjust accordingly.
    (edebug-eval-defun): Redefine as an obsolete wrapper.
    
    * lisp/progmodes/elisp-mode.el (elisp--eval-defun):
    Use `load-read-function` so it obeys `edebug-all-(defs|forms)`.
    (elisp--eval-defun): Fix recent regression introduced with
    `elisp--eval-defun-result`.

If I'm not mistaken, the first "Edebug: defun" message is printed in
elisp--eval-defun, inside the call to load-read-function.  The second
one is printed by the call to eval-region.  This code path is newly
exercised because edebug--eval-defun is now an :around advice after
bae2cfe63cbd11eaf348dfa7fbb2b9f7362fc747.  I'm not sure, but I think
that the author's intent was to reduce code duplication (with possibly
other benefits).

My proposed patch first checks if edebugging is active and then avoids
that eval-region prints by setting it's PRINTFLAG argument to nil:

diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index acf7123225..d1a4200df2 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -1612,6 +1612,7 @@ elisp--eval-defun
   (let ((debug-on-error eval-expression-debug-on-error)
        (print-length eval-expression-print-length)
        (print-level eval-expression-print-level)
+        (edebugging edebug-all-defs)
         elisp--eval-defun-result)
     (save-excursion
       ;; Arrange for eval-region to "read" the (possibly) altered form.
@@ -1629,8 +1630,9 @@ elisp--eval-defun
         ;; Alter the form if necessary.
         (let ((form (eval-sexp-add-defvars
                      (elisp--eval-defun-1
-                      (macroexpand form)))))
-          (eval-region beg end standard-output
+                      (macroexpand form))))
+              (should-print (not edebugging)))
+          (eval-region beg end should-print
                        (lambda (_ignore)
                          ;; Skipping to the end of the specified region
                          ;; will make eval-region return.

This solves the problem and does not introduce any further regression.
Any feedback on if this is TRT?  If the patch looks good, I can
accompany it with a comment that explains the reasoning, tests, etc.

Thanks.