bug#40727: 27.0.91; 'cl-loop ... across ... and' seems broken

[Noam Postavsky <npostavs@gmail.com>]

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

tags 40727 + confirmed
quit

Philipp Stephani <p.stephani2@gmail.com> writes:

> In Emacs 26:
>
> emacs -Q -batch -l cl-lib -eval '(cl-loop for x across [1 2] and y across [3 4])'
>
> works (i.e. no error), but in Emacs 27 it gives an error
>
> Symbol’s value as variable is void: --cl-vec--
>
> This doesn't happen with "for" instead of "and", and also doesn't happen
> with lists and "in".

It's because of [1: bfca19e475].  I think it should be possible to fix
by grouping the bindings put into 'loop-for-bindings', but meanwhile we
should revert that change, at least on the release branch.  It doesn't
revert automatically because there was meanwhile another change to use
'cl--push-clause-loop-body' instead of 'push' on the involved lines, but
after manual fixup your example works correctly.

[1: bfca19e475]: 2018-01-08 00:33:15 +0900
  cl-loop: Calculate the array length just once
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=bfca19e475c01f13dbacc7f8b7bb1aecf46cb7e4


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 3922 bytes --]

From 56a55199614b60e5ffd2968217a0d115779bb23d Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Thu, 30 Apr 2020 07:54:49 -0400
Subject: [PATCH] Revert "cl-loop: Calculate the array length just once"

It fails when using 'and' (parallel bindings) for arrays (Bug#40727).
* lisp/emacs-lisp/cl-macs.el (cl--parse-loop-clause): Revert to
recomputing array length.
* test/lisp/emacs-lisp/cl-macs-tests.el (cl-macs-loop-and-arrays): New
test.
---
 lisp/emacs-lisp/cl-macs.el            | 14 ++++----------
 test/lisp/emacs-lisp/cl-macs-tests.el |  6 ++++++
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index d56f4151df..20bd1883c3 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -1322,13 +1322,11 @@ cl--parse-loop-clause
 
 	       ((memq word '(across across-ref))
 		(let ((temp-vec (make-symbol "--cl-vec--"))
-                      (temp-len (make-symbol "--cl-len--"))
 		      (temp-idx (make-symbol "--cl-idx--")))
 		  (push (list temp-vec (pop cl--loop-args)) loop-for-bindings)
-		  (push (list temp-len `(length ,temp-vec)) loop-for-bindings)
 		  (push (list temp-idx -1) loop-for-bindings)
 		  (cl--push-clause-loop-body
-                   `(< (setq ,temp-idx (1+ ,temp-idx)) ,temp-len))
+                   `(< (setq ,temp-idx (1+ ,temp-idx)) (length ,temp-vec)))
 		  (if (eq word 'across-ref)
 		      (push (list var `(aref ,temp-vec ,temp-idx))
 			    cl--loop-symbol-macs)
@@ -1342,7 +1340,6 @@ cl--parse-loop-clause
 				    (error "Expected `of'"))))
 		      (seq (cl--pop2 cl--loop-args))
 		      (temp-seq (make-symbol "--cl-seq--"))
-	              (temp-len (make-symbol "--cl-len--"))
 		      (temp-idx
                        (if (eq (car cl--loop-args) 'using)
                            (if (and (= (length (cadr cl--loop-args)) 2)
@@ -1353,19 +1350,16 @@ cl--parse-loop-clause
 		  (push (list temp-seq seq) loop-for-bindings)
 		  (push (list temp-idx 0) loop-for-bindings)
 		  (if ref
-                      (progn
+                      (let ((temp-len (make-symbol "--cl-len--")))
 			(push (list temp-len `(length ,temp-seq))
 			      loop-for-bindings)
 			(push (list var `(elt ,temp-seq ,temp-idx))
 			      cl--loop-symbol-macs)
-			(cl--push-clause-loop-body `(< ,temp-idx ,temp-len)))
-                    ;; Evaluate seq length just if needed, that is, when seq is not a cons.
-                    (push (list temp-len (or (consp seq) `(length ,temp-seq)))
-			  loop-for-bindings)
+                        (cl--push-clause-loop-body `(< ,temp-idx ,temp-len)))
 		    (push (list var nil) loop-for-bindings)
 		    (cl--push-clause-loop-body `(and ,temp-seq
                                                      (or (consp ,temp-seq)
-                                                         (< ,temp-idx ,temp-len))))
+                                                         (< ,temp-idx (length ,temp-seq)))))
 		    (push (list var `(if (consp ,temp-seq)
                                          (pop ,temp-seq)
                                        (aref ,temp-seq ,temp-idx)))
diff --git a/test/lisp/emacs-lisp/cl-macs-tests.el b/test/lisp/emacs-lisp/cl-macs-tests.el
index 9ca84f156a..77609a42a9 100644
--- a/test/lisp/emacs-lisp/cl-macs-tests.el
+++ b/test/lisp/emacs-lisp/cl-macs-tests.el
@@ -39,6 +39,12 @@ cl-macs-loop-and-assignment
                           collect (list c b a))
                  '((4.0 2 1) (8.3 6 5) (10.4 9 8)))))
 
+(ert-deftest cl-macs-loop-and-arrays ()
+  "Bug#40727"
+  (should (equal (cl-loop for y = (- (or x 0)) and x across [1 2]
+                          collect (cons x y))
+                 '((1 . 0) (2 . -1)))))
+
 (ert-deftest cl-macs-loop-destructure ()
   (should (equal (cl-loop for (a b c) in '((1 2 4.0) (5 6 8.3) (8 9 10.4))
                           collect (list c b a))
-- 
2.11.0


[-- Attachment #3: Type: text/plain, Size: 164 bytes --]


By the way, while adding the test case I found an additional regression
involving loop termination by a 'var = ...' clause.  I'll open another
bug about it soon.