bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[dalanicolai]

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

Message-ID: <87czvonp73.fsf@dalanicolai-at-gmail.com>
--text follows this line--

The docstring of the map-elt function from the map.el package (version
3.0) mentions that TESTFN is deprecated because "its default depends on
the MAP argument". However when I try e.g.

(map-elt '(("A1" . 3)) "A1")

it returns nil.

When I add the correct TESTFN

(map-elt '(("A1" . 3)) "A1" nil 'string=)

it does correctly return 3.

So it seems to me that TESTFN is not yet deprecated, or that otherwise I
am understanding it incorrectly. In that case I would label this as a
documentation bug.


In GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.25,
cairo version 1.16.0)
 of 2021-02-18 built on daniel-fedora
Repository revision: 185121da6978553d538d37d6d0e67dc52e13311f
Repository branch: feature/native-comp
Windowing system distributor 'The X.Org Foundation', version 11.0.12010000
System Description: Fedora 34 (Workstation Edition Prerelease)

Configured using:
 'configure --with-nativecomp'

Configured features:
CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG JSON
LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NATIVE_COMP
NOTIFY INOTIFY PDUMPER PNG RSVG SOUND THREADS TIFF TOOLKIT_SCROLL_BARS
X11 XDBE XIM XPM GTK3 ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=none
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  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

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
rfc822 mml mml-sec epa derived epg epg-config gnus-util rmail
rmail-loaddefs auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache json text-property-search time-date subr-x
mm-decode mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils
mailheader sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr
mail-utils map seq byte-opt gv bytecomp byte-compile cconv help-fns
radix-tree cl-print debug backtrace help-mode easymenu find-func
cl-loaddefs cl-lib iso-transl tooltip eldoc electric uniquify ediff-hook
vc-hooks lisp-float-type mwheel term/x-win x-win term/common-win x-dnd
tool-bar dnd fontset image regexp-opt fringe tabulated-list replace
newcomment text-mode elisp-mode lisp-mode prog-mode register page
tab-bar menu-bar rfn-eshadow isearch timer select scroll-bar mouse
jit-lock font-lock syntax facemenu font-core term/tty-colors frame
minibuffer 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 cl-preloaded nadvice button loaddefs faces
cus-face pcase macroexp files window text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote threads dbusbind inotify lcms2 dynamic-setting
system-font-setting font-render-setting cairo move-toolbar gtk x-toolkit
x multi-tty make-network-process nativecomp emacs)

Memory information:
((conses 16 74272 8338)
 (symbols 48 7113 0)
 (strings 32 20890 1678)
 (string-bytes 1 713441)
 (vectors 16 13521)
 (vector-slots 8 292167 12582)
 (floats 8 25 33)
 (intervals 56 238 0)
 (buffers 992 13))

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

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Basil L. Contovounesios]

dalanicolai <dalanicolai@gmail.com> writes:

