From: Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
To: Alan Mackenzie <acm@muc.de>
Cc: 74052@debbugs.gnu.org
Subject: bug#74052: Master: edebug fails to instrument nested pcase guard form.
Date: Mon, 28 Oct 2024 09:49:55 -0400 [thread overview]
Message-ID: <jwvldy8l53n.fsf-monnier+emacs@gnu.org> (raw)
In-Reply-To: <Zx62lYyJrqBg2PtD@MAC.fritz.box> (Alan Mackenzie's message of "Sun, 27 Oct 2024 21:54:29 +0000")
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
next prev parent reply other threads:[~2024-10-28 13:49 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2024-10-28 16:46 ` Alan Mackenzie
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=jwvldy8l53n.fsf-monnier+emacs@gnu.org \
--to=bug-gnu-emacs@gnu.org \
--cc=74052@debbugs.gnu.org \
--cc=acm@muc.de \
--cc=monnier@iro.umontreal.ca \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.