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

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

[Daniel Martín via Bug reports for GNU Emacs, the Swiss army knife of text editors]

This is a regression from Emacs 27.2.

Steps to reproduce the problem:

1. emacs -Q
2. Place the point inside any Elisp function.
3. C-u C-M-x

Expected result:

"Edebug: <name of the function>" shows up in the echo area.

Actual result:

"<name of the function>" shows up in the echo area.  That makes it
difficult to know if the user evaluated or instrumented the function.

If you open the *Messages* buffer, the reason why we don't see it in the
echo area is because the "Edebug:" message is ovewritten by a subsequent
message that prints the name of the defun.

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

[Lars Ingebrigtsen]

Daniel Martín <mardani29@yahoo.es> writes:

> This is a regression from Emacs 27.2.
>
> Steps to reproduce the problem:
>
> 1. emacs -Q
> 2. Place the point inside any Elisp function.
> 3. C-u C-M-x
>
> Expected result:
>
> "Edebug: <name of the function>" shows up in the echo area.
>
> Actual result:
>
> "<name of the function>" shows up in the echo area.  That makes it
> difficult to know if the user evaluated or instrumented the function.
>
> If you open the *Messages* buffer, the reason why we don't see it in the
> echo area is because the "Edebug:" message is ovewritten by a subsequent
> message that prints the name of the defun.

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

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Daniel Martín via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> bae2cfe63cbd11eaf348dfa7fbb2b9f7362fc747.  I'm not sure, but I think
> that the author's intent was to reduce code duplication

Definitely.

> (with possibly other benefits).

This was a nasty case of duplication where the two versions worked quite
differently, yet trying to mimic the other's result.  The worst part is
that depending on whether `edebug.el` was loaded either of the two codes
could be used, so any difference in behavior in the "normal C-M-x case"
was a bug, so the "mimicking" had to be as perfect as possible.

> 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.

I think you'll need a (defvar edebug-all-defs) before that.

> @@ -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 replaces `standard-output` with t, which is probably OK in most
cases, but just to be sure, I'd use

             (should-print (if (not edebugging) standard-output)))

> This solves the problem and does not introduce any further regression.
> Any feedback on if this is TRT?

This kind of dependency on Edebug is undesirable, but it's OK.
I can see 2 other approaches:
- Refrain from emitting the message if some message was emitted already
  (i.e. checking `current-message`).  This is likely brittle (and would
  definitely break when `standard-output` points elsewhere ;-)
- Don't print here but print from within the `eval-region`, just like we do
  in the Edebug case.  Arguably this would be the cleanest in terms of
  interaction between the Edebug and the non-Edebug case.  But it likely
  requires significantly more changes and might introduce more
  problems elsewhere.

> If the patch looks good, I can accompany it with a comment that
> explains the reasoning, tests, etc.

Please do and thank you.


        Stefan

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

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> If the patch looks good, I can accompany it with a comment that
>> explains the reasoning, tests, etc.
>
> Please do and thank you.

This was almost a year ago -- Daniel, did you do any further work here?

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

[Daniel Martín via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> If the patch looks good, I can accompany it with a comment that
>>> explains the reasoning, tests, etc.
>>
>> Please do and thank you.
>
> This was almost a year ago -- Daniel, did you do any further work here?

Here's a new version of the patch, which addresses Stefan's feedback.
I've added a test because in the docstring of eval-defun we document
that we print "Edebug: FUNCTION'" in the instrumented case, so I assume
it's a behavior we'd like to keep working.

Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-instrumented-eval-defun-not-printing-Edebug-to-t.patch --]
[-- Type: text/x-patch, Size: 2836 bytes --]

From 7649176669fbe49260ec2a0cee1865c3f4cf5f2f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Mart=C3=ADn?= <mardani29@yahoo.es>
Date: Wed, 24 Aug 2022 23:34:18 +0200
Subject: [PATCH] Fix instrumented eval-defun not printing "Edebug:" to the
 echo area

* lisp/progmodes/elisp-mode.el (elisp--eval-defun): Determine if we're
instrumenting a function and call eval-region with PRINTFLAG set to
nil if so.  (Bug#50245)

* test/lisp/progmodes/elisp-mode-tests.el
(eval-defun-prints-edebug-when-instrumented): Add a new test.
---
 lisp/progmodes/elisp-mode.el            |  8 ++++++--
 test/lisp/progmodes/elisp-mode-tests.el | 10 ++++++++++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index 0c4a9bfdbe..bc8833a693 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -1643,7 +1643,10 @@ elisp--eval-defun
   ;; FIXME: the print-length/level bindings should only be applied while
   ;; printing, not while evaluating.
   (defvar elisp--eval-defun-result)
+  ;; FIXME: This Edebug dependency is undesirable.  See bug#50245
+  (defvar edebug-all-defs)
   (let ((debug-on-error eval-expression-debug-on-error)
+        (edebugging edebug-all-defs)
         elisp--eval-defun-result)
     (save-excursion
       ;; Arrange for eval-region to "read" the (possibly) altered form.
@@ -1668,8 +1671,9 @@ elisp--eval-defun
                          (elisp--eval-defun-1
                           (macroexpand form)))))
 	      (print-length eval-expression-print-length)