> The docstring of the map-elt function from the map.el package (version
> 3.0) mentions that TESTFN is deprecated because "its default depends on
> the MAP argument". However when I try e.g.
>
> (map-elt '(("A1" . 3)) "A1")
>
> it returns nil.

This is expected, as alist keys are tested with eq by default.

That's what the docstring is trying to warn about: alists default to
testing with eq, but can also use eql, equal, or anything else.

Hash tables, OTOH, are limited to the test function that they were
created with.

So TESTFN doesn't always work as expected depending on the map type.

> When I add the correct TESTFN
>
> (map-elt '(("A1" . 3)) "A1" nil 'string=)
>
> it does correctly return 3.
>
> So it seems to me that TESTFN is not yet deprecated, or that otherwise I
> am understanding it incorrectly.

Deprecation means "this is not recommended" and "support for this may be
removed in a future version".

So to me TESTFN seems to be working as intended.

> In that case I would label this as a documentation bug.

What would you like to see clarified in the documentation?

Thanks,

-- 
Basil

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Michael Heerdegen]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> dalanicolai <dalanicolai@gmail.com> writes:
>
> > The docstring of the map-elt function from the map.el package (version
> > 3.0) mentions that TESTFN is deprecated because "its default depends on
> > the MAP argument". However when I try e.g.
> >
> > (map-elt '(("A1" . 3)) "A1")
> >
> > it returns nil.
>
> This is expected, as alist keys are tested with eq by default.
>
> That's what the docstring is trying to warn about: alists default to
> testing with eq, but can also use eql, equal, or anything else.

Is it that obvious?  We have `assoc' and `assq' built-in - to me it's
not obvious that "alist keys are tested with eq by default".  It's the
default for `alist-get', ok, which is used by the implementation, but
not everybody will know that.  I would add a sentence about that.

Michael.

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[dalanicolai]

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

For clarity I will insert the first lines of the docstrings here:

map-elt is a Lisp closure in ‘map.el’.
>
> (map-elt MAP KEY &optional DEFAULT)
>
>   Probably introduced at or before Emacs version 26.1.
>
> Lookup KEY in MAP and return its associated value.
> If KEY is not found, return DEFAULT which defaults to nil.
>
> TESTFN is deprecated.  Its default depends on the MAP argument.
>
> In the base definition, MAP can be an alist, plist, hash-table,
> or array.
>

First I agree with Michael that this docstring assumes a lot of knowledge
from the programmer.
The sentence "its default depends on the MAP argument" could be more
explicit, to make the docstring friendly to new elisp programmers.
e.g. mention eq for alists (and also the TESTFN's for the other ones)

Second calling the TESTFN deprecated is misleading if it a basic
requirement for the basic thing I am trying to achieve (i.e. matching a
string).
So it should probably mention that it is not required if you want to use
the MAP's default TESTFN, but otherwise it is required (while deprecated
sounds to me like there shouldn't be a need to use it)

Thanks for your answer!




On Thu, 25 Mar 2021 at 03:39, Michael Heerdegen <michael_heerdegen@web.de>
wrote:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
> > dalanicolai <dalanicolai@gmail.com> writes:
> >
> > > The docstring of the map-elt function from the map.el package (version
> > > 3.0) mentions that TESTFN is deprecated because "its default depends on
> > > the MAP argument". However when I try e.g.
> > >
> > > (map-elt '(("A1" . 3)) "A1")
> > >
> > > it returns nil.
> >
> > This is expected, as alist keys are tested with eq by default.
> >
> > That's what the docstring is trying to warn about: alists default to
> > testing with eq, but can also use eql, equal, or anything else.
>
> Is it that obvious?  We have `assoc' and `assq' built-in - to me it's
> not obvious that "alist keys are tested with eq by default".  It's the
> default for `alist-get', ok, which is used by the implementation, but
> not everybody will know that.  I would add a sentence about that.
>
> Michael.
>

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

bug#47368: [External] : bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Drew Adams]

> > This is expected, as alist keys are tested with eq by default.

Since when?  Where?  Expected by whom, and by what code?

> > That's what the docstring is trying to warn about: alists default to
> > testing with eq, but can also use eql, equal, or anything else.
> 
> Is it that obvious?  We have `assoc' and `assq' built-in - to me it's
> not obvious that "alist keys are tested with eq by default".  It's the
> default for `alist-get', ok, which is used by the implementation, but
> not everybody will know that.  I would add a sentence about that.

+1.

Alists are general.  They can be used in many ways.
Their keys can be tested in multiple ways.  Neither
code nor doc should assume anything about how an
alist is composed or treated - nothing beyond the
fact that at least some of the list elements are
likely to be conses.

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Michael Heerdegen]

Hi Stefan,

we are discussing here the limitation for `map-elt' calls with alists
caused by deprecating the TESTFN argument (done by you a while ago).

What's a good way to solve this?  Obviously the map abstraction doesn't
fit so super well for alists because unlike the other map type alists
don't know "their" test function.  But disallowing alists that don't
test with `eq' seems an unnecessary restriction.  Can we say that the
argument is allowed only for alists?

Regards,

Michael.


I <michael_heerdegen@web.de> wrote:

> > > The docstring of the map-elt function from the map.el package (version
> > > 3.0) mentions that TESTFN is deprecated because "its default depends on
> > > the MAP argument". However when I try e.g.
> > >
> > > (map-elt '(("A1" . 3)) "A1")
> > >
> > > it returns nil.
> >
> > This is expected, as alist keys are tested with eq by default.
> >
> > That's what the docstring is trying to warn about: alists default to
> > testing with eq, but can also use eql, equal, or anything else.
>
> Is it that obvious?  We have `assoc' and `assq' built-in - to me it's
> not obvious that "alist keys are tested with eq by default".  It's the
> default for `alist-get', ok, which is used by the implementation, but
> not everybody will know that.  I would add a sentence about that.

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[dalanicolai]

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

You probably already noticed it, but I only notice just now that the TESTFN
option also has been removed from the calling convention with
`(advertised-calling-convention (map key &optional default) "27.1")`. (Just
to add to my previous answer)

On Fri, 26 Mar 2021 at 04:59, Michael Heerdegen <michael_heerdegen@web.de>
wrote:

> Hi Stefan,
>
> we are discussing here the limitation for `map-elt' calls with alists
> caused by deprecating the TESTFN argument (done by you a while ago).
>
> What's a good way to solve this?  Obviously the map abstraction doesn't
> fit so super well for alists because unlike the other map type alists
> don't know "their" test function.  But disallowing alists that don't
> test with `eq' seems an unnecessary restriction.  Can we say that the
> argument is allowed only for alists?
>
> Regards,
>
> Michael.
>
>
> I <michael_heerdegen@web.de> wrote:
>
> > > > The docstring of the map-elt function from the map.el package
> (version
> > > > 3.0) mentions that TESTFN is deprecated because "its default depends
> on
> > > > the MAP argument". However when I try e.g.
> > > >
> > > > (map-elt '(("A1" . 3)) "A1")
> > > >
> > > > it returns nil.
> > >
> > > This is expected, as alist keys are tested with eq by default.
> > >
> > > That's what the docstring is trying to warn about: alists default to
> > > testing with eq, but can also use eql, equal, or anything else.
> >
> > Is it that obvious?  We have `assoc' and `assq' built-in - to me it's
> > not obvious that "alist keys are tested with eq by default".  It's the
> > default for `alist-get', ok, which is used by the implementation, but
> > not everybody will know that.  I would add a sentence about that.
>
>

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

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Stefan Monnier]

> What's a good way to solve this?  Obviously the map abstraction doesn't
> fit so super well for alists because unlike the other map type alists
> don't know "their" test function.  But disallowing alists that don't
> test with `eq' seems an unnecessary restriction.  Can we say that the
> argument is allowed only for alists?

How 'bout always using `equal`?


        Stefan

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[dalanicolai]

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

Well, as a hobbyist programmer, this makes more sense to me anyway. If
users don't want the behavior of using `equal` then they could just as well
use `alist-get`.

I hope you don't mind if I write down some thoughts (and macro's) I have on
my mind.
Personally, I think it would be great for growing the community, if
emacs-lisp got more approachable (in the direction of python).
I actually got here because, as an exercise, I was trying to implement Norvig's
sudoku solver <https://norvig.com/sudoku.html> in emacs-lisp, which was a
quite frustrating exercise (this is no complaint, but just a fact).
I think it would be great if emacs-lisp could look, and become readable and
usable, more like that (in which map.el and seq.el are doing a very nice
job of course). So that emacs-lisp could actually
become a nice and friendly teaching language, which is equally fun to
script in as in python.

