bug#68075: 30.0.50; New special form `handler-bind`

[Eli Zaretskii]

> Cc: monnier@iro.umontreal.ca
> Date: Thu, 28 Dec 2023 01:33:09 -0500
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> I have now pushed to `scratch/handler-bind` a set of patches I'd like to
> commit to `master`.  They add the new special form `handler-bind`, which
> provides a functionality a bit halfway between `condition-case` and
> `signal-hook-function`.
> 
> Among other things, this allows us to fix bug#11218, bug#65267, and
> bug#67196.  It also makes it possible to move code out of
> `signal_or_quit`.
> 
> The complete log can be found below, but the oneliners are:
> 
>     New special form `handler-bind`
>     (eval-expression): Fix bug#67196
>     ert.el: Use `handler-bind` to record backtraces
>     Fix ert-tests.el for the new `handler-bind` code
>     Use handler-bind to repair bytecomp-tests
>     emacs-module-tests.el (mod-test-non-local-exit-signal-test): Repair test
>     startup.el: Use `handler-bind` to implement `--debug-init`
>     Move batch backtrace code to `top_level_2`
>     (macroexp--with-extended-form-stack): Use plain `let`
>     eval.c: Add new var `lisp-eval-depth-reserve`
>     (signal_or_quit): Preserve error object identity
>     (backtrace-on-redisplay-error): Use `handler-bind`
> 
> As you can see, the first commit adds the new feature, and subsequent
> ones basically make use of it in various different places.
> 
> Among those commits, I'll note the introduction of a new variable
> `lisp-eval-depth-reserve`, which lets us control how much Emacs can grow
> `max-lisp-eval-depth` to run the debugger.  This is not indispensable,
> but seemed like a good idea.  I also subtly changed the way errors
> are built such that we can rely on the error object being `eq` to itself
> as it moves up from the call to `signal` up to its `condition-case`
> handler, which should make it possible to keep extra info about it
> in an `eq` hashtable, for example.
> 
> Comments, objections?

Assuming by the above you meant that the branch is from your POV ready
to land on master, please find some review comments below.

> diff --git a/doc/lispref/control.texi b/doc/lispref/control.texi
> index d4bd8c1..4107963 100644
> --- a/doc/lispref/control.texi
> +++ b/doc/lispref/control.texi
> @@ -2293,6 +2293,44 @@ Handling Errors
>  @code{condition-case-unless-debug} rather than @code{condition-case}.
>  @end defmac
>  
> +Occasionally, we want to catch some errors and record some information
> +about the conditions in which they occurred, such as the full
> +backtrace, or the current buffer.  This kinds of information is sadly
> +not available in the handlers of a @code{condition-case} because the
> +stack is unwound before running that handler, so the handler is run in
> +the dynamic context of the @code{condition-case} rather than that of
> +the place where the error was signaled.  For those circumstances, you
> +can use the following form:

The reference to "unwinding the stack before running handler" uses
terminology and assumes knowledge we don't generally expect from
readers of the ELisp manual.  This should be reworded to use the
terminology we use in other cases where we talk about "unwinding" and
error handlers.

> +@defmac handler-bind handlers body@dots{}
> +This special form runs @var{body} and if it executes without error,
> +the value it returns becomes the value of the @code{handler-bind}
> +form.  In this case, the @code{handler-bind} has no effect.
> +
> +@var{handlers} should be a list of elements of the form
> +@code{(@var{conditions} @var{handler})} where @var{conditions} is an
> +error condition name to be handled, or a list of condition names, and
> +@var{handler} should be a form whose evaluation should return a function.

CONDITIONs are symbols, aren't they?  The above says "condition
names", which could be interpreted as if they were strings instead.

> +Before running @var{body}, @code{handler-bind} evaluates all the
> +@var{handler} forms and installs those handlers to be active during
> +the evaluation of @var{body}.  These handlers are searched together
> +with those installed by @code{condition-case}.

This reference to condition-case might confuse: what does
condition-case have to do with handler-bind?  And if it is expected
that handler-bind is used inside condition-case, there should be some
text about it, and the examples we hopefully add should illustrate
that.

>                                                  When the innermost
> +matching handler is one installed by @code{handler-bind}, the
> +@var{handler} function is called with a single argument holding the
> +error description.

Is "error description" the same as "error name" above?  If so, let's
please use consistent wording to minimize the chances for confusion
and misunderstandings.

> +@var{handler} is called in the dynamic context where the error
> +happened, without first unwinding the stack, meaning that all the
> +dynamic bindings are still in effect,

Should we tell something about the effects of lexical-binding on those
"dynamic bindings"?

>                                       except that all the error
> +handlers between the code that signaled the error and the
> +@code{handler-bind} are temporarily suspended.

Not sure I understand what it means for those handlers to be
"suspended".  can the text say more about that?

>                                                  Like any normal
> +function, @var{handler} can exit non-locally, typically via
> +@code{throw}, or it can return normally.  If @var{handler} returns
> +normally, it means the handler @emph{declined} to handle the error and
> +the search for an error handler is continued where it left off.
> +@end defmac

I think we should have a couple of examples here, to show the utility
of handler-bind and how it is different from condition-case.

