bug#46819: 28.0.50; Undesireable warning: Empty let body from the `benchmark-run' implementation

bug#46819: 28.0.50; Undesireable warning: Empty let body from the `benchmark-run' implementation

[Matt Armstrong]

This behavior began with Stefan's recent commit d41a4ad4ae (*
lisp/emacs-lisp/macroexp.el ;; (macroexp--expand-all): Warn on empty let
bodies, 2021-02-15)

`benchmark-run' has a timing `doloop' with an empty body, which `doloop`
expands to a `let` with an empty body (in the lexical-binding case).

Save the following into my-benchmark-warning.el

----------------------------------------------------------------------
;;; Code:

(require 'benchmark)

(defun my-list-benchmark ()
  "Lorem ipsum dolor."
  (benchmark-run 100
    (list 1 2 3)))

(provide 'my-benchmark-warning)
;;; my-benchmark-warning.el ends here
----------------------------------------------------------------------

Then run this and you'll see the interaction below:

----------------------------------------------------------------------
% ./src/emacs -Q --batch -f batch-byte-compile ./my-benchmark-warning.el

In my-list-benchmark:
./my-benchmark-warning.el:32:8: Warning: Empty let body
----------------------------------------------------------------------





In GNU Emacs 28.0.50 (build 1, x86_64-apple-darwin20.3.0, NS appkit-2022.30 Version 11.2.1 (Build 20D74))
 of 2021-02-27 built on matts-mbp-2016.lan
Repository revision: de33de1a660283f45a10acfaedef20c460553dbd
Repository branch: master
Windowing system distributor 'Apple', version 10.3.2022
System Description:  macOS 11.2.1

Configured using:
 'configure --without-info'

Configured features:
ACL GLIB GMP GNUTLS JPEG JSON LIBXML2 MODULES NOTIFY KQUEUE NS PDUMPER
PNG RSVG THREADS TIFF TOOLKIT_SCROLL_BARS ZLIB

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

Major mode: Special

Minor modes in effect:
  winner-mode: t
  electric-pair-mode: t
  auto-insert-mode: t
  display-time-mode: t
  show-paren-mode: t
  keyfreq-autosave-mode: t
  keyfreq-mode: t
  icomplete-mode: t
  magit-auto-revert-mode: t
  global-auto-revert-mode: t
  global-git-commit-mode: t
  async-bytecomp-package-mode: t
  shell-dirtrack-mode: t
  global-flycheck-mode: t
  which-key-mode: t
  override-global-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-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
  buffer-read-only: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
/Users/matt/git/notmuch/emacs/notmuch-mua hides /Users/matt/.emacs.d/elpa/notmuch-0.31.3/notmuch-mua
/Users/matt/git/notmuch/emacs/notmuch-tree hides /Users/matt/.emacs.d/elpa/notmuch-0.31.3/notmuch-tree
/Users/matt/git/notmuch/emacs/notmuch-show hides /Users/matt/.emacs.d/elpa/notmuch-0.31.3/notmuch-show
/Users/matt/git/notmuch/emacs/notmuch-tag hides /Users/matt/.emacs.d/elpa/notmuch-0.31.3/notmuch-tag
/Users/matt/git/notmuch/emacs/coolj hides /Users/matt/.emacs.d/elpa/notmuch-0.31.3/coolj
/Users/matt/git/notmuch/emacs/notmuch-wash hides /Users/matt/.emacs.d/elpa/notmuch-0.31.3/notmuch-wash
/Users/matt/git/notmuch/emacs/notmuch-maildir-fcc hides /Users/matt/.emacs.d/elpa/notmuch-0.31.3/notmuch-maildir-fcc
/Users/matt/git/notmuch/emacs/notmuch-compat hides /Users/matt/.emacs.d/elpa/notmuch-0.31.3/notmuch-compat
/Users/matt/git/notmuch/emacs/notmuch hides /Users/matt/.emacs.d/elpa/notmuch-0.31.3/notmuch
/Users/matt/git/notmuch/emacs/notmuch-address hides /Users/matt/.emacs.d/elpa/notmuch-0.31.3/notmuch-address
/Users/matt/git/notmuch/emacs/rstdoc hides /Users/matt/.emacs.d/elpa/notmuch-0.31.3/rstdoc
/Users/matt/git/notmuch/emacs/notmuch-lib hides /Users/matt/.emacs.d/elpa/notmuch-0.31.3/notmuch-lib
/Users/matt/git/notmuch/emacs/notmuch-print hides /Users/matt/.emacs.d/elpa/notmuch-0.31.3/notmuch-print
/Users/matt/git/notmuch/emacs/notmuch-draft hides /Users/matt/.emacs.d/elpa/notmuch-0.31.3/notmuch-draft
/Users/matt/git/notmuch/emacs/notmuch-jump hides /Users/matt/.emacs.d/elpa/notmuch-0.31.3/notmuch-jump
/Users/matt/git/notmuch/emacs/notmuch-parser hides /Users/matt/.emacs.d/elpa/notmuch-0.31.3/notmuch-parser
/Users/matt/git/notmuch/emacs/notmuch-query hides /Users/matt/.emacs.d/elpa/notmuch-0.31.3/notmuch-query
/Users/matt/git/notmuch/emacs/notmuch-message hides /Users/matt/.emacs.d/elpa/notmuch-0.31.3/notmuch-message
/Users/matt/git/notmuch/emacs/notmuch-hello hides /Users/matt/.emacs.d/elpa/notmuch-0.31.3/notmuch-hello
/Users/matt/git/notmuch/emacs/notmuch-crypto hides /Users/matt/.emacs.d/elpa/notmuch-0.31.3/notmuch-crypto
/Users/matt/git/notmuch/emacs/make-deps hides /Users/matt/.emacs.d/elpa/notmuch-0.31.3/make-deps
/Users/matt/git/notmuch/emacs/notmuch-company hides /Users/matt/.emacs.d/elpa/notmuch-0.31.3/notmuch-company

Features:
(shadow sort company-oddmuse company-keywords company-etags
company-gtags company-dabbrev-code company-dabbrev company-files
company-clang company-capf company-cmake company-semantic
company-template company-bbdb mail-extr emacsbug sendmail org-element
avl-tree ol-w3m ol-rmail ol-mhe ol-irc ol-info ol-gnus nnselect
gnus-search eieio-opt speedbar ezimage dframe gnus-art mm-uu mml2015
gnus-sum gnus-group gnus-undo gnus-start gnus-dbus dbus gnus-cloud
nnimap nnmail mail-source utf7 netrc nnoo gnus-spec gnus-int gnus-range
gnus-win ol-eww eww xdg url-queue mm-url gnus nnheader ol-docview
doc-view jka-compr image-mode exif ol-bibtex bibtex ol-bbdb winner
cus-start cus-load elec-pair slime-fancy slime-indentation
slime-cl-indent cl-indent slime-trace-dialog slime-fontifying-fu
slime-package-fu slime-references slime-compiler-notes-tree
slime-scratch slime-presentations bridge slime-macrostep macrostep
slime-mdot-fu slime-enclosing-context slime-fuzzy slime-fancy-trace
slime-fancy-inspector slime-c-p-c slime-editing-commands slime-autodoc
slime-repl elp slime-parse slime gud apropos arc-mode archive-mode
hyperspec slime-autoloads warnings org-protocol notmuch notmuch-tree
notmuch-jump notmuch-hello notmuch-show notmuch-print notmuch-crypto
notmuch-mua notmuch-message notmuch-draft notmuch-maildir-fcc
notmuch-address notmuch-company notmuch-parser notmuch-wash coolj
notmuch-query goto-addr icalendar diary-lib diary-loaddefs notmuch-tag
notmuch-lib notmuch-compat hl-line mm-view mml-smime smime dig
elisp-format protbuf org-drill persist org-id org-agenda org-refile org
ob ob-tangle ob-ref ob-lob ob-table ob-exp org-macro org-footnote
org-src ob-comint org-pcomplete org-list org-faces org-entities
org-version ob-emacs-lisp ob-core ob-eval org-table ol org-keys
org-compat org-macs org-loaddefs cal-menu calendar cal-loaddefs diminish
ctypes skeleton autoinsert time paren ert debug backtrace keyfreq
generic icomplete highlight-symbol cl hi-lock racket-mode
racket-bug-report racket-collection tq racket-repl-buffer-name
racket-stepper racket-logger racket-profile racket-smart-open racket-xp
racket-xp-complete racket-show pos-tip racket-imenu racket-edit hideshow
racket-repl semantic/symref/grep semantic/symref semantic/util-modes
semantic/util semantic pp semantic/tag semantic/lex semantic/fw
mode-local cedet racket-eldoc racket-describe shr kinsoku svg xml dom
racket-visit racket-complete racket-common racket-parens racket-indent
racket-font-lock racket-ppss racket-keywords-and-builtins racket-doc
racket-cmd racket-util racket-browse-url racket-custom sh-script smie
executable company-lsp company lsp-mode lsp-protocol tree-widget
wid-edit pcase network-stream nsm markdown-mode noutline outline lv
inline ht f s ewoc dash-functional bindat projectile grep ibuf-ext
ibuffer ibuffer-loaddefs cider tramp-sh cider-debug cider-browse-ns
cider-mode cider-inspector cider-completion cider-profile cider-eval
cider-repl-history pulse cider-repl cider-resolve cider-test
cider-overlays cider-stacktrace cider-doc cider-browse-spec
cider-clojuredocs cider-eldoc cider-client cider-common cider-connection
cider-util color cider-popup sesman-browser nrepl-client tramp
tramp-loaddefs trampver tramp-integration files-x tramp-compat
parse-time iso8601 ls-lisp queue nrepl-dict cider-compat spinner
parseedn parseclj-parser parseclj-lex a sesman vc vc-dispatcher edmacro
kmacro clojure-mode lisp-mnt align magit-submodule magit-obsolete
magit-blame magit-stash magit-reflog magit-bisect magit-push magit-pull
magit-fetch magit-clone magit-remote magit-commit magit-sequence
magit-notes magit-worktree magit-tag magit-merge magit-branch
magit-reset magit-files magit-refs magit-status magit magit-repos
magit-apply magit-wip magit-log which-func imenu magit-diff smerge-mode
diff diff-mode magit-core magit-autorevert autorevert filenotify
magit-margin magit-transient magit-process magit-mode git-commit
transient format-spec magit-git magit-section magit-utils crm log-edit
message rmc puny dired dired-loaddefs rfc822 mml mml-sec epa derived epg
epg-config gnus-util rmail rmail-loaddefs time-date mm-decode mm-bodies
mm-encode mail-parse rfc2231 rfc2047 rfc2045 mm-util ietf-drums
mail-prsvr mailabbrev mail-utils gmm-utils mailheader pcvs-util add-log
with-editor async-bytecomp async shell pcomplete server ido flycheck
find-func rx dash go-mode find-file ffap thingatpt etags fileloop
generator xref project compile text-property-search comint ansi-color
ring which-key advice exec-path-from-shell cl-extra help-mode
use-package use-package-ensure use-package-delight use-package-diminish
use-package-bind-key bind-key easy-mmode use-package-core finder-inf
info package browse-url url url-proxy url-privacy url-expand url-methods
url-history url-cookie url-domsuf url-util mailcap url-handlers
url-parse auth-source cl-seq eieio eieio-core cl-macs eieio-loaddefs
password-cache json subr-x map url-vars seq byte-opt gv bytecomp
byte-compile cconv cl-loaddefs cl-lib iso-transl 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 tab-bar menu-bar easymenu rfn-eshadow
isearch timer select scroll-bar mouse jit-lock font-lock syntax facemenu
font-core term/tty-colors frame minibuffer 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
cl-preloaded nadvice button loaddefs faces cus-face macroexp files
window 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 766437 32631)
 (symbols 48 56488 4)
 (strings 32 248491 4562)
 (string-bytes 1 7334590)
 (vectors 16 89116)
 (vector-slots 8 956750 71031)
 (floats 8 732 300)
 (intervals 56 545 265)
 (buffers 992 12))

bug#46819: 28.0.50; Undesireable warning: Empty let body from the `benchmark-run' implementation

[Stefan Monnier]

> This behavior began with Stefan's recent commit d41a4ad4ae (*
> lisp/emacs-lisp/macroexp.el ;; (macroexp--expand-all): Warn on empty let
> bodies, 2021-02-15)
>
> `benchmark-run' has a timing `doloop' with an empty body, which `doloop`
> expands to a `let` with an empty body (in the lexical-binding case).

