cl-loop's "with" destructuring-bind looks broken

cl-loop's "with" destructuring-bind looks broken

[Tomohiro Matsuyama]

Hi,

I have faced an issue on 24.3.90.2 when using my extension that
includes an expression like:

    (cl-loop with (a b c) = '(1 2 3) return (+ a b c))

This expression causes an error:

    Attempt to set a constant symbol: nil

On a stable release, at least 24.3.1, it works fine and evalutes to 6.

ANSI Common Lisp allows such destructuring-bind with "with" clause, by
the way.

Regards,
Tomohiro

Re: cl-loop's "with" destructuring-bind looks broken

[Daniel Colascione]

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

On 04/19/2014 10:36 PM, Tomohiro Matsuyama wrote:
> Hi,
> 
> I have faced an issue on 24.3.90.2 when using my extension that
> includes an expression like:
> 
>     (cl-loop with (a b c) = '(1 2 3) return (+ a b c))

It should be fixed now. I'd appreciate if Stefan took a look at the fix,
trunk revision 117000. I don't know why that (eq body 'setq) test was
there in the first place, but removing it doesn't seem to break anything.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: cl-loop's "with" destructuring-bind looks broken

[Stefan Monnier]

>> I have faced an issue on 24.3.90.2 when using my extension that
>> includes an expression like:
>> (cl-loop with (a b c) = '(1 2 3) return (+ a b c))
> It should be fixed now.

Only in `trunk', tho.

> I'd appreciate if Stefan took a look at the fix, trunk revision
> 117000.  I don't know why that (eq body 'setq) test was there in the
> first place, but removing it doesn't seem to break anything.

I installed the patch below in emacs-24, which is a bit more careful
(your patch ends up let-binding variables redundantly, and it renames
vars in cases where it's not necessary).


        Stefan


=== modified file 'lisp/emacs-lisp/cl-macs.el'
--- lisp/emacs-lisp/cl-macs.el	2014-03-24 20:41:08 +0000
+++ lisp/emacs-lisp/cl-macs.el	2014-04-21 21:09:22 +0000
@@ -1542,12 +1542,14 @@
               (if (and (cl--unused-var-p temp) (null expr))
                   nil ;; Don't bother declaring/setting `temp' since it won't
 		      ;; be used when `expr' is nil, anyway.
-                (when (and (eq body 'setq) (cl--unused-var-p temp))
+                (when (or (null temp)
+                          (and (eq body 'setq) (cl--unused-var-p temp)))
                   ;; Prefer a fresh uninterned symbol over "_to", to avoid
                   ;; warnings that we set an unused variable.
                   (setq temp (make-symbol "--cl-var--"))
                   ;; Make sure this temp variable is locally declared.
-                  (push (list (list temp)) cl--loop-bindings))
+                  (when (eq body 'setq)
+                    (push (list (list temp)) cl--loop-bindings)))
                 (push (list temp expr) new))
               (while (consp spec)
                 (push (list (pop spec)