bug#24641: 26.0.50; let-alist: Incorrect byte-compile warning about unused lexical variable

bug#24641: 26.0.50; let-alist: Incorrect byte-compile warning about unused lexical variable

[Philipp]

$ cat /tmp/letalist.el 
;; -*- lexical-binding: t; -*-
(print
 (let-alist '((outer . ((inner . value))))
   (let-alist .outer .inner)))
   
$ emacs -Q -batch -f batch-byte-compile /tmp/letalist.el 

In toplevel form:
/tmp/letalist.el:2:1:Warning: Unused lexical variable ‘\.inner’

This warning is incorrect because '.inner' is used in the inner
let-alist form.


In GNU Emacs 26.0.50.2 (x86_64-unknown-linux-gnu, GTK+ Version 3.10.8)
 of 2016-10-07 built on localhost
Repository revision: 1686a0cde3d6adced3b5393945d6a9ab71b4a3c9
Windowing system distributor 'The X.Org Foundation', version 11.0.11702000
System Description:	Ubuntu 14.04 LTS

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Configured features:
XPM JPEG TIFF GIF PNG SOUND GSETTINGS NOTIFY GNUTLS FREETYPE XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11

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

Major mode: Lisp Interaction

Minor modes in effect:
  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

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message subr-x puny seq byte-opt gv
bytecomp byte-compile cl-extra help-mode cconv cl-loaddefs pcase cl-lib
dired dired-loaddefs format-spec rfc822 mml easymenu mml-sec
password-cache epa derived epg epg-config 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 time-date mule-util tooltip eldoc electric uniquify
ediff-hook vc-hooks lisp-float-type mwheel term/x-win 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 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 charscript 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 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 97276 8627)
 (symbols 48 20242 0)
 (miscs 40 330 118)
 (strings 32 17820 3260)
 (string-bytes 1 578593)
 (vectors 16 13702)
 (vector-slots 8 448689 6242)
 (floats 8 181 14)
 (intervals 56 190 0)
 (buffers 976 11)
 (heap 1024 46517 1112))

bug#24641: 26.0.50; let-alist: Incorrect byte-compile warning about unused lexical variable

[npostavs]

retitle 24641 let-alist redundantly binds variables already bound by nested let-alist calls
severity 24641 minor
tags 24641 confirmed
quit

Philipp <p.stephani2@gmail.com> writes:

> $ cat /tmp/letalist.el 
> ;; -*- lexical-binding: t; -*-
> (print
>  (let-alist '((outer . ((inner . value))))
>    (let-alist .outer .inner)))
>    
> $ emacs -Q -batch -f batch-byte-compile /tmp/letalist.el 
>
> In toplevel form:
> /tmp/letalist.el:2:1:Warning: Unused lexical variable ‘\.inner’
>
> This warning is incorrect because '.inner' is used in the inner
> let-alist form.

The byte compiler warning correct, the problem is in let-alist: both the
outer and inner let-alist calls let-bind '.inner'.  Here is the
macroexpansion:

