bug#26486: 25.1.91; spurious warnings from cconv.el

bug#26486: 25.1.91; spurious warnings from cconv.el

[John Williams]

When I byte compile a file where a macro expansion introduces an unused
lexical veriable, I get warnings from cconv.el, even if I wrap the
expansion in (with-no-warnings) and set byte-compile-warnings to nil.
IMHO warnings of this type should never be issued for code that comes
from a macro expansion, but it should at least be possible to suppress
them with the usual mechanisms.  Here's a file that demonstrates the
problem when it's compiled with byte-compile-file:

========================================================================
;; -*- lexical-binding: t; byte-compile-warnings: nil  -*-

(defmacro test ()
  `(let (unused)))

(with-no-warnings
  (test))
========================================================================



In GNU Emacs 25.1.91.1 (x86_64-pc-linux-gnu, GTK+ Version 3.10.8),
modified by Debian
Windowing system distributor 'The X.Org Foundation', version 11.0.11803000
System Description:     Ubuntu 14.04.5 LTS

Configured using:
 'configure --build x86_64-linux-gnu --build x86_64-linux-gnu
 --prefix=/usr --sharedstatedir=/var/lib --libexecdir=/usr/lib
 --localstatedir=/var/lib --infodir=/usr/share/info
 --mandir=/usr/share/man --with-pop=yes
 --enable-locallisppath=/etc/google-emacs:/etc/emacs:/usr/local/share/emacs/25.1.91+gg1+2/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/25.1.91+gg1+2/site-lisp:/usr/share/emacs/site-lisp
 --with-crt-dir=/usr/lib/x86_64-linux-gnu --disable-build-details
 --disable-silent-rules --with-modules GOOGLE_VERSION=25.1.91+gg1+2
 --with-x=yes --with-x-toolkit=gtk3 --with-toolkit-scroll-bars
 build_alias=x86_64-linux-gnu 'CFLAGS=-g -O2 -fstack-protector
 --param=ssp-buffer-size=4 -Wformat -Werror=format-security -Wall'
 'LDFLAGS=-Wl,-Bsymbolic-functions -Wl,-z,relro
 -Wl,-fuse-ld=gold,--export-dynamic-symbol=__google_auxv'
 'CPPFLAGS=-D_FORTIFY_SOURCE=2 -DGOOGLE_EMACS_DEFINE_AUXV''

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GCONF GSETTINGS
NOTIFY LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 MODULES

Important settings:
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Emacs-Lisp

Minor modes in effect:
  diff-auto-refine-mode: t
  tooltip-mode: t
  global-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
  line-number-mode: t
  transient-mark-mode: t

Recent messages:
nil
Mark set
nil
Quit
Type "q" in help window to restore its previous buffer.
Undo! [22 times]
Saving file /usr/local/google/home/jrw/homedir/test.el...
Wrote /usr/local/google/home/jrw/homedir/test.el
Compiling /usr/local/google/home/jrw/homedir/test.el...done
Wrote /usr/local/google/home/jrw/homedir/test.elc
Quit

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message dired format-spec rfc822 mml
mml-sec password-cache epg epg-config gnus-util mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils byte-opt
compile comint ansi-color ring warnings thingatpt help-fns bytecomp
byte-compile cl-extra help-mode cl-loaddefs pcase cl-lib cconv vc-git
diff-mode easymenu easy-mmode time-date mule-util tooltip eldoc electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list newcomment elisp-mode lisp-mode prog-mode register page
menu-bar rfn-eshadow timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core 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 charscript case-table epa-hook jka-cmpr-hook help
simple abbrev 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
dbusbind inotify dynamic-setting system-font-setting font-render-setting
move-toolbar gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 105959 7747)
 (symbols 48 21355 0)
 (miscs 40 404 190)
 (strings 32 19965 4463)
 (string-bytes 1 608139)
 (vectors 16 14200)
 (vector-slots 8 469169 6038)
 (floats 8 184 167)
 (intervals 56 1103 0)
 (buffers 976 22)
 (heap 1024 30017 1036))

bug#26486: 25.1.91; spurious warnings from cconv.el

[Noam Postavsky]

retitle 26486 25.1.91; unused var warning from cconv.el can't be suppressed
severity 26486 minor
tag 26486 confirmed
quit

On Thu, Apr 13, 2017 at 2:59 PM, John Williams <jrw@pobox.com> wrote:
> When I byte compile a file where a macro expansion introduces an unused
> lexical veriable, I get warnings from cconv.el, even if I wrap the
> expansion in (with-no-warnings) and set byte-compile-warnings to nil.
> IMHO warnings of this type should never be issued for code that comes
> from a macro expansion, but it should at least be possible to suppress
> them with the usual mechanisms.  Here's a file that demonstrates the
> problem when it's compiled with byte-compile-file:
>
> ========================================================================
> ;; -*- lexical-binding: t; byte-compile-warnings: nil  -*-
>
> (defmacro test ()
>   `(let (unused)))
>
> (with-no-warnings
>   (test))
> ========================================================================

This happens also without a macro.

