bug#39919: 26.3; Incorrect byte-compiler warning

bug#39919: 26.3; Incorrect byte-compiler warning

[Mike Woolley]

Compiling this minimal test:

;; -*- lexical-binding: t -*-

(dotimes (i 10 t)
    (prin1 i))

Gives the incorrect warning: "Unused lexical variable ‘i’”, even though `i' is clearly used inside the loop.

However the following version (dropping the [result] form from `dotimes') does not give the warning:

(dotimes (i 10)
    (prin1 i))

This would seem like a bug to me.
Please advise?

Thanks,
Mike



In GNU Emacs 26.3 (build 1, x86_64-apple-darwin18.2.0, NS appkit-1671.20 Version 10.14.3 (Build 18D109))
 of 2019-09-02 built on builder10-14.porkrind.org
Windowing system distributor 'Apple', version 10.3.1894
Recent messages:
Undo!
next-line: End of buffer [2 times]
Mark set [2 times]
auto trimmed 0 chars
Saving file /Users/mike/tmp/test.el...
Wrote /Users/mike/tmp/test.el
Mark saved where search started
Saving file /Users/mike/tmp/test.el...
Wrote /Users/mike/tmp/test.el
Type "q" to restore previous buffer. [3 times]
previous-line: Beginning of buffer [2 times]
Configured using:
 'configure --with-ns '--enable-locallisppath=/Library/Application
 Support/Emacs/${version}/site-lisp:/Library/Application
 Support/Emacs/site-lisp' --with-modules'

Configured features:
NOTIFY ACL GNUTLS LIBXML2 ZLIB TOOLKIT_SCROLL_BARS NS MODULES THREADS

Important settings:
  value of $LANG: en_GB.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Emacs-Lisp

Minor modes in effect:
  shell-dirtrack-mode: t
  diff-auto-refine-mode: t
  global-git-gutter-mode: t
  git-gutter-mode: t
  global-company-mode: t
  company-mode: t
  global-flycheck-mode: t
  flycheck-mode: t
  helm-mode: t
  async-bytecomp-package-mode: t
  clean-aindent-mode: t
  yas-global-mode: t
  yas-minor-mode: t
  global-auto-revert-mode: t
  global-semantic-decoration-mode: t
  global-semanticdb-minor-mode: t
  global-semantic-idle-scheduler-mode: t
  global-semantic-highlight-func-mode: t
  global-semantic-stickyfunc-mode: t
  semantic-mode: t
  helm-autoresize-mode: t
  which-function-mode: t
  show-paren-mode: t
  recentf-mode: t
  msb-mode: t
  gud-tooltip-mode: t
  display-time-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  temp-buffer-resize-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort face-remap flyspell ispell mail-extr gnus-msg gnus-art
mm-uu mml2015 mm-view mml-smime smime dig gnus-sum gnus-group gnus-undo
gnus-start gnus-cloud nnimap nnmail mail-source tls gnutls utf7 netrc
nnoo gnus-spec gnus-int gnus-range gnus-win gnus nnheader emacsbug
message rmc puny rfc822 mml mml-sec epa derived epg gnus-util rmail
rmail-loaddefs mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums
mm-util mail-prsvr mail-utils helm-command eieio-opt speedbar sb-image
dframe help-fns radix-tree windmove misearch multi-isearch ido
wildcard-to-regexp dired-explore dired-sort-menu ls-lisp dired-x dired
dired-loaddefs helm-x-files bs helm-for-files helm-bookmark
helm-adaptive bookmark pp add-log winner image-file helm-external
helm-net browse-url xml url url-proxy url-privacy url-expand url-methods
url-history url-cookie url-domsuf url-util mailcap tramp tramp-compat
tramp-loaddefs trampver shell pcomplete parse-time ffap server
gnuemacs-config my-commands rg rg-ibuffer rg-result wgrep-rg wgrep s
rg-history rg-header rg-compat ibuf-ext ibuffer ibuffer-loaddefs grep
helm-ls-git vc-git diff-mode vc vc-dispatcher git-gutter-fringe
git-gutter helm-company helm-elisp helm-eval edebug helm-info
company-oddmuse company-keywords company-etags company-gtags
company-dabbrev-code company-dabbrev company-files company-capf
company-cmake company-xcode company-clang company-semantic company-eclim
company-template company-bbdb company pcase flycheck json map rx dash
helm-mode helm-config helm-easymenu async-bytecomp xahk-mode thingatpt
htmlize clean-aindent-mode yasnippet-snippets yasnippet elec-pair
gc-info autorevert filenotify semantic/decorate/mode semantic/decorate
semantic/db-mode semantic/db eieio-base semantic/idle semantic/format
ezimage semantic/tag-ls semantic/find semantic/ctxt semantic/util-modes
semantic/util semantic semantic/tag semantic/lex semantic/fw mode-local
find-func cedet tempo filladapt helm-gtags subr-x pulse helm-files
helm-buffers helm-tags helm-locate helm-grep helm-regexp format-spec
helm-utils helm-help helm-types helm edmacro kmacro helm-source
eieio-compat helm-multi-match helm-lib advice async ggtags etags xref
project compile ewoc use-package-diminish my-generic generic generic-x
jka-compr which-func imenu paren recentf tree-widget wid-edit msb gud
easy-mmode comint ansi-color ring time cus-start cus-load redo+ b
b-window b-undo b-movement b-editing b-marking b-bookmark fringe-helper
b-compat diminish poet-theme use-package-ensure use-package-core
finder-inf info package epg-config url-handlers url-parse auth-source
cl-seq eieio eieio-core cl-macs eieio-loaddefs password-cache url-vars
seq byte-opt bytecomp byte-compile cconv cl-extra help-mode easymenu
my-compat cl gv cl-loaddefs cl-lib time-date tooltip eldoc electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel term/ns-win ns-win
ucs-normalize mule-util term/common-win tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode elisp-mode
lisp-mode prog-mode register page menu-bar rfn-eshadow isearch timer
select scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript charprop case-table epa-hook jka-cmpr-hook
help simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote threads kqueue cocoa ns multi-tty make-network-process emacs)

Memory information:
((conses 16 676901 15049)
 (symbols 48 49397 24)
 (miscs 40 261 1018)
 (strings 32 171654 3031)
 (string-bytes 1 4841259)
 (vectors 16 78649)
 (vector-slots 8 1272975 58328)
 (floats 8 323 248)
 (intervals 56 1704 225)
 (buffers 992 24))

bug#39919: 26.3; Incorrect byte-compiler warning

[Michael Heerdegen]

Mike Woolley <mike@bulsara.com> writes:

> This would seem like a bug to me.

Yes, the warning is wrong.  There is already a corresponding "FIXME" in
the source:

|  ;; FIXME: This let often leads to "unused var" warnings.


Michael.

bug#39919: 26.3; Incorrect byte-compiler warning

[Mike Woolley]

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

Yes this was pointed out to me :-)

Looks like we get the unused variable warning when there is a result form which does not contain the loop variable.

[I guess the normal use case would indeed be to use the loop var in the result form and so writing it like this was probably a reasonable compromise…]

I will adjust my code to work around it.

Thanks,
Mike

> On 5 Mar 2020, at 13:50, Michael Heerdegen <michael_heerdegen@web.de> wrote:
> 
> Mike Woolley <mike@bulsara.com> writes:
> 
>> This would seem like a bug to me.
> 
> Yes, the warning is wrong.  There is already a corresponding "FIXME" in
> the source:
> 
> |  ;; FIXME: This let often leads to "unused var" warnings.
> 
> 
> Michael.


[-- Attachment #2: Type: text/html, Size: 1992 bytes --]

bug#39919: 26.3; Incorrect byte-compiler warning

[Michael Heerdegen]

Mike Woolley <mike@bulsara.com> writes:

> [I guess the normal use case would indeed be to use the loop var in
> the result form and so writing it like this was probably a reasonable
> compromise…]

No, it's just the warning is not easy to avoid, it's not raised
explicitly, it's an accident that it's raised in cases like yours which
are valid cases, even if they are not the majority of uses.

So I think it's worth to fix this.

Michael.

bug#39919: 26.3; Incorrect byte-compiler warning

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> So I think it's worth to fix this.

Hmm, no, actually, there were already two old bug reports about this
issue: bug#16206 and 31232.  The RESULT argument of `dotimes' had been
deprecated as a result of that discussion.

So I'm going to merge this report with the others and close it.

Michael.

bug#39919: 26.3; Incorrect byte-compiler warning

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > Hmm, no, actually, there were already two old bug reports about this
  > issue: bug#16206 and 31232.  The RESULT argument of `dotimes' had been
  > deprecated as a result of that discussion.

I can't easily obtain that discussion.  Perhaps that discussion
turned up some other reason to deprecated that argument.  But if it
was solely to avoid these warnings, I am surprised it is hard.

Does this patch fix the problem?

diff -u /home/rms/emacs-git/build-oct-2/lisp/subr.el.\~1\~ /home/rms/emacs-git/build-oct-2/lisp/subr.el
--- /home/rms/emacs-git/build-oct-2/lisp/subr.el.~1~	2019-10-02 11:07:09.046065358 -0400
+++ /home/rms/emacs-git/build-oct-2/lisp/subr.el	2020-03-06 19:31:20.053693281 -0500
@@ -281,7 +281,7 @@
                (setq ,counter (1+ ,counter)))
              ,@(if (cddr spec)
                    ;; FIXME: This let often leads to "unused var" warnings.
-                   `((let ((,(car spec) ,counter)) ,@(cddr spec))))))
+                   `((let ((,(car spec) ,counter)) ,(car spec) ,@(cddr spec))))))
       `(let ((,temp ,end)
              (,(car spec) ,start))
          (while (< ,(car spec) ,temp)

Diff finished.  Fri Mar  6 19:34:23 2020

-- 
Dr Richard Stallman
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#39919: 26.3; Incorrect byte-compiler warning

[Mike Woolley]

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

Yes that fixes the issue in the test cases I sent.
Also looks good in my real code where I noticed the problem!

Thanks Richard,
Mike

> On 7 Mar 2020, at 04:23, Richard Stallman <rms@gnu.org> wrote:
> 
> [[[ To any NSA and FBI agents reading my email: please consider    ]]]
> [[[ whether defending the US Constitution against all enemies,     ]]]
> [[[ foreign or domestic, requires you to follow Snowden's example. ]]]
> 
>> Hmm, no, actually, there were already two old bug reports about this
>> issue: bug#16206 and 31232.  The RESULT argument of `dotimes' had been
>> deprecated as a result of that discussion.
> 
> I can't easily obtain that discussion.  Perhaps that discussion
> turned up some other reason to deprecated that argument.  But if it
> was solely to avoid these warnings, I am surprised it is hard.
> 
> Does this patch fix the problem?
> 
> diff -u /home/rms/emacs-git/build-oct-2/lisp/subr.el.\~1\~ /home/rms/emacs-git/build-oct-2/lisp/subr.el
> --- /home/rms/emacs-git/build-oct-2/lisp/subr.el.~1~	2019-10-02 11:07:09.046065358 -0400
> +++ /home/rms/emacs-git/build-oct-2/lisp/subr.el	2020-03-06 19:31:20.053693281 -0500
> @@ -281,7 +281,7 @@
>                (setq ,counter (1+ ,counter)))
>              ,@(if (cddr spec)
>                    ;; FIXME: This let often leads to "unused var" warnings.
> -                   `((let ((,(car spec) ,counter)) ,@(cddr spec))))))
> +                   `((let ((,(car spec) ,counter)) ,(car spec) ,@(cddr spec))))))
>       `(let ((,temp ,end)
>              (,(car spec) ,start))
>          (while (< ,(car spec) ,temp)
> 
> Diff finished.  Fri Mar  6 19:34:23 2020
> 
> -- 
> Dr Richard Stallman
> Chief GNUisance of the GNU Project (https://gnu.org)
> Founder, Free Software Foundation (https://fsf.org)
> Internet Hall-of-Famer (https://internethalloffame.org)
> 
> 


[-- Attachment #2: Type: text/html, Size: 4325 bytes --]

bug#39919: 26.3; Incorrect byte-compiler warning

[Michael Heerdegen]

Mike Woolley <mike@bulsara.com> writes:

> Yes that fixes the issue in the test cases I sent.
> Also looks good in my real code where I noticed the problem!

Seems the current warning is a compromise.  It is not perfect because it
is not really clear what the warning means.  I would vote for a clear
"argument is deprecated" warning.

Michael.

bug#39919: 26.3; Incorrect byte-compiler warning

[Mike Woolley]

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

I think people using `dotimes' from `cl’ are going to expect it to work like in Common Lisp (as that’s the whole point).

