Package: emacs;
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.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 74052 in the body.
You can then email your comments to 74052 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
View this report as an mbox folder, status mbox, maintainer mbox
monnier <at> iro.umontreal.ca, bug-gnu-emacs <at> gnu.org:bug#74052; Package emacs.
(Sun, 27 Oct 2024 21:56:02 GMT) Full text and rfc822 format available.Alan Mackenzie <acm <at> muc.de>:monnier <at> iro.umontreal.ca, bug-gnu-emacs <at> gnu.org.
(Sun, 27 Oct 2024 21:56:02 GMT) Full text and rfc822 format available.Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
From: Alan Mackenzie <acm <at> muc.de> To: bug-gnu-emacs <at> gnu.org Cc: acm <at> muc.de Subject: Master: edebug fails to instrument nested pcase guard form. Date: Sun, 27 Oct 2024 21:54:29 +0000
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).
bug-gnu-emacs <at> gnu.org:bug#74052; Package emacs.
(Mon, 28 Oct 2024 13:51:02 GMT) Full text and rfc822 format available.Message #8 received at 74052 <at> debbugs.gnu.org (full text, mbox):
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: Alan Mackenzie <acm <at> muc.de> Cc: 74052 <at> debbugs.gnu.org Subject: Re: bug#74052: Master: edebug fails to instrument nested pcase guard form. Date: Mon, 28 Oct 2024 09:49:55 -0400
Hi Alan,
> #########################################################################
>
> 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.
+1
> 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."
+1
> + ;; 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".
> @@ -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)))
[ Having a hard time imagining how I ended up writing it like I did. ]
+1
> @@ -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`).
And thanks!
Stefan
Alan Mackenzie <acm <at> muc.de>:Alan Mackenzie <acm <at> muc.de>:Message #13 received at 74052-done <at> debbugs.gnu.org (full text, mbox):
From: Alan Mackenzie <acm <at> muc.de> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: Dmitry Gutov <dmitry <at> gutov.dev>, 74052-done <at> debbugs.gnu.org, acm <at> muc.de Subject: Re: bug#74052: Master: edebug fails to instrument nested pcase guard form. Date: Mon, 28 Oct 2024 16:46:59 +0000
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).
Debbugs Internal Request <help-debbugs <at> gnu.org>
to internal_control <at> debbugs.gnu.org.
(Tue, 26 Nov 2024 12:24:06 GMT) Full text and rfc822 format available.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.