bug#16206: 24.3; Incorrect unused variable byte-compiler warning in dotimes

bug#16206: 24.3; Incorrect unused variable byte-compiler warning in dotimes

[Christopher Wellons]

When `lexical-binding' is t the byte-compiler issues an invalid warning
for `dotimes' when the optional "result" form is used. For example,
byte-compile a file with these contents.

    ;;; -*- lexical-binding: t; -*-
    (defun foo ()
      (dotimes (i 1 t) i))

Or evaluate this form.

    (let ((lexical-binding t))
      (byte-compile (lambda () (dotimes (i 1 t) i))))

Output:

    Warning: Unused lexical variable `i'

If the t is removed the warning goes away.

bug#16206: 24.3; Incorrect unused variable byte-compiler warning in dotimes

[Stefan Monnier]

> When `lexical-binding' is t the byte-compiler issues an invalid warning
> for `dotimes' when the optional "result" form is used. For example,
> byte-compile a file with these contents.

>     ;;; -*- lexical-binding: t; -*-
>     (defun foo ()
>       (dotimes (i 1 t) i))

Indeed.  This is a known problem.  I strongly recommend you don't use
this third argument unless it makes use of `i' (which is basically the
only case where it's useful, AFAIK).

IOW use

   (defun foo ()
     (dotimes (i 1) i)
     t)


-- Stefan

bug#16206: 24.3; Incorrect unused variable byte-compiler warning in dotimes

[Juri Linkov]

>> When `lexical-binding' is t the byte-compiler issues an invalid warning
>> for `dotimes' when the optional "result" form is used. For example,
>> byte-compile a file with these contents.
>
>>     ;;; -*- lexical-binding: t; -*-
>>     (defun foo ()
>>       (dotimes (i 1 t) i))
>
> Indeed.  This is a known problem.  I strongly recommend you don't use
> this third argument unless it makes use of `i' (which is basically the
> only case where it's useful, AFAIK).

I can't imagine a case when `i' could be useful in the third argument,
because after the last loop `i' is just equal to the COUNT arg, i.e.
instead of `i'

  (let ((count 10))
    (dotimes (i count i) (print i)))

it's clearer to use `count'

  (let ((count 10))
    (dotimes (i count count) (print i)))

in cases when the result depends only on the value of `count'.
But in most cases the result is calculated inside the body
as demonstrated by examples in (info "(eintr) dotimes")

So maybe better to fix the line marked in the implementation of `dotimes'
by FIXME?

bug#16206: 24.3; Incorrect unused variable byte-compiler warning in dotimes

[Stefan Monnier]

>   (let ((count 10))
>     (dotimes (i count count) (print i)))

I don't disagree with you: I think this 3rd field is a misfeature
of dotimes.  But IIRC there is code out there which uses it.
It can be marginally useful in cases such as:

    (dotimes (i (read-number "Nb of entries: ") i)
      ...blabla...)

which would otherwise need to explicitly bind the output of
`read-number` to a variable, hence something like:

    (let ((n (read-number "Nb of entries: ")))
      (dotimes (i n)
        ...blabla...)
      n)

But basically, IIRC last time this question came up we kept the current
behavior because while this shed's color is not great, at least it's the
same color as Common Lisp's.

BTW, you say:

> it's clearer to use `count'
>
>   (let ((count 10))
>     (dotimes (i count count) (print i)))

but I really don't like the way the overall output is "hidden" in this
third field; it gives a very unusual order of execution.
I personally consider:

   (let ((count 10))
     (dotimes (i count) (print i))
     count)

to be much more clear.  Which is why I think the current behavior of
complaining when the third field is used (except in the very rare case
where the third field refers to the iteration variable) is a fairly
good compromise.


        Stefan

bug#16206: 24.3; Incorrect unused variable byte-compiler warning in dotimes

[Juri Linkov]

>>   (let ((count 10))
>>     (dotimes (i count count) (print i)))
>
> I don't disagree with you: I think this 3rd field is a misfeature
> of dotimes.  But IIRC there is code out there which uses it.

And I agree with you that the 3rd field is a misfeature -
in more modern Lisp languages like Clojure there is no 3rd field.

But there is the need to unlearn it - to update the documentation
and examples:

diff --git a/lisp/subr.el b/lisp/subr.el
index 9cf7d59..379cf33 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -223,7 +223,8 @@ dotimes
   "Loop a certain number of times.
 Evaluate BODY with VAR bound to successive integers running from 0,
 inclusive, to COUNT, exclusive.  Then evaluate RESULT to get
-the return value (nil if RESULT is omitted).
+the return value (nil if RESULT is omitted).  Note that RESULT
+should not be used unless it makes use of VAR.
 
 \(fn (VAR COUNT [RESULT]) BODY...)"
   (declare (indent 1) (debug dolist))
