bug#6583: 23.2; cl loop macro with `and' clause

[Alex <agrambot@gmail.com>]

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

tags 6583 patch
quit

Kevin Ryde <user42@zip.com.au> writes:

> Evaluating
>
>     (require 'cl)
>     (loop for elem in '(1 2 3)
>           for k = elem and j = 99
>           do
>           (print k))
>
> shows
>
>     1
>     1
>     2
>
> where I thought it might be
>
>     1
>     2
>     3
>
> I'm don't know much about the cl loop macro but thought the `for k' step
> would be evaluated after the `for elem' step, "sequential" per the cl
> info manual near the end of "For Clauses"
>
>     If you include several `for' clauses in a row, they are treated
>     sequentially
>
> The 1,2,3 is what you get from pasting the same form into clisp, if that
> suggests what an actual common lisp does or should do.  And in Emacs
> it's had if you omit the "and j",
>
>     (loop for elem in '(1 2 3)
>           for k = elem
>           do
>           (print k))
>     =>
>     1 2 3
>
> Nosing around the macro expansion I wondered if the "step" of k/j gets
> mispositioned if there's an `and', but it's hard to be sure.
>
>
> I struck this when making a loop over an alist where I thought to take
> apart the key and value with an `and' as they didn't need to be
> sequential,
>
>     (loop for elem in my-alist
>           for k = (car elem) and v = (cdr elem)
>           do
>           ...
>
> Alas the effect of the "1 1 2" was to double the first element and omit
> the last.

This problem appears to have been present even in the initial revision
(fcd737693e8), specifically in the (eq word '=) clause.

I see no reason for the (or ands (eq (car args) 'and)) consequent, as
what it does when it detects an 'and is:

* In the first iteration, set var (k) to the first form (elem) at the
  beginning of the iteration

* In subsequent iterations, set var (k) to itself at the beginning of
  the iteration (noop)

* At the end of every iteration, set var (k) to the second form
  (defaulting to the first form, elem).

The last point is the main problem: you can't set the variable to the
first form at the end of the iteration as it might depend on other loop
variables (in this case elem) that are updated at the beginning of the
iteration.

I've attached a patch below that appears to solve this issue. With the
patch, var is set to the first or second form at the beginning of the
iteration (just like it is when no 'and is present).


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: loop --]
[-- Type: text/x-diff, Size: 1877 bytes --]

From 48c59a8609e325690d9568d991bea7fd199a1acd Mon Sep 17 00:00:00 2001
From: Alexander Gramiak <agrambot@gmail.com>
Date: Sat, 10 Jun 2017 12:26:48 -0600
Subject: [PATCH] Fix 'and in cl-loop's for-as-equals-then subclause

Update variables in the subclause correctly, and only once (Bug#6583).

* lisp/emacs-lisp/cl-macs.el (cl--parse-loop-clause): Remove faulty
conditional branch.
---
 lisp/emacs-lisp/cl-macs.el | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index db1518ce61..8222f0dc34 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -1273,22 +1273,13 @@ cl--parse-loop-clause
 		       (then (if (eq (car cl--loop-args) 'then)
                                  (cl--pop2 cl--loop-args) start)))
 		  (push (list var nil) loop-for-bindings)
-		  (if (or ands (eq (car cl--loop-args) 'and))
-		      (progn
-			(push `(,var
-				(if ,(or cl--loop-first-flag
-					 (setq cl--loop-first-flag
-					       (make-symbol "--cl-var--")))
-				    ,start ,var))
-			      loop-for-sets)
-			(push (list var then) loop-for-steps))
-		    (push (list var
-				(if (eq start then) start
-				  `(if ,(or cl--loop-first-flag
-					    (setq cl--loop-first-flag
-						  (make-symbol "--cl-var--")))
-				       ,start ,then)))
-			  loop-for-sets))))
+                  (push (list var
+                              (if (eq start then) start
+                                `(if ,(or cl--loop-first-flag
+                                          (setq cl--loop-first-flag
+                                                (make-symbol "--cl-var--")))
+                                     ,start ,then)))
+                        loop-for-sets)))
 
 	       ((memq word '(across across-ref))
 		(let ((temp-vec (make-symbol "--cl-vec--"))
-- 
2.11.0