GNU bug report logs - #70524
[PATCH] Fix `map-elt` with `setf` for subplaces

Previous Next

To reply to this bug, email your comments to 70524 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Okamsn <okamsn <at> protonmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Fix `map-elt` with `setf` for subplaces
Date: Tue, 23 Apr 2024 02:10:42 +0000

[Message part 1 (text/plain, inline)]

Hello,

Currently, the use

     (let ((arr (vector 0 1 2 3 4 5 6)))
       (setf (map-elt (cl-subseq arr 3) 0)
             27)
       arr)

expands to

     (let ((arr (vector 0 1 2 3 4 5 6)))
       (let* ((v arr))
         (condition-case nil
             (with-no-warnings
               (map-put! (cl-subseq v 3) 0 27 nil))
           (map-not-inplace
            (let* ((new (map-insert (cl-subseq v 3) 0 27)))
              (progn
                (cl-replace v new :start1 3 :end1 nil)
                new))
            27)))
       arr)

which does not modify the original variable `arr` due to how `map-put!` 
is being used. With the attached patch, it would expand to

     (let ((arr (vector 0 1 2 3 4 5 6)))
       (let* ((v arr))
         (condition-case nil
             (with-no-warnings
               (let* ((m (cl-subseq v 3)))
                 (progn
                   (map-put! m 0 27 nil)
                   (let* ((new m))
                     (progn
                       (cl-replace v new :start1 3 :end1 nil)
                       new))
                   27)))
           (map-not-inplace
            (let* ((new (map-insert (cl-subseq v 3) 0 27)))
              (progn
                (cl-replace v new :start1 3 :end1 nil)
                new))
            27)))
       arr)

which correctly sets the value using `cl-replace` as the setter for 
`cl-subseq`.

Thank you.

[0001-Make-setf-with-map-elt-work-with-subplaces.patch (text/x-patch, attachment)]

Message #8 received at 70524 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Okamsn via "Bug reports for GNU Emacs, the Swiss army knife of text
 editors" <bug-gnu-emacs <at> gnu.org>
Cc: Okamsn <okamsn <at> protonmail.com>, 70524 <at> debbugs.gnu.org
Subject: Re: bug#70524: [PATCH] Fix `map-elt` with `setf` for subplaces
Date: Wed, 24 Apr 2024 08:06:11 +0200

Hello Okamsn,

let's CC Stefan.  Nice to see you are working on this stuff.

> Hello,
>
> Currently, the use
>
>      (let ((arr (vector 0 1 2 3 4 5 6)))
>        (setf (map-elt (cl-subseq arr 3) 0)
>              27)
>        arr)
>
> expands to [...]

But... I must admit I'm not really convinced that this has to be
changed.

First, it is not crystal clear what the semantics should be in this
case, because `cl-subseq' as a function creates a new sequence.  But
it's also a gv so it's debatable, ok.

But second - doesn't your patch lead to very inefficient code in this
example, where nearly all elements of the original sequence get replaced
by themselves in a loop (through the setter of `cl-subseq')?

Maybe there are other examples.  But cases where the first argument
given to `map-elt' returns a part of the original structure (like e.g. a
`car' call) work (and there the semantics is also clearer).

So I wonder if this change is really an improvement.

But if we install it,


---
 lisp/emacs-lisp/map.el            | 6 +++++-
 test/lisp/emacs-lisp/map-tests.el | 5 +++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/map.el b/lisp/emacs-lisp/map.el
