From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#68075: 30.0.50; New special form `handler-bind` Date: Thu, 28 Dec 2023 10:03:16 +0200 Message-ID: <835y0i92kb.fsf@gnu.org> References: Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="29127"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 68075@debbugs.gnu.org To: Stefan Monnier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Thu Dec 28 09:04:14 2023 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1rIlNF-0007MB-VY for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 28 Dec 2023 09:04:14 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rIlN6-0003rh-AV; Thu, 28 Dec 2023 03:04:04 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rIlN5-0003rY-8C for bug-gnu-emacs@gnu.org; Thu, 28 Dec 2023 03:04:03 -0500 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1rIlN5-0001gd-03 for bug-gnu-emacs@gnu.org; Thu, 28 Dec 2023 03:04:03 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1rIlN4-0004Ok-2b for bug-gnu-emacs@gnu.org; Thu, 28 Dec 2023 03:04:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 28 Dec 2023 08:04:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 68075 X-GNU-PR-Package: emacs Original-Received: via spool by 68075-submit@debbugs.gnu.org id=B68075.170375062116874 (code B ref 68075); Thu, 28 Dec 2023 08:04:02 +0000 Original-Received: (at 68075) by debbugs.gnu.org; 28 Dec 2023 08:03:41 +0000 Original-Received: from localhost ([127.0.0.1]:38358 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rIlMi-0004O5-3K for submit@debbugs.gnu.org; Thu, 28 Dec 2023 03:03:40 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:44366) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rIlMg-0004Ns-54 for 68075@debbugs.gnu.org; Thu, 28 Dec 2023 03:03:39 -0500 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rIlMa-0001f7-8Q; Thu, 28 Dec 2023 03:03:32 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=G3poZ8LlkoI2xGAc8fdjCFt2brITKIVdGWvx+7v8a1g=; b=qh9QZlSiv3nl AqBw81yl/1tW/Ms9gWIWGcLOJ3KalSoI2PNa3obI4RrTyJkzAIvkqTfU2ByqHAuH5TKOmZe0GIBlI 84mUxePrXi6y5AyGimfmztxC7k2bTaZOXuHlQki7nkOmdrJE4VyCMglomKx/hO69Fu4PYITSCxLc/ G2N034R8q4Oz70+uiF2Hp1AFkH/rZpTzbyPzBYIDWfB6tDja88fd1ndqfi316nEE+LMFNH8TQqCh9 v2IskpxoWkmhmrudUuryJiKcwHSro4l6jsfYH11MLejxBSjZ6dGS0QKh/Q+kSgE5AG4DkbMvjNf8b iL2ek9+ayVjpM1L6eD3uKQ==; In-Reply-To: (bug-gnu-emacs@gnu.org) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:276959 Archived-At: > 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" > > 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.