Actually, I got the feeling that it would be nice to have
list-comprehension like syntax also. Therefore, I tried to create some
general `array` and `table` macro's here
<https://github.com/dalanicolai/list-factory/blob/main/list-factory.el>
(which is my first macro exercise ever).
As it is just an exercise it is undocumented, but you can very easily get
the idea from looking at the (commented out) tests at the bottom of that
file. Also, from looking at map.el, I assume I should probably
implement it using cl-defgeneric.

Haha... sorry for the elaborate answer.  You can neglect most of this
message, but maybe someone is interested and shares some of these ideas (or
likes the idea of these macro's). I just couldn't resist to share these
thoughts...

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

bug#47368: [External] : bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Basil L. Contovounesios]

[ BTW, your MUA seems to have mangled the email subject, and I received
  the mail twice.]

Drew Adams <drew.adams@oracle.com> writes:

>> > This is expected, as alist keys are tested with eq by default.
>
> Since when?

Since the introduction of map.el in Emacs 25.

> Where?  Expected by whom, and by what code?

By the function being discussed.

-- 
Basil

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Basil L. Contovounesios]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> What's a good way to solve this?  Obviously the map abstraction doesn't
>> fit so super well for alists because unlike the other map type alists
>> don't know "their" test function.  But disallowing alists that don't
>> test with `eq' seems an unnecessary restriction.  Can we say that the
>> argument is allowed only for alists?
>
> How 'bout always using `equal`?

No objections here.  CCing Nicolas in case he has any comments.

Just some code archaeology for more context:

- Pre-Emacs-25 map-elt used assoc.
- Emacs 25 map-elt used alist-get without TESTFN.
- Emacs 26 map-elt gained TESTFN at the same time that alist-get did,
  in https://bugs.gnu.org/27584.
- Emacs 27 deprecated map-elt's TESTFN.

-- 
Basil

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Basil L. Contovounesios]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>> dalanicolai <dalanicolai@gmail.com> writes:
>>
>> > The docstring of the map-elt function from the map.el package (version
>> > 3.0) mentions that TESTFN is deprecated because "its default depends on
>> > the MAP argument". However when I try e.g.
>> >
>> > (map-elt '(("A1" . 3)) "A1")
>> >
>> > it returns nil.
>>
>> This is expected, as alist keys are tested with eq by default.
>>
>> That's what the docstring is trying to warn about: alists default to
>> testing with eq, but can also use eql, equal, or anything else.
>
> Is it that obvious?  We have `assoc' and `assq' built-in - to me it's
> not obvious that "alist keys are tested with eq by default".  It's the
> default for `alist-get', ok, which is used by the implementation, but
> not everybody will know that.  I would add a sentence about that.

There used to be such a sentence until the argument was deprecated.
I agree that whatever we decide on should be made clear.

-- 
Basil

bug#47368: [External] : bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Drew Adams]

> >> > This is expected, as alist keys are tested with eq by default.
> >
> > Since when?
> 
> Since the introduction of map.el in Emacs 25.

The general statement you made, that alist keys are treated with eq by default, is false, AFAIK.  That may be true of `map-elt', but it's not true in general (AFAIK).

