bug#35546: 27.0.50; setf return value for new alist entries is wrong

bug#35546: 27.0.50; setf return value for new alist entries is wrong

[Tassilo Horn]

The docs of setf state:

--8<---------------cut here---------------start------------->8---
setf is an autoloaded Lisp macro in ‘gv.el’.

(setf PLACE VAL PLACE VAL ...)

  Probably introduced at or before Emacs version 24.3.

Set each PLACE to the value of its VAL.
This is a generalized version of ‘setq’; the PLACEs may be symbolic
references such as (car x) or (aref x i), as well as plain symbols.
For example, (setf (cadr x) y) is equivalent to (setcar (cdr x) y).
The return value is the last VAL in the list.
--8<---------------cut here---------------end--------------->8---

According to the last sentence, I'd assume that

  (setq my-alist nil)
  (setf (alist-get 'foo my-alist) "foo-value")

would return "foo-value", but infact it returns ((foo . "foo-value")).
As soon as there is an association for 'foo, it returns the new value,
i.e., "foo-value".

So I'd say the return value is incorrect when setf adds a new entry to
an alist.



In GNU Emacs 27.0.50 (build 9, x86_64-pc-linux-gnu)
 of 2019-05-03 built on jiffyarch
Repository revision: 9ae94ebdfa80cf3983c254696b5ab998f7296aec
Repository branch: master
System Description: Arch Linux

Recent messages:
20190503T153517.050> Expiring articles...
20190503T153517.469> Expiring articles...done
20190503T153519.125> Saving Gnus registry (7833 entries) to ~/.gnus.d/.gnus.registry.eieio...
20190503T153521.663> Saving Gnus registry (size 7833) to ~/.gnus.d/.gnus.registry.eieio...done
20190503T153521.663> Saving /home/horn/.gnus.d/.newsrc.eld...

Saving file /home/horn/.gnus.d/.newsrc.eld...
Wrote /home/horn/.gnus.d/.newsrc.eld
20190503T153521.927> Saving /home/horn/.gnus.d/.newsrc.eld...done
Making completion list...

Configured using:
 'configure --without-x --without-x-toolkit'

Configured features:
SOUND GPM DBUS NOTIFY INOTIFY ACL GNUTLS LIBXML2 ZLIB XIM THREADS
LIBSYSTEMD PDUMPER GMP

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

Major mode: Group

Minor modes in effect:
  hl-line-mode: t
  gnus-topic-mode: t
  rcirc-track-minor-mode: t
  intero-global-mode: t
  beacon-mode: t
  global-aggressive-indent-mode: t
  recentf-mode: t
  which-key-mode: t
  global-company-mode: t
  global-magit-file-mode: t
  magit-auto-revert-mode: t
  global-git-commit-mode: t
  async-bytecomp-package-mode: t
  shell-dirtrack-mode: t
  override-global-mode: t
  icomplete-mode: t
  minibuffer-depth-indicate-mode: t
  electric-pair-mode: t
  global-subword-mode: t
  subword-mode: t
  save-place-mode: t
  savehist-mode: t
  show-paren-mode: t
  gnus-undo-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  global-prettify-symbols-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow emacsbug pp sort gnus-cite gnus-bcklg gnus-async gnus-ml hl-line
nndraft nnmh nnagent rot13 utf-7 nnml nnnil gnus-agent gnus-srvr
gnus-score score-mode nnvirtual gnus-cache gnus-demon nntp spam
spam-stat gnus-uu yenc gnus-msg gnus-gravatar mail-extr gravatar
url-cache gnus-topic gnus-registry registry eieio-base gnutls
network-stream rcirc-color th-private rcirc term/screen term/xterm xterm
company-oddmuse company-keywords company-etags company-gtags
company-dabbrev-code company-dabbrev company-files company-capf
company-cmake company-xcode company-clang company-semantic company-eclim
company-template company-bbdb highlight-symbol org-rmail org-mhe org-irc
org-info org-gnus nnir org-docview doc-view jka-compr image-mode image
org-bibtex bibtex org-bbdb org-w3m paredit auto-package-update
finder-inf generic fish-mode cargo cargo-process rust-mode intero
flycheck hindent haskell-mode haskell-cabal haskell-utils
haskell-font-lock haskell-indentation haskell-string
haskell-sort-imports haskell-lexeme haskell-align-imports haskell-compat
haskell-complete-module haskell-ghc-support flymake-proc flymake mwheel
etags fileloop xref project compile dabbrev haskell-customize web-mode
disp-table beacon aggressive-indent rainbow-mode vc-git vc-dir ewoc vc
vc-dispatcher epa-file org-element avl-tree generator org org-macro
org-footnote org-pcomplete org-list org-faces org-entities org-version
ob-emacs-lisp ob ob-tangle org-src ob-ref ob-lob ob-table ob-keys ob-exp
ob-comint ob-core ob-eval org-compat org-macs org-loaddefs find-func
cal-menu calendar cal-loaddefs dired-x boxquote rect smtpmail-multi
smtpmail sendmail ecomplete yasnippet auto-dictionary flyspell ispell
recentf tree-widget tramp tramp-loaddefs trampver tramp-integration
files-x tramp-compat ucs-normalize which-key highlight-parentheses
company-restclient know-your-http-well http-status-codes http-relations
http-methods http-headers company restclient forge-list forge-commands
forge-semi forge-bitbucket buck forge-gogs gogs forge-gitea gtea
forge-gitlab glab forge-github ghub-graphql treepy graphql pcase ghub
let-alist forge-notify forge-revnote forge-pullreq forge-issue
forge-topic bug-reference forge-post markdown-mode color thingatpt
noutline outline forge-repo forge forge-core forge-db closql
emacsql-sqlite emacsql emacsql-compiler magit-submodule magit-obsolete
magit-blame magit-stash magit-reflog magit-bisect magit-push magit-pull
magit-fetch magit-clone magit-remote magit-commit magit-sequence
magit-notes magit-worktree magit-tag magit-merge magit-branch
magit-reset magit-files magit-refs magit-status magit magit-repos
magit-apply magit-wip magit-log which-func imenu magit-diff smerge-mode
diff-mode magit-core magit-autorevert autorevert filenotify magit-margin
magit-transient magit-process magit-mode transient git-commit magit-git
magit-section magit-utils crm log-edit pcvs-util add-log with-editor
async-bytecomp advice async shell pcomplete comint regexp-opt ansi-color
ring server dash visual-filename-abbrev debbugs soap-client url-http
url-auth url-gw nsm url url-proxy url-privacy url-expand url-methods
url-history url-cookie url-domsuf url-util warnings rng-xsd rng-dt
rng-util xsd-regexp xml use-package-ensure use-package-bind-key bind-key
easy-mmode icomplete mb-depth rx bs windmove elec-pair cap-words
superword subword saveplace savehist paren smiley gnus-art mm-uu mml2015
mm-view mml-smime smime dig mailcap gnus-sum gnus-group gnus-undo
gnus-start gnus-cloud nnimap nnmail mail-source utf7 netrc nnoo
parse-time gnus-spec gnus-int gnus-range message rmc puny dired
dired-loaddefs format-spec rfc822 mml mml-sec epa derived epg mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
gnus-win gnus wid-edit nnheader gnus-util rmail rmail-loaddefs rfc2047
rfc2045 ietf-drums text-property-search time-date mm-util mail-prsvr
mail-utils edmacro kmacro dracula-theme cl-extra help-mode
use-package-core mule-util info tool-bar package easymenu epg-config
url-handlers url-parse auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache json subr-x map url-vars seq byte-opt gv
bytecomp byte-compile cconv cl-loaddefs cl-lib tooltip eldoc electric
uniquify ediff-hook vc-hooks lisp-float-type tabulated-list replace
newcomment text-mode elisp-mode lisp-mode prog-mode register page
menu-bar rfn-eshadow isearch timer select 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 threads dbusbind
inotify multi-tty make-network-process emacs)

Memory information:
((conses 16 1027720 149343)
 (symbols 48 45019 2)
 (strings 32 263584 14021)
 (string-bytes 1 8434902)
 (vectors 16 86395)
 (vector-slots 8 1343237 56812)
 (floats 8 599 889)
 (intervals 56 23553 1660)
 (buffers 992 50))

bug#35546: 27.0.50; setf return value for new alist entries is wrong

[Michael Heerdegen]

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

Tassilo Horn <tsdh@gnu.org> writes:

> According to the last sentence, I'd assume that
>
>   (setq my-alist nil)
>   (setf (alist-get 'foo my-alist) "foo-value")
>
> would return "foo-value", but infact it returns ((foo . "foo-value")).
> As soon as there is an association for 'foo, it returns the new value,
> i.e., "foo-value".

I agree.  I think fixing should be quite straightforward:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-alist-get-gv-setter-not-returning-VAL.patch --]
[-- Type: text/x-diff, Size: 2357 bytes --]

From 2fc149e3afa411f5b64c743de362378568f97bd6 Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen@web.de>
Date: Mon, 6 May 2019 14:58:24 +0200
Subject: [PATCH] Fix alist-get gv setter not returning VAL

This fixes Bug#35546.

* lisp/emacs-lisp/gv.el (alist-get): Make setter return the set value
to preserve 'setf' semantics.
---
 lisp/emacs-lisp/gv.el | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/lisp/emacs-lisp/gv.el b/lisp/emacs-lisp/gv.el
index 4ea3ce84fc..bdd1574820 100644
--- a/lisp/emacs-lisp/gv.el
+++ b/lisp/emacs-lisp/gv.el
@@ -392,18 +392,20 @@ setf
                                  ,(funcall setter
                                            `(cons (setq ,p (cons ,k ,v))
                                                   ,getter)))))
-                         (cond
-                          ((null remove) set-exp)
-                          ((or (eql v default)
-                               (and (eq (car-safe v) 'quote)
-                                    (eq (car-safe default) 'quote)
-                                    (eql (cadr v) (cadr default))))
-                           `(if ,p ,(funcall setter `(delq ,p ,getter))))
-                          (t
-                           `(cond
-                             ((not (eql ,default ,v)) ,set-exp)
-                             (,p ,(funcall setter
-                                           `(delq ,p ,getter)))))))))))))))
+                         `(progn
+                            ,(cond
+                             ((null remove) set-exp)
+                             ((or (eql v default)
+                                  (and (eq (car-safe v) 'quote)
+                                       (eq (car-safe default) 'quote)
+                                       (eql (cadr v) (cadr default))))
+                              `(if ,p ,(funcall setter `(delq ,p ,getter))))
+                             (t
+                              `(cond
+                                ((not (eql ,default ,v)) ,set-exp)
+                                (,p ,(funcall setter
+                                              `(delq ,p ,getter))))))
+                            ,v))))))))))


 ;;; Some occasionally handy extensions.
