bug#31641: 26.1; iter-do variable not left unused warning

bug#31641: 26.1; iter-do variable not left unused warning

[Christopher Wellons]

When byte-compiling an iter-do form with a variable intended to be left 
unused, the compiler emits a false warning:

;;; -*- lexical-binding: t; -*-
(require 'generator)
(iter-do (_ i))
;; -> "Warning: variable ‘_’ not left unused"

Giving the variable a name has the opposite effect, though it's not 
a false warning in this case:

(iter-do (v i))
;; -> "Unused lexical variable ‘v’"

This issue does not occur with similar, long-established forms:

;;; -*- lexical-binding: t; -*-
(dolist (_ '(a b c)))
(dotimes (_ 10))
;; -> no warnings

bug#31641: 26.1; iter-do variable not left unused warning

[Noam Postavsky]

tags 31641 + confirmed
severity 31641 minor
quit

Christopher Wellons <wellons@nullprogram.com> writes:

> When byte-compiling an iter-do form with a variable intended to be
> left unused, the compiler emits a false warning:
>
> ;;; -*- lexical-binding: t; -*-
> (require 'generator)
> (iter-do (_ i))
> ;; -> "Warning: variable ‘_’ not left unused"

Looking at the expansion, I guess the setf should be dropped if the
variable name starts with _.

(let (_
      #3=#:iter-do-result11
      (#1=#:iter-do-iterator-done8 nil)
      (#2=#:iter-do-iterator10 i))
  (while (not #1#)
    (condition-case #4=#:iter-do-condition9
        (setf _ (iter-next #2#))
      (iter-end-of-sequence
       (setf #3# (cdr #4#))
       (setf #1# t)))
    (unless #1#))
  #3#)

bug#31641: 26.1; iter-do variable not left unused warning

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

>> When byte-compiling an iter-do form with a variable intended to be
>> left unused, the compiler emits a false warning:
>>
>> ;;; -*- lexical-binding: t; -*-
>> (require 'generator)
>> (iter-do (_ i))
>> ;; -> "Warning: variable ‘_’ not left unused"
>
> Looking at the expansion, I guess the setf should be dropped if the
> variable name starts with _.
>
> (let (_
>       #3=#:iter-do-result11
>       (#1=#:iter-do-iterator-done8 nil)
>       (#2=#:iter-do-iterator10 i))
>   (while (not #1#)
>     (condition-case #4=#:iter-do-condition9
>         (setf _ (iter-next #2#))
>       (iter-end-of-sequence
>        (setf #3# (cdr #4#))
>        (setf #1# t)))
>     (unless #1#))
>   #3#)

The following patch does this, but I'm not sure whether this is correct
or not -- in other cases, the _ convention just removes the warning, but
doesn't change the semantics.

I wondered whether we could just suppress this warning like this:

             ,(if (string-match-p "\\`_" (symbol-name var))
                  `(with-suppressed-warnings ((not-unused ,var))
                     (setf ,var (iter-next ,it-symbol)))
                `(setf ,var (iter-next ,it-symbol)))

But no, cconv--analyze-use is called too early, and would have to be
taught about `with-suppressed-warnings'...  which, looking at the code,
isn't immediately obvious how to do.

So does anybody have any ideas here?

diff --git a/lisp/emacs-lisp/generator.el b/lisp/emacs-lisp/generator.el
index 9eb6d95964..0b644cc72c 100644
--- a/lisp/emacs-lisp/generator.el
+++ b/lisp/emacs-lisp/generator.el
@@ -731,7 +731,10 @@ iter-do
            (,it-symbol ,iterator))
        (while (not ,done-symbol)
          (condition-case ,condition-symbol
-             (setf ,var (iter-next ,it-symbol))
+             ;; Variables that start with an underscore shouldn't be set.
+             ,(if (string-match-p "\\`_" (symbol-name var))
+                  nil
+                `(setf ,var (iter-next ,it-symbol)))
            (iter-end-of-sequence
             (setf ,result-symbol (cdr ,condition-symbol))
             (setf ,done-symbol t)))


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

bug#31641: 26.1; iter-do variable not left unused warning

[Stefan Monnier]

>>> When byte-compiling an iter-do form with a variable intended to be
>>> left unused, the compiler emits a false warning:
>>>
>>> ;;; -*- lexical-binding: t; -*-
>>> (require 'generator)
>>> (iter-do (_ i))
>>> ;; -> "Warning: variable ‘_’ not left unused"
>>
>> Looking at the expansion, I guess the setf should be dropped if the
>> variable name starts with _.
>>
>> (let (_
>>       #3=#:iter-do-result11
>>       (#1=#:iter-do-iterator-done8 nil)
>>       (#2=#:iter-do-iterator10 i))
>>   (while (not #1#)
>>     (condition-case #4=#:iter-do-condition9
>>         (setf _ (iter-next #2#))
>>       (iter-end-of-sequence
>>        (setf #3# (cdr #4#))
>>        (setf #1# t)))
>>     (unless #1#))
>>   #3#)

FWIW, I find the above expansion to provide somewhat "dirty" semantics
in the sense that

    (let ((funs '()))
      (iter-do (n i) (push (lambda () n) funs))
      funs)

will return a list of functions which all return the same value (the
last `n`).

You can clean up this semantics and the warning at the same time by
using an expansion like:

    (let (#3=#:iter-do-result11
          (#1=#:iter-do-iterator-done8 nil)
          (#2=#:iter-do-iterator10 i))
      (while (not #1#)
        (let ((_ (condition-case #4=#:iter-do-condition9
                     (iter-next #2#)
                   (iter-end-of-sequence
                    (setf #3# (cdr #4#))
                    (setf #1# t)))
        (unless #1# [BODY])
      #3#)

BTW, I think we can remove the duplicate #1 test by moving the body of
the `while` into its test, e.g.:

    (let (#3=#:iter-do-result11
          (#1=#:iter-do-iterator-done8 nil)
          (#2=#:iter-do-iterator10 i))
      (while
        (let ((_ (condition-case #4=#:iter-do-condition9
                     (iter-next #2#)
                   (iter-end-of-sequence
                    (setf #3# (cdr #4#))
                    (setf #1# t)))))
          (unless #1#
            [BODY]
            t)))
      #3#)

It's too bad that [BODY] can throw `iter-end-of-sequence`, since
otherwise we could move the `condition-case` outside of the loop and get
something more efficient.


        Stefan


> The following patch does this, but I'm not sure whether this is correct
> or not -- in other cases, the _ convention just removes the warning, but
> doesn't change the semantics.
>
> I wondered whether we could just suppress this warning like this:
>
>              ,(if (string-match-p "\\`_" (symbol-name var))
>                   `(with-suppressed-warnings ((not-unused ,var))
>                      (setf ,var (iter-next ,it-symbol)))
>                 `(setf ,var (iter-next ,it-symbol)))
>
> But no, cconv--analyze-use is called too early, and would have to be
> taught about `with-suppressed-warnings'...  which, looking at the code,
> isn't immediately obvious how to do.
>
> So does anybody have any ideas here?
>
> diff --git a/lisp/emacs-lisp/generator.el b/lisp/emacs-lisp/generator.el
> index 9eb6d95964..0b644cc72c 100644
> --- a/lisp/emacs-lisp/generator.el
> +++ b/lisp/emacs-lisp/generator.el
> @@ -731,7 +731,10 @@ iter-do
>             (,it-symbol ,iterator))
>         (while (not ,done-symbol)
>           (condition-case ,condition-symbol
> -             (setf ,var (iter-next ,it-symbol))
> +             ;; Variables that start with an underscore shouldn't be set.
> +             ,(if (string-match-p "\\`_" (symbol-name var))
> +                  nil
> +                `(setf ,var (iter-next ,it-symbol)))
>             (iter-end-of-sequence
>              (setf ,result-symbol (cdr ,condition-symbol))
>              (setf ,done-symbol t)))

bug#31641: 26.1; iter-do variable not left unused warning

[Lars Ingebrigtsen]

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

> FWIW, I find the above expansion to provide somewhat "dirty" semantics
> in the sense that
>
>     (let ((funs '()))
>       (iter-do (n i) (push (lambda () n) funs))
>       funs)
>
> will return a list of functions which all return the same value (the
> last `n`).
>
> You can clean up this semantics and the warning at the same time by
> using an expansion like:

If I'm reading that correctly, that does seem like more obvious
semantics, but is it too late to change this now?  I'm not sure how much
generator.el is used in the wild yet...

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

bug#31641: 26.1; iter-do variable not left unused warning

[Stefan Monnier]

> If I'm reading that correctly, that does seem like more obvious
> semantics, but is it too late to change this now?  I'm not sure how much
> generator.el is used in the wild yet...

The change in semantics is fairly subtle (e.g. Common-Lisp allows both
choices for the similar choice of behavior in `dotimes` and `dolist`),
so it's rather unlikely to break existing code, IMO (and I think the
likelihood would be higher if the change were in the other direction).


        Stefan

bug#31641: 26.1; iter-do variable not left unused warning

[Lars Ingebrigtsen]

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

> You can clean up this semantics and the warning at the same time by
> using an expansion like:
>
>     (let (#3=#:iter-do-result11
>           (#1=#:iter-do-iterator-done8 nil)
>           (#2=#:iter-do-iterator10 i))
>       (while (not #1#)
>         (let ((_ (condition-case #4=#:iter-do-condition9
>                      (iter-next #2#)
>                    (iter-end-of-sequence
>                     (setf #3# (cdr #4#))
>                     (setf #1# t)))
>         (unless #1# [BODY])
>       #3#)

So I tried this...

diff --git a/lisp/emacs-lisp/generator.el b/lisp/emacs-lisp/generator.el
index 9eb6d95964..dfd2513350 100644
--- a/lisp/emacs-lisp/generator.el
+++ b/lisp/emacs-lisp/generator.el
@@ -725,17 +725,18 @@ iter-do
         (condition-symbol (cps--gensym "iter-do-condition"))
         (it-symbol (cps--gensym "iter-do-iterator"))
         (result-symbol (cps--gensym "iter-do-result")))
-    `(let (,var
-           ,result-symbol
+    `(let (,result-symbol
            (,done-symbol nil)
            (,it-symbol ,iterator))
        (while (not ,done-symbol)
-         (condition-case ,condition-symbol
-             (setf ,var (iter-next ,it-symbol))
-           (iter-end-of-sequence
-            (setf ,result-symbol (cdr ,condition-symbol))
-            (setf ,done-symbol t)))
-         (unless ,done-symbol ,@body))
+         (let ((,var
+                (condition-case ,condition-symbol
+                    (iter-next ,it-symbol)
+                  (iter-end-of-sequence
+                   (setf ,result-symbol (cdr ,condition-symbol))
+                   (setf ,done-symbol t)))))
+           (unless ,done-symbol
+             ,@body)))
        ,result-symbol)))
 
 (defvar cl--loop-args)

But then this fails:

1 unexpected results:
   FAILED  iter-lambda-variable-shadowing

Uhm...  oh, it fails even without that change?  ...  If I just say "make
check", then it doesn't fail, but if I say "make generator-tests", then
it fails?  Very odd.

> BTW, I think we can remove the duplicate #1 test by moving the body of
> the `while` into its test, e.g.:
>
>     (let (#3=#:iter-do-result11
>           (#1=#:iter-do-iterator-done8 nil)
>           (#2=#:iter-do-iterator10 i))
>       (while
>         (let ((_ (condition-case #4=#:iter-do-condition9
>                      (iter-next #2#)
>                    (iter-end-of-sequence
>                     (setf #3# (cdr #4#))
>                     (setf #1# t)))))
>           (unless #1#
>             [BODY]
>             t)))
>       #3#)

Indeed.  I've now pushed something like this to Emacs 28.

> It's too bad that [BODY] can throw `iter-end-of-sequence`, since
> otherwise we could move the `condition-case` outside of the loop and get
> something more efficient.

Yup.

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

bug#31641: 26.1; iter-do variable not left unused warning

[Basil L. Contovounesios]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> diff --git a/lisp/emacs-lisp/generator.el b/lisp/emacs-lisp/generator.el
> index 9eb6d95964..0b644cc72c 100644
> --- a/lisp/emacs-lisp/generator.el
> +++ b/lisp/emacs-lisp/generator.el
> @@ -731,7 +731,10 @@ iter-do
>             (,it-symbol ,iterator))
>         (while (not ,done-symbol)
>           (condition-case ,condition-symbol
> -             (setf ,var (iter-next ,it-symbol))
> +             ;; Variables that start with an underscore shouldn't be set.
> +             ,(if (string-match-p "\\`_" (symbol-name var))
> +                  nil
> +                `(setf ,var (iter-next ,it-symbol)))

Nit - AKA the following?

  ,(unless (string-prefix-p "_" (symbol-name var))
     `(setf ,var (iter-next ,it-symbol)))

-- 
Basil