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

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

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

dalanicolai
Message-ID: <[hidden email]>
--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))
Reply | Threaded
Open this post in threaded view
|

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.



Reply | Threaded
Open this post in threaded view
|

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

dalanicolai
In reply to this post by dalanicolai
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 <[hidden email]> 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 <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

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

Stefan Monnier
In reply to this post by dalanicolai
> 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




Reply | Threaded
Open this post in threaded view
|

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

Basil L. Contovounesios
Stefan Monnier <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Drew Adams
In reply to this post by 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?



Reply | Threaded
Open this post in threaded view
|

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

Basil L. Contovounesios
Drew Adams <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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.



Reply | Threaded
Open this post in threaded view
|

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

Basil L. Contovounesios
Drew Adams <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Michael Heerdegen
In reply to this post by Basil L. Contovounesios
"Basil L. Contovounesios" <[hidden email]> 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.



Reply | Threaded
Open this post in threaded view
|

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

Michael Heerdegen
In reply to this post by Stefan Monnier
Stefan Monnier <[hidden email]> 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.



Reply | Threaded
Open this post in threaded view
|

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

Basil L. Contovounesios
In reply to this post by Basil L. Contovounesios
Trying the CC again, this time with a hopefully correct address.
Sorry about the noise.

"Basil L. Contovounesios" <[hidden email]> writes:

> Stefan Monnier <[hidden email]> 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