diff --git a/etc/NEWS b/etc/NEWS
index bde9b89..06896d4 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -433,6 +433,9 @@ names" in the Tramp manual for full documentation of these facilities.
 \f
 * Incompatible Lisp Changes in Emacs 27.1
 
+** The RESULT argument of ‘dotimes’ should not be used
+unless it makes use of the VAR argument.
+
 ** The 'repetitions' argument of 'benchmark-run' can now also be a variable.
 ** The FILENAME argument to 'file-name-base' is now mandatory and no
 longer defaults to 'buffer-file-name'.
diff --git a/doc/lispref/control.texi b/doc/lispref/control.texi
index adec632..dc23d2d 100644
--- a/doc/lispref/control.texi
+++ b/doc/lispref/control.texi
@@ -703,6 +703,7 @@ Iteration
 (inclusive) to @var{count} (exclusive), binding the variable @var{var}
 to the integer for the current iteration.  Then it returns the value
 of evaluating @var{result}, or @code{nil} if @var{result} is omitted.
+Note that the @var{result} should not be used unless it makes use of @var{var}.
 Here is an example of using @code{dotimes} to do something 100 times:
 
 @example
diff --git a/doc/misc/cl.texi b/doc/misc/cl.texi
index bf85b00..f74214e 100644
--- a/doc/misc/cl.texi
+++ b/doc/misc/cl.texi
@@ -1712,9 +1712,10 @@ Iteration
 The body is executed with @var{var} bound to the integers
 from zero (inclusive) to @var{count} (exclusive), in turn.  Then
 @c FIXME lispref does not state this part explicitly, could move this there.
-the @code{result} form is evaluated with @var{var} bound to the total
+the @var{result} form is evaluated with @var{var} bound to the total
 number of iterations that were done (i.e., @code{(max 0 @var{count})})
-to get the return value for the loop form.
+to get the return value for the loop form.  Note that the @var{result}
+should not be used unless it makes use of @var{var}.
 @end defmac
 
 @defmac cl-do-symbols (var [obarray [result]]) forms@dots{}
diff --git a/doc/lispintro/emacs-lisp-intro.texi b/doc/lispintro/emacs-lisp-intro.texi
index b672d7c..4d514aa 100644
--- a/doc/lispintro/emacs-lisp-intro.texi
+++ b/doc/lispintro/emacs-lisp-intro.texi
@@ -11013,9 +11013,8 @@ dotimes
 loops a specific number of times.
 
 The first argument to @code{dotimes} is assigned the numbers 0, 1, 2
-and so forth each time around the loop, and the value of the third
-argument is returned.  You need to provide the value of the second
-argument, which is how many times the macro loops.
+and so forth each time around the loop.  You need to provide the value
+of the second argument, which is how many times the macro loops.
 
 @need 1250
 For example, the following binds the numbers from 0 up to, but not
@@ -11027,17 +11026,18 @@ dotimes
 @smallexample
 @group
 (let (value)      ; otherwise a value is a void variable
-  (dotimes (number 3 value)
-    (setq value (cons number value))))
+  (dotimes (number 3)
+    (setq value (cons number value)))
+  value)
 
 @result{} (2 1 0)
 @end group
 @end smallexample
 
 @noindent
-@code{dotimes} returns @code{value}, so the way to use
-@code{dotimes} is to operate on some expression @var{number} number of
-times and then return the result, either as a list or an atom.
+The way to use @code{dotimes} is to operate on some expression
+@var{number} number of times and then return the result, either as
+a list or an atom.
 
 @need 1250
 Here is an example of a @code{defun} that uses @code{dotimes} to add
@@ -11048,8 +11048,9 @@ dotimes
 (defun triangle-using-dotimes (number-of-rows)
   "Using `dotimes', add up the number of pebbles in a triangle."
 (let ((total 0))  ; otherwise a total is a void variable
-  (dotimes (number number-of-rows total)
-    (setq total (+ total (1+ number))))))
+  (dotimes (number number-of-rows)
+    (setq total (+ total (1+ number))))
+  total))
 
 (triangle-using-dotimes 4)
 @end group
