bug#23105: 25.0.92; map-put does not change its argument MAP

bug#23105: 25.0.92; map-put does not change its argument MAP

[Sho Takemori]

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

I'm not sure if this is a bug, but the function map-put does not change its
argument MAP if the map is an alist and the key is not found.

For example, the following code returns "value" as expected:

(require 'map)
(let ((m (make-hash-table)))
  (map-put m 'key "value")
  (map-elt m 'key))

If I replace m by an alist, then the following code returns nil.
I expected it returned "value" as above.

(let ((m nil))
  (map-put m 'key "value")
  (map-elt m 'key))

By expanding map-put, I found that it returns a new list if "m" is a list
and
the key is not in "m".

Best regards,
Sho Takemori

In GNU Emacs 25.0.92.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.16.7)
 of 2016-03-24 built on K430-Ubuntu
Repository revision: c0165ea4d6ecf81db6728782c7322c311ee0a783
Windowing system distributor 'The X.Org Foundation', version 11.0.11702000
System Description: Ubuntu 15.10

Configured using:
 'configure --with-sound=no --with-file-notification=yes'

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

Important settings:
  value of $LC_MONETARY: ja_JP.UTF-8
  value of $LC_NUMERIC: ja_JP.UTF-8
  value of $LC_TIME: ja_JP.UTF-8
  value of $LANG: ja_JP.UTF-8
  value of $XMODIFIERS: @im=fcitx
  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

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
Mark set
map
"value"
Mark set
nil [2 times]
scroll-up-command: End of buffer

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 help-fns mail-prsvr mail-utils map
seq byte-opt gv bytecomp byte-compile cconv cl-extra help-mode easymenu
cl-loaddefs pcase cl-lib time-date mule-util japan-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 95859 6137)
 (symbols 48 20522 0)
 (miscs 40 327 121)
 (strings 32 16995 4550)
 (string-bytes 1 505060)
 (vectors 16 13958)
 (vector-slots 8 527873 8024)
 (floats 8 182 21)
 (intervals 56 220 12)
 (buffers 976 11)
 (heap 1024 50485 944))

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

bug#23105: 25.0.92; map-put does not change its argument MAP

[Michael Heerdegen]

Sho Takemori <stakemorii@gmail.com> writes:

> By expanding map-put, I found that it returns a new list if "m" is a
> list and the key is not in "m".

Indeed, good catch.

The implementation of `map-put' is wrong: it binds the evaluated place
expression to a new symbol and uses that symbol as place to operate on.

The intention presumably was to avoid code duplication in the macro
expansion - which is good - but it fails to change the original place.


Michael.

bug#23105: 25.0.92; map-put does not change its argument MAP

[Nicolas Petton]

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

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Sho Takemori <stakemorii@gmail.com> writes:
>
>> By expanding map-put, I found that it returns a new list if "m" is a
>> list and the key is not in "m".
>
> Indeed, good catch.
>
> The implementation of `map-put' is wrong: it binds the evaluated place
> expression to a new symbol and uses that symbol as place to operate on.
>
> The intention presumably was to avoid code duplication in the macro
> expansion - which is good - but it fails to change the original place.

Indeed, it is wrong.  I will have a look at the issue.

Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

bug#23105: 25.0.92; map-put does not change its argument MAP

[Nicolas Petton]

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

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Sho Takemori <stakemorii@gmail.com> writes:
>
>> By expanding map-put, I found that it returns a new list if "m" is a
>> list and the key is not in "m".
>
> Indeed, good catch.
>
> The implementation of `map-put' is wrong: it binds the evaluated place
> expression to a new symbol and uses that symbol as place to operate on.
>
> The intention presumably was to avoid code duplication in the macro
> expansion - which is good - but it fails to change the original place.

Now that I have a second look at it, I don't see why `map-put' (and
`map-delete' for that matter) are macros.  I'll see if I can replace
them with functions and have all the tests green.

Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

bug#23105: 25.0.92; map-put does not change its argument MAP

[Nicolas Petton]

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

Nicolas Petton <nicolas@petton.fr> writes:

> Now that I have a second look at it, I don't see why `map-put' (and
> `map-delete' for that matter) are macros.

Forget about that, I overlooked the issue.

Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

bug#23105: 25.0.92; map-put does not change its argument MAP

[Sho Takemori]

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

Thanks for the fix. But after the fix, if MAP is a list literal, then it
raises an error.
For example, this raises an error.
(map-put '((a . 1)) 'a "value")

This works.
(map-put [0 1 2] 1 "value")

It is an expected error?

Sho Takemori


2016-03-25 23:12 GMT+09:00 Nicolas Petton <nicolas@petton.fr>:

> Sho Takemori <stakemorii@gmail.com> writes:
>
> > I'm not sure if this is a bug, but the function map-put does not change
> its
> > argument MAP if the map is an alist and the key is not found.
>
> I pushed a fix to emacs-25.
>
> Nico
>

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

bug#23105: 25.0.92; map-put does not change its argument MAP

[Nicolas Petton]

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

Sho Takemori <stakemorii@gmail.com> writes:

> Thanks for the fix. But after the fix, if MAP is a list literal, then it
> raises an error.
> For example, this raises an error.
> (map-put '((a . 1)) 'a "value")
>
> This works.
> (map-put [0 1 2] 1 "value")
>
> It is an expected error?

Yes, in the sense that the same error will be raised if you try to do it
with a setf on an alist:

(setf (alist-get 'b '((a . 1))) 3) ;; => invalid-function (a . 1)

(let ((alist '((a . 1))))
  (setf (alist-get 'b alist) 3)) ;; => works fine

Note however that mutating literal lists is in general not a good idea,
as conses might be shared.

Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

bug#23105: 25.0.92; map-put does not change its argument MAP

[Sho Takemori]

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

OK. Thanks for the reply.


2016-03-26 0:14 GMT+09:00 Nicolas Petton <nicolas@petton.fr>:

> Sho Takemori <stakemorii@gmail.com> writes:
>
> > Thanks for the fix. But after the fix, if MAP is a list literal, then it
> > raises an error.
> > For example, this raises an error.
> > (map-put '((a . 1)) 'a "value")
> >
> > This works.
> > (map-put [0 1 2] 1 "value")
> >
> > It is an expected error?
>
> Yes, in the sense that the same error will be raised if you try to do it
> with a setf on an alist:
>
> (setf (alist-get 'b '((a . 1))) 3) ;; => invalid-function (a . 1)
>
> (let ((alist '((a . 1))))
>   (setf (alist-get 'b alist) 3)) ;; => works fine
>
> Note however that mutating literal lists is in general not a good idea,
> as conses might be shared.
>
> Nico
>

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

bug#23105: 25.0.92; map-put does not change its argument MAP

[Michael Heerdegen]

Nicolas Petton <nicolas@petton.fr> writes:

> > It is an expected error?
>
> Yes, in the sense that the same error will be raised if you try to do it
> with a setf on an alist:

> [...]

> Note however that mutating literal lists is in general not a good idea,
> as conses might be shared.

Calling `map-put' in this way is also not useful.


Michael.

bug#23105: 25.0.92; map-put does not change its argument MAP

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Calling `map-put' in this way is also not useful.

Maybe you could add to the docstring that `map-put' accepts a place
expression (or generalized variable).


Thanks,

Michael

bug#23105: 25.0.92; map-put does not change its argument MAP

[Nicolas Petton]

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

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Michael Heerdegen <michael_heerdegen@web.de> writes:
>
>> Calling `map-put' in this way is also not useful.
>
> Maybe you could add to the docstring that `map-put' accepts a place
> expression (or generalized variable).

Yes, I guess that'd be useful.

Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 512 bytes --]