GNU bug report logs -
#74052
Master: edebug fails to instrument nested pcase guard form.
Previous Next
Reported by: Alan Mackenzie <acm <at> muc.de>
Date: Sun, 27 Oct 2024 21:56:02 UTC
Severity: normal
Done: Alan Mackenzie <acm <at> muc.de>
Bug is archived. No further changes may be made.
Full log
View this message in rfc822 format
[Message part 1 (text/plain, inline)]
Your message dated Mon, 28 Oct 2024 16:46:59 +0000
with message-id <Zx_AA3F6mL3r_xCW <at> MAC.fritz.box>
and subject line Re: bug#74052: Master: edebug fails to instrument nested pcase guard form.
has caused the debbugs.gnu.org bug report #74052,
regarding Master: edebug fails to instrument nested pcase guard form.
to be marked as done.
(If you believe you have received this mail in error, please contact
help-debbugs <at> gnu.org.)
--
74052: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=74052
GNU Bug Tracking System
Contact help-debbugs <at> gnu.org with problems
[Message part 2 (message/rfc822, inline)]
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 '¬)) 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).
[Message part 3 (message/rfc822, inline)]
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).
This bug report was last modified 286 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.