Just deprecating the result parameter because it’s too hard doesn’t seem like a good solution :-)

Thanks,
Mike

> On 28 Apr 2020, at 03:52, Michael Heerdegen <michael_heerdegen@web.de> wrote:
> 
> Mike Woolley <mike@bulsara.com> writes:
> 
>> Yes that fixes the issue in the test cases I sent.
>> Also looks good in my real code where I noticed the problem!
> 
> Seems the current warning is a compromise.  It is not perfect because it
> is not really clear what the warning means.  I would vote for a clear
> "argument is deprecated" warning.
> 
> Michael.


[-- Attachment #2: Type: text/html, Size: 2208 bytes --]

bug#31232: bug#39919: 26.3; Incorrect byte-compiler warning

[Michael Heerdegen]

Mike Woolley <mike@bulsara.com> writes:

> I think people using `dotimes' from `cl’ are going to expect it to
> work like in Common Lisp (as that’s the whole point).

But note that dotimes is in subr.el, and cl-dotimes is separate (though
it's currently implemented on top of dotimes).

> Just deprecating the result parameter because it’s too hard doesn’t
> seem like a good solution :-)

I regret that I said it like that, no, that's not a reason.  The main
reason is that the whole existence (usefulness) of the RESULT argument
and its position in the syntax are questionable.

Michael.

bug#31232: bug#39919: 26.3; Incorrect byte-compiler warning

[Mike Woolley]

Thanks for the explanation Michael.

How about changing dotimes as you suggested, but making cl-dotimes have it’s own implementation with the result parameter fix applied? 

Then everyone’s a winner - those who want the CL behaviour can have it, but core Emacs will have a cleaner version.

Thanks,
Mike

> On 28 Apr 2020, at 19:13, Michael Heerdegen <michael_heerdegen@web.de> wrote:
> 
> Mike Woolley <mike@bulsara.com> writes:
> 
>> I think people using `dotimes' from `cl’ are going to expect it to
>> work like in Common Lisp (as that’s the whole point).
> 
> But note that dotimes is in subr.el, and cl-dotimes is separate (though
> it's currently implemented on top of dotimes).
> 
>> Just deprecating the result parameter because it’s too hard doesn’t
>> seem like a good solution :-)
> 
> I regret that I said it like that, no, that's not a reason.  The main
> reason is that the whole existence (usefulness) of the RESULT argument
> and its position in the syntax are questionable.
> 
> Michael.
> 

bug#31232: bug#39919: 26.3; Incorrect byte-compiler warning

[Michael Heerdegen]

Mike Woolley <mike@bulsara.com> writes:

> Thanks for the explanation Michael.
>
> How about changing dotimes as you suggested, but making cl-dotimes
> have it’s own implementation with the result parameter fix applied?
>
> Then everyone’s a winner - those who want the CL behaviour can have
> it, but core Emacs will have a cleaner version.

Exactly my idea after thinking about what I had written :-)

Michael.

bug#39919: 26.3; Incorrect byte-compiler warning

[Michael Heerdegen]

Richard Stallman <rms@gnu.org> writes:

> I can't easily obtain that discussion.  Perhaps that discussion
> turned up some other reason to deprecated that argument.

Yes: the reason was that it's not very useful, the order of
execution is strange, and it's easy to avoid, so, to cite Stefan, "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."

> But if it was solely to avoid these warnings, I am surprised it is
> hard.
>
> Does this patch fix the problem?

Yes, that should work.  I don't consider it a good fix because it just
hides the underlying problem (I guess that's why the original author
added the FIXME instead of fixing it in this obvious way).  We have
manifestations of the same issue in other places that, AFAIR, can't be
fixed in the same way.

Anyway, I think the warning currently raised is not helpful, it is
confusing if you didn't read the related bug reports.  I guess we could
just say that the argument is deprecated.  Maybe that didn't happen
because dotimes in Common Lisp has that third argument too (as mentioned
in the other reports).

Michael.