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

[Stefan Monnier]

> 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