> > Where?  Expected by whom, and by what code?
> 
> By the function being discussed.

Does the doc string of the function being discussed, `map-elt' say this?  Does it say anything at all about how keys are compared?

I have only Emacs 27.1 - the latest release available on MS Windows, and there the doc string says NADA about how keys are compared.  Nothing about what it means for "if KEY is not found".

At the very least, if such is still the case then the doc needs to updated to specify how keys are compared, IMHO.

I'm hoping that doc more recent than Emacs 27.1 already takes care of this.  You say, for example:

   That's what the docstring is trying to warn about:
   alists default to testing with eq, but can also use
   eql, equal, or anything else.

I don't see (in 27.1) where the doc string warns about
any such thing.  Nothing about eq being the default,
and nothing about testing being also possible with the
others you mention.

Not only that, but the doc string says that TESTFN
is deprecated, but there's no other mention of TESTFN.

What's TESTFN?  Where is it specified?  It's not part
of the function signature that's shown.  How can you
refer to it if there's no indication anywhere here of
what it is?  This makes no sense to me.

And why is there this line at the end of the doc string?

  Undocumented

What on earth is that supposed to mean to a reader of
the `map-elt' doc?

bug#47368: [External] : bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Basil L. Contovounesios]

Drew Adams <drew.adams@oracle.com> writes:

>> >> > This is expected, as alist keys are tested with eq by default.
>> >
>> > Since when?
>> 
>> Since the introduction of map.el in Emacs 25.
>
> The general statement you made, that alist keys are treated with eq by
> default, is false, AFAIK.  That may be true of `map-elt', but it's not
> true in general (AFAIK).

This bug report is about map-elt, not alists in general.

>> > Where?  Expected by whom, and by what code?
>> 
>> By the function being discussed.
>
> Does the doc string of the function being discussed, `map-elt' say
> this?  Does it say anything at all about how keys are compared?

It used to, before the TESTFN argument was deprecated.  But the whole
point of this discussion is that one size doesn't fit all, since map-elt
is a generic function that can be adapted to heterogeneous types and
semantics, both within Emacs core and external packages.

> I have only Emacs 27.1 - the latest release available on MS Windows,
> and there the doc string says NADA about how keys are compared.
> Nothing about what it means for "if KEY is not found".
>
> At the very least, if such is still the case then the doc needs to
> updated to specify how keys are compared, IMHO.
>
> I'm hoping that doc more recent than Emacs 27.1 already takes care of
> this.  You say, for example:
>
>    That's what the docstring is trying to warn about:
>    alists default to testing with eq, but can also use
>    eql, equal, or anything else.
>
> I don't see (in 27.1) where the doc string warns about
> any such thing.

"TESTFN is deprecated.  Its default depends on the MAP argument."

> Nothing about eq being the default, and nothing about testing being
> also possible with the others you mention.
>
> Not only that, but the doc string says that TESTFN
> is deprecated, but there's no other mention of TESTFN.
>
> What's TESTFN?  Where is it specified?  It's not part
> of the function signature that's shown.  How can you
> refer to it if there's no indication anywhere here of
> what it is?  This makes no sense to me.

All of these points are already being discussed in this thread.

Patches with improvements are always welcome.

> And why is there this line at the end of the doc string?
>
>   Undocumented
>
> What on earth is that supposed to mean to a reader of
> the `map-elt' doc?

The limitations of and suggestions for improvements to the documentation
generated for generic functions are already discussed elsewhere, and are
not specific to the current issue.

-- 
Basil

bug#47368: [External] : bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Drew Adams]

> > Nothing about eq being the default, and nothing about
> > testing being also possible with the others you mention.
> >
> > Not only that, but the doc string says that TESTFN
> > is deprecated, but there's no other mention of TESTFN.
> >
> > What's TESTFN?  Where is it specified?  It's not part
> > of the function signature that's shown.  How can you
> > refer to it if there's no indication anywhere here of
> > what it is?  This makes no sense to me.
> 
> All of these points are already being discussed in this thread.

And yet in this thread you asked, seeming to suggest
that the doc here is fine as is:

  What would you like to see clarified in the documentation?

Seems pretty clear that this doc has more than one
problem, as I guess (hope) you acknowledge now.

If the function handles different kinds of data
(alists, hash tables, ... whatnot), and if its
behavior can depend on an equality predicate that
it can't know (can't even be passed as an arg),
then _that_, at the very least, should be stated
in the doc.

I'd think that if a test function _can_ be used for
at least some types of data, such as alists, then it
should be allowed as an optional arg.

Just because a function is generic, that doesn't mean
it has to always act with lowest-common-denominator
behavior.  A hash table has a given comparer.  But so
what?

Is `eq' the _default_ comparer, or is it the only one
for some data types?

  "its default depends on the MAP argument".

