bug#70524: [PATCH] Fix `map-elt` with `setf` for subplaces

bug#70524: [PATCH] Fix `map-elt` with `setf` for subplaces

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

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

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.

[-- Attachment #2: 0001-Make-setf-with-map-elt-work-with-subplaces.patch --]
[-- Type: text/x-patch, Size: 2490 bytes --]

From 1ac65858dc2f241975df8620ac79c973fcc6dad4 Mon Sep 17 00:00:00 2001
From: Earl Hyatt <okamsn@protonmail.com>
Date: Mon, 22 Apr 2024 21:45:03 -0400
Subject: [PATCH] Make setf with map-elt work with subplaces.

* lisp/emacs-lisp/map.el (map-elt): Using the setting function defined
by 'gv-letplace' to make sure that we capture the modification, even
when using 'map-put!'.  Otherwise, the sub-place might be understood as
a normal function call that will create the value to modify instead.

* test/lisp/emacs-lisp/map-tests.el (test-setf-map-with-function): Test
that sub-places work with 'map-elt' as expected.  For example, the use
of 'substring' should be understood as wanting to modify an existing
string, not wanting to modify the new string that would be created by a
call to 'substring'.
---
 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
                            ,(funcall msetter
                                      `(map-insert ,mgetter ,key ,v))
diff --git a/test/lisp/emacs-lisp/map-tests.el b/test/lisp/emacs-lisp/map-tests.el
index dc8121b0582..c79bac54cb3 100644
--- a/test/lisp/emacs-lisp/map-tests.el
+++ b/test/lisp/emacs-lisp/map-tests.el
@@ -723,6 +723,11 @@ test-setf-map-with-function
     ;; Check that the function is only called once.
     (should (= num 1))))
 
+(ert-deftest test-setf-map-with-subplace ()
+  (let ((arr (string ?0 ?1 ?2 ?3 ?4 ?5 ?6)))
+    (setf (map-elt (substring arr 3) 0) ?x)
+    (should (equal arr (string ?0 ?1 ?2 ?x ?4 ?5 ?6)))))
+
 (ert-deftest test-map-plist-member ()
   "Test `map--plist-member' and `map--plist-member-1'."
   (dolist (mem '(map--plist-member map--plist-member-1))
-- 
2.34.1

bug#70524: [PATCH] Fix `map-elt` with `setf` for subplaces

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

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.

bug#70524: [PATCH] Fix `map-elt` with `setf` for subplaces

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

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

bug#70524: [PATCH] Fix `map-elt` with `setf` for subplaces

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

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.

bug#70524: [PATCH] Fix `map-elt` with `setf` for subplaces

[Augusto Stoffel]

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.

bug#70524: [PATCH] Fix `map-elt` with `setf` for subplaces

[Augusto Stoffel]

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.

bug#70524: [PATCH] Fix `map-elt` with `setf` for subplaces

[Augusto Stoffel]

On Thu, 25 Apr 2024 at 01:59, okamsn@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.

bug#70524: [PATCH] Fix `map-elt` with `setf` for subplaces

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

okamsn@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.

bug#70524: [PATCH] Fix `map-elt` with `setf` for subplaces

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

Michael Heerdegen wrote:
> okamsn@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.

bug#70524: [PATCH] Fix `map-elt` with `setf` for subplaces

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

> 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

bug#70524: [PATCH] Fix `map-elt` with `setf` for subplaces

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

okamsn@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.