bug#26068: [PATCH] bug in generator function with pcase

bug#26068: [PATCH] bug in generator function with pcase

[Paul Pogonyshev]

[-- Attachment #1: Type: text/plain, Size: 398 bytes --]

To reproduce:

(iter-next (funcall (iter-lambda () (pcase (list 1 2)
                                      (`(,a ,b) (iter-yield (+ a b)))))))

Patch is attached. I have no idea if it affects something else, but
matching symbols with `eq' on their names (`eq' on strings?!) seems
outright wrong to me.

On a side note: bug report I sent yesterday with M-x report-emacs-bug
was silently lost.

Paul

[-- Attachment #2: cl-macs.diff --]
[-- Type: text/plain, Size: 1841 bytes --]

diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index 40342f3fe4..58bcdd52ac 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -2059,8 +2059,8 @@ except that it additionally expands symbol macros."
           (pcase exp
             ((pred symbolp)
              ;; Perform symbol-macro expansion.
-             (when (cdr (assq (symbol-name exp) env))
-               (setq exp (cadr (assq (symbol-name exp) env)))))
+             (when (cdr (assq exp env))
+               (setq exp (cadr (assq exp env)))))
             (`(setq . ,_)
              ;; Convert setq to setf if required by symbol-macro expansion.
              (let* ((args (mapcar (lambda (f) (cl--sm-macroexpand f env))
@@ -2078,7 +2078,7 @@ except that it additionally expands symbol macros."
              (let ((letf nil) (found nil) (nbs ()))
                (dolist (binding bindings)
                  (let* ((var (if (symbolp binding) binding (car binding)))
-                        (sm (assq (symbol-name var) env)))
+                        (sm (assq var env)))
                    (push (if (not (cdr sm))
                              binding
                            (let ((nexp (cadr sm)))
@@ -2149,7 +2149,7 @@ by EXPANSION, and (setq NAME ...) will act like (setf EXPANSION ...).
             (let ((expansion
                    ;; FIXME: For N bindings, this will traverse `body' N times!
                    (macroexpand-all (macroexp-progn body)
-                                    (cons (list (symbol-name (caar bindings))
+                                    (cons (list (caar bindings)
                                                 (cl-cadar bindings))
                                           macroexpand-all-environment))))
               (if (or (null (cdar bindings)) (cl-cddar bindings))

bug#26068: [PATCH] bug in generator function with pcase

[Mark Oteiza]

Paul Pogonyshev <pogonyshev@gmail.com> writes:

> To reproduce:
>
> (iter-next (funcall (iter-lambda () (pcase (list 1 2)
>                                       (`(,a ,b) (iter-yield (+ a b)))))))
>
> Patch is attached. I have no idea if it affects something else, but
> matching symbols with `eq' on their names (`eq' on strings?!) seems
> outright wrong to me.
>
> On a side note: bug report I sent yesterday with M-x report-emacs-bug
> was silently lost.

Applied as 0d112c00.

bug#26068: [PATCH] bug in generator function with pcase

[Tino Calancha]

Paul Pogonyshev <pogonyshev@gmail.com> writes:

> To reproduce:
>
> (iter-next (funcall (iter-lambda () (pcase (list 1 2)
>                                       (`(,a ,b) (iter-yield (+ a b)))))))
>
> Patch is attached. I have no idea if it affects something else, but
> matching symbols with `eq' on their names (`eq' on strings?!) seems
> outright wrong to me.
It looks also weird to me.  It seems written on purpose in order to
not expand fuction names in `cl-symbol-macrolet'.

Stefan has added a new test 2 days ago which fails after your
patch:
* test/lisp/emacs-lisp/cl-lib-tests.el (cl-lib-symbol-macrolet)

bug#26068: [PATCH] bug in generator function with pcase

[Paul Pogonyshev]

[-- Attachment #1: Type: text/plain, Size: 1009 bytes --]

Attached is the testcase from the bug as a patch. It might be best to
reformulate it without `pcase' though, to avoid depending on its
implementation details.

The testcase fails before the patch, passes now. So, I don't know
anything about `cl-flet', but the patch was not without reasons.

Paul

On 1 April 2017 at 15:08, Tino Calancha <tino.calancha@gmail.com> wrote:
> Paul Pogonyshev <pogonyshev@gmail.com> writes:
>
>> To reproduce:
>>
>> (iter-next (funcall (iter-lambda () (pcase (list 1 2)
>>                                       (`(,a ,b) (iter-yield (+ a b)))))))
>>
>> Patch is attached. I have no idea if it affects something else, but
>> matching symbols with `eq' on their names (`eq' on strings?!) seems
>> outright wrong to me.
> It looks also weird to me.  It seems written on purpose in order to
> not expand fuction names in `cl-symbol-macrolet'.
>
> Stefan has added a new test 2 days ago which fails after your
> patch:
> * test/lisp/emacs-lisp/cl-lib-tests.el (cl-lib-symbol-macrolet)

[-- Attachment #2: pcase-generator-test.diff --]
[-- Type: text/plain, Size: 829 bytes --]

diff --git a/test/lisp/emacs-lisp/generator-tests.el b/test/lisp/emacs-lisp/generator-tests.el
index 1a567ac70f..11542ab15d 100644
--- a/test/lisp/emacs-lisp/generator-tests.el
+++ b/test/lisp/emacs-lisp/generator-tests.el
@@ -282,3 +282,10 @@ identical output.
 (ert-deftest cps-test-declarations-preserved ()
   (should (equal (documentation 'generator-with-docstring) "Documentation!"))
   (should (equal (get 'generator-with-docstring 'lisp-indent-function) 5)))
+
+;; Bug #26068.  The problem is that `pcase' uses several `x' symbols,
+;; which are not identical, but have the same name.
+(ert-deftest cps-test-symbols-with-same-name ()
+  (should (equal (iter-next (funcall (iter-lambda () (pcase (list 1 2)
+                                                       (`(,a ,b) (iter-yield (+ a b)))))))
+                 3)))

bug#26068: [PATCH] bug in generator function with pcase

[Stefan Monnier]

> +;; Bug #26068.  The problem is that `pcase' uses several `x' symbols,
> +;; which are not identical, but have the same name.
> +(ert-deftest cps-test-symbols-with-same-name ()
> +  (should (equal (iter-next (funcall (iter-lambda () (pcase (list 1 2)
> +                                                       (`(,a ,b) (iter-yield (+ a b)))))))
> +                 3)))

Right.  I guess the core if the issue would be reproduced with something like

    (defmacro cl-lib-symbol-macrolet-4+5 ()
      (let* ((sname "x")
             (s1 (make-symbol sname))
             (s2 (make-symbol sname)))
        `(cl-symbol-macrolet ((,s1 4)
                              (,s2 5))
           (+ ,s1 ,s2))))
    (ert-deftest cl-lib-symbol-macrolet-2 ()
      (should (equal (cl-lib-symbol-macrolet-4+5) (+ 4 5))))

Before your patch, the macro's code returned 10 rather than 9.

I installed this test in cl-lib-tests.el.


        Stefan