Maybe that too isn't generic enough?  If `eq' is all
that's allowed for some types then it makes no sense
to speak of a "default" those cases.

You said:

  That's what the docstring is trying to warn about:
  alists default to testing with eq, but can also use
  eql, equal, or anything else.

  Hash tables, OTOH, are limited to the test function
  that they were created with.

  So TESTFN doesn't always work as expected depending
  on the map type.

Why not allow TESTFN, and state that it applies only
to some types (naming some of them)?  Why cripple it
just because it's limited for some types?

Are you sure deprecating TESTFN was a great idea?

If it will remain deprecated, then at least add a
statement like what you wrote (above), to the doc.

And let users know how to use other than `eq' with
an alist etc., since you tell them that `eq' is
only the default and they can use other comparers.

And especially if the types are limited to alist,
hash table, and array, just _how_ a key is found
should be specified.  It makes no sense (to me)
for the doc to say only "If KEY is found".

FWIW, a cursory look at the code suggests the type
can also be a plist, but the doc currently mentions
only the other 3 types.  That too seems wrong.

bug#47368: [External] : bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Basil L. Contovounesios]

Drew Adams <drew.adams@oracle.com> writes:

>> > Nothing about eq being the default, and nothing about
>> > testing being also possible with the others you mention.
>> >
>> > Not only that, but the doc string says that TESTFN
>> > is deprecated, but there's no other mention of TESTFN.
>> >
>> > What's TESTFN?  Where is it specified?  It's not part
>> > of the function signature that's shown.  How can you
>> > refer to it if there's no indication anywhere here of
>> > what it is?  This makes no sense to me.
>> 
>> All of these points are already being discussed in this thread.
>
> And yet in this thread you asked, seeming to suggest
> that the doc here is fine as is:
>
>   What would you like to see clarified in the documentation?
>
> Seems pretty clear that this doc has more than one
> problem, as I guess (hope) you acknowledge now.

You seem to have misunderstood my cited text.

The issues you have mentioned have already been acknowledged.  What
remains to be done is decide on how to improve map-elt's interface, so
that its documentation can be updated accordingly.

> FWIW, a cursory look at the code suggests the type
> can also be a plist, but the doc currently mentions
> only the other 3 types.  That too seems wrong.

This is already fixed on master, and in version 3.0 of the map.el
package on GNU ELPA: https://elpa.gnu.org/packages/map.html

-- 
Basil

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Michael Heerdegen]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> Just some code archaeology for more context:
>
> - Pre-Emacs-25 map-elt used assoc.
> - Emacs 25 map-elt used alist-get without TESTFN.
> - Emacs 26 map-elt gained TESTFN at the same time that alist-get did,
>   in https://bugs.gnu.org/27584.
> - Emacs 27 deprecated map-elt's TESTFN.

And `map-put!' has been added recently.  For alists it compares with
`equal', and it's used by the setter for the general `map-elt' variable,
so we currently don't have consistence.

Michael.

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> How 'bout always using `equal`?

I think it would be an improvement.

But are alists the only exception - are we sure there are no useful
implementations of map types were the TESTFN is not constant?  Like a
dictionary where we sometimes want to look up a word case-sensitively,
and if none found, case-insensitively?

Michael.

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Basil L. Contovounesios]

Trying the CC again, this time with a hopefully correct address.
Sorry about the noise.

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> What's a good way to solve this?  Obviously the map abstraction doesn't
>>> fit so super well for alists because unlike the other map type alists
>>> don't know "their" test function.  But disallowing alists that don't
>>> test with `eq' seems an unnecessary restriction.  Can we say that the
>>> argument is allowed only for alists?
>>
>> How 'bout always using `equal`?
>
> No objections here.  CCing Nicolas in case he has any comments.
>
> Just some code archaeology for more context:
>
> - Pre-Emacs-25 map-elt used assoc.
> - Emacs 25 map-elt used alist-get without TESTFN.
> - Emacs 26 map-elt gained TESTFN at the same time that alist-get did,
>   in https://bugs.gnu.org/27584.
> - Emacs 27 deprecated map-elt's TESTFN.

