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

[Stefan Monnier]

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

That's right (tho I did expect that the doc needed some improvement :-)

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

See patch below for my attempt to clarify.

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

I took this nomenclature from `condition-case` where we say:

    [...]
    Here @var{conditions} is an error condition name to be handled, or
    a list of condition names (which can include @code{debug} to allow
    the debugger to run before the handler).

I added a mention that these names are symbols.

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

I'm trying to explain that when looking for a handler, we look both for
condition-case handler and handler-bind handlers and we use whichever is
"closest", i.e. more deeply nested.  So just like a local
`condition-case` overrides temporarily an outer one, the same holds not
only among `handler-bind`s but also between `condition-case` and
`handler-bind` as well.

See the patch for my attempt to clarify.

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

No, it's I like to call the error object:

    @cindex error description
    The argument @var{var} is a variable.  @code{condition-case} does not
    bind this variable when executing the @var{protected-form}, only when it
    handles an error.  At that time, it binds @var{var} locally to an
    @dfn{error description}, which is a list giving the particulars of the
    error.  The error description has the form @code{(@var{error-symbol}
    . @var{data})}.  The handler can refer to this list to decide what to
    do.  For example, if the error is for failure opening a file, the file
    name is the second element of @var{data}---the third element of the
    error description.

> If so, let's please use consistent wording to minimize the chances for
> confusion and misunderstandings.

I wasn't familiar with the term "error description" either, but that's
apparently what we use in the Texinfo doc, so I used it specifically to
try and "use consistent wording" :-)

>> +@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"?

It's not related to `handler-bind` in any case, so if
we want to say something about it, we should do it elsewhere (and
I think we already do when we discuss lexical binding).

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

Indeed, it's a delicate part of he semantics (the one that introduces
the need for the SKIP_CONDITIONS thingy in the C code).
Let me know how you like the new text below.

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

I added two examples.

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

Hmm... yes that's not right, thanks.

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

OK.

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

OK.

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

Just a namespace uncleanliness I noticed when working on this code.

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

Thanks.

>> +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"?

The actual error object (aka "error description").

> If "error", then what does "error matches CONDITIONS" mean
> in practice?

It means that the "type" or "error name" of the error if a "subtype" of
one of CONDITIONS.

Given that we represent errors as (ERROR-NAME . ERROR-DATA) rather than
as objects, the "type" of an error is just the ERROR-NAME symbol.
And the "subtyping" relation is defined by the `parent` arg to
`define-error`.

This is exactly the same as for `condition-case`, of course.

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

Thanks, yes.

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

Same as for `condition-case`, we keep searching in surrounding handlers.
Not sure how to say it more clearly without becoming too verbose for
a docstring.  That's what the Texinfo doc is for, no?

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

Haven't implemented that yet.
`handler-bind` is not going to be used nearly as often as
`condition-case`, but I don't think it's very high priority to implement
this feature.
[ A more important completion feature in this area would be to complete
  the name of existing generic functions after `cl-defmethod` :-)  ]

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

That's the "redisplay_counter" patch we discussed elsewhere.  I have it
in the `scratch/handler-bind-2` branch, but it's not really related to
`handler-bind`.

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

Of course :-)

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

Yup.  For the first, I'm not yet sure if it really makes
`signal-hook-function` obsolete (I have PoC patches on `handler-bind-2`
to remove existing uses, but I'm not sure these DTRT).
For the second, it's an API problem we can't really fix without breaking
backward compatibility.  If we're lucky, we can declare
`signal-hook-function` obsolete and those problems will disappear.

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

Here, I'm really asking reviewers whether they think we should use
`list_of_error` here (which would require making it non-static and
declaring it in `lisp.h` or somesuch).

>> +;; (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?

The commit message explains it:

    Now that `ert.el` uses `handler-bind` instead of `debugger`, some
    details of the behavior have changed.  More specifically,
    three tests are now broken, but these basically tested the failure
    of ERT's machinery to record errors when ERT was run within
    a `condition-case`.
    AFAICT, these tests do not check for a behavior that we want,
    so rather than "fix" them, I disabled them.

Maybe I should delete them rather than comment them out?

I guess I could also keep them commented out but with the above commit
message turned into a comment, but in that case I have to move those
3 tests so they can be together next to the new comment.

Either way works for me.

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

Yup.


        Stefan