(let
    ((#1=#:alist
      '((outer
         (inner . value)))))
  (let
      ((\.outer
        (cdr
         (assq 'outer #1#)))
       (\.inner ; <------------ this one is unused
        (cdr
         (assq 'inner #1#))))
    (let
        ((#2=#:alist \.outer))
      (let
          ((\.inner
            (cdr
             (assq 'inner #2#))))
        \.inner))))

bug#24641: 26.0.50; let-alist: Incorrect byte-compile warning about unused lexical variable

[Philipp Stephani]

[-- Attachment #1.1: Type: text/plain, Size: 553 bytes --]

Philipp <p.stephani2@gmail.com> schrieb am Sa., 8. Okt. 2016 um 14:48 Uhr:

>
>
> $ cat /tmp/letalist.el
>
> ;; -*- lexical-binding: t; -*-
>
> (print
>
>  (let-alist '((outer . ((inner . value))))
>
>    (let-alist .outer .inner)))
>
>
>
> $ emacs -Q -batch -f batch-byte-compile /tmp/letalist.el
>
>
>
> In toplevel form:
>
> /tmp/letalist.el:2:1:Warning: Unused lexical variable ‘\.inner’
>
>
>
> This warning is incorrect because '.inner' is used in the inner
>
> let-alist form.
>
>
>
>
I've attached a patch.

[-- Attachment #1.2: Type: text/html, Size: 1133 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-consider-nested-let-alist-forms.txt --]
[-- Type: text/plain; charset=US-ASCII;  name="0001-Don-t-consider-nested-let-alist-forms.txt", Size: 2333 bytes --]

From 1d731a6fd2d60a4645cbecd8502a04e4ed7932e3 Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Sat, 8 Oct 2016 15:29:32 +0200
Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20consider=20nested=20let-alist?=
 =?UTF-8?q?=20forms?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

See Bug#24641.

* lisp/emacs-lisp/let-alist.el (let-alist--deep-dot-search): Don’t
consider symbols in nested ‘let-alist’ forms.

* test/lisp/emacs-lisp/let-alist-tests.el
(let-alist--deep-dot-search--nested): Add a unit test.
---
 lisp/emacs-lisp/let-alist.el            | 5 +++++
 test/lisp/emacs-lisp/let-alist-tests.el | 8 ++++++++
 2 files changed, 13 insertions(+)

diff --git a/lisp/emacs-lisp/let-alist.el b/lisp/emacs-lisp/let-alist.el
index 3507a39..d706917 100644
--- a/lisp/emacs-lisp/let-alist.el
+++ b/lisp/emacs-lisp/let-alist.el
@@ -76,6 +76,11 @@ let-alist--deep-dot-search
         ;; with other results in the clause below.
         (list (cons data (intern (replace-match "" nil nil name)))))))
    ((not (consp data)) nil)
+   ((eq (car data) 'let-alist)
+    ;; For nested ‘let-alist’ forms, ignore symbols appearing in the
+    ;; inner body because they don’t refer to the alist currently
+    ;; being processed.  See Bug#24641.
+    (let-alist--deep-dot-search (cadr data)))
    (t (append (let-alist--deep-dot-search (car data))
               (let-alist--deep-dot-search (cdr data))))))
 
diff --git a/test/lisp/emacs-lisp/let-alist-tests.el b/test/lisp/emacs-lisp/let-alist-tests.el
index 80d418c..657a27a 100644
--- a/test/lisp/emacs-lisp/let-alist-tests.el
+++ b/test/lisp/emacs-lisp/let-alist-tests.el
@@ -88,4 +88,12 @@ let-alist--test-counter
                  '(cdr (assq 'baz (cdr (assq 'bar (cdr (assq 'foo var))))))))
   (should (equal (let-alist--access-sexp '..foo.bar.baz 'var) '.foo.bar.baz)))
 
+(ert-deftest let-alist--deep-dot-search--nested ()
+  "Check that nested `let-alist' forms don't generate spurious bindings.
+See Bug#24641."
+  (should (equal (let-alist--deep-dot-search '(foo .bar (baz .qux)))
+                 '((.bar . bar) (.qux . qux))))
+  (should (equal (let-alist--deep-dot-search '(foo .bar (let-alist .qux .baz)))
+                 '((.bar . bar) (.qux . qux)))))  ; no .baz
+
 ;;; let-alist.el ends here
-- 
2.10.0

bug#24641: 26.0.50; let-alist: Incorrect byte-compile warning about unused lexical variable

[npostavs]

Philipp Stephani <p.stephani2@gmail.com> writes:
>
> I've attached a patch.

Looks good to me.

bug#24641: 26.0.50; let-alist: Incorrect byte-compile warning about unused lexical variable

[Philipp Stephani]

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

<npostavs@users.sourceforge.net> schrieb am Sa., 8. Okt. 2016 um 16:34 Uhr:

> Philipp Stephani <p.stephani2@gmail.com> writes:
>
> >
>
> > I've attached a patch.
>
>
>
> Looks good to me.
>
>
Thanks, pushed to master.

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

bug#24641: 26.0.50; let-alist: Incorrect byte-compile warning about unused lexical variable

[Mark Oteiza]

Philipp Stephani <p.stephani2@gmail.com> writes:

> <npostavs@users.sourceforge.net> schrieb am Sa., 8. Okt. 2016 um 16:34 Uhr:
>
>  Philipp Stephani <p.stephani2@gmail.com> writes:
>
>  > I've attached a patch.
>
>  Looks good to me.
>
> Thanks, pushed to master. 

The package version should probably get bumped, WDYT?

bug#24641: 26.0.50; let-alist: Incorrect byte-compile warning about unused lexical variable

[Philipp Stephani]

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

Mark Oteiza <mvoteiza@udel.edu> schrieb am So., 9. Okt. 2016 um 22:51 Uhr:

> Philipp Stephani <p.stephani2@gmail.com> writes:
>
> > <npostavs@users.sourceforge.net> schrieb am Sa., 8. Okt. 2016 um 16:34
> Uhr:
> >
> >  Philipp Stephani <p.stephani2@gmail.com> writes:
> >
> >  > I've attached a patch.
> >
> >  Looks good to me.
> >
> > Thanks, pushed to master.
>
> The package version should probably get bumped, WDYT?
>

Maybe, is there a policy for this? IIUC now that let-alist is part of Emacs
core we don't care about package versions any more?

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

bug#24641: 26.0.50; let-alist: Incorrect byte-compile warning about unused lexical variable

[Mark Oteiza]

On 15/10/16 at 08:24pm, Philipp Stephani wrote:
> Mark Oteiza <mvoteiza@udel.edu> schrieb am So., 9. Okt. 2016 um 22:51 Uhr:
> > The package version should probably get bumped, WDYT?
> 
> Maybe, is there a policy for this? IIUC now that let-alist is part of Emacs
> core we don't care about package versions any more?

As long as it lives in ELPA and supports older emacs, we should care.
Since it was a bug in let-alist, I'm inclined to bump it (to 1.0.5).

(I just came across some let-alist stuff written oddly because of the
original bug, hence the necrobump)