-- 
Basil

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Adam Porter]

Hi,

If I may chime in here, I'd like for map-elt to be usable with alists
having string keys.  It may not generally be very common in Elisp to
use alists with string keys, but in certain contexts, like preparing
JSON maps for encoding, it is.

In one of my packages, I came up with this workaround, which may be
tolerable when only used once in the whole package, but wouldn't be
nice to use more often:

    (cl-letf (((symbol-function 'alist-get)
               (lambda (key alist &optional _default _remove _testfn)
                 (cdr (assoc-string key alist)))))
      (let ((alist (list (cons "foo" "FOO")
                         (cons "bar" "BAR"))))
        (map-elt alist "foo")))  ;;=> "FOO"

In one of my other packages, I would have to use it more often, so I
guess I'll use alist-get for now.

In general, I think that using `equal' is a good solution.  It seems
like map-elt is intended to abstract over some Lisp implementation
details (to some extent, anyway), so using `equal' instead of `eq'
seems sensible, since I think it will usually DWIM.  If I really need
to compare Lisp object identity rather than equality, I'll probably
know how, and I probably won't need to do that as often, anyway.

Could this change be made for Emacs 28 and tagged for a 3.1 release of map.el?

Thanks,
Adam

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Michael Heerdegen]

Adam Porter <adam@alphapapa.net> writes:

> Could this change be made for Emacs 28 and tagged for a 3.1 release of
> map.el?

Let's try to CC Nicolas again, in the past he always showed up sooner or
later, and he wanted to stay involved.

Michael.

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Let's try to CC Nicolas again, in the past he always showed up sooner
> or later, and he wanted to stay involved.

Hmm, seems his address "nicolas@petton.fr" is not reachable ATM?  I can
try again later or try to contact him in some other way.

Michael.

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Adam Porter]

On Fri, Jul 30, 2021 at 9:18 PM Michael Heerdegen
<michael_heerdegen@web.de> wrote:
>
> Michael Heerdegen <michael_heerdegen@web.de> writes:
>
> > Let's try to CC Nicolas again, in the past he always showed up sooner
> > or later, and he wanted to stay involved.
>
> Hmm, seems his address "nicolas@petton.fr" is not reachable ATM?  I can
> try again later or try to contact him in some other way.

Friendly ping.  :)  The cutting of the Emacs 28 release branch
approaches, and I'd really like to have this fixed in Emacs 28.

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Michael Heerdegen]

Adam Porter <adam@alphapapa.net> writes:

> Friendly ping.  :)  The cutting of the Emacs 28 release branch
> approaches, and I'd really like to have this fixed in Emacs 28.

Basil, are you maybe able to care about this one?  I'm on vacation and
don't have too much time.

Michael.

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Michael Heerdegen [2021-09-14 16:40 +0200] wrote:

> Adam Porter <adam@alphapapa.net> writes:
>
>> Friendly ping.  :)  The cutting of the Emacs 28 release branch
>> approaches, and I'd really like to have this fixed in Emacs 28.
>
> Basil, are you maybe able to care about this one?

What was the conclusion?  That map-elt should use equal for alists?
If so, sure, I'll try to propose a patch soon.

-- 
Basil

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Michael Heerdegen]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> Michael Heerdegen [2021-09-14 16:40 +0200] wrote:
>
> > Adam Porter <adam@alphapapa.net> writes:
> >
> >> Friendly ping.  :)  The cutting of the Emacs 28 release branch
> >> approaches, and I'd really like to have this fixed in Emacs 28.
> >
> > Basil, are you maybe able to care about this one?
>
> What was the conclusion?  That map-elt should use equal for alists?

That was the least controversial solution discussed, at least to my
impression.

But would that include to change the default of `alist-get', too?  I
guess these should behave consistently.

[ Personally I would still be interested in an answer to the question:
why don't we consider to keep the TESTFN arg at least for the list map
type case? ]


Michael.

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Michael Heerdegen [2021-09-15 02:48 +0200] wrote:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>> What was the conclusion?  That map-elt should use equal for alists?
>
> That was the least controversial solution discussed, at least to my
> impression.
>
> But would that include to change the default of `alist-get', too?  I
> guess these should behave consistently.

That sounds a bit too radical to me within the scope of this bug report,
and for Emacs 28.  Since alist-get has the type in its name, and allows
for a testfn argument, I personally don't see any conflict between
map-elt and alist-get having different default behaviour on alists.

> [ Personally I would still be interested in an answer to the question:
> why don't we consider to keep the TESTFN arg at least for the list map
> type case? ]

I don't feel too strongly either way, and it's not my decision, but I
guess the point is that if you already know that your map is an alist,
then you can "customise" the behaviour of map-elt by using a different
accessor function, since map.el wants to provide a type-agnostic API.

