bug#38206: 26.1; cl-prettyexpand incorrectly expands pushing to structures

bug#38206: 26.1; cl-prettyexpand incorrectly expands pushing to structures

[Braun Gábor]

Hi,

Create file test.el:

(require 'cl-lib)
(cl-defstruct test slot)
(cl-prettyexpand '(push 1 (test-slot foo)))
(message "%s" (buffer-string))


Let Emacs evaluate the file:

$ emacs -Q -batch -l ./test.el
Expanding...
Formatting...


(let* ((v test-slot)
       (v (or (cl-block test-p
                (and (memq (type-of foo) cl-struct-test-tags) t))
              (signal 'wrong-type-argument (list 'test foo))))
       (v (aref foo 1)))
  (\(setf\ cl-block\) (cons 1 (cl-block v v v)) v v v))


The form in the output is obviously an incorrect expansion of
(push 1 (test-slot foo)).
Two errors are
(1) the reference to (non-existent) free variable `test-slot' in the 
first
line and
(2) calling the nonexistent function `(setf cl-block)' in the last line.

Best wishes,

     Gábor


In GNU Emacs 26.1 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.24.4)
 of 2019-02-03, modified by Debian built on zam904
Windowing system distributor 'The X.Org Foundation', version 
11.0.12004000
System Description:	Debian GNU/Linux 10 (buster)


Configured using:
 'configure --build x86_64-linux-gnu --prefix=/usr
 --sharedstatedir=/var/lib --libexecdir=/usr/lib
 --localstatedir=/var/lib --infodir=/usr/share/info
 --mandir=/usr/share/man --enable-libsystemd --with-pop=yes
 --enable-locallisppath=/etc/emacs:/usr/local/share/emacs/26.1/site-
lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/26.1/site-lisp:/
usr/share/emacs/site-lisp
 --with-sound=alsa --without-gconf --with-mailutils --build
 x86_64-linux-gnu --prefix=/usr --sharedstatedir=/var/lib
 --libexecdir=/usr/lib --localstatedir=/var/lib
 --infodir=/usr/share/info --mandir=/usr/share/man --enable-libsystemd
 --with-pop=yes
 --enable-locallisppath=/etc/emacs:/usr/local/share/emacs/26.1/site-
lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/26.1/site-lisp:/
usr/share/emacs/site-lisp
 --with-sound=alsa --without-gconf --with-mailutils --with-x=yes
 --with-x-toolkit=gtk3 --with-toolkit-scroll-bars 'CFLAGS=-g -O2
 -fdebug-prefix-map=/build/emacs-26.1+1=. -fstack-protector-strong
 -Wformat -Werror=format-security -Wall' 'CPPFLAGS=-Wdate-time
 -D_FORTIFY_SOURCE=2' LDFLAGS=-Wl,-z,relro'

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

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

Major mode: Lisp Interaction

Minor modes in effect:
  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
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny seq byte-opt bytecomp
byte-compile cconv dired dired-loaddefs format-spec rfc822 mml 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 cl-extra help-mode easymenu cl-seq cl-macs gv cl-loaddefs
cl-lib elec-pair 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 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 dbusbind inotify lcms2 dynamic-setting system-font-setting
font-render-setting move-toolbar gtk x-toolkit x multi-tty
make-network-process emacs)

Memory information:
((conses 16 99333 7644)
 (symbols 48 20637 2)
 (miscs 40 53 104)
 (strings 32 28919 1306)
 (string-bytes 1 758567)
 (vectors 16 15133)
 (vector-slots 8 500702 9540)
 (floats 8 51 108)
 (intervals 56 308 6)
 (buffers 992 12))

bug#38206: 26.1; cl-prettyexpand incorrectly expands pushing to structures

[Lars Ingebrigtsen]

Braun Gábor <braungb88@gmail.com> writes:

> (let* ((v test-slot)
>        (v (or (cl-block test-p
>                 (and (memq (type-of foo) cl-struct-test-tags) t))
>               (signal 'wrong-type-argument (list 'test foo))))
>        (v (aref foo 1)))
>   (\(setf\ cl-block\) (cons 1 (cl-block v v v)) v v v))
>
> The form in the output is obviously an incorrect expansion of
> (push 1 (test-slot foo)).

Yup.  However, if we give a FULL parameter to the function, it gives the
correct results:

(cl-prettyexpand '(push 1 (test-slot foo)) t)
=>
(progn
  (or (and (memq (type-of foo) cl-struct-test-tags) t)
      (signal 'wrong-type-argument (list 'test foo)))
  (let* ((v foo))
    (aset v 1 (cons 1 (aref v 1)))))

The definition is

