Package: emacs;
To reply to this bug, email your comments to 68938 AT debbugs.gnu.org.
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#68938; Package emacs.
(Mon, 05 Feb 2024 17:20:01 GMT) Full text and rfc822 format available.Alan Mackenzie <acm <at> muc.de>:monnier <at> iro.umontreal.ca, bug-gnu-emacs <at> gnu.org.
(Mon, 05 Feb 2024 17:20:01 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 Subject: Emacs "master". Incorrect code generated by pcase. Date: Mon, 5 Feb 2024 17:18:40 +0000
Hello Stefan and Emacs.
In a development version of Emacs, last synched with master in December,
I have added the following pcase clause to macroexp--expand-all in
lisp/emacs-lisp/macroexp.el:
(`(,(and 'defalias d
(guard (and (null defining-symbol)
(symbol-with-pos-p d))))
',sym . ,_)
;; Here, don't change the form; just set `defining-symbol'
;; for a (defalias 'foo ...) in the source code.
;; (when (symbol-with-pos-p d)
(setq defining-symbol sym)
form)
.. pcase expands that clause to this cond clause:
((eq x0 'defalias)
(cond
((let* ((d x0))
(and (null defining-symbol) (symbol-with-pos-p d)))
(let* ((x14 (cdr-safe form)))
(cond
((consp x14)
(let* ((x15 (car-safe x14)))
(cond
((consp x15)
(let* ((x16 (car-safe x15)))
(cond
((eq x16 'quote)
(let* ((x17 (cdr-safe x15)))
(cond
((consp x17)
(let* ((x18 (car-safe x17)) (x19 (cdr-safe x17)))
(cond
((null x19)
(let ((d x0) (sym x18))
(ignore d) (setq defining-symbol sym) form))
((consp x0)
(let* ((x21 (car-safe x0)))
(if (eq x21 'lambda) (funcall pcase-3 x0 x14)
(funcall pcase-2 x0))))
(t (funcall pcase-2 x0)))))
((consp x0)
(let* ((x23 (car-safe x0)))
(if (eq x23 'lambda) (funcall pcase-3 x0 x14)
(funcall pcase-2 x0))))
(t (funcall pcase-2 x0)))))
((consp x0)
(let* ((x25 (car-safe x0)))
(if (eq x25 'lambda) (funcall pcase-3 x0 x14)
(funcall pcase-2 x0))))
(t (funcall pcase-2 x0)))))
((consp x0)
(let* ((x27 (car-safe x0)))
(if (eq x27 'lambda) (funcall pcase-3 x0 x14)
(funcall pcase-2 x0))))
(t (funcall pcase-2 x0)))))
((consp x0)
(let* ((x29 (car-safe x0)))
(if (eq x29 'lambda) (funcall pcase-3 x0 x14) (funcall pcase-2 x0))))
(t (funcall pcase-2 x0)))))
((consp x0)
(let* ((x31 (car-safe x0)))
(if (eq x31 'lambda)
(let* ((x33 (cdr-safe form))) (funcall pcase-3 x0 x33))
(funcall pcase-2 x0))))
(t (funcall pcase-2 x0))))
.. This contains errors:
(i) Although it has been established that x0 is 'defalias, there are many
tests (consp x0).
(ii) There are calls of the form (funcall pcase-2 x0), i.e. (funcall
pcase-2 'defalias). This causes a wrong-number-of-arguments error.
(iii) There is no sign of the final `form' being returned, though this
may be being done elsewhere.
--
Alan Mackenzie (Nuremberg, Germany).
bug-gnu-emacs <at> gnu.org:bug#68938; Package emacs.
(Mon, 05 Feb 2024 18:28:02 GMT) Full text and rfc822 format available.Message #8 received at 68938 <at> debbugs.gnu.org (full text, mbox):
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: Alan Mackenzie <acm <at> muc.de> Cc: 68938 <at> debbugs.gnu.org Subject: Re: bug#68938: Emacs "master". Incorrect code generated by pcase. Date: Mon, 05 Feb 2024 13:27:21 -0500
> In a development version of Emacs, last synched with master in December,
> I have added the following pcase clause to macroexp--expand-all in
> lisp/emacs-lisp/macroexp.el:
>
> (`(,(and 'defalias d
> (guard (and (null defining-symbol)
> (symbol-with-pos-p d))))
> ',sym . ,_)
> ;; Here, don't change the form; just set `defining-symbol'
> ;; for a (defalias 'foo ...) in the source code.
> ;; (when (symbol-with-pos-p d)
> (setq defining-symbol sym)
> form)
Where did do you add it?
> .. pcase expands that clause to this cond clause:
>
> ((eq x0 'defalias)
> (cond
> ((let* ((d x0))
> (and (null defining-symbol) (symbol-with-pos-p d)))
> (let* ((x14 (cdr-safe form)))
> (cond
> ((consp x14)
> (let* ((x15 (car-safe x14)))
> (cond
> ((consp x15)
> (let* ((x16 (car-safe x15)))
> (cond
> ((eq x16 'quote)
> (let* ((x17 (cdr-safe x15)))
> (cond
> ((consp x17)
> (let* ((x18 (car-safe x17)) (x19 (cdr-safe x17)))
> (cond
> ((null x19)
> (let ((d x0) (sym x18))
> (ignore d) (setq defining-symbol sym) form))
> ((consp x0)
> (let* ((x21 (car-safe x0)))
> (if (eq x21 'lambda) (funcall pcase-3 x0 x14)
> (funcall pcase-2 x0))))
> (t (funcall pcase-2 x0)))))
> ((consp x0)
> (let* ((x23 (car-safe x0)))
> (if (eq x23 'lambda) (funcall pcase-3 x0 x14)
> (funcall pcase-2 x0))))
> (t (funcall pcase-2 x0)))))
> ((consp x0)
> (let* ((x25 (car-safe x0)))
> (if (eq x25 'lambda) (funcall pcase-3 x0 x14)
> (funcall pcase-2 x0))))
> (t (funcall pcase-2 x0)))))
> ((consp x0)
> (let* ((x27 (car-safe x0)))
> (if (eq x27 'lambda) (funcall pcase-3 x0 x14)
> (funcall pcase-2 x0))))
> (t (funcall pcase-2 x0)))))
> ((consp x0)
> (let* ((x29 (car-safe x0)))
> (if (eq x29 'lambda) (funcall pcase-3 x0 x14) (funcall pcase-2 x0))))
> (t (funcall pcase-2 x0)))))
> ((consp x0)
> (let* ((x31 (car-safe x0)))
> (if (eq x31 'lambda)
> (let* ((x33 (cdr-safe form))) (funcall pcase-3 x0 x33))
> (funcall pcase-2 x0))))
> (t (funcall pcase-2 x0))))
>
> .. This contains errors:
>
> (i) Although it has been established that x0 is 'defalias, there are many
> tests (consp x0).
Yup, clearly some missed optimization.
I added your code just before the
(`(function ,(and f `(lambda . ,_)))
branch, and I didn't see such poor code, so it seems that it depends on
further details.
On further inspection I see a similar problem in another branch,
where it generated:
((pcase--flip memq '(defconst defvar) x71)
(let* ((x78 (cdr-safe form)))
(cond
((consp x78)
(let* ((x79 (car-safe x78)))
(cond
((symbolp x79)
(let ((name x79))
(push name macroexp--dynvars)
(macroexp--all-forms form 2)))
((consp x71)
(let* ((x81 (car-safe x71)))
(if (eq x81 'lambda) (funcall pcase-2 x71 x78)
(funcall pcase-1 x71))))
(t (funcall pcase-1 x71)))))
((consp x71)
(let* ((x83 (car-safe x71)))
(if (eq x83 'lambda) (funcall pcase-2 x71 x78)
(funcall pcase-1 x71))))
(t (funcall pcase-1 x71)))))
where we do that same useless (consp x71) test. I think this case is
"normal" (the branch's test is basically (memq x71 '(defconst defvar),
i.e. more complex than (eq x71 'defalias)) and I seem to remember
consciously punting on handling such things in
`pcase--mutually-exclusive-p`.
Your case doesn't sound like one I'm aware of, OTOH.
> (ii) There are calls of the form (funcall pcase-2 x0), i.e. (funcall
> pcase-2 'defalias). This causes a wrong-number-of-arguments error.
I don't see this problem here. In my case it's
(funcall pcase-1 x71)
but the number of arguments is right since pcase-1 is defined a bit
earlier as:
(lambda (func)
(let ((handler (function-get func 'compiler-macro)))
(if (null handler) (macroexp--all-forms form 1)
(unless (functionp handler)
(with-demoted-errors "macroexp--expand-all: %S"
(autoload-do-load (indirect-function func) func)))
(let ((newform (macroexp--compiler-macro handler form)))
(if (eq form newform)
(if
(equal form
(setq newform
(macroexp--all-forms form 1)))
form
(setq form
(macroexp--compiler-macro handler newform))
(if (eq newform form) newform
(macroexp--expand-all form)))
(macroexp--expand-all newform))))))
> (iii) There is no sign of the final `form' being returned, though this
> may be being done elsewhere.
I see the following in your code sample:
(cond
((null x19)
(let ((d x0) (sym x18))
(ignore d) (setq defining-symbol sym) form))
which seems to be correctly returning `form`.
Stefan
bug-gnu-emacs <at> gnu.org:bug#68938; Package emacs.
(Tue, 06 Feb 2024 13:00:02 GMT) Full text and rfc822 format available.Message #11 received at 68938 <at> debbugs.gnu.org (full text, mbox):
From: Alan Mackenzie <acm <at> muc.de> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: acm <at> muc.de, 68938 <at> debbugs.gnu.org Subject: Re: bug#68938: Emacs "master". Incorrect code generated by pcase. Date: Tue, 6 Feb 2024 12:58:53 +0000
Hello, Stefan.
On Mon, Feb 05, 2024 at 13:27:21 -0500, Stefan Monnier wrote:
> > In a development version of Emacs, last synched with master in December,
> > I have added the following pcase clause to macroexp--expand-all in
> > lisp/emacs-lisp/macroexp.el:
> > (`(,(and 'defalias d
> > (guard (and (null defining-symbol)
> > (symbol-with-pos-p d))))
> > ',sym . ,_)
> > ;; Here, don't change the form; just set `defining-symbol'
> > ;; for a (defalias 'foo ...) in the source code.
> > ;; (when (symbol-with-pos-p d)
> > (setq defining-symbol sym)
> > form)
> Where did do you add it?
Just before the (`(function ,(and f `(lambda . ,_))) clause, like you
did.
> > .. pcase expands that clause to this cond clause:
> > ((eq x0 'defalias)
> > (cond
> > ((let* ((d x0))
> > (and (null defining-symbol) (symbol-with-pos-p d)))
> > (let* ((x14 (cdr-safe form)))
> > (cond
> > ((consp x14)
> > (let* ((x15 (car-safe x14)))
> > (cond
> > ((consp x15)
> > (let* ((x16 (car-safe x15)))
> > (cond
> > ((eq x16 'quote)
> > (let* ((x17 (cdr-safe x15)))
> > (cond
> > ((consp x17)
> > (let* ((x18 (car-safe x17)) (x19 (cdr-safe x17)))
> > (cond
> > ((null x19)
> > (let ((d x0) (sym x18))
> > (ignore d) (setq defining-symbol sym) form))
> > ((consp x0)
> > (let* ((x21 (car-safe x0)))
> > (if (eq x21 'lambda) (funcall pcase-3 x0 x14)
> > (funcall pcase-2 x0))))
> > (t (funcall pcase-2 x0)))))
> > ((consp x0)
> > (let* ((x23 (car-safe x0)))
> > (if (eq x23 'lambda) (funcall pcase-3 x0 x14)
> > (funcall pcase-2 x0))))
> > (t (funcall pcase-2 x0)))))
> > ((consp x0)
> > (let* ((x25 (car-safe x0)))
> > (if (eq x25 'lambda) (funcall pcase-3 x0 x14)
> > (funcall pcase-2 x0))))
> > (t (funcall pcase-2 x0)))))
> > ((consp x0)
> > (let* ((x27 (car-safe x0)))
> > (if (eq x27 'lambda) (funcall pcase-3 x0 x14)
> > (funcall pcase-2 x0))))
> > (t (funcall pcase-2 x0)))))
> > ((consp x0)
> > (let* ((x29 (car-safe x0)))
> > (if (eq x29 'lambda) (funcall pcase-3 x0 x14) (funcall pcase-2 x0))))
> > (t (funcall pcase-2 x0)))))
> > ((consp x0)
> > (let* ((x31 (car-safe x0)))
> > (if (eq x31 'lambda)
> > (let* ((x33 (cdr-safe form))) (funcall pcase-3 x0 x33))
> > (funcall pcase-2 x0))))
> > (t (funcall pcase-2 x0))))
> >
> > .. This contains errors:
> >
> > (i) Although it has been established that x0 is 'defalias, there are many
> > tests (consp x0).
> Yup, clearly some missed optimization.
> I added your code just before the
> (`(function ,(and f `(lambda . ,_)))
> branch, and I didn't see such poor code, so it seems that it depends on
> further details.
Yes. I think my bug report was premature at best. I was part way
through amending backquote.el to be able to add the position information
into things like the lambda in
(push `(,bsym (lambda ,(mapcar #'car varvals)
,@ignores ,@code))
defs))
in pcase--expand. I think this bit of code deals with creating pcase-1,
etc., so it seems highly likely my tentative changes were to blame.
> On further inspection I see a similar problem in another branch,
> where it generated:
> ((pcase--flip memq '(defconst defvar) x71)
> (let* ((x78 (cdr-safe form)))
> (cond
> ((consp x78)
> (let* ((x79 (car-safe x78)))
> (cond
> ((symbolp x79)
> (let ((name x79))
> (push name macroexp--dynvars)
> (macroexp--all-forms form 2)))
> ((consp x71)
> (let* ((x81 (car-safe x71)))
> (if (eq x81 'lambda) (funcall pcase-2 x71 x78)
> (funcall pcase-1 x71))))
> (t (funcall pcase-1 x71)))))
> ((consp x71)
> (let* ((x83 (car-safe x71)))
> (if (eq x83 'lambda) (funcall pcase-2 x71 x78)
> (funcall pcase-1 x71))))
> (t (funcall pcase-1 x71)))))
> where we do that same useless (consp x71) test. I think this case is
> "normal" (the branch's test is basically (memq x71 '(defconst defvar),
> i.e. more complex than (eq x71 'defalias)) and I seem to remember
> consciously punting on handling such things in
> `pcase--mutually-exclusive-p`.
> Your case doesn't sound like one I'm aware of, OTOH.
I think I should just close the bug as not a bug.
It was one of these things I couldn't understand or do anything about,
but right after reporting it, the solution became obvious, and I
couldn't reproduce the bug easily any more. I've got my amendments to
backquote.el working, now.
> > (ii) There are calls of the form (funcall pcase-2 x0), i.e. (funcall
> > pcase-2 'defalias). This causes a wrong-number-of-arguments error.
> I don't see this problem here. In my case it's
> (funcall pcase-1 x71)
> but the number of arguments is right since pcase-1 is defined a bit
> earlier as:
> (lambda (func)
> (let ((handler (function-get func 'compiler-macro)))
> (if (null handler) (macroexp--all-forms form 1)
> (unless (functionp handler)
> (with-demoted-errors "macroexp--expand-all: %S"
> (autoload-do-load (indirect-function func) func)))
> (let ((newform (macroexp--compiler-macro handler form)))
> (if (eq form newform)
> (if
> (equal form
> (setq newform
> (macroexp--all-forms form 1)))
> form
> (setq form
> (macroexp--compiler-macro handler newform))
> (if (eq newform form) newform
> (macroexp--expand-all form)))
> (macroexp--expand-all newform))))))
> > (iii) There is no sign of the final `form' being returned, though this
> > may be being done elsewhere.
> I see the following in your code sample:
> (cond
> ((null x19)
> (let ((d x0) (sym x18))
> (ignore d) (setq defining-symbol sym) form))
> which seems to be correctly returning `form`.
Yes. Sorry about this bug report, which turned out to be a time waster.
> Stefan
--
Alan Mackenzie (Nuremberg, Germany).
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.