Thanks,

-- 
Basil

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

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

> [ Personally I would still be interested in an answer to the question:
> why don't we consider to keep the TESTFN arg at least for the list map
> type case? ]

This would only make sense if we add corresponding args to several
other functions of `map.el`, and it would bring us back to the problem
that this arg would be ignored for most map types and couldn't be obeyed
without paying a high cost.

If you know it's a list and you want to use a specific test, then don't
use `map-elt`.


        Stefan

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Basil L. Contovounesios [2021-09-14 21:22 +0100] wrote:

> Michael Heerdegen [2021-09-14 16:40 +0200] wrote:
>
>> Adam Porter <adam@alphapapa.net> writes:
>>
>>> Friendly ping.  :)  The cutting of the Emacs 28 release branch
>>> approaches, and I'd really like to have this fixed in Emacs 28.
>>
>> Basil, are you maybe able to care about this one?
>
> What was the conclusion?  That map-elt should use equal for alists?
> If so, sure, I'll try to propose a patch soon.

How's the attached?

Thanks,

-- 
Basil


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Consistently-test-alist-keys-with-equal-in-map.el.patch --]
[-- Type: text/x-diff, Size: 6299 bytes --]

From a62bb01dd43a9957fe5f04552390a0b3e46e32f9 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Wed, 15 Sep 2021 00:17:26 +0100
Subject: [PATCH] Consistently test alist keys with equal in map.el

* etc/NEWS: Announce new default behavior of map-elt and map-delete
on alists.

