From: Philipp <p.stephani2@gmail.com>
To: Stefan Monnier <monnier@iro.umontreal.ca>
Cc: 41988@debbugs.gnu.org
Subject: bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs
Date: Sat, 10 Apr 2021 17:07:48 +0200 [thread overview]
Message-ID: <BF0D4E7E-D962-49BE-913F-3D6669156216@gmail.com> (raw)
In-Reply-To: <CAArVCkTOX6kDAF6KHFy7BJzJAeO4w15mFdUkZVwTgE-SJqf9rg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1204 bytes --]
> Am 05.04.2021 um 16:32 schrieb Philipp Stephani <p.stephani2@gmail.com>:
>
>>>>> so I think it would be reasonable to prevent. We already
>>>>> disable backtracking for literal symbols, and I think forms that require
>>>>> multiple &define forms with backtracking should be exceedingly rare and can
>>>>> be rewritten as you did with cl-flet.
>>>> Emitting a warning would be much more helpful than just silently
>>>> "cut"ting the backtracking.
>>> A gate isn't silent, it would cause a hard error in this case.
>>
>> What I meant is that a gate would just make the old cl-flet spec fail in
>> most cases, with no explanation why that spec now fails even though it
>> worked in the past.
>
> Yes, we'd need to at least announce the change in NEWS as an
> incompatible Lisp change. However, I think overall that's still better
> than the current situation: with your fix to cl-flet the problematic
> constructs shouldn't occur any more in the Emacs codebase, and I
> wouldn't expect such constructs to be very frequent "in the wild", so
> the overall breakage would be very small, and if it can avoid
> similarly subtle bugs then I think it's warranted.
Here's a patch.
[-- Attachment #2: 0001-Edebug-Disable-backtracking-when-hitting-a-define-ke.patch --]
[-- Type: application/octet-stream, Size: 5544 bytes --]
From 9e2183edea41adf275f057a75232ea0b2c51e726 Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Thu, 18 Mar 2021 12:40:08 +0100
Subject: [PATCH] Edebug: Disable backtracking when hitting a &define keyword.
Edebug doesn't deal well with backtracking out of definitions, see
Bug#41988. Rather than trying to support this rare situation (e.g. by
implementing a multipass parser), prevent it by adding an implicit
gate.
* lisp/emacs-lisp/edebug.el (edebug--match-&-spec-op): Disable
backtracking when hitting a &define keyword.
* test/lisp/emacs-lisp/edebug-tests.el
(edebug-tests-duplicate-&define): New unit test.
(edebug-tests--duplicate-&define): New helper macro.
* doc/lispref/edebug.texi (Backtracking): Mention &define in the list
of constructs that disable backtracking.
* etc/NEWS: Document new behavior.
---
doc/lispref/edebug.texi | 10 +++++-----
etc/NEWS | 3 +++
lisp/emacs-lisp/edebug.el | 18 ++++++++++--------
test/lisp/emacs-lisp/edebug-tests.el | 25 +++++++++++++++++++++++++
4 files changed, 43 insertions(+), 13 deletions(-)
diff --git a/doc/lispref/edebug.texi b/doc/lispref/edebug.texi
index 8942f55aff..323130f237 100644
--- a/doc/lispref/edebug.texi
+++ b/doc/lispref/edebug.texi
@@ -1510,11 +1510,11 @@ Backtracking
must be in the form itself rather than at a higher level.
Backtracking is also disabled after successfully matching a quoted
-symbol or string specification, since this usually indicates a
-recognized construct. But if you have a set of alternative constructs that
-all begin with the same symbol, you can usually work around this
-constraint by factoring the symbol out of the alternatives, e.g.,
-@code{["foo" &or [first case] [second case] ...]}.
+symbol, string specification, or @code{&define} keyword, since this
+usually indicates a recognized construct. But if you have a set of
+alternative constructs that all begin with the same symbol, you can
+usually work around this constraint by factoring the symbol out of the
+alternatives, e.g., @code{["foo" &or [first case] [second case] ...]}.
Most needs are satisfied by these two ways that backtracking is
automatically disabled, but occasionally it is useful to explicitly
diff --git a/etc/NEWS b/etc/NEWS
index a0f05d8cf1..9ae3740482 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -2524,6 +2524,9 @@ back in Emacs 23.1. The affected functions are: 'make-obsolete',
** The 'values' variable is now obsolete.
+** The '&define' keyword in an Edebug specification now disables
+backtracking.
+
\f
* Lisp Changes in Emacs 28.1
diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index f1455ffe73..365bc74874 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -1942,14 +1942,16 @@ edebug--match-&-spec-op
;; Normally, &define is interpreted specially other places.
;; This should only be called inside of a spec list to match the remainder
;; of the current list. e.g. ("lambda" &define args def-body)
- (edebug-make-form-wrapper
- cursor
- (edebug-before-offset cursor)
- ;; Find the last offset in the list.
- (let ((offsets (edebug-cursor-offsets cursor)))
- (while (consp offsets) (setq offsets (cdr offsets)))
- offsets)
- specs))
+ (prog1 (edebug-make-form-wrapper
+ cursor
+ (edebug-before-offset cursor)
+ ;; Find the last offset in the list.
+ (let ((offsets (edebug-cursor-offsets cursor)))
+ (while (consp offsets) (setq offsets (cdr offsets)))
+ offsets)
+ specs)
+ ;; Stop backtracking here (Bug#41988).
+ (setq edebug-gate t)))
(cl-defmethod edebug--match-&-spec-op ((_ (eql &name)) cursor specs)
"Compute the name for `&name SPEC FUN` spec operator.
diff --git a/test/lisp/emacs-lisp/edebug-tests.el b/test/lisp/emacs-lisp/edebug-tests.el
index dcb261c2eb..7d45432e57 100644
--- a/test/lisp/emacs-lisp/edebug-tests.el
+++ b/test/lisp/emacs-lisp/edebug-tests.el
@@ -1061,5 +1061,30 @@ edebug-tests-duplicate-symbol-backtrack
"edebug-anon10001"
"edebug-tests-duplicate-symbol-backtrack"))))))
+(defmacro edebug-tests--duplicate-&define (_arg)
+ "Helper macro for the ERT test `edebug-tests-duplicate-&define'.
+The Edebug specification is similar to the one used by `cl-flet'
+previously; see Bug#41988."
+ (declare (debug (&or (&define name function-form) (defun)))))
+
+(ert-deftest edebug-tests-duplicate-&define ()
+ "Check that Edebug doesn't backtrack out of `&define' forms.
+This avoids potential duplicate definitions (Bug#41988)."
+ (with-temp-buffer
+ (print '(defun edebug-tests-duplicate-&define ()
+ (edebug-tests--duplicate-&define
+ (edebug-tests-duplicate-&define-inner () nil)))
+ (current-buffer))
+ (let* ((edebug-all-defs t)
+ (edebug-initial-mode 'Go-nonstop)
+ (instrumented-names ())
+ (edebug-new-definition-function
+ (lambda (name)
+ (when (memq name instrumented-names)
+ (error "Duplicate definition of `%s'" name))
+ (push name instrumented-names)
+ (edebug-new-definition name))))
+ (should-error (eval-buffer) :type 'invalid-read-syntax))))
+
(provide 'edebug-tests)
;;; edebug-tests.el ends here
--
2.24.3 (Apple Git-128)
next prev parent reply other threads:[~2021-04-10 15:07 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-21 16:58 bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs Philipp
[not found] ` <mailman.222.1592758804.2574.bug-gnu-emacs@gnu.org>
2020-06-21 23:48 ` Alan Mackenzie
2020-08-08 11:01 ` Philipp Stephani
2020-08-08 14:59 ` Alan Mackenzie
2020-08-09 11:33 ` Philipp Stephani
2020-08-09 16:35 ` Alan Mackenzie
2020-08-10 13:32 ` Philipp Stephani
2021-03-02 15:59 ` Stefan Monnier
2021-03-02 17:28 ` Philipp Stephani
2021-03-08 16:33 ` Philipp Stephani
2021-03-08 16:37 ` Philipp Stephani
2021-03-08 17:41 ` Stefan Monnier
2021-03-14 16:32 ` Philipp
2021-03-14 17:38 ` Stefan Monnier
2021-03-18 11:19 ` Philipp
2021-03-18 14:01 ` Stefan Monnier
2021-03-21 13:34 ` Philipp
2021-03-21 14:37 ` Stefan Monnier
2021-04-04 18:40 ` Philipp Stephani
2021-04-04 20:16 ` Stefan Monnier
2021-04-05 14:32 ` Philipp Stephani
2021-04-10 15:07 ` Philipp [this message]
2021-04-10 15:51 ` Stefan Monnier
2021-04-10 16:23 ` Philipp
2021-04-10 17:29 ` Eli Zaretskii
2021-04-10 18:12 ` Stefan Monnier
2021-04-10 19:54 ` Philipp
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BF0D4E7E-D962-49BE-913F-3D6669156216@gmail.com \
--to=p.stephani2@gmail.com \
--cc=41988@debbugs.gnu.org \
--cc=monnier@iro.umontreal.ca \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).