(defun cl-prettyexpand (form &optional full)
  "Expand macros in FORM and insert the pretty-printed result.
Optional argument FULL non-nil means to expand all macros,
including `cl-block' and `cl-eval-when'."
[...]
    (setq form (macroexpand-all form
                                (and (not full) '((cl-block) (cl-eval-when)))))

The bug was introduced by a rewrite in 2012, I think, which changed the
implementation radically, which made it pass in that list as a totally
bogus ENVIRONMENT to macroexpand-all.

I think the right change here is to just deprecate the FULL parameter
and remove the 

                                (and (not full) '((cl-block) (cl-eval-when)))))

bit.  Stefan?

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

bug#38206: 26.1; cl-prettyexpand incorrectly expands pushing to structures

[Stefan Monnier]

> The bug was introduced by a rewrite in 2012, I think, which changed the
> implementation radically, which made it pass in that list as a totally
> bogus ENVIRONMENT to macroexpand-all.

Not sure why you see it that way.  `C-x v h` tells me:

    commit 6fa6c4aedbc9f33cf8ed67fdb7794c3b4ff6660a
    Author: Stefan Monnier <monnier@iro.umontreal.ca>
    Date:   Thu Jun 7 15:48:22 2012 -0400
    
        Move old compatiblity to cl.el.  Remove cl-macroexpand-all.
        [...]
    
    diff --git a/lisp/emacs-lisp/cl-extra.el b/lisp/emacs-lisp/cl-extra.el
    --- a/lisp/emacs-lisp/cl-extra.el
    +++ b/lisp/emacs-lisp/cl-extra.el
    @@ -798,2 +681,2 @@
    -    (setq form (cl-macroexpand-all form
    -				   (and (not full) '((cl-block) (cl-eval-when)))))
    +    (setq form (macroexpand-all form
    +                                (and (not full) '((cl-block) (cl-eval-when)))))
    
    commit 7c1898a7b93053cd0431f46f02d82c0a31bfb8bf
    Author: Stefan Monnier <monnier@iro.umontreal.ca>
    Date:   Sun Jun 3 21:05:17 2012 -0400
    
        * lisp/emacs-lisp/cl-lib.el: Rename from cl.el.
        * lisp/emacs-lisp/cl.el: New compatibility file.
        * emacs-lisp/cl-lib.el, lisp/emacs-lisp/cl-seq.el, lisp/emacs-lisp/cl-macs.el:
        * lisp/emacs-lisp/cl-extra.el: Rename all top-level functions and variables
        to obey the "cl-" prefix.
        * lisp/emacs-lisp/macroexp.el (macroexpand-all-1): Adjust to new name.
    
    diff --git a/lisp/emacs-lisp/cl-extra.el b/lisp/emacs-lisp/cl-extra.el
    --- a/lisp/emacs-lisp/cl-extra.el
    +++ b/lisp/emacs-lisp/cl-extra.el
    @@ -802,2 +798,2 @@
         (setq form (cl-macroexpand-all form
    -				   (and (not full) '((block) (eval-when)))))
    +				   (and (not full) '((cl-block) (cl-eval-when)))))
    
    commit fcd737693e8e320acd70f91ec8e0728563244805
    Author: Richard M. Stallman <rms@gnu.org>
    Date:   Fri Jul 30 20:15:09 1993 +0000
    
        entered into RCS
    
    diff --git a/lisp/emacs-lisp/cl-extra.el b/lisp/emacs-lisp/cl-extra.el
    --- /dev/null
    +++ b/lisp/emacs-lisp/cl-extra.el
    @@ -0,0 +920,2 @@
    +    (setq form (cl-macroexpand-all form
    +				   (and (not full) '((block) (eval-when)))))

so it seems that it's been with us since at least 1993.

> I think the right change here is to just deprecate the FULL parameter
> and remove the
>
>                                 (and (not full) '((cl-block) (cl-eval-when)))))
>
> bit.  Stefan?

100% agreement.


        Stefan

bug#38206: 26.1; cl-prettyexpand incorrectly expands pushing to structures

[Lars Ingebrigtsen]

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

> Not sure why you see it that way.  `C-x v h` tells me:

[...]

>     -    (setq form (cl-macroexpand-all form
>     -				   (and (not full) '((cl-block) (cl-eval-when)))))

I just assumed that the parameter was meaningful for
cl-macroexpand-all...  that is, that the ENV parameter for that function
was something other thatn ENVIRONMENT for macroexpand-all.  But I see I
was wrong now; sorry.  I have besmirched your patch.

>> I think the right change here is to just deprecate the FULL parameter
>> and remove the
>>
>>                                 (and (not full) '((cl-block)
>> (cl-eval-when)))))
>>
>> bit.  Stefan?
>
> 100% agreement.

OK, I'll go ahead and fix that up.

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

bug#38206: 26.1; cl-prettyexpand incorrectly expands pushing to structures

[Stefan Monnier]

> I just assumed that the parameter was meaningful for
> cl-macroexpand-all...  that is, that the ENV parameter for that function
> was something other thatn ENVIRONMENT for macroexpand-all.  But I see I
> was wrong now; sorry.  I have besmirched your patch.

I just asked my lawyer to see whether it's best to sue you for libel
here in Canada or in Norway.  Either way, you'll hear from her soon.

> OK, I'll go ahead and fix that up.

Thanks,


        Stefan

bug#38206: 26.1; cl-prettyexpand incorrectly expands pushing to structures

[Lars Ingebrigtsen]

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

> I just asked my lawyer to see whether it's best to sue you for libel
> here in Canada or in Norway.  Either way, you'll hear from her soon.

Your lawyers will hear from my Kuala Lumpan lawyers!

>> OK, I'll go ahead and fix that up.
>
> Thanks,

OK; done.

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

bug#38206: 26.1; cl-prettyexpand incorrectly expands pushing to structures

[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. ]]]

  > Your lawyers will hear from my Kuala Lumpan lawyers!

Do they defend the lumpanproletariat?

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