index d3d71a36ee4..facfdd8de7b 100644
--- a/lisp/emacs-lisp/map.el
+++ b/lisp/emacs-lisp/map.el
@@ -167,7 +167,11 @@ map-elt
                        `(condition-case nil
                             ;; Silence warnings about the hidden 4th arg.
                             (with-no-warnings
-                              (map-put! ,mgetter ,key ,v ,testfn))
+                              ,(macroexp-let2 nil m mgetter
+                                 `(progn
+                                    (map-put! ,m ,key ,v ,testfn)
+                                    ,(funcall msetter m)
+                                    ,v)))
                           (map-not-inplace

I guess you should move the `with-no-warnings' wrapper along with the
comment to the inside, around the `map-put!' it is intended for.

Michael.

Message #14 received at 70524 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: okamsn <at> protonmail.com, 70524 <at> debbugs.gnu.org
Subject: Re: bug#70524: [PATCH] Fix `map-elt` with `setf` for subplaces
Date: Wed, 24 Apr 2024 16:14:51 -0400

>> Currently, the use
>>
>>      (let ((arr (vector 0 1 2 3 4 5 6)))
>>        (setf (map-elt (cl-subseq arr 3) 0)
>>              27)
>>        arr)
>>
>> expands to [...]
>
> But... I must admit I'm not really convinced that this has to be
> changed.

I'm also unconvinced.  AFAICT the same problem occurs with

    (setf (aref (cl-subseq arr 3) 0) 27)

and I can't think of a good reason why `map-elt` should behave differently.
Furthermore, the change would also fundamentally change the way
`map-elt` can be used as a gv-place, in the sense that

    (setf (map-elt (funcall foo bar) 0) 27)

would signal an error during macroexpansion because (funcall foo bar)
is not a valid gv-place.

> But second - doesn't your patch lead to very inefficient code in this
> example, where nearly all elements of the original sequence get replaced
> by themselves in a loop (through the setter of `cl-subseq')?

Yes, that's another issue.  I think if we want to support such `setf` in
a way that is good enough to that we can recommend its use, we'd need to
turn it into something that behaves more or less like:

    (map-set! arr (+ 3 0) 27)

But I must admit that I don't know how to get there.


        Stefan

Message #17 received at 70524 <at> debbugs.gnu.org (full text, mbox):

From: okamsn <at> protonmail.com
To: Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: 70524 <at> debbugs.gnu.org
Subject: Re: bug#70524: [PATCH] Fix `map-elt` with `setf` for subplaces
Date: Thu, 25 Apr 2024 01:59:36 +0000

Stefan Monnier wrote:
>>> Currently, the use
>>>
>>>       (let ((arr (vector 0 1 2 3 4 5 6)))
>>>         (setf (map-elt (cl-subseq arr 3) 0)
>>>               27)
>>>         arr)
>>>
>>> expands to [...]
>>
>> But... I must admit I'm not really convinced that this has to be
>> changed.
> 
> I'm also unconvinced.  AFAICT the same problem occurs with
> 
>      (setf (aref (cl-subseq arr 3) 0) 27)
> 
> and I can't think of a good reason why `map-elt` should behave differently.

I would have assumed that `aref` would work with subplaces. I am still 
thinking of `setf` like a more flexible version of Python allowing 
things like `my_list[0][0] = 27`.

> Furthermore, the change would also fundamentally change the way
> `map-elt` can be used as a gv-place, in the sense that
> 
>      (setf (map-elt (funcall foo bar) 0) 27)
> 
> would signal an error during macroexpansion because (funcall foo bar)
> is not a valid gv-place.


I am trying to come up with an example that triggers the second path 
using `map-insert` in the examples that I sent. In the second path, the 
current version of the `map-elt` setter will already correctly set the 
subplace using `cl-replace`. I am trying to find an example where the 
inconsistency makes a difference.

My purpose with this patch and for bug#68863 regarding `seq-subseq` 
(which does not currently support `setf`, and I think should allow 
subplaces like `substring` claims to) was for destructuring as 
`setf`-able places, like in cl-loop's `for VAR in-ref LIST`. I have 
implemented that for my Emacs Lisp package 
(https://github.com/okamsn/loopy), but not all of the `setf`-able 
destructuring constructs support sub-places in the expected way, due to 
how some of the GV expansions are defined. I did not consider calling 
`setf` on a function's output.

For example, with the two patches, one can do

     ;; => (1 2 [29 99] 29)
     (let ((arr (vector 1 2 88 99)))
       (loopy-ref (([a b &rest [&whole c &map (0 map-idx-0)]]
                    arr))
         (setf map-idx-0 29)
         (list a b c map-idx-0)))

and have the `setf`-able destructuring work as expected.

 >> But second - doesn't your patch lead to very inefficient code in this
 >> example, where nearly all elements of the original sequence get replaced
 >> by themselves in a loop (through the setter of `cl-subseq')?
 >
 > Yes, that's another issue.  I think if wvale want to support such 
`setf` in
 > a way that is good enough to that we can recommend its use, we'd need to
 > turn it into something that behaves more or less like:
 > val
 >      (map-set! arr (+ 3 0) 27)
 >
 > But I must admit that I don't know how to get there.

This is true.  I could have used `setf (map-elt (cl-subseq arr 3 4) 0) 
27)` to be more efficient.

Thank you.

Message #20 received at 70524 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Okamsn via "Bug reports for GNU Emacs, the Swiss army knife of text
 editors" <bug-gnu-emacs <at> gnu.org>
Cc: Michael Heerdegen <michael_heerdegen <at> web.de>, okamsn <at> protonmail.com,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 70524 <at> debbugs.gnu.org
Subject: Re: bug#70524: [PATCH] Fix `map-elt` with `setf` for subplaces
Date: Thu, 25 Apr 2024 14:03:47 +0200

On Tue, 23 Apr 2024 at 02:10, Okamsn via "Bug reports for GNU Emacs, the Swiss army knife of text editors" wrote:

> Hello,
>
> Currently, the use
>
>      (let ((arr (vector 0 1 2 3 4 5 6)))
>        (setf (map-elt (cl-subseq arr 3) 0)
>              27)
>        arr)
>
> expands to
>
>      (let ((arr (vector 0 1 2 3 4 5 6)))
>        (let* ((v arr))
>          (condition-case nil
>              (with-no-warnings
>                (map-put! (cl-subseq v 3) 0 27 nil))
>            (map-not-inplace
>             (let* ((new (map-insert (cl-subseq v 3) 0 27)))
>               (progn
>                 (cl-replace v new :start1 3 :end1 nil)
>                 new))
>             27)))
>        arr)

Since map-put! may raise a not-in-place signal, and I doubt the macro
expansion checks for whatever condition it is that leads to that, I
would say this use-case is essentially broken.

Message #26 received at 70524 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: 70524 <at> debbugs.gnu.org
Cc: michael_heerdegen <at> web.de, okamsn <at> protonmail.com, monnier <at> iro.umontreal.ca
Subject: Re: bug#70524: [PATCH] Fix `map-elt` with `setf` for subplaces
Date: Thu, 25 Apr 2024 14:42:19 +0200

On Thu, 25 Apr 2024 at 14:03, Augusto Stoffel wrote:

> On Tue, 23 Apr 2024 at 02:10, Okamsn via "Bug reports for GNU Emacs, the Swiss army knife of text editors" wrote:
>
>> Hello,
>>
>> Currently, the use
>>
>>      (let ((arr (vector 0 1 2 3 4 5 6)))
>>        (setf (map-elt (cl-subseq arr 3) 0)
>>              27)
>>        arr)
>>
>> expands to
>>
>>      (let ((arr (vector 0 1 2 3 4 5 6)))
>>        (let* ((v arr))
>>          (condition-case nil
>>              (with-no-warnings
>>                (map-put! (cl-subseq v 3) 0 27 nil))
>>            (map-not-inplace
>>             (let* ((new (map-insert (cl-subseq v 3) 0 27)))
>>               (progn
>>                 (cl-replace v new :start1 3 :end1 nil)
>>                 new))
>>             27)))
>>        arr)
>
> Since map-put! may raise a not-in-place signal, and I doubt the macro
> expansion checks for whatever condition it is that leads to that, I
> would say this use-case is essentially broken.

Sorry, just ignore that :-).

What I actually wanted to say is that IMO there's a general conceptual
problem to consider map-like values as places.  There's no reasonable
way (cl-subseq arr 3) can be seen as a place AFAICT.

But even in a language like Python (where map-like things can be seen as
places, since there are no liked lists to ruin the party), I don't
understand what the above code is supposed to do.  This code

  arr = [0, 1, 2, 3, 4, 5, 6]
  arr[3:][0] = 27
  arr

will just copy a portion of arr as new list, mutate its first element,
then throw array that copy.

Message #29 received at 70524 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: okamsn <at> protonmail.com
Cc: Michael Heerdegen <michael_heerdegen <at> web.de>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 70524 <at> debbugs.gnu.org
Subject: Re: bug#70524: [PATCH] Fix `map-elt` with `setf` for subplaces
Date: Thu, 25 Apr 2024 14:49:22 +0200

On Thu, 25 Apr 2024 at 01:59, okamsn <at> protonmail.com wrote:

> I would have assumed that `aref` would work with subplaces. I am still 
> thinking of `setf` like a more flexible version of Python allowing 
> things like `my_list[0][0] = 27`.

The right way to achieve this IMO would be the one indicated in
bug#62068:

  (setq my-list (map-insert-in my-list '(0 0) 27))

(I believe I even had an implementation for it, but never submitted the
patch.)

If there's a reasonable way to make a bunch of nested setf's expand to
this, I don't know.  I guess it should be possible, but I'm also not
sure it would be really convenient.

Message #32 received at 70524 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: okamsn <at> protonmail.com
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 70524 <at> debbugs.gnu.org
Subject: Re: bug#70524: [PATCH] Fix `map-elt` with `setf` for subplaces
Date: Fri, 26 Apr 2024 14:19:34 +0200

okamsn <at> protonmail.com writes:

> My purpose with this patch and for bug#68863 regarding `seq-subseq`
> (which does not currently support `setf`, and I think should allow
> subplaces like `substring` claims to)

The `substring' gv-setter doesn't need a loop however, it creates a new
string using `concat'.  Your patch would probably "work" ok in this
case, but I'm not convinced that this would be an improvement, still for
the same reasons.

> was for destructuring as `setf`-able places, like in cl-loop's `for
> VAR in-ref LIST`. I have implemented that for my Emacs Lisp package
> (https://github.com/okamsn/loopy), but not all of the `setf`-able
> destructuring constructs support sub-places in the expected way, due
> to how some of the GV expansions are defined.

But if loopy would base on an inefficient implementations this would not
be useful.

Are there examples where your patch is really a clear improvement?


Michael.

Message #35 received at 70524 <at> debbugs.gnu.org (full text, mbox):

From: okamsn <at> protonmail.com
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 70524 <at> debbugs.gnu.org
Subject: Re: bug#70524: [PATCH] Fix `map-elt` with `setf` for subplaces
Date: Mon, 29 Apr 2024 01:08:42 +0000

Michael Heerdegen wrote:
> okamsn <at> protonmail.com writes:
> 
>> My purpose with this patch and for bug#68863 regarding `seq-subseq`
>> (which does not currently support `setf`, and I think should allow
>> subplaces like `substring` claims to)
> 
> The `substring' gv-setter doesn't need a loop however, it creates a new
> string using `concat'.  Your patch would probably "work" ok in this
> case, but I'm not convinced that this would be an improvement, still for
> the same reasons.
> 
>> was for destructuring as `setf`-able places, like in cl-loop's `for
>> VAR in-ref LIST`. I have implemented that for my Emacs Lisp package
>> (https://github.com/okamsn/loopy), but not all of the `setf`-able
>> destructuring constructs support sub-places in the expected way, due
>> to how some of the GV expansions are defined.
> 
> But if loopy would base on an inefficient implementations this would not
> be useful.
> 
> Are there examples where your patch is really a clear improvement?

Hello,

I have found cases in Loopy where I am using `(setf (map-elt (map-elt 
...))` and similar. From what you and others have said, it sounds like 
this luckily happened to work but should not have been relied upon. I 
have now seen the thread bug#62068 about `map-nested-elt` mentioned by 
Augusto Stoffel, and I agree with the thoughts there that an improvement 
can be made for that use case. That could be having `map-elt` support 
sub-places in general, or it could be having `map-nested-elt` be a 
generalized variable to support the one case.

On efficiency and maybe outside of the sub-place question, I used 
`seq-map` because I thought that it would be a good way to make sure 
that the generic sequence was only moved through once. For a 
hypothetical `seq-replace`, do you think it would be better to use a 
combination `seq-concat`, `seq-take`, and `seq-subseq` and to assume 
that they are efficient implementations for the generic version? Do you 
think that it would be better if there were different implementations 
for each combination of the built-in sequence types, like the checks 
`cl-replace` has for lists and arrays?

Thank you.

> 
> Michael.

Message #38 received at 70524 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: okamsn <at> protonmail.com
Cc: Michael Heerdegen <michael_heerdegen <at> web.de>, 70524 <at> debbugs.gnu.org
Subject: Re: bug#70524: [PATCH] Fix `map-elt` with `setf` for subplaces
Date: Sun, 28 Apr 2024 21:54:32 -0400

> I have found cases in Loopy where I am using `(setf (map-elt (map-elt 
> ...))` and similar.  From what you and others have said, it sounds like 
> this luckily happened to work but should not have been relied upon.

No: `map-elt` is supposed to return a pre-existing object from the map,
so the side-effecting on it should work just fine, without needing any
luck (assuming side-effecting the map can be done, of course).

The problem is when you do (setf (map-elt (SOMETHING) ..) ..) and
(SOMETHING) is an operation which (builds and) returns a fresh new
value, such as `cl-subseq`.


        Stefan

Message #41 received at 70524 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: okamsn <at> protonmail.com
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 70524 <at> debbugs.gnu.org
Subject: Re: bug#70524: [PATCH] Fix `map-elt` with `setf` for subplaces
Date: Tue, 30 Apr 2024 18:17:19 +0200

okamsn <at> protonmail.com writes:

> On efficiency and maybe outside of the sub-place question, I used
> `seq-map` because I thought that it would be a good way to make sure
> that the generic sequence was only moved through once. For a
> hypothetical `seq-replace`, do you think it would be better to use a
> combination `seq-concat`, `seq-take`, and `seq-subseq` and to assume
> that they are efficient implementations for the generic version? Do you
> think that it would be better if there were different implementations
> for each combination of the built-in sequence types, like the checks
> `cl-replace` has for lists and arrays?

Probably yes, that would be the consequence.  Haven't thought about
whether `seq-replace' is something we want, though.

Michael.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.