Subject: bug#74052: closed (Re: bug#74052: Master: edebug fails to instrument nested pcase guard form.)

From: [email-masked] (GNU bug Tracking System)
Date: Mon, 28 Oct 2024 16:48:02 +0000

Your bug report

#74052: Master: edebug fails to instrument nested pcase guard form.

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
74052: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=74052
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#74052: Master: edebug fails to instrument nested pcase guard form.
From: Alan Mackenzie <[email-masked]>
To: Stefan Monnier <[email-masked]>
Cc: Dmitry Gutov <[email-masked]>, [email-masked], [email-masked]
Date: Mon, 28 Oct 2024 16:46:59 +0000
List-Id: <debbugs-submit.debbugs.gnu.org>

Hello, Stefan.

On Mon, Oct 28, 2024 at 09:49:55 -0400, Stefan Monnier wrote:
> Hi Alan,

[ .... ]

> > +  ;; Note: PF is evaluated in FUN rather than in this function, so that
> > +  ;; it can use any dynamic bindings created there.

> Nitpick: PF is not "evaluated" but "called".

Nits are important.  :-)  I've corrected that.

[ .... ]

> > @@ -84,14 +84,17 @@ 'pcase-FUN
> >  (defun pcase--edebug-match-pat-args (head pf)
> >    ;; (cl-assert (null (cdr head)))
> >    (setq head (car head))
> > -  (or (alist-get head '((quote sexp)
> > -                        (or    &rest pcase-PAT)
> > -                        (and   &rest pcase-PAT)
> > -                        (guard form)
> > -                        (pred  &or ("not" pcase-FUN) pcase-FUN)
> > -                        (app   pcase-FUN pcase-PAT)))
> > +  (let ((specs
> > +         (alist-get head '((quote sexp)
> > +                           (or    &rest pcase-PAT)
> > +                           (and   &rest pcase-PAT)
> > +                           (guard form)
> > +                           (pred  &or ("not" pcase-FUN) pcase-FUN)
> > +                           (app   pcase-FUN pcase-PAT)))))
> > +    (if specs
> > +        (funcall pf specs)
> >        (let ((me (pcase--get-macroexpander head)))
> > -        (funcall pf (and me (symbolp me) (edebug-get-spec me))))))
> > +        (funcall pf (and me (symbolp me) (edebug-get-spec me)))))))