It'd be easy to put an explicit nil in the loop's body, but....

The idea was that in actual (non-macroexpanded) source code, empty let
bodies are usually the result of a misplaced paren.  But it's probably
more often annoying than useful since we can't easily distinguish a "let
from source code" from a "let from a macro expansion".

So I'm also voting to revert that change.
Any objection?


        Stefan

bug#46819: 28.0.50; Undesireable warning: Empty let body from the `benchmark-run' implementation

[Lars Ingebrigtsen]

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

> It'd be easy to put an explicit nil in the loop's body, but....
>
> The idea was that in actual (non-macroexpanded) source code, empty let
> bodies are usually the result of a misplaced paren.  But it's probably
> more often annoying than useful since we can't easily distinguish a "let
> from source code" from a "let from a macro expansion".
>
> So I'm also voting to revert that change.
> Any objection?

It is a nice and helpful warning, though, so if it could be fixed in
another way, that'd be great.  (But I have no idea how, of course.)

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

bug#46819: 28.0.50; Undesireable warning: Empty let body from the `benchmark-run' implementation

[Matt Armstrong]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>> It'd be easy to put an explicit nil in the loop's body, but....
>>
>> The idea was that in actual (non-macroexpanded) source code, empty
>> let bodies are usually the result of a misplaced paren.  But it's
>> probably more often annoying than useful since we can't easily
>> distinguish a "let from source code" from a "let from a macro
>> expansion".
>>
>> So I'm also voting to revert that change.  Any objection?
>
> It is a nice and helpful warning, though, so if it could be fixed in
> another way, that'd be great.  (But I have no idea how, of course.)

