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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 50245 in the body.
You can then email your comments to 50245 AT debbugs.gnu.org in the normal way.

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

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

From: Daniel Martín <mardani29 <at> yahoo.es>
To: bug-gnu-emacs <at> gnu.org
Subject: 28.0.50; Instrumenting a function does not show "Edebug:" in the
 echo area
Date: Sun, 29 Aug 2021 03:04:20 +0200

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.

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Daniel Martín <mardani29 <at> yahoo.es>
Cc: 50245 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#50245: 28.0.50; Instrumenting a function does not show
 "Edebug:" in the echo area
Date: Sun, 29 Aug 2021 21:48:53 +0200

Daniel Martín <mardani29 <at> 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

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

From: Daniel Martín <mardani29 <at> yahoo.es>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 50245 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#50245: 28.0.50; Instrumenting a function does not show
 "Edebug:" in the echo area
Date: Sat, 02 Oct 2021 15:06:01 +0200

Lars Ingebrigtsen <larsi <at> 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 <at> 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.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Daniel Martín <mardani29 <at> yahoo.es>
Cc: 50245 <at> debbugs.gnu.org, Lars Ingebrigtsen <larsi <at> gnus.org>
Subject: Re: bug#50245: 28.0.50; Instrumenting a function does not show
 "Edebug:" in the echo area
Date: Sat, 02 Oct 2021 11:06:48 -0400

> 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

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 50245 <at> debbugs.gnu.org, Daniel Martín <mardani29 <at> yahoo.es>
Subject: Re: bug#50245: 28.0.50; Instrumenting a function does not show
 "Edebug:" in the echo area
Date: Mon, 22 Aug 2022 17:39:15 +0200

Stefan Monnier <monnier <at> 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?

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

From: Daniel Martín <mardani29 <at> yahoo.es>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 50245 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#50245: 28.0.50; Instrumenting a function does not show
 "Edebug:" in the echo area
Date: Thu, 25 Aug 2022 00:32:09 +0200

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

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Stefan Monnier <monnier <at> 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.

[0001-Fix-instrumented-eval-defun-not-printing-Edebug-to-t.patch (text/x-patch, attachment)]

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Daniel Martín <mardani29 <at> yahoo.es>
Cc: 50245 <at> debbugs.gnu.org, Lars Ingebrigtsen <larsi <at> gnus.org>
Subject: Re: bug#50245: 28.0.50; Instrumenting a function does not show
 "Edebug:" in the echo area
Date: Wed, 24 Aug 2022 22:06:53 -0400

> 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

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 50245 <at> debbugs.gnu.org, Daniel Martín <mardani29 <at> yahoo.es>
Subject: Re: bug#50245: 28.0.50; Instrumenting a function does not show
 "Edebug:" in the echo area
Date: Thu, 25 Aug 2022 14:38:13 +0200

Stefan Monnier <monnier <at> 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).

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Daniel Martín <mardani29 <at> yahoo.es>
Cc: 50245 <at> debbugs.gnu.org, Lars Ingebrigtsen <larsi <at> gnus.org>
Subject: Re: bug#50245: 28.0.50; Instrumenting a function does not show
 "Edebug:" in the echo area
Date: Thu, 25 Aug 2022 08:47:54 -0400

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

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 50245 <at> debbugs.gnu.org,
 Daniel Martín <mardani29 <at> yahoo.es>
Subject: Re: bug#50245: 28.0.50; Instrumenting a function does not show
 "Edebug:" in the echo area
Date: Thu, 25 Aug 2022 09:13:25 -0400

Lars Ingebrigtsen [2022-08-25 14:38:13] wrote:
> Stefan Monnier <monnier <at> 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

Previous Next

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