bug#62119: 29.0.60; Add map-insert!, eventually get rid of map-not-inplace error

bug#62119: 29.0.60; Add map-insert!, eventually get rid of map-not-inplace error

[Augusto Stoffel]

In my understanding, map.el should operate on values, like a functional
language, an not on objects (a.k.a. identities or places) like
procedural scripting languages.

This confusion is what originates the map-not-inplace error, which
should not exist at all, for the same reason that nconc, nreverse et
al. don't have this problem.

Also, this is not to say that map.el should not provide GV getters and
setters; it should.  But its functions should operate on values, even
the destructive ones.

So I suggest adding the following:

--8<---------------cut here---------------start------------->8---
(cl-defgeneric map-insert! (map key value)
  "Return a new map like MAP except that it associates KEY with VALUE.
This may destructively modify MAP to produce the new map."
  (map-insert map key value))

(cl-defmethod map-insert! ((map list) key value)
  (if (map--plist-p map)
      (if-let ((tail (memq key map)))
          (prog1 map
            (setf (cadr tail) value))
        `(,key ,value ,@map))
    (if-let ((pair (assoc key map)))
        (prog1 map
          (setf (cdr pair) value))
      `((,key . ,value) ,@map))))

(cl-defmethod map-insert! ((map hash-table) key value)
  (puthash key value map))
--8<---------------cut here---------------end--------------->8---

This gives:

  (map-insert! nil 'a 1)
  => ((a . 1))

  (map-insert! '((b . 2)) 'a 1)
  => ((a . 1) (b . 2))

While one the other hand:

  (map-put! nil 'a 1)
  => Debugger entered--Lisp error: (map-not-inplace nil)

  (map-put! '((b . 2)) 'a 1)
  => Debugger entered--Lisp error: (map-not-inplace ((b . 2)))

I would then suggest to make map-put! obsolete, due to its limitation
and conceptual confusion.  Also, the docstring could be clarified like
this:

--8<---------------cut here---------------start------------->8---
(cl-defgeneric map-put! (obj key value &optional testfn)
  "Associate KEY with VALUE in the map OBJ.
If KEY is already present in OBJ, replace the associated value
with VALUE.
This operates by modifying OBJ in place.  OBJ must be an object that
can be modified in place; otherwise signal a `map-not-inplace' error."
  ;; `testfn' only exists for backward compatibility with `map-put'!
  (declare (advertised-calling-convention (map key value) "27.1")))
--8<---------------cut here---------------end--------------->8---

As to the naming of map-insert!, we should first decide if the
exclamation mark is the convention we want for destructive operations.
One issue here is that map-delete is destructive.  In fact, there should
be a destructive and a non-destructive version of that method too.  In
theory no program should break if we changed map-delete to be
non-destructive (it currently can be so, but as usual there's no promise
of that).

As to my other recent tickets on map.el, I'm willing to provide the
patches once it's agreed what exactly should be done.

bug#62119: 29.0.60; Add map-insert!, eventually get rid of map-not-inplace error

[Michael Heerdegen]

Augusto Stoffel <arstoffel@gmail.com> writes:

> In my understanding, map.el should operate on values, like a
> functional language, an not on objects (a.k.a. identities or places)
> like procedural scripting languages.

I guess some people will disagree that we should make that a core
assumption in this library.

> So I suggest adding the following:
>
> (cl-defgeneric map-insert! (map key value)
>   "Return a new map like MAP except that it associates KEY with VALUE.
> This may destructively modify MAP to produce the new map."
>   (map-insert map key value))
>
> (cl-defmethod map-insert! ((map list) key value)
>   (if (map--plist-p map)
>       (if-let ((tail (memq key map)))
>           (prog1 map
>             (setf (cadr tail) value))
>         `(,key ,value ,@map))
>     (if-let ((pair (assoc key map)))
>         (prog1 map
>           (setf (cdr pair) value))
>       `((,key . ,value) ,@map))))
>
> (cl-defmethod map-insert! ((map hash-table) key value)
>   (puthash key value map))

But I like this idea.

> I would then suggest to make map-put! obsolete, due to its limitation
> and conceptual confusion.  Also, the docstring could be clarified like
> this:
>
> (cl-defgeneric map-put! (obj key value &optional testfn)
>   "Associate KEY with VALUE in the map OBJ.
> If KEY is already present in OBJ, replace the associated value
> with VALUE.
> This operates by modifying OBJ in place.  OBJ must be an object that
> can be modified in place; otherwise signal a `map-not-inplace' error."
>   ;; `testfn' only exists for backward compatibility with `map-put'!
>   (declare (advertised-calling-convention (map key value) "27.1")))

I don't like renaming the first argument to OBJ (you forgot to change
the calling convention btw).  Using a sensible name for the first
argument is important.  Apart from that I found the original version of
the docstring not worse or less clear than yours.

> As to the naming of map-insert!, we should first decide if the
> exclamation mark is the convention we want for destructive operations.

Personally I would rather expect that a *!-named function modifies
something in some location and the return value is not the important
part.

> One issue here is that map-delete is destructive.  In fact, there should
> be a destructive and a non-destructive version of that method too.

I like your `map-insert!', and this will also have advantages.  I'm not
sure we want to give up the old generics (like `map-put!') however.  I'm
also not sure if it would be worth it to have both at the same time,
since the cost is that we need to have separate implementations for all
generics.

I how others think about this suggestions.


Thanks,

Michael.