* lisp/emacs-lisp/map.el: Bump to version 3.2.
(map-elt): Test alist keys with equal by default.  Betray a little
bit more information in the docstring on which functions are used
for which map types.  (Bug#47368)
(map-put): Update docstring accordingly.
(map--plist-delete): Consistently test plist keys with eq.
(map-delete): Consistently test alist keys with equal.

* test/lisp/emacs-lisp/map-tests.el (test-map-elt-testfn): Update
for new map-elt behavior.
(test-map-put!-alist, test-map-delete-alist): New tests.
---
 etc/NEWS                          |  6 ++++++
 lisp/emacs-lisp/map.el            | 18 +++++++++-------
 test/lisp/emacs-lisp/map-tests.el | 34 ++++++++++++++++++++++++++-----
 3 files changed, 46 insertions(+), 12 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 0c9dded458..b2ffbff2ef 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -3558,6 +3558,12 @@ and well-behaved enough to lose the "internal" marker.
 
 ** map.el
 
+---
+*** Alist keys are now consistently compared with 'equal' by default.
+Until now, 'map-elt' and 'map-delete' compared alist keys with 'eq' by
+default.  They now use 'equal' instead, for consistency with
+'map-put!' and 'map-contains-key'.
+
 *** Pcase 'map' pattern added keyword symbols abbreviation.
 A pattern like '(map :sym)' binds the map's value for ':sym' to 'sym',
 equivalent to '(map (:sym sym))'.
diff --git a/lisp/emacs-lisp/map.el b/lisp/emacs-lisp/map.el
index 77431f0c59..e0af448eaf 100644
--- a/lisp/emacs-lisp/map.el
+++ b/lisp/emacs-lisp/map.el
@@ -5,7 +5,7 @@
 ;; Author: Nicolas Petton <nicolas@petton.fr>
 ;; Maintainer: emacs-devel@gnu.org
 ;; Keywords: extensions, lisp
-;; Version: 3.1
+;; Version: 3.2
 ;; Package-Requires: ((emacs "26"))
 
 ;; This file is part of GNU Emacs.
@@ -103,10 +103,14 @@ map--plist-p
   (and (consp list) (atom (car list))))
 
 (cl-defgeneric map-elt (map key &optional default testfn)
-  "Lookup KEY in MAP and return its associated value.
+  "Look up KEY in MAP and return its associated value.
 If KEY is not found, return DEFAULT which defaults to nil.
 
-TESTFN is deprecated.  Its default depends on the MAP argument.
+TESTFN is the function to use for comparing keys.  It is
+deprecated because its default and valid values depend on the MAP
+argument.  Generally, alist keys are compared with `equal', plist
+keys with `eq', and hash-table keys with the hash-table's test
+function.
 
 In the base definition, MAP can be an alist, plist, hash-table,
 or array."
@@ -136,7 +140,7 @@ map-elt
     :list (if (map--plist-p map)
               (let ((res (plist-member map key)))
                 (if res (cadr res) default))
-            (alist-get key map default nil testfn))
+            (alist-get key map default nil (or testfn #'equal)))
     :hash-table (gethash key map default)
     :array (if (map-contains-key map key)
                (aref map key)
@@ -147,7 +151,7 @@ map-put
 If KEY is already present in MAP, replace the associated value
 with VALUE.
 When MAP is an alist, test equality with TESTFN if non-nil,
-otherwise use `eql'.
+otherwise use `equal'.
 
 MAP can be an alist, plist, hash-table, or array."
   (declare (obsolete "use map-put! or (setf (map-elt ...) ...) instead" "27.1"))
@@ -157,7 +161,7 @@ map--plist-delete
   (let ((tail map) last)
     (while (consp tail)
       (cond
-       ((not (equal key (car tail)))
+       ((not (eq key (car tail)))
         (setq last tail)
         (setq tail (cddr last)))
        (last
@@ -177,7 +181,7 @@ map-delete
   ;; FIXME: Signal map-not-inplace i.s.o returning a different list?
   (if (map--plist-p map)
       (map--plist-delete map key)
-    (setf (alist-get key map nil t) nil)
+    (setf (alist-get key map nil t #'equal) nil)
     map))
 
 (cl-defmethod map-delete ((map hash-table) key)
diff --git a/test/lisp/emacs-lisp/map-tests.el b/test/lisp/emacs-lisp/map-tests.el
index c0f0dbc92b..afade8e295 100644
--- a/test/lisp/emacs-lisp/map-tests.el
+++ b/test/lisp/emacs-lisp/map-tests.el
@@ -85,11 +85,13 @@ test-map-elt-default
     (should (= 5 (map-elt map 0 5)))))
 
 (ert-deftest test-map-elt-testfn ()
-  (let ((map (list (cons "a" 1) (cons "b" 2)))
-        ;; Make sure to use a non-eq "a", even when compiled.
-        (noneq-key (string ?a)))
-    (should-not (map-elt map noneq-key))
-    (should (map-elt map noneq-key nil #'equal))))
+  (let* ((a (string ?a))
+         (map `((,a . 0) (,(string ?b) . 1))))
+    (should (= (map-elt map a) 0))
+    (should (= (map-elt map "a") 0))
+    (should (= (map-elt map (string ?a)) 0))
+    (should (= (map-elt map "b") 1))
+    (should (= (map-elt map (string ?b)) 1))))
 
 (ert-deftest test-map-elt-with-nil-value ()
   (should-not (map-elt '((a . 1) (b)) 'b 2)))
@@ -129,6 +131,19 @@ test-map-put!-new-keys
         (setf (map-elt map size) 'v)
         (should (eq (map-elt map size) 'v))))))
 
+(ert-deftest test-map-put!-alist ()
+  "Test `map-put!' test function on alists."
+  (let ((key (string ?a))
+        (val 0)
+        map)
+    (should-error (map-put! map key val) :type 'map-not-inplace)
+    (setq map (list (cons key val)))
+    (map-put! map key (1- val))
+    (should (equal map '(("a" . -1))))
+    (map-put! map (string ?a) (1+ val))
+    (should (equal map '(("a" . 1))))
+    (should-error (map-put! map (string ?a) val #'eq) :type 'map-not-inplace)))
+
 (ert-deftest test-map-put-alist-new-key ()
   "Regression test for Bug#23105."
   (let ((alist (list (cons 0 'a))))
@@ -197,6 +212,15 @@ test-map-delete-empty
   (with-empty-maps-do map
     (should (eq map (map-delete map t)))))
 
+(ert-deftest test-map-delete-alist ()
+  "Test `map-delete' test function on alists."
+  (let* ((a (string ?a))
+         (map `((,a) (,(string ?b)))))
+    (setq map (map-delete map a))
+    (should (equal map '(("b"))))
+    (setq map (map-delete map (string ?b)))
+    (should-not map)))
+
 (ert-deftest test-map-nested-elt ()
   (let ((vec [a b [c d [e f]]]))
     (should (eq (map-nested-elt vec '(2 2 0)) 'e)))
-- 
2.33.0

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> If you know it's a list and you want to use a specific test, then don't
> use `map-elt`.

That's the part I missed.  Makes sense.

Michael.

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Michael Heerdegen]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> How's the attached?

From what I understand, looks all good.


Regards,

Michael.

bug#47368: 28.0.50; map-elt returns nil without "deprecated" TESTFN

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

close 47368 28.1
quit

Michael Heerdegen [2021-09-15 23:55 +0200] wrote:
> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>> How's the attached?
> From what I understand, looks all good.

Thanks.  There were no further comments, so I pushed the patch, and am
therefore closing this report.

Consistently test alist keys with equal in map.el
14495e33af 2021-09-21 13:32:49 +0100
https://git.sv.gnu.org/cgit/emacs.git/commit/?id=14495e33afa0b8038c494f1e6e90065683ccbd07

-- 
Basil