* bug#74052: Master: edebug fails to instrument nested pcase guard form.
@ 2024-10-27 21:54 Alan Mackenzie
2024-10-28 13:49 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 3+ messages in thread
From: Alan Mackenzie @ 2024-10-27 21:54 UTC (permalink / raw)
To: 74052; +Cc: acm, Stefan Monnier
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).
^ permalink raw reply related [flat|nested] 3+ messages in thread
* bug#74052: Master: edebug fails to instrument nested pcase guard form.
2024-10-27 21:54 bug#74052: Master: edebug fails to instrument nested pcase guard form Alan Mackenzie
@ 2024-10-28 13:49 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-10-28 16:46 ` Alan Mackenzie
0 siblings, 1 reply; 3+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-10-28 13:49 UTC (permalink / raw)
To: Alan Mackenzie; +Cc: 74052
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* bug#74052: Master: edebug fails to instrument nested pcase guard form.
2024-10-28 13:49 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-10-28 16:46 ` Alan Mackenzie
0 siblings, 0 replies; 3+ messages in thread
From: Alan Mackenzie @ 2024-10-28 16:46 UTC (permalink / raw)
To: Stefan Monnier; +Cc: Dmitry Gutov, 74052-done, acm
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).
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-10-28 16:46 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-27 21:54 bug#74052: Master: edebug fails to instrument nested pcase guard form Alan Mackenzie
2024-10-28 13:49 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-10-28 16:46 ` Alan Mackenzie
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).