master 53dfd85: Edebug: Disable backtracking when hitting a &define keyword.

[Philipp Stephani]

branch: master
commit 53dfd85a7f971875e716a55f010ee508bce89eed
Author: Philipp Stephani <phst@google.com>
Commit: Philipp Stephani <phst@google.com>

    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 8942f55..323130f 100644
--- a/doc/lispref/edebug.texi
+++ b/doc/lispref/edebug.texi
@@ -1510,11 +1510,11 @@ form specifications (that is, @code{form}, @code{body}, 
@code{def-form}, and
 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 a0f05d8..9ae3740 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 f1455ff..365bc74 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -1942,14 +1942,16 @@ a sequence of elements."
   ;; 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 dcb261c..7d45432 100644
--- a/test/lisp/emacs-lisp/edebug-tests.el
+++ b/test/lisp/emacs-lisp/edebug-tests.el
@@ -1061,5 +1061,30 @@ backtracking (Bug#42701)."
                        "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