> +1 (tho I'd hoist the common `(funcall pf` out of the `if`).

Done that too, by a slight rearrangement of the code.

I've committed the (corrected) patch, and I'm closing the bug with this
post.

> And thanks!

And thank you too for such a quick reply!

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Subject: Master: edebug fails to instrument nested pcase guard form.
From: Alan Mackenzie <[email-masked]>
To: [email-masked]
Cc: [email-masked]
Date: Sun, 27 Oct 2024 21:54:29 +0000
List-Id: <debbugs-submit.debbugs.gnu.org>

Hello, Emacs.

With a recent master version (and most likely, and emacs-30 branch
version):

(i) emacs -Q
(ii) insert the following into *scratch*:

(defun foo ()
  (let (
      )
    (match-b

  , leaving point after match-b.
(iii) C-x C-f path/to/lisp/progmodes/elisp-mode.el.
(iv) Move to function elisp-completion-at-point and instrument it for
  edebug with C-u C-M-x.
(v) C-x b *scratch* RET.
(vi) M-TAB # to invoke completion on match-b.
(vii) Step through elisp-completion-at-point using edebug commands
  aiming to step through the pcase guard clause at L788.
(viii) Note that point doesn't stop in this guard clause.  This is a
  bug.

#########################################################################

Diagnosis:
----------

In pcase.el pcase--edebug-match-pat-args, which is the function of an
edebug &interpose clause, the code fails to call PF (the supplied
parsing function).  This call is absolutely required by the interface of
the pertinent version of function edebug--match-&-spec-op.  It's absence
leads to the lack of instrumentation of the guard form.

#########################################################################

Proposed patch:
---------------

As well as fixing pcase--edebug-match-pat-args, the patch also corrects
bugs in the doc string of the pertinent version of
edebug--match-&-spec-op, and simplifies a use of `apply'.

diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index b96d2437b8a..f6710de126c 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -1803,12 +1803,21 @@ edebug-&optional-wrapper
 
 (cl-defmethod edebug--match-&-spec-op ((_ (eql '&interpose)) cursor specs)
   "Compute the specs for `&interpose SPEC FUN ARGS...'.
-Extracts the head of the data by matching it against SPEC,
-and then matches the rest by calling (FUN HEAD PF ARGS...)
-where PF is the parsing function which FUN can call exactly once,
-passing it the specs that it needs to match.
-Note that HEAD will always be a list, since specs are defined to match
-a sequence of elements."
+SPECS is a list (SPEC FUN ARGS...), where SPEC is an edebug
+specification, FUN is the function from the &interpose form which
+transforms the edebug spec, and the optional ARGS is a list of final
+arguments to be supplied to FUN.
+
+Extracts the head of the data by matching it against SPEC, and then
+matches the rest by calling (FUN HEAD PF ARGS...).  PF is the parsing
+function which FUN must call exactly once, passing it one argument, the
+specs that it needs to match.  FUN's value must be the value of this PF
+call, which in turn will be the value of this function.
+
+Note that HEAD will always be a list, since specs is defined to match a
+sequence of elements."
+  ;; Note: PF is evaluated in FUN rather than in this function, so that
+  ;; it can use any dynamic bindings created there.
   (pcase-let*
       ((`(,spec ,fun . ,args) specs)
        (exps (edebug-cursor-expressions cursor))
@@ -1817,14 +1826,14 @@ edebug-&optional-wrapper
                     (length (edebug-cursor-expressions cursor))))
        (head (seq-subseq exps 0 consumed)))
     (cl-assert (eq (edebug-cursor-expressions cursor) (nthcdr consumed exps)))
-    (apply fun `(,head
-                 ,(lambda (newspecs)
-                    ;; FIXME: What'd be the difference if we used
-                    ;; `edebug-match-sublist', which is what
-                    ;; `edebug-list-form-args' uses for the similar purpose
-                    ;; when matching "normal" forms?
-                    (append instrumented-head (edebug-match cursor newspecs)))
-                 ,@args))))
+    (apply fun head
+               (lambda (newspecs)
+                 ;; FIXME: What'd be the difference if we used
+                 ;; `edebug-match-sublist', which is what
+                 ;; `edebug-list-form-args' uses for the similar purpose
+                 ;; when matching "normal" forms?
+                 (append instrumented-head (edebug-match cursor newspecs)))
+               args)))
 
 (cl-defmethod edebug--match-&-spec-op ((_ (eql '&not)) cursor specs)
   ;; If any specs match, then fail
diff --git a/lisp/emacs-lisp/pcase.el b/lisp/emacs-lisp/pcase.el
index 898d460c144..bd0868b3ff0 100644
--- a/lisp/emacs-lisp/pcase.el
+++ b/lisp/emacs-lisp/pcase.el
@@ -84,14 +84,17 @@ 'pcase-FUN
 (defun pcase--edebug-match-pat-args (head pf)
   ;; (cl-assert (null (cdr head)))
   (setq head (car head))
-  (or (alist-get head '((quote sexp)
-                        (or    &rest pcase-PAT)
-                        (and   &rest pcase-PAT)
-                        (guard form)
-                        (pred  &or ("not" pcase-FUN) pcase-FUN)
-                        (app   pcase-FUN pcase-PAT)))
+  (let ((specs
+         (alist-get head '((quote sexp)
+                           (or    &rest pcase-PAT)
+                           (and   &rest pcase-PAT)
+                           (guard form)
+                           (pred  &or ("not" pcase-FUN) pcase-FUN)
+                           (app   pcase-FUN pcase-PAT)))))
+    (if specs
+        (funcall pf specs)
       (let ((me (pcase--get-macroexpander head)))
-        (funcall pf (and me (symbolp me) (edebug-get-spec me))))))
+        (funcall pf (and me (symbolp me) (edebug-get-spec me)))))))
 
 (defun pcase--get-macroexpander (s)
   "Return the macroexpander for pcase pattern head S, or nil."

-- 
Alan Mackenzie (Nuremberg, Germany).