> +@defopt lisp-eval-depth-reserve
> +In order to be able to debug infinite recursion errors, Entry to the
                                                         ^^^
Typo.

> +Lisp debugger increases temporarily the value of
> +@code{max-lisp-eval-depth}, if there is little room left, to make sure
> +the debugger itself has room to execute.  The same happens when
> +running the handler of a @code{handler-bind}.

Please add here a cross-reference to where handler-bind is documented.

> +The variable @code{lisp-eval-depth-reserve} bounds the extra depth
> +that Emacs can add to @code{max-lisp-eval-depth} for those
> +exceptional circumstances.
> +@end defopt

I think this should document the default value.

> ++++
> +** New special form 'handler-bind'.
> +Provides a functionality similar to `condition-case` except it runs the
> +handler code without unwinding the stack, such that we can record the
> +backtrace and other dynamic state at the point of the error.

Please add here a link to the node in the ELisp manual where
handler-bind is described.

> -(defmacro displaying-byte-compile-warnings (&rest body)
> +(defmacro displaying-byte-compile-warnings (&rest body) ;FIXME: Namespace!
                                                           ^^^^^^^^^^^^^^^^^^
What about this FIXME?

> +(defmacro handler-bind (handlers &rest body)
> +  "Setup error HANDLERS around execution of BODY.
> +HANDLERS is a list of (CONDITIONS HANDLER) where
> +CONDITIONS should be a list of condition names (symbols) or
> +a single condition name and HANDLER is a form whose evaluation
                          ^
A comma is missing there.

> +returns a function.
> +When an error is signaled during execution of BODY, if that
> +error matches CONDITIONS, then the associated HANDLER
> +function is called with the error as argument.
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^
"Error" or "condition name"?  If "error", then what does "error
matches CONDITIONS" mean in practice?

> +HANDLERs can either transfer the control via a non-local exit,
> +or return normally.  If they return normally the search for an
                        ^^^^^^^^^^^^^^
I'd suggest "If a handler returns" instead.  There's just one
HANDLER involved here, right?

> +error handler continues from where it left off."

This "from where it left off" sounds confusing: what does it mean?
IOW, how is it different from saying

  If a HANDLER returns normally, other CONDITIONS are searched for a
  match, and, if found, their HANDLERs are called.

Btw, this begs the question: what happens if none of the CONDITIONS
match?  In particular, what happens if a HANDLER is called, returns
normally, and none of the other CONDITIONS match?

> +  ;; FIXME: Completion support as in `condition-case'?
        ^^^^^
What about this FIXME?

> @@ -317,6 +312,7 @@ call_debugger (Lisp_Object arg)
>    /* Interrupting redisplay and resuming it later is not safe under
>       all circumstances.  So, when the debugger returns, abort the
>       interrupted redisplay by going back to the top-level.  */
> +  /* FIXME: Move this to the redisplay code?  */
        ^^^^^
And this one?

> +DEFUN ("handler-bind-1", Fhandler_bind_1, Shandler_bind_1, 1, MANY, 0,
> +       doc: /* Setup error handlers around execution of BODYFUN.
> +BODYFUN be a function and it is called with no arguments.
> +CONDITIONS should be a list of condition names (symbols).
> +When an error is signaled during executon of BODYFUN, if that
> +error matches one of CONDITIONS, then the associated HANDLER is
> +called with the error as argument.
> +HANDLER should either transfer the control via a non-local exit,
> +or return normally.
> +If it returns normally, the search for an error handler continues
> +from where it left off.

"Where it left off" strikes again...

> +      specpdl_ref count = SPECPDL_INDEX ();
> +      max_ensure_room (20);
> +      /* FIXME: 'handler-bind' makes `signal-hook-function' obsolete?  */
> +      /* FIXME: Here we still "split" the error object
> +         into its error-symbol and its error-data?  */
>        call2 (Vsignal_hook_function, error_symbol, data);
> +      unbind_to (count, Qnil);

FIXMEs again.

>  top_level_2 (void)
>  {
> -  return Feval (Vtop_level, Qnil);
> +  /* If we're in batch mode, print a backtrace unconditionally when
> +     encountering an error, to help with debugging.  */
> +  bool setup_handler = noninteractive;
> +  if (setup_handler)
> +    /* FIXME: Should we (re)use `list_of_error` from `xdisp.c`? */
> +    push_handler_bind (list1 (Qerror), Qdebug_early__handler, 0);

And again.

> +;; (ert-deftest ert-test-fail-debug-with-condition-case ()
> +;;   (let ((test (make-ert-test :body (lambda () (ert-fail "failure 
> message")))))
> +;;     (condition-case condition
> +;;         (progn
> +;;           (let ((ert-debug-on-error t))
> +;;             (ert-run-test test))
> +;;           (cl-assert nil))
> +;;       ((error)
> +;;        (cl-assert (equal condition '(ert-test-failed "failure message")) 
> t)))))

What about this and other places where some code was commented-out,
but not removed?  Those seem to be tests that you disable - why?  Are
they no longer pertinent, or do they fail for some reason that should
be fixed?

Last, but not least: do all the tests in the test suite pass after
these changes, both with and without native-compilation?

Thanks.