bug#47552: 27.1; cl-defstruct field names matching read-only variables -> bad code

bug#47552: 27.1; cl-defstruct field names matching read-only variables -> bad code

[Matt Armstrong]

I confirmed this in 27 and 28.

Evaluate these forms in *scratch* or M-x ielm:

    (require 'cl-macs)
    (cl-defstruct a gcs-done)
    (make-a)
    *** Eval error ***  Wrong type argument: numberp, nil

Success is expected, as occurs for structs that don't happen to have
"gcs-done" fields.

The issue is related to the generated code for `make-a', which boils
down to let binding gcs-done to nil:

    (let ((gcs-done)))

Eval the above to get the same error.

Perhaps the code generated for the make- functions should use
make-symbol or gensym instead?  Or a fixed series of field0...fieldN
symbols?  Why risk potentially binding dynamic vars?

For reference, here is how `make-a' is generated.

(defun make-a (&rest --cl-rest--)
    (let* ((gcs-done
	    (car (cdr (plist-member --cl-rest-- ':gcs-done)))))
      (progn
	(let ((--cl-keys-- --cl-rest--))
	  (while --cl-keys--
	    (cond
	     ((memq
	       (car --cl-keys--) '(:gcs-done :allow-other-keys))
	      (setq --cl-keys-- (cdr (cdr --cl-keys--))))
	     ((car (cdr (memq ':allow-other-keys --cl-rest--)))
	      (setq --cl-keys-- nil))
	     (t (error "Keyword argument %s not one of (:gcs-done)"
		       (car --cl-keys--))))))
	(record 'a gcs-done))))


In GNU Emacs 27.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.23, cairo version 1.16.0)
 of 2020-11-07, modified by Debian built on x86-ubc-01
Windowing system distributor 'The X.Org Foundation', version 11.0.12010000
System Description: Debian GNU/Linux bullseye/sid

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

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/27.1/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/27.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/27.1/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/27.1/site-lisp:/usr/share/emacs/site-lisp
 --with-sound=alsa --without-gconf --with-mailutils --with-cairo
 --with-x=yes --with-x-toolkit=gtk3 --with-toolkit-scroll-bars
 'CFLAGS=-g -O2
 -fdebug-prefix-map=/build/emacs-6jKC2B/emacs-27.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 CAIRO SOUND GPM DBUS GSETTINGS GLIB NOTIFY
INOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT LIBOTF
ZLIB TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM MODULES THREADS LIBSYSTEMD
JSON PDUMPER LCMS2 GMP

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
  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 dired dired-loaddefs
format-spec rfc822 mml easymenu mml-sec password-cache epa derived epg
epg-config gnus-util rmail rmail-loaddefs text-property-search time-date
subr-x seq byte-opt gv bytecomp byte-compile cconv mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader cl-loaddefs
cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils
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 tab-bar menu-bar 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 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 dbusbind
inotify lcms2 dynamic-setting system-font-setting font-render-setting
cairo move-toolbar gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 44948 7866)
 (symbols 48 6003 1)
 (strings 32 15436 2234)
 (string-bytes 1 500128)
 (vectors 16 10073)
 (vector-slots 8 129761 10564)
 (floats 8 19 39)
 (intervals 56 243 0)
 (buffers 1000 11))

bug#47552: 27.1; cl-defstruct field names matching read-only variables -> bad code

[Lars Ingebrigtsen]

Matt Armstrong <matt@rfc20.org> writes:

> I confirmed this in 27 and 28.
>
> Evaluate these forms in *scratch* or M-x ielm:
>
>     (require 'cl-macs)
>     (cl-defstruct a gcs-done)
>     (make-a)
>     *** Eval error ***  Wrong type argument: numberp, nil
>
> Success is expected, as occurs for structs that don't happen to have
> "gcs-done" fields.
>
> The issue is related to the generated code for `make-a', which boils
> down to let binding gcs-done to nil:
>
>     (let ((gcs-done)))
>
> Eval the above to get the same error.
>
> Perhaps the code generated for the make- functions should use
> make-symbol or gensym instead?  Or a fixed series of field0...fieldN
> symbols?  Why risk potentially binding dynamic vars?

Using a gensym seems like an obvious solution to me, but perhaps Stefan
has an opinion here (added to the CCs).

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

bug#47552: 27.1; cl-defstruct field names matching read-only variables -> bad code

[Stefan Monnier]

>> The issue is related to the generated code for `make-a', which boils
>> down to let binding gcs-done to nil:
>>
>>     (let ((gcs-done)))
>>
>> Eval the above to get the same error.
>>
>> Perhaps the code generated for the make- functions should use
>> make-symbol or gensym instead?  Or a fixed series of field0...fieldN
>> symbols?  Why risk potentially binding dynamic vars?
>
> Using a gensym seems like an obvious solution to me, but perhaps Stefan
> has an opinion here (added to the CCs).

I'm pretty sure that's the right solution, *but* I don't think it's
obvious how to get there: `cl-defstruct` defines the constructor
using `cl-defsubst` and its `&key` arguments, so the `:gcs-gone` keyword
argument inevitably maps to a `gcs-done` variable by definition of how
`&key` is supposed to work.

So I suspect that in order to fix it, we'd need to stop using `&key`, or
to use a more sophisticated version (which I think we'd have to
implement first) which lets you specify separately the keyword and the
matching variable name (and then make sure that the inlining
optimizations still work for it).


        Stefan

bug#47552: 27.1; cl-defstruct field names matching read-only variables -> bad code

[Lars Ingebrigtsen]

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

> I'm pretty sure that's the right solution, *but* I don't think it's
> obvious how to get there: `cl-defstruct` defines the constructor
> using `cl-defsubst` and its `&key` arguments, so the `:gcs-gone` keyword
> argument inevitably maps to a `gcs-done` variable by definition of how
> `&key` is supposed to work.

I'm having a hard time following the code in cl-defstruct -- even where
things are actually defined.

But...  Indeed doing this "doesn't work":

(cl-defsubst foo4 (&key gcs-done)
  gcs-done)

(foo4 :foo 1)
-> Debugger entered--Lisp error: (wrong-type-argument numberp nil)

But:

(foo4 :gcs-done 1)
=> 1

Hm...

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

bug#47552: 27.1; cl-defstruct field names matching read-only variables -> bad code

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
> > I'm pretty sure that's the right solution, *but* I don't think it's
> > obvious how to get there: `cl-defstruct` defines the constructor
> > using `cl-defsubst` and its `&key` arguments, so the `:gcs-gone` keyword
> > argument inevitably maps to a `gcs-done` variable by definition of how
> > `&key` is supposed to work.
>
> I'm having a hard time following the code in cl-defstruct -- even where
> things are actually defined.
>
> But...  Indeed doing this "doesn't work":

It still doesn't work - I think this bug had been closed by accident?
The original recipe still fails.

Here is another incarnation of this bug:

#+begin_src emacs-lisp
  (require 'cl-lib)
  (defun xyz ())

  (cl-defstruct barf "Doc" (buffer-file-name (xyz)))

  (defun barf-foo ()
    (let ((barf (make-barf)))
      barf))
#+end_src

~~>

| Debugger entered--Lisp error: (wrong-type-argument stringp (xyz))
|   (let* ((buffer-file-name (car (cdr (or (plist-member --cl-rest-- ':buffer-file-name) ...
|   make-barf--cmacro((make-barf))
|   apply(make-barf--cmacro (make-barf) nil)
|   macroexp--compiler-macro(make-barf--cmacro (make-barf))
|   #f(compiled-function (form func) #<bytecode -0x1ad7ff3c206126be>)(((make-barf)) make-barf)
|   macroexp--expand-all((make-barf))

Here the problem is that the variable `buffer-file-name' is not allowed
to be bound to something that is not a string.

Michael.

bug#47552: 27.1; cl-defstruct field names matching read-only variables -> bad code

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

> But...  Indeed doing this "doesn't work":
>
> (cl-defsubst foo4 (&key gcs-done)
>   gcs-done)
>
> (foo4 :foo 1)
> -> Debugger entered--Lisp error: (wrong-type-argument numberp nil)

I think the problem is that ELisp function arguments are defined as
being always-statically-scoped, but the macroexpansion of

    (cl-defun foo4 (&key gcs-done)
      gcs-done)

uses `let` rather than `lambda` to bind `gcs-done`, so it ends up being
dynamically-scoped.  Maybe we should introduce something like

    (defmacro slet* (bindings &rest body)
      (named-let expand ((bindings bindings))
        (pcase-exhaustive bindings
          ('() (macroexp-progn body))
          (`((,var ,exp) . ,bindings)
           (let ((rest (expand bindings)))
     	     (if (macroexp--dynamic-variable-p var)
     	         `(funcall (lambda (,var) ,rest) ,exp)
     	       (macroexp-let* `((,var ,exp)) rest)))))))

Except I see that `macroexpand-all` will incorrectly turn the
funcall+lambda into a `let`.
Some wise ass knew about it but did it anyway.  They even wrote
a comment about it:

    ;; In lexical-binding mode, let and functions don't bind vars in the same way
    ;; (let obey special-variable-p, but functions don't).  But luckily, this
    ;; doesn't matter here, because function's behavior is underspecified so it
    ;; can safely be turned into a `let', even though the reverse is not true.

so we need to either fix that `macroexp--unfold-lambda` or circumvent it
by obfuscating the code, e.g.:

    (defmacro slet* (bindings &rest body)
      (named-let expand ((bindings bindings))
        (pcase-exhaustive bindings
          ('() (macroexp-progn body))
          (`((,var ,exp) . ,bindings)
           (let ((rest (expand bindings)))
     	     (if (macroexp--dynamic-variable-p var)
     	         `(funcall (identity (lambda (,var) ,rest)) ,exp)
     	       (macroexp-let* `((,var ,exp)) rest)))))))

Another way to look at it is that maybe we should introduce an
`un-defvar`, such that we can use

    (un-defvar gcs-done)
    (cl-defun foo4 (&key gcs-done)
      gcs-done)

and have `gcs-done` bound statically.


        Stefan

bug#47552: 27.1; cl-defstruct field names matching read-only variables -> bad code

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

>     (defmacro slet* (bindings &rest body)
>       (named-let expand ((bindings bindings))
>         (pcase-exhaustive bindings
>           ('() (macroexp-progn body))
>           (`((,var ,exp) . ,bindings)
>            (let ((rest (expand bindings)))
>      	     (if (macroexp--dynamic-variable-p var)
>      	         `(funcall (identity (lambda (,var) ,rest)) ,exp)
>      	       (macroexp-let* `((,var ,exp)) rest)))))))

Not sure we want to expose that to the language, so I turned it into
a function in `cl-macs.el` for use by `cl-defun/defmacro/defsubst/...`
and pushed it to `master`.

I believe this fixes the bug.


        Stefan

bug#47552: 27.1; cl-defstruct field names matching read-only variables -> bad code

[Michael Heerdegen]

Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

> I believe this fixes the bug.

Thanks.  Didn't read the fix yet, but

|  load-with-code-conversion("/home/micha/software/emacs/lisp/emacs-lisp/cl-preloaded.el" "/home/micha/software/emacs/lisp/emacs-lisp/cl-preloaded.el" nil nil)
|   load("emacs-lisp/cl-preloaded")
|   load("loadup.el")
| Eager macro-expansion failure: (void-function seq-some)
| make[2]: *** [Makefile:916: bootstrap-emacs.pdmp] Error 255
| make[2]: Leaving directory '/home/micha/software/emacs/src'
| make[1]: *** [Makefile:544: src] Error 2
| make[1]: Leaving directory '/home/micha/software/emacs'
| make[1]: Entering directory '/home/micha/software/emacs'

is printed when building from scratch.

TIA,

Michael.

bug#47552: 27.1; cl-defstruct field names matching read-only variables -> bad code

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

> |  load-with-code-conversion("/home/micha/software/emacs/lisp/emacs-lisp/cl-preloaded.el" "/home/micha/software/emacs/lisp/emacs-lisp/cl-preloaded.el" nil nil)
> |   load("emacs-lisp/cl-preloaded")
> |   load("loadup.el")
> | Eager macro-expansion failure: (void-function seq-some)
> | make[2]: *** [Makefile:916: bootstrap-emacs.pdmp] Error 255
> | make[2]: Leaving directory '/home/micha/software/emacs/src'
> | make[1]: *** [Makefile:544: src] Error 2
> | make[1]: Leaving directory '/home/micha/software/emacs'
> | make[1]: Entering directory '/home/micha/software/emacs'
>
> is printed when building from scratch.

Damn!  OK, should work again now.  Sorry 'bout that.


        Stefan

bug#47552: 27.1; cl-defstruct field names matching read-only variables -> bad code

[Michael Heerdegen]

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

> Damn!  OK, should work again now.  Sorry 'bout that.

Works, thanks.

One (very small) downside of the code generated now is that it may
trigger "Lexical argument shadows the dynamic variable" warnings.
'date' for example is bad as a slot name when "diary" is loaded.

I think these warnings can safely be ignored but I don't know if there
is a way to get rid of them (and there may be a lot since the
`defstruct' call is not the only place where a warning is emitted: also
some defined functions lead to those warnings).

Michael.

bug#47552: 27.1; cl-defstruct field names matching read-only variables -> bad code

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

> One (very small) downside of the code generated now is that it may
> trigger "Lexical argument shadows the dynamic variable" warnings.

Yup.  The current code doesn't offer a way to silence them with
`with-suppressed-warnings`, AFAICT :-(

> 'date' for example is bad as a slot name when "diary" is loaded.

AFAIK, `date` is not globally defined as dynbound by diary, so it should
not be a problem.  More specifically, the `date` variable is treated as
dynbound only inside calendar's own files, but not in code found in
other files.
If you found it to be otherwise, please report it as a bug.


        Stefan

bug#47552: 27.1; cl-defstruct field names matching read-only variables -> bad code

[Michael Heerdegen]

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

> AFAIK, `date` is not globally defined as dynbound by diary, so it should
> not be a problem.  More specifically, the `date` variable is treated as
> dynbound only inside calendar's own files, but not in code found in
> other files.

Hmm, indeed.

> If you found it to be otherwise, please report it as a bug.

It turned out I had created an alias for the variable (using
`defvaralias') long ago, more or less only to get rid of warnings at
that moment, AFAIR.  Seems that this makes the variable special
globally.

Michael.