From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" Newsgroups: gmane.emacs.bugs Subject: bug#68075: 30.0.50; New special form `handler-bind` Date: Thu, 28 Dec 2023 13:12:12 -0500 Message-ID: References: <835y0i92kb.fsf@gnu.org> Reply-To: Stefan Monnier Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="33437"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: 68075@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Thu Dec 28 19:13:22 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 1rIusk-0008UN-39 for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 28 Dec 2023 19:13:22 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rIusT-00033H-Jd; Thu, 28 Dec 2023 13:13:05 -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 1rIusQ-000331-Nv for bug-gnu-emacs@gnu.org; Thu, 28 Dec 2023 13:13: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 1rIusQ-0003tN-Fi for bug-gnu-emacs@gnu.org; Thu, 28 Dec 2023 13:13:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1rIusP-0007ZZ-R0 for bug-gnu-emacs@gnu.org; Thu, 28 Dec 2023 13:13:01 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Monnier Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 28 Dec 2023 18:13:01 +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.170378715029063 (code B ref 68075); Thu, 28 Dec 2023 18:13:01 +0000 Original-Received: (at 68075) by debbugs.gnu.org; 28 Dec 2023 18:12:30 +0000 Original-Received: from localhost ([127.0.0.1]:40317 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rIurt-0007Yg-8f for submit@debbugs.gnu.org; Thu, 28 Dec 2023 13:12:30 -0500 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:25513) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rIurp-0007YP-6y for 68075@debbugs.gnu.org; Thu, 28 Dec 2023 13:12:27 -0500 Original-Received: from pmg2.iro.umontreal.ca (localhost.localdomain [127.0.0.1]) by pmg2.iro.umontreal.ca (Proxmox) with ESMTP id 7B81A831F3; Thu, 28 Dec 2023 13:12:19 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1703787133; bh=BpfdZMzHDNPSwo8tApFUo3WYtzKvO7qd/Kn90N0zXq4=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=QLZ4R5Fy7bKf5C/a3BJfyqiuHLWzreWG1xFWtFBwslh8c/q1WY69OSa5S1lgH2Yn1 8JWTTO+qOVw+CBirMldBQcoOEIsPy9KczzuJgIRJHO3PrzM9FaD7l+uDOrbbu3GqtN Hv3lDDp3oBG6LTBecTevTsXTm/vu9Qcht0eYBXbrlRVf9TSs1bPS0c3CCipPXBrLuU oVQBLBwz2xvVLBw2i1Pi58Rny4KYgxjAonzhVOUpYMXZqzW0QOz9fT6cMT0L1ZGkeo imqz9jr92G7tcGwCqSyzsSBk8Z6Wpq8lOmZWTbZ/MkfOibFbRo7QdYMOVLWVXPlXKE tI2r9Fp89DxOw== Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg2.iro.umontreal.ca (Proxmox) with ESMTP id 83BE980C88; Thu, 28 Dec 2023 13:12:13 -0500 (EST) Original-Received: from pastel (65-110-221-238.cpe.pppoe.ca [65.110.221.238]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id 5B0E112110F; Thu, 28 Dec 2023 13:12:13 -0500 (EST) In-Reply-To: <835y0i92kb.fsf@gnu.org> (Eli Zaretskii's message of "Thu, 28 Dec 2023 10:03:16 +0200") 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:276988 Archived-At: >> 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