unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#47552: 27.1; cl-defstruct field names matching read-only variables -> bad code
@ 2021-04-01 18:38 Matt Armstrong
  2021-04-04 20:17 ` Lars Ingebrigtsen
  0 siblings, 1 reply; 4+ messages in thread
From: Matt Armstrong @ 2021-04-01 18:38 UTC (permalink / raw)
  To: 47552

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))





^ permalink raw reply	[flat|nested] 4+ messages in thread

* bug#47552: 27.1; cl-defstruct field names matching read-only variables -> bad code
  2021-04-01 18:38 bug#47552: 27.1; cl-defstruct field names matching read-only variables -> bad code Matt Armstrong
@ 2021-04-04 20:17 ` Lars Ingebrigtsen
  2021-04-04 22:59   ` Stefan Monnier
  0 siblings, 1 reply; 4+ messages in thread
From: Lars Ingebrigtsen @ 2021-04-04 20:17 UTC (permalink / raw)
  To: Matt Armstrong; +Cc: monnier, 47552

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





^ permalink raw reply	[flat|nested] 4+ messages in thread

* bug#47552: 27.1; cl-defstruct field names matching read-only variables -> bad code
  2021-04-04 20:17 ` Lars Ingebrigtsen
@ 2021-04-04 22:59   ` Stefan Monnier
  2021-04-11 16:31     ` Lars Ingebrigtsen
  0 siblings, 1 reply; 4+ messages in thread
From: Stefan Monnier @ 2021-04-04 22:59 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: Matt Armstrong, 47552

>> 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






^ permalink raw reply	[flat|nested] 4+ messages in thread

* bug#47552: 27.1; cl-defstruct field names matching read-only variables -> bad code
  2021-04-04 22:59   ` Stefan Monnier
@ 2021-04-11 16:31     ` Lars Ingebrigtsen
  0 siblings, 0 replies; 4+ messages in thread
From: Lars Ingebrigtsen @ 2021-04-11 16:31 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Matt Armstrong, 47552

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





^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-04-11 16:31 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-01 18:38 bug#47552: 27.1; cl-defstruct field names matching read-only variables -> bad code Matt Armstrong
2021-04-04 20:17 ` Lars Ingebrigtsen
2021-04-04 22:59   ` Stefan Monnier
2021-04-11 16:31     ` Lars Ingebrigtsen

unofficial mirror of bug-gnu-emacs@gnu.org 

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://yhetil.org/emacs-bugs/0 emacs-bugs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 emacs-bugs emacs-bugs/ https://yhetil.org/emacs-bugs \
		bug-gnu-emacs@gnu.org
	public-inbox-index emacs-bugs

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.yhetil.org/yhetil.emacs.bugs
	nntp://news.gmane.io/gmane.emacs.bugs


code repositories for project(s) associated with this inbox:

	https://git.savannah.gnu.org/cgit/emacs.git

AGPL code for this site: git clone http://ou63pmih66umazou.onion/public-inbox.git