;; -*- lexical-binding: t; byte-compile-warnings: nil  -*-
(with-no-warnings (let (unused)))

bug#26486: 25.1.91; unused var warning from cconv.el can't be suppressed

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> This happens also without a macro.
>
> ;; -*- lexical-binding: t; byte-compile-warnings: nil  -*-
> (with-no-warnings (let (unused)))

This currently gives a warning about both an unused variable and an
empty let body.

The following seemingly straightforward patch fixes both these things,
I'd appreciate it if somebody had a look over it first, though.  :-)
(So Stefan M added to the CCs.)

diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index 70999648d4..d97bc1073b 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -325,6 +325,7 @@ byte-compile-warnings
   docstrings  docstrings that are too wide (longer than 80 characters,
               or `fill-column', whichever is bigger)
   suspicious  constructs that usually don't do what the coder wanted.
+  empty-body  the body of a binding form is empty.
 
 If the list begins with `not', then the remaining elements specify warnings to
 suppress.  For example, (not mapcar) will suppress warnings about mapcar."
diff --git a/lisp/emacs-lisp/cconv.el b/lisp/emacs-lisp/cconv.el
index f663710902..f1579cda8b 100644
--- a/lisp/emacs-lisp/cconv.el
+++ b/lisp/emacs-lisp/cconv.el
@@ -259,7 +259,8 @@ cconv--warn-unused-msg
               (not (intern-soft var))
               (eq ?_ (aref (symbol-name var) 0))
 	      ;; As a special exception, ignore "ignore".
-	      (eq var 'ignored))
+	      (eq var 'ignored)
+              (not (byte-compile-warning-enabled-p 'unbound var)))
        (let ((suggestions (help-uni-confusable-suggestions (symbol-name var))))
          (format "Unused lexical %s `%S'%s"
                  varkind var
diff --git a/lisp/emacs-lisp/macroexp.el b/lisp/emacs-lisp/macroexp.el
index df864464b7..e9c2eb3fee 100644
--- a/lisp/emacs-lisp/macroexp.el
+++ b/lisp/emacs-lisp/macroexp.el
@@ -319,14 +319,16 @@ macroexp--expand-all
       (`(,(and fun (or 'let 'let*)) . ,(or `(,bindings . ,body)
                                            pcase--dontcare))
        (macroexp--cons fun
-                       (macroexp--cons (macroexp--all-clauses bindings 1)
-                                       (if (null body)
-                                           (macroexp-unprogn
-                                            (macroexp-warn-and-return
-                                             (format "Empty %s body" fun)
-                                             nil t))
-                                         (macroexp--all-forms body))
-                                       (cdr form))
+                       (macroexp--cons
+                        (macroexp--all-clauses bindings 1)
+                        (if (null body)
+                            (macroexp-unprogn
+                             (macroexp-warn-and-return
+                              (and (byte-compile-warning-enabled-p 'empty-body)
+                                   (format "Empty %s body" fun))
+                              nil t))
+                          (macroexp--all-forms body))
+                        (cdr form))
                        form))
       (`(,(and fun `(lambda . ,_)) . ,args)
        ;; Embedded lambda in function position.


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

bug#26486: 25.1.91; unused var warning from cconv.el can't be suppressed

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen [2021-07-05 17:17:06] wrote:

> Noam Postavsky <npostavs@users.sourceforge.net> writes:
>
>> This happens also without a macro.
>>
>> ;; -*- lexical-binding: t; byte-compile-warnings: nil  -*-
>> (with-no-warnings (let (unused)))
>
> This currently gives a warning about both an unused variable and an
> empty let body.
>
> The following seemingly straightforward patch fixes both these things,
> I'd appreciate it if somebody had a look over it first, though.  :-)
> (So Stefan M added to the CCs.)
[...]
> @@ -259,7 +259,8 @@ cconv--warn-unused-msg
>                (not (intern-soft var))
>                (eq ?_ (aref (symbol-name var) 0))
>  	      ;; As a special exception, ignore "ignore".
> -	      (eq var 'ignored))
> +	      (eq var 'ignored)
> +              (not (byte-compile-warning-enabled-p 'unbound var)))
>         (let ((suggestions (help-uni-confusable-suggestions (symbol-name var))))
>           (format "Unused lexical %s `%S'%s"
>                   varkind var

LGTM, thank you.  This said, I suspect that it may fail to correctly
heed `with-suppressed-warnings`.  More specifically, I think we may want
to add a "category" argument to `macroexp-warn-and-return` which is then
matched against `byte-compile-warnings` (either directly inside
`macroexp-warn-and-return` or within the subsequent handling of
`macroexp--funcall-if-compiled`).

> diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
> index 70999648d4..d97bc1073b 100644
> --- a/lisp/emacs-lisp/bytecomp.el
> +++ b/lisp/emacs-lisp/bytecomp.el
> @@ -325,6 +325,7 @@ byte-compile-warnings
>    docstrings  docstrings that are too wide (longer than 80 characters,
>                or `fill-column', whichever is bigger)
>    suspicious  constructs that usually don't do what the coder wanted.
> +  empty-body  the body of a binding form is empty.
>  
>  If the list begins with `not', then the remaining elements specify warnings to
>  suppress.  For example, (not mapcar) will suppress warnings about mapcar."
> diff --git a/lisp/emacs-lisp/cconv.el b/lisp/emacs-lisp/cconv.el
> index f663710902..f1579cda8b 100644
> --- a/lisp/emacs-lisp/cconv.el
> +++ b/lisp/emacs-lisp/cconv.el
> diff --git a/lisp/emacs-lisp/macroexp.el b/lisp/emacs-lisp/macroexp.el
> index df864464b7..e9c2eb3fee 100644
> --- a/lisp/emacs-lisp/macroexp.el
> +++ b/lisp/emacs-lisp/macroexp.el
> @@ -319,14 +319,16 @@ macroexp--expand-all
>        (`(,(and fun (or 'let 'let*)) . ,(or `(,bindings . ,body)
>                                             pcase--dontcare))
>         (macroexp--cons fun
> -                       (macroexp--cons (macroexp--all-clauses bindings 1)
> -                                       (if (null body)
> -                                           (macroexp-unprogn
> -                                            (macroexp-warn-and-return
> -                                             (format "Empty %s body" fun)
> -                                             nil t))
> -                                         (macroexp--all-forms body))
> -                                       (cdr form))
> +                       (macroexp--cons
> +                        (macroexp--all-clauses bindings 1)
> +                        (if (null body)
> +                            (macroexp-unprogn
> +                             (macroexp-warn-and-return
> +                              (and (byte-compile-warning-enabled-p 'empty-body)
> +                                   (format "Empty %s body" fun))
> +                              nil t))
> +                          (macroexp--all-forms body))
> +                        (cdr form))
>                         form))
>        (`(,(and fun `(lambda . ,_)) . ,args)
>         ;; Embedded lambda in function position.

We already discussed the use(ful|less)ness of this empty-body warning,
and I don't think it's useful enough to justify adding a new warning
category for it.

When `byte-compile-warnings` is nil, arguably `macroexp-warn-and-return`
should just silence all warnings.


        Stefan

bug#26486: 25.1.91; unused var warning from cconv.el can't be suppressed

[Lars Ingebrigtsen]

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

> LGTM, thank you.

Thanks; applied.

> This said, I suspect that it may fail to correctly
> heed `with-suppressed-warnings`.  More specifically, I think we may want
> to add a "category" argument to `macroexp-warn-and-return` which is then
> matched against `byte-compile-warnings` (either directly inside
> `macroexp-warn-and-return` or within the subsequent handling of
> `macroexp--funcall-if-compiled`).

Right.  Could you fix that?  :-)

> We already discussed the use(ful|less)ness of this empty-body warning,
> and I don't think it's useful enough to justify adding a new warning
> category for it.
>
> When `byte-compile-warnings` is nil, arguably `macroexp-warn-and-return`
> should just silence all warnings.

Right...  but it feels kinda hackish to add the check for just
`byte-compile-warnings' there, since we don't have access to the
category at that point, so I altered the call site instead.  But perhaps
`macroexp-warn-and-return` should just be extended with a category
parameter and then the check moved into the function instead...  I dunno...

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

bug#26486: 25.1.91; unused var warning from cconv.el can't be suppressed

[Basil L. Contovounesios]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Noam Postavsky <npostavs@users.sourceforge.net> writes:
>
>> This happens also without a macro.
>>
>> ;; -*- lexical-binding: t; byte-compile-warnings: nil  -*-
>> (with-no-warnings (let (unused)))
>
> This currently gives a warning about both an unused variable and an
> empty let body.
>
> The following seemingly straightforward patch fixes both these things,
> I'd appreciate it if somebody had a look over it first, though.  :-)
> (So Stefan M added to the CCs.)

I suspect this patch causes the following:

$ cat foo.el
;;; foo.el --- foo -*- lexical-binding: t -*-
(let ())
;;; foo.el ends here
$ emacs -Q -script foo.el
Eager macro-expansion failure: (void-function byte-compile-warning-enabled-p)

Thanks,

-- 
Basil

bug#26486: 25.1.91; unused var warning from cconv.el can't be suppressed

[Lars Ingebrigtsen]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> I suspect this patch causes the following:
>
> $ cat foo.el
> ;;; foo.el --- foo -*- lexical-binding: t -*-
> (let ())
> ;;; foo.el ends here
> $ emacs -Q -script foo.el
> Eager macro-expansion failure: (void-function byte-compile-warning-enabled-p)

Yup.  The function is used elsewhere in macroexp, too, but there it's
guarded by an fboundp.

I think we should probably consider moving that function (and the
variables defined by it) somewhere else (perhaps a new file), because it
seems the requiring going on there is sort of in an odd direction.

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

bug#26486: 25.1.91; unused var warning from cconv.el can't be suppressed

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I think we should probably consider moving that function (and the
> variables defined by it) somewhere else (perhaps a new file), because it
> seems the requiring going on there is sort of in an odd direction.

(But I pushed a quick fix now.)

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