false warnings when compiling with lexical-binding and cl-lib.

false warnings when compiling with lexical-binding and cl-lib.

[Thierry Volpiatto]

Hi,
here a sample file containing a function using loop:

--8<---------------cut here---------------start------------->8---
;;; test.el --- test loop -*- lexical-binding: t -*-

(require 'cl-lib)

(defun test-with (l)
  (cl-loop with lst
           for i in l
           unless (member i lst)
           collect i into lst
           finally return lst))
--8<---------------cut here---------------end--------------->8---

When compiling this file I have a warning about unused arg `lst'.
Is this wanted ?

Thanks.

-- 
Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: false warnings when compiling with lexical-binding and cl-lib.

[Stefan Monnier]

> here a sample file containing a function using loop:

> --8<---------------cut here---------------start------------->8---
> ;;; test.el --- test loop -*- lexical-binding: t -*-

> (require 'cl-lib)

> (defun test-with (l)
>   (cl-loop with lst
>            for i in l
>            unless (member i lst)
>            collect i into lst
>            finally return lst))
> --8<---------------cut here---------------end--------------->8---

> When compiling this file I have a warning about unused arg `lst'.
> Is this wanted ?

No, it's not wanted.  But noone so far as been able to come up with
a patch to fix it (the code to fix is the cl-loop macro: look at the
macro-expanded code to see the unused `lst').


        Stefan

Re: false warnings when compiling with lexical-binding and cl-lib.

[Thierry Volpiatto]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> here a sample file containing a function using loop:
>
>> --8<---------------cut here---------------start------------->8---
>> ;;; test.el --- test loop -*- lexical-binding: t -*-
>
>> (require 'cl-lib)
>
>> (defun test-with (l)
>>   (cl-loop with lst
>>            for i in l
>>            unless (member i lst)
>>            collect i into lst
>>            finally return lst))
>> --8<---------------cut here---------------end--------------->8---
>
>> When compiling this file I have a warning about unused arg `lst'.
>> Is this wanted ?
>
> No, it's not wanted.  But noone so far as been able to come up with
> a patch to fix it (the code to fix is the cl-loop macro: look at the
> macro-expanded code to see the unused `lst').

Ok thanks.


-- 
Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: false warnings when compiling with lexical-binding and cl-lib.

[Thierry Volpiatto]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> here a sample file containing a function using loop:
>
>> --8<---------------cut here---------------start------------->8---
>> ;;; test.el --- test loop -*- lexical-binding: t -*-
>
>> (require 'cl-lib)
>
>> (defun test-with (l)
>>   (cl-loop with lst
>>            for i in l
>>            unless (member i lst)
>>            collect i into lst
>>            finally return lst))
>> --8<---------------cut here---------------end--------------->8---
>
>> When compiling this file I have a warning about unused arg `lst'.
>> Is this wanted ?
>
> No, it's not wanted.  But noone so far as been able to come up with
> a patch to fix it (the code to fix is the cl-loop macro: look at the
> macro-expanded code to see the unused `lst').
Ok I see:

,----
| (let* ((lst nil) (--cl-var-- l) (i nil) (lst nil))
`----


-- 
Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: false warnings when compiling with lexical-binding and cl-lib.

[Thierry Volpiatto]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> here a sample file containing a function using loop:
>
>> --8<---------------cut here---------------start------------->8---
>> ;;; test.el --- test loop -*- lexical-binding: t -*-
>
>> (require 'cl-lib)
>
>> (defun test-with (l)
>>   (cl-loop with lst
>>            for i in l
>>            unless (member i lst)
>>            collect i into lst
>>            finally return lst))
>> --8<---------------cut here---------------end--------------->8---
>
>> When compiling this file I have a warning about unused arg `lst'.
>> Is this wanted ?
>
> No, it's not wanted.  But noone so far as been able to come up with
> a patch to fix it (the code to fix is the cl-loop macro: look at the
> macro-expanded code to see the unused `lst').

This patch avoid adding a new binding by the 'into' clause when the
binding have been already added by a 'with' clause.
I wonder if this is correct, real common-lisp return an error in such
case, WDYT?

--8<---------------cut here---------------start------------->8---
diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index 2209297..63cf7a1 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -1564,9 +1564,11 @@ If BODY is `setq', then use SPECS for assignments rather than for bindings."
 
 (defun cl--loop-handle-accum (def &optional func) ; uses loop-*
   (if (eq (car cl--loop-args) 'into)
-      (let ((var (cl--pop2 cl--loop-args)))
+      (let* ((var (cl--pop2 cl--loop-args))
+             (binding (list (list var def))))
 	(or (memq var cl--loop-accum-vars)
-	    (progn (push (list (list var def)) cl--loop-bindings)
+            (member binding cl--loop-bindings)
+	    (progn (push binding cl--loop-bindings)
 		   (push var cl--loop-accum-vars)))
 	var)
     (or cl--loop-accum-var
--8<---------------cut here---------------end--------------->8---

-- 
Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: false warnings when compiling with lexical-binding and cl-lib.

[Stefan Monnier]

> This patch avoid adding a new binding by the 'into' clause when the
> binding have been already added by a 'with' clause.
> I wonder if this is correct, real common-lisp return an error in such
> case, WDYT?

Oh, wait, then the warning is OK.  It indicates that the `with lst' is
extraneous and unused.  IOW, there's no bug to fix.  We could change
cl-loop to signal an error, but it doesn't seem to be worth the trouble.


        Stefan

Re: false warnings when compiling with lexical-binding and cl-lib.

[Thierry Volpiatto]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> This patch avoid adding a new binding by the 'into' clause when the
>> binding have been already added by a 'with' clause.
>> I wonder if this is correct, real common-lisp return an error in such
>> case, WDYT?
>
> Oh, wait, then the warning is OK.  It indicates that the `with lst' is
> extraneous and unused.  IOW, there's no bug to fix.  We could change
> cl-loop to signal an error, but it doesn't seem to be worth the trouble.

Yes, maybe the error message can be a little more explicit, common-lisp send a
message like this:

,----
|  Variable LST in INTO clause is a duplicate
| current LOOP context: COLLECT I INTO LST FINALLY.
|    [Condition of type SB-INT:COMPILED-PROGRAM-ERROR]
`----

Also the warning is only at compile time, should we return an error on
evaluation ?
This can be detected and handled from the same function 
i.e (cl--loop-handle-accum)

-- 
Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: false warnings when compiling with lexical-binding and cl-lib.

[Thierry Volpiatto]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> This patch avoid adding a new binding by the 'into' clause when the
>>> binding have been already added by a 'with' clause.
>>> I wonder if this is correct, real common-lisp return an error in such
>>> case, WDYT?
>>
>> Oh, wait, then the warning is OK.  It indicates that the `with lst' is
>> extraneous and unused.  IOW, there's no bug to fix.  We could change
>> cl-loop to signal an error, but it doesn't seem to be worth the trouble.
>
> Yes, maybe the error message can be a little more explicit, common-lisp send a
> message like this:
>
> ,----
> |  Variable LST in INTO clause is a duplicate
> | current LOOP context: COLLECT I INTO LST FINALLY.
> |    [Condition of type SB-INT:COMPILED-PROGRAM-ERROR]
> `----
>
> Also the warning is only at compile time, should we return an error on
> evaluation ?
> This can be detected and handled from the same function 
> i.e (cl--loop-handle-accum)

--8<---------------cut here---------------start------------->8---
diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index 2209297..3faf23b 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -1564,9 +1564,13 @@ If BODY is `setq', then use SPECS for assignments rather than for bindings."
 
 (defun cl--loop-handle-accum (def &optional func) ; uses loop-*
   (if (eq (car cl--loop-args) 'into)
-      (let ((var (cl--pop2 cl--loop-args)))
+      (let* ((var (cl--pop2 cl--loop-args))
+             (binding (list (list var def))))
 	(or (memq var cl--loop-accum-vars)
-	    (progn (push (list (list var def)) cl--loop-bindings)
+            (cl-assert (not (member binding cl--loop-bindings))
+                       nil "Variable %s in INTO clause is a duplicate"
+                       (upcase (symbol-name (caar binding))))
+	    (progn (push binding cl--loop-bindings)
 		   (push var cl--loop-accum-vars)))
 	var)
     (or cl--loop-accum-var
--8<---------------cut here---------------end--------------->8---

-- 
Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: false warnings when compiling with lexical-binding and cl-lib.

[Stefan Monnier]

>> Also the warning is only at compile time, should we return an error on
>> evaluation ?

I don't see why we should bother with an error.

   (let* ((a 1) (a 2)) ...)

is not very useful, but it's not an error.  The warning we currently
have is not perfect because it applies to the post-macroexpansion code,
so the user may not immediately understand what's going on, but your
suggested patch has the following downsides:
- it breaks previously working code.
- it does not tell you where (approximate line number) the error is located.
So if we want to improve on the current code, I think rather than
`cl-assert' we should use macroexp--warn-and-return (grep for it to see
sample uses).  But the patch won't be as trivial.


        Stefan