diff --git a/test/lisp/filenotify-tests.el b/test/lisp/filenotify-tests.el
index 219fa74..fa1ac95 100644
--- a/test/lisp/filenotify-tests.el
+++ b/test/lisp/filenotify-tests.el
@@ -1129,14 +1129,16 @@ file-notify--test-with-events
 	     ;; w32notify fires both `deleted' and `renamed' events.
 	     ((string-equal (file-notify--test-library) "w32notify")
 	      (let (r)
-		(dotimes (_i n r)
-		  (setq r (append '(deleted renamed) r)))))
+		(dotimes (_i n)
+		  (setq r (append '(deleted renamed) r)))
+                r))
 	     ;; cygwin fires `changed' and `deleted' events, sometimes
 	     ;; in random order.
 	     ((eq system-type 'cygwin)
 	      (let (r)
-		(dotimes (_i n (cons :random r))
-		  (setq r (append '(changed deleted) r)))))
+		(dotimes (_i n)
+		  (setq r (append '(changed deleted) r)))
+                (cons :random r)))
 	     (t (make-list n 'renamed)))
           (let ((source-file-list source-file-list)
                 (target-file-list target-file-list))
diff --git a/test/src/emacs-module-tests.el b/test/src/emacs-module-tests.el
index 8b6328d..9ef5a47 100644
--- a/test/src/emacs-module-tests.el
+++ b/test/src/emacs-module-tests.el
@@ -138,8 +138,9 @@ mod-test-emacs
 
 (defun multiply-string (s n)
   (let ((res ""))
-    (dotimes (i n res)
-      (setq res (concat res s)))))
+    (dotimes (i n)
+      (setq res (concat res s)))
+    res))
 
 (ert-deftest mod-test-globref-make-test ()
   (let ((mod-str (mod-test-globref-make))

bug#16206: 24.3; Incorrect unused variable byte-compiler warning in dotimes

[Stefan Monnier]

> -the return value (nil if RESULT is omitted).
> +the return value (nil if RESULT is omitted).  Note that RESULT
> +should not be used unless it makes use of VAR.

I would drop the "unless it makes use of VAR".


        Stefan

bug#16206: 24.3; Incorrect unused variable byte-compiler warning in dotimes

[Michael Heerdegen]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> > -the return value (nil if RESULT is omitted).
> > +the return value (nil if RESULT is omitted).  Note that RESULT
> > +should not be used unless it makes use of VAR.
>
> I would drop the "unless it makes use of VAR".

Yes.

For example,

#+begin_src emacs-lisp
(let ((count 10) (result 0))
   (dotimes (i count result)
     (print i)
     (setq result (+ result i))))
#+end_src

produces the warning - but doesn't RESULT "make use of the VAR"?

With the suggested patch, we would have a strange macro argument, using
it can produce strange compiler warnings, and we would have an obscure
description how these should be avoided.  It has to stop.


Michael.

bug#16206: 24.3; Incorrect unused variable byte-compiler warning in dotimes

[Juri Linkov]

>> -the return value (nil if RESULT is omitted).
>> +the return value (nil if RESULT is omitted).  Note that RESULT
>> +should not be used unless it makes use of VAR.
>
> I would drop the "unless it makes use of VAR".

But without that addition the sentence makes no sense when
the documentation describes the RESULT argument and then
at the end says that "RESULT should not be used".
And I have no better idea how to handle its obsolescence:
maybe just remove all mentions of RESULT from documentation?

bug#16206: 24.3; Incorrect unused variable byte-compiler warning in dotimes

[Stefan Monnier]

> But without that addition the sentence makes no sense when
> the documentation describes the RESULT argument and then
> at the end says that "RESULT should not be used".

"Its use is deprecated".


        Stefan

bug#16206: 24.3; Incorrect unused variable byte-compiler warning in dotimes

[Juri Linkov]

Version: 27.0.50

>> But without that addition the sentence makes no sense when
>> the documentation describes the RESULT argument and then
>> at the end says that "RESULT should not be used".
>
> "Its use is deprecated".

Done in f4eeb0f.

bug#31232: bug#16206: 24.3; Incorrect unused variable byte-compiler warning in dotimes

[Lars Ingebrigtsen]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> But without that addition the sentence makes no sense when
>> the documentation describes the RESULT argument and then
>> at the end says that "RESULT should not be used".
>
> "Its use is deprecated".

I've touched it up further -- I thought it was unclear what "it" was
referring to, and I've now also mentioned that using it may result in
byte compilation warnings.

With that change, I don't think there's anything more to fix here.  It
was mentioned that perhaps cl-dotimes should have a different
implementation, but I, too, think the RESULT bit is awkward, so I don't
think it's worth it.  So I'm closing this bug report.

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

bug#16206: 24.3; Incorrect unused variable byte-compiler warning in dotimes

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Tue, 24 Apr 2018 22:21:31 +0300
> Cc: Christopher Wellons <wellons@nullprogram.com>, 16206@debbugs.gnu.org
> 
> diff --git a/etc/NEWS b/etc/NEWS
> index bde9b89..06896d4 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -433,6 +433,9 @@ names" in the Tramp manual for full documentation of these facilities.
>  \f
>  * Incompatible Lisp Changes in Emacs 27.1
>  
> +** The RESULT argument of ‘dotimes’ should not be used
> +unless it makes use of the VAR argument.

This isn't a change, let alone an incompatible one.  We don't call out
documentation changes in NEWS.

Thanks.