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

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

[Kevin Ryde]

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.


In GNU Emacs 23.2.1 (i486-pc-linux-gnu, GTK+ Version 2.20.0)
 of 2010-05-16 on raven, modified by Debian
configured using `configure  '--build' 'i486-linux-gnu' '--build' 'i486-linux-gnu' '--prefix=/usr' '--sharedstatedir=/var/lib' '--libexecdir=/usr/lib' '--localstatedir=/var/lib' '--infodir=/usr/share/info' '--mandir=/usr/share/man' '--with-pop=yes' '--enable-locallisppath=/etc/emacs23:/etc/emacs:/usr/local/share/emacs/23.2/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/23.2/site-lisp:/usr/share/emacs/site-lisp:/usr/share/emacs/23.2/leim' '--with-x=yes' '--with-x-toolkit=gtk' '--with-toolkit-scroll-bars' 'build_alias=i486-linux-gnu' 'CFLAGS=-DDEBIAN -g -O2' 'LDFLAGS=-g' 'CPPFLAGS=''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: en_AU
  value of $XMODIFIERS: nil
  locale-coding-system: iso-latin-1-unix
  default enable-multibyte-characters: t

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

[Thierry Volpiatto]

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))

IMHO this is not correct, 'and' clauses should be used after conditionals
(e.g if, when etc..)

,----
| ELISP>  (loop for elem in '(1 2 3)
|            for k = elem
|            if (oddp k)
|              collect k and do (print k))
`----

> shows
>
>     1
>     1
>     2

Instead of returning this, emacs should throw an error as SBCL does for
same code.

,----
| CL-USER> (loop for i in '(1 2 3)
|               for k = i and j = 99
|               (print k))
| ; Evaluation aborted.
`----

> 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.
>
>
> In GNU Emacs 23.2.1 (i486-pc-linux-gnu, GTK+ Version 2.20.0)
>  of 2010-05-16 on raven, modified by Debian
> configured using `configure  '--build' 'i486-linux-gnu' '--build' 'i486-linux-gnu' '--prefix=/usr' '--sharedstatedir=/var/lib' '--libexecdir=/usr/lib' '--localstatedir=/var/lib' '--infodir=/usr/share/info' '--mandir=/usr/share/man' '--with-pop=yes' '--enable-locallisppath=/etc/emacs23:/etc/emacs:/usr/local/share/emacs/23.2/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/23.2/site-lisp:/usr/share/emacs/site-lisp:/usr/share/emacs/23.2/leim' '--with-x=yes' '--with-x-toolkit=gtk' '--with-toolkit-scroll-bars' 'build_alias=i486-linux-gnu' 'CFLAGS=-DDEBIAN -g -O2' 'LDFLAGS=-g' 'CPPFLAGS=''
>
> Important settings:
>   value of $LC_ALL: nil
>   value of $LC_COLLATE: nil
>   value of $LC_CTYPE: nil
>   value of $LC_MESSAGES: nil
>   value of $LC_MONETARY: nil
>   value of $LC_NUMERIC: nil
>   value of $LC_TIME: nil
>   value of $LANG: en_AU
>   value of $XMODIFIERS: nil
>   locale-coding-system: iso-latin-1-unix
>   default enable-multibyte-characters: t
>
>
>
>

-- 
Thierry Volpiatto
Gpg key: http://pgp.mit.edu/

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

[Lawrence Mitchell]

Kevin Ryde wrote:
[...]

> 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
>           ...

Note the idiomatic way of writing this loop:

(loop for (k . v) in my-alist
      do ...)

Although this does not address the question of the bug.

-- 
Lawrence Mitchell <wence@gmx.li>

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

[Noam Postavsky]

tag 6583 + confirmed
found 6583 24.5
found 6583 25.0.94
quit

Thierry Volpiatto <thierry.volpiatto <at> gmail.com> wrote:
> IMHO this is not correct, 'and' clauses should be used after conditionals
> (e.g if, when etc..)

No, it should also work for 'for' clauses, as documented in CL Hyperspec [1]

    for-as-clause::= {for | as} for-as-subclause {and for-as-subclause}*

and Emacs' CL manual [2]

    If you include several ‘for’ clauses in a row, they are treated
    sequentially (as if by ‘let*’ and ‘setq’).  You can instead use the
    word ‘and’ to link the clauses, in which case they are processed in
    parallel (as if by ‘let’ and ‘cl-psetq’).

The SBCL example is just missing the 'do', that's why it failed.


[1]: http://www.lispworks.com/documentation/lw51/CLHS/Body/m_loop.htm
[2]: http://www.gnu.org/software/emacs/manual/html_node/cl/For-Clauses.html

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

[Alex]

[-- 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

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

[npostavs]

Alex <agrambot@gmail.com> writes:
>
> 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).

I can't claim to fully understand the loop macro implementation, but
your patch breaks this example from the manual `(cl) For Clauses':

     (cl-loop for x below 5 for y = nil then x collect (list x y))
             => ((0 nil) (1 1) (2 2) (3 3) (4 4))
     (cl-loop for x below 5 and y = nil then x collect (list x y))
             => ((0 nil) (1 0) (2 1) (3 2) (4 3))

With your patch the second loop gives ((0 nil) (1 1) (2 2) (3 3) (4 4))
like the first.

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

[Alex]

npostavs@users.sourceforge.net writes:

> Alex <agrambot@gmail.com> writes:

> I can't claim to fully understand the loop macro implementation, but
> your patch breaks this example from the manual `(cl) For Clauses':
>
>      (cl-loop for x below 5 for y = nil then x collect (list x y))
>              => ((0 nil) (1 1) (2 2) (3 3) (4 4))
>      (cl-loop for x below 5 and y = nil then x collect (list x y))
>              => ((0 nil) (1 0) (2 1) (3 2) (4 3))
>
> With your patch the second loop gives ((0 nil) (1 1) (2 2) (3 3) (4 4))
> like the first.

You're right, sorry. This breaks loops with variables that are updated
in loop-for-steps rather than loop-for-sets. When I started testing
other cases I was accidentally using the pre-patch branch to do so.

I can't think of an easy solution to cover both problems right now. If
no one better suited can figure this out, I'll come back to this after
completing an ert suite for cl-loop (of which I'm part-way through).

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

[Lars Ingebrigtsen]

Alex <agrambot@gmail.com> writes:

> You're right, sorry. This breaks loops with variables that are updated
> in loop-for-steps rather than loop-for-sets. When I started testing
> other cases I was accidentally using the pre-patch branch to do so.
>
> I can't think of an easy solution to cover both problems right now. If
> no one better suited can figure this out, I'll come back to this after
> completing an ert suite for cl-loop (of which I'm part-way through).

Alex, this was three years ago.  Did you make any progress here?  :-)

    (loop for elem in '(1 2 3)
          for k = elem and j = 99
          do
          (print k))

still displays 1 1 2, which has to be wrong...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no