Package: emacs;
Reported by: Philipp <p.stephani2 <at> gmail.com>
Date: Sun, 21 Jun 2020 17:00:02 UTC
Severity: normal
Found in version 28.0.50
Done: Philipp Stephani <p.stephani2 <at> gmail.com>
Bug is archived. No further changes may be made.
Message #73 received at 41988 <at> debbugs.gnu.org (full text, mbox):
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: Philipp <p.stephani2 <at> gmail.com> Cc: 41988 <at> debbugs.gnu.org Subject: Re: bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs Date: Sat, 10 Apr 2021 11:51:50 -0400
> Here's a patch.
LGTM, as far as I'm concerned you can push it to `master` and we'll just
cross our fingers ;-)
Stefan
> From 9e2183edea41adf275f057a75232ea0b2c51e726 Mon Sep 17 00:00:00 2001
> From: Philipp Stephani <phst <at> 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.
> +
>
> * 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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.