It might be hard to fix in the general case.  Think about this example:

(let (...)
  (some-macro blah))

What if that macro expands to nothing, based on some criteria not
entirely under programmer control?  Is the code "wrong"?

bug#46819: 28.0.50; Undesireable warning: Empty let body from the `benchmark-run' implementation

[Lars Ingebrigtsen]

Matt Armstrong <matt@rfc20.org> writes:

> It might be hard to fix in the general case.  Think about this example:
>
> (let (...)
>   (some-macro blah))
>
> What if that macro expands to nothing, based on some criteria not
> entirely under programmer control?  Is the code "wrong"?

No.  But if the code is

(let (...)
  )

then it probably is.  So the warning (if retained) has to be done on the
form read before doing any macro expansions.

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

bug#46819: 28.0.50; Undesireable warning: Empty let body from the `benchmark-run' implementation

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> It might be hard to fix in the general case.  Think about this example:
>>
>> (let (...)
>>   (some-macro blah))
>>
>> What if that macro expands to nothing, based on some criteria not
>> entirely under programmer control?  Is the code "wrong"?

(And besides -- the macro can't really expand to nothing here, can it?
(nil is something.)  Or am I misunderstanding what it's warning about?)

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

bug#46819: 28.0.50; Undesireable warning: Empty let body from the `benchmark-run' implementation

[Stefan Monnier]

> It might be hard to fix in the general case.  Think about this example:
>
> (let (...)
>   (some-macro blah))
>
> What if that macro expands to nothing,

No can do.  A macro call has to expand to exactly one expression (that
expression can be nil, but it's still one expression).


        Stefan

bug#46819: 28.0.50; Undesireable warning: Empty let body from the `benchmark-run' implementation

[Matt Armstrong]

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

>> It might be hard to fix in the general case.  Think about this example:
>>
>> (let (...)
>>   (some-macro blah))
>>
>> What if that macro expands to nothing,
>
> No can do.  A macro call has to expand to exactly one expression (that
> expression can be nil, but it's still one expression).
>
>         Stefan

Doh, of course.  That'd be a C macro I was thinking of...

bug#46819: 28.0.50; Undesireable warning: Empty let body from the `benchmark-run' implementation

[Matt Armstrong]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>> It'd be easy to put an explicit nil in the loop's body, but....
>>
>> The idea was that in actual (non-macroexpanded) source code, empty let
>> bodies are usually the result of a misplaced paren.  But it's probably
>> more often annoying than useful since we can't easily distinguish a "let
>> from source code" from a "let from a macro expansion".
>>
>> So I'm also voting to revert that change.
>> Any objection?
>
> It is a nice and helpful warning, though, so if it could be fixed in
> another way, that'd be great.  (But I have no idea how, of course.)

So, after a little flurry of a conversation, this went quiet.

How about we fix benchmark's use of `let', and leave the warning in
place.  It does not seem to be generating many other complaints, so it
may not be particularly annoying after all.  I'm currently working with
`benchmark-run' a bunch and would like the warning gone from that one
use.

I can make a patch, but a one liner might be quicker for one of you to
just commit directly.  ;-)

bug#46819: 28.0.50; Undesireable warning: Empty let body from the `benchmark-run' implementation

[Lars Ingebrigtsen]

Matt Armstrong <matt@rfc20.org> writes:

> How about we fix benchmark's use of `let', and leave the warning in
> place.  It does not seem to be generating many other complaints, so it
> may not be particularly annoying after all.  I'm currently working with
> `benchmark-run' a bunch and would like the warning gone from that one
> use.
>
> I can make a patch, but a one liner might be quicker for one of you to
> just commit directly.  ;-)

Yup; now done.

I think we should keep the byte-compilation warning -- at least for
now.  If it leads to false positives all over the place, then perhaps
not, but it's a useful warning, in my opinion.

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