-	      (print-level eval-expression-print-level))
-          (eval-region beg end standard-output
+	      (print-level eval-expression-print-level)
+              (should-print (if (not edebugging) standard-output)))
+          (eval-region beg end should-print
                        (lambda (_ignore)
                          ;; Skipping to the end of the specified region
                          ;; will make eval-region return.
diff --git a/test/lisp/progmodes/elisp-mode-tests.el b/test/lisp/progmodes/elisp-mode-tests.el
index 8e4dfa8bb8..345e8880c8 100644
--- a/test/lisp/progmodes/elisp-mode-tests.el
+++ b/test/lisp/progmodes/elisp-mode-tests.el
@@ -183,6 +183,16 @@ eval-last-sexp-print-format-large-int-echo
         (call-interactively #'eval-last-sexp)
         (should (equal (current-message) "66 (#o102, #x42, ?B)"))))))
 
+;;; eval-defun
+
+(ert-deftest eval-defun-prints-edebug-when-instrumented ()
+  (skip-unless (not noninteractive))
+  (with-temp-buffer
+    (let ((current-prefix-arg '(4)))
+      (erase-buffer) (insert "(defun foo ())") (message nil)
+      (call-interactively #'eval-defun)
+      (should (equal (current-message) "Edebug: foo")))))
+
 ;;; eldoc
 
 (defun elisp-mode-tests--face-propertized-string (string)
-- 
2.34.1

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Here's a new version of the patch, which addresses Stefan's feedback.

Thanks.  One question, tho:

> @@ -1643,7 +1643,10 @@ elisp--eval-defun
>    ;; FIXME: the print-length/level bindings should only be applied while
>    ;; printing, not while evaluating.
>    (defvar elisp--eval-defun-result)
> +  ;; FIXME: This Edebug dependency is undesirable.  See bug#50245
> +  (defvar edebug-all-defs)
>    (let ((debug-on-error eval-expression-debug-on-error)
> +        (edebugging edebug-all-defs)
>          elisp--eval-defun-result)
>      (save-excursion
>        ;; Arrange for eval-region to "read" the (possibly) altered form.

What makes us think that `edebug-all-defs` will always be defined when
we get here?  Doesn't this code signal an error if called before
edebug.el is loaded (e.g. in the case of `emacs -Q` soon followed by
a plain `C-M-x`)?


        Stefan

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

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> What makes us think that `edebug-all-defs` will always be defined when
> we get here?  Doesn't this code signal an error if called before
> edebug.el is loaded (e.g. in the case of `emacs -Q` soon followed by
> a plain `C-M-x`)?

edebug-all-defs is autoloaded, so it's safe to use here (and the (defvar
isn't needed, so I removed it).

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen [2022-08-25 14:38:13] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> What makes us think that `edebug-all-defs` will always be defined when
>> we get here?  Doesn't this code signal an error if called before
>> edebug.el is loaded (e.g. in the case of `emacs -Q` soon followed by
>> a plain `C-M-x`)?
> edebug-all-defs is autoloaded, so it's safe to use here (and the (defvar
> isn't needed, so I removed it).

Ah, even better,


        Stefan

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> Here's a new version of the patch, which addresses Stefan's feedback.
>
> Thanks.  One question, tho:
>
>> @@ -1643,7 +1643,10 @@ elisp--eval-defun
>>    ;; FIXME: the print-length/level bindings should only be applied while
>>    ;; printing, not while evaluating.
>>    (defvar elisp--eval-defun-result)
>> +  ;; FIXME: This Edebug dependency is undesirable.  See bug#50245
>> +  (defvar edebug-all-defs)
>>    (let ((debug-on-error eval-expression-debug-on-error)
>> +        (edebugging edebug-all-defs)
>>          elisp--eval-defun-result)
>>      (save-excursion
>>        ;; Arrange for eval-region to "read" the (possibly) altered form.
>
> What makes us think that `edebug-all-defs` will always be defined when
> we get here?  Doesn't this code signal an error if called before
> edebug.el is loaded (e.g. in the case of `emacs -Q` soon followed by
> a plain `C-M-x`)?

IOW, I'd remove the `defvar` and use (bound-and-true-p
edebug-all-defs) instead.


        Stefan