--
2.20.1


[-- Attachment #3: Type: text/plain, Size: 11 bytes --]



Michael.

bug#35546: 27.0.50; setf return value for new alist entries is wrong

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Tassilo Horn <tsdh@gnu.org> writes:
>
> > According to the last sentence, I'd assume that
> >
> >   (setq my-alist nil)
> >   (setf (alist-get 'foo my-alist) "foo-value")
> >
> > would return "foo-value", but infact it returns ((foo . "foo-value")).

I quickly looked if there are any more such cases in gv.el.  I found
'cons' and 'logand'.  While 'cons' also seems easy to fix, I'm
struggling with 'logand', because I don't understand what it is doing.
Not only that "it should arguably fail when trying to set a value
outside of the mask" as described in the file header, I also don't see
how the used formula makes more sense than setting the place to just V.


Michael.

bug#35546: 27.0.50; setf return value for new alist entries is wrong

[npostavs]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> struggling with 'logand', because I don't understand what it is doing.
> Not only that "it should arguably fail when trying to set a value
> outside of the mask" as described in the file header, I also don't see
> how the used formula makes more sense than setting the place to just V.

I think the rationale goes like this:

Suppose you want to *get* the bottom 4 bits of PLACE, you do

    (logand PLACE #x0F)

    ;; Example:
    (let ((var #xABCD))
      (logand var #x0F)) ;=> #xD

Suppose you want to *set* the bottom 4 bits of PLACE, you do

    (setf (logand PLACE #x0F) VALUE)

    ;; Example:
    (let ((var #xABCD))
      (setf (logand var #x0F) 9)
      var) ;=> #xABC9

bug#35546: 27.0.50; setf return value for new alist entries is wrong

[Michael Heerdegen]

npostavs@gmail.com writes:

> I think the rationale goes like this:
>
> Suppose you want to *get* the bottom 4 bits of PLACE, you do
>
>     (logand PLACE #x0F)
>
>     ;; Example:
>     (let ((var #xABCD))
>       (logand var #x0F)) ;=> #xD
>
> Suppose you want to *set* the bottom 4 bits of PLACE, you do
>
>     (setf (logand PLACE #x0F) VALUE)
>
>     ;; Example:
>     (let ((var #xABCD))
>       (setf (logand var #x0F) 9)
>       var) ;=> #xABC9

Ah, ok, thanks.  So, from all solutions it takes that one with the least
changes to the bits of PLACE.

If the setter code would be more like

  (funcall setter `(logif ,mask ,v ,getter))

it would be better readable, with the disadvantage that it would not
work any more.

Anyway, there is no reason that the setter currently does not return V,
right?


Thanks,

Michael.

bug#35546: 27.0.50; setf return value for new alist entries is wrong

[npostavs]

Michael Heerdegen <michael_heerdegen@web.de> writes:

>>     (setf (logand PLACE #x0F) V)

> Anyway, there is no reason that the setter currently does not
> return V, right?

I can't see any compelling reason to return the whole PLACE value after
update, so I guess it's just oversight.  So yeah, it should return V
just like the setters for `cl-subseq' and `nth' return the new value,
not the whole sequence.

bug#35546: 27.0.50; setf return value for new alist entries is wrong

[Štěpán Němec]

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

tags 35546 + patch
thanks

On Tue, 07 May 2019 12:50:38 -0400
npostavs@gmail.com wrote:

> Michael Heerdegen <michael_heerdegen@web.de> writes:
>
>>>     (setf (logand PLACE #x0F) V)
>
>> Anyway, there is no reason that the setter currently does not
>> return V, right?
>
> I can't see any compelling reason to return the whole PLACE value after
> update, so I guess it's just oversight.  So yeah, it should return V
> just like the setters for `cl-subseq' and `nth' return the new value,
> not the whole sequence.

I noticed `substring' had the same problem.

Patch fixing that, together with cond and logand, attached.

-- 
Štěpán


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Preserve-setf-semantics-in-substring-cons-logand-exp.patch --]
[-- Type: text/x-patch, Size: 3528 bytes --]

From 90fcc412e85716ff348c3cc6fdcc06eb08b1f6ae Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C5=A0t=C4=9Bp=C3=A1n=20N=C4=9Bmec?= <stepnem@gmail.com>
Date: Sun, 12 Apr 2020 00:27:51 +0200
Subject: [PATCH] Preserve setf semantics in 'substring', 'cons', 'logand'
 expanders

* doc/lispref/variables.texi (Adding Generalized Variables): Fix example.
* lisp/emacs-lisp/cl-lib.el (substring)
* lisp/emacs-lisp/gv.el (cons, logand): Return the value being
assigned, as specified for 'setf'.  (bug#35546)
---
 doc/lispref/variables.texi |  7 +++++--
 lisp/emacs-lisp/cl-lib.el  |  7 +++++--
 lisp/emacs-lisp/gv.el      | 18 ++++++++++++------
 3 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/doc/lispref/variables.texi b/doc/lispref/variables.texi
index abcd4bbd0f..94c8c88796 100644
--- a/doc/lispref/variables.texi
+++ b/doc/lispref/variables.texi
@@ -2585,8 +2585,11 @@ Adding Generalized Variables
       (macroexp-let2* nil ((start from) (end to))
         (funcall do `(substring ,getter ,start ,end)
                  (lambda (v)
-                   (funcall setter `(cl--set-substring
-                                     ,getter ,start ,end ,v))))))))
+                   (macroexp-let2 nil v v
+                     `(progn
+                        ,(funcall setter `(cl--set-substring
+                                           ,getter ,start ,end ,v))
+                        ,v))))))))
 @end example
 @end defmac
 
diff --git a/lisp/emacs-lisp/cl-lib.el b/lisp/emacs-lisp/cl-lib.el
index 7a26d9a90f..7a4d3c9c3e 100644
--- a/lisp/emacs-lisp/cl-lib.el
+++ b/lisp/emacs-lisp/cl-lib.el
@@ -619,8 +619,11 @@ substring
       (macroexp-let2* nil ((start from) (end to))
         (funcall do `(substring ,getter ,start ,end)
                  (lambda (v)
-                   (funcall setter `(cl--set-substring
-                                     ,getter ,start ,end ,v))))))))
+                   (macroexp-let2 nil v v
+                     `(progn
+                        ,(funcall setter `(cl--set-substring
+                                           ,getter ,start ,end ,v))
+                        ,v))))))))
 
 ;;; Miscellaneous.
 
diff --git a/lisp/emacs-lisp/gv.el b/lisp/emacs-lisp/gv.el
index 065a968877..aa1516cc5e 100644
--- a/lisp/emacs-lisp/gv.el
+++ b/lisp/emacs-lisp/gv.el
@@ -517,9 +517,12 @@ gv-delay-error
          (gv-letplace (dgetter dsetter) d
            (funcall do
                     `(cons ,agetter ,dgetter)
-                    (lambda (v) `(progn
-                              ,(funcall asetter `(car ,v))
-                              ,(funcall dsetter `(cdr ,v)))))))))
+                    (lambda (v)
+                      (macroexp-let2 nil v v
+                        `(progn
+                           ,(funcall asetter `(car ,v))
+                           ,(funcall dsetter `(cdr ,v))
+                           ,v))))))))
 
 (put 'logand 'gv-expander
      (lambda (do place &rest masks)
@@ -529,9 +532,12 @@ gv-delay-error
            (funcall
             do `(logand ,getter ,mask)
             (lambda (v)
-              (funcall setter
-                       `(logior (logand ,v ,mask)
-                                (logand ,getter (lognot ,mask))))))))))
+              (macroexp-let2 nil v v
+                `(progn
+                   ,(funcall setter
+                             `(logior (logand ,v ,mask)
+                                      (logand ,getter (lognot ,mask))))
+                   ,v))))))))
 
 ;;; References
 
-- 
2.26.0

bug#35546: 27.0.50; setf return value for new alist entries is wrong

[Noam Postavsky]

Štěpán Němec <stepnem@gmail.com> writes:

>                   (lambda (v)
> +                   (macroexp-let2 nil v v

Binding v to the value of v seems needlessly confusing.  How about
renaming the lambda parameter to valexp or somthing like that?

bug#35546: 27.0.50; setf return value for new alist entries is wrong

[Štěpán Němec]

On Sun, 12 Apr 2020 08:34:14 -0400
Noam Postavsky wrote:

> Štěpán Němec <stepnem@gmail.com> writes:
>
>>                   (lambda (v)
>> +                   (macroexp-let2 nil v v
>
> Binding v to the value of v seems needlessly confusing.  How about
> renaming the lambda parameter to valexp or somthing like that?

I kind of agree, although really, I wish that would be the most
confusing part about gv.el. :-]

I did it like that because 1. this practice seems pretty common in Emacs
sources already, 2. it makes for the minimal change here.

I am certainly not against renaming the variable, though.

Thanks,

  Štěpán

bug#35546: 27.0.50; setf return value for new alist entries is wrong

[Michael Heerdegen]

Štěpán Němec <stepnem@gmail.com> writes:

> >>                   (lambda (v)
> >> +                   (macroexp-let2 nil v v
> >
> > Binding v to the value of v seems needlessly confusing.  How about
> > renaming the lambda parameter to valexp or somthing like that?
>
> I kind of agree, although really, I wish that would be the most
> confusing part about gv.el. :-]
>
> I did it like that because 1. this practice seems pretty common in Emacs
> sources already, 2. it makes for the minimal change here.

It's a matter of taste.  I read it like that `macroexp-let2' arranges
that the expression v refers to is evaluated only once, but not anything
else (if used correctly), so keeping the name is not more confusing than
torturing the reader with one more variable to remember in that already
not so easy to read code.

Apart from that debatable point the change looks reasonable to me.


Thanks,

Michael.

bug#35546: 27.0.50; setf return value for new alist entries is wrong

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Apart from that debatable point the change looks reasonable to me.

Štěpán, it doesn't seem like you applied your patch?  

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

bug#35546: 27.0.50; setf return value for new alist entries is wrong

[Štěpán Němec]

On Wed, 19 Aug 2020 12:34:15 +0200
Lars Ingebrigtsen wrote:

> Michael Heerdegen <michael_heerdegen@web.de> writes:
>
>> Apart from that debatable point the change looks reasonable to me.
>
> Štěpán, it doesn't seem like you applied your patch?  

Indeed, and it's not the only patch I've left hanging in there.

I have now pushed this to master as

2020-04-12T00:27:51+02:00!stepnem@gmail.com
0e01d5aa72 (Preserve setf semantics in 'substring', 'cons', 'logand' expanders)
https://git.sv.gnu.org/cgit/emacs.git/commit/?id=0e01d5aa72

and will work on the other ones in the following days. 

I apologize for the delay, and thank you for the final prod.

I am also closing this bug report.

-- 
Štěpán