GNU bug report logs - #58531
29.0.50; Wrong predicate used by map-elt gv getter

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 58531 in the body.
You can then email your comments to 58531 AT debbugs.gnu.org in the normal way.

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

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

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.50; Wrong predicate used by map-elt gv getter
Date: Sat, 15 Oct 2022 00:45:02 +0300

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

0. emacs -Q
1. M-x load-library RET map RET
2. (let ((l (list (cons 'a 1))))
     (setf (map-elt l "a" nil #'string=) 2)
     l)
   C-j

This correctly gives ((a . 2)).

3. (let ((l (list (cons 'a 1))))
     (cl-incf (map-elt l "a" nil #'string=))
     l)
   C-j

This gives the following backtrace:

[backtrace.txt (text/plain, inline)]

Debugger entered--Lisp error: (wrong-type-argument number-or-marker-p nil)
  +(nil 1)
  (let* ((v (+ (map-elt l "a" nil) 1))) (condition-case nil (with-no-warnings (map-put! l "a" v #'string=)) (map-not-inplace (setq l (map-insert l "a" v)) v)))
  (let ((l (list (cons 'a 1)))) (let* ((v (+ (map-elt l "a" nil) 1))) (condition-case nil (with-no-warnings (map-put! l "a" v #'string=)) (map-not-inplace (setq l (map-insert l "a" v)) v))) l)
  (progn (let ((l (list (cons 'a 1)))) (let* ((v (+ (map-elt l "a" nil) 1))) (condition-case nil (with-no-warnings (map-put! l "a" v #'string=)) (map-not-inplace (setq l (map-insert l "a" v)) v))) l))
  eval((progn (let ((l (list (cons 'a 1)))) (let* ((v (+ (map-elt l "a" nil) 1))) (condition-case nil (with-no-warnings (map-put! l "a" v #'string=)) (map-not-inplace (setq l (map-insert l "a" v)) v))) l)) t)
  elisp--eval-last-sexp(t)
  eval-last-sexp(t)
  eval-print-last-sexp(nil)
  funcall-interactively(eval-print-last-sexp nil)
  call-interactively(eval-print-last-sexp nil nil)
  command-execute(eval-print-last-sexp)

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

Whereas it should give the same result as step 2.
Patch to follow.

Thanks,

-- 
Basil

In GNU Emacs 29.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.16.0, Xaw3d scroll bars) of 2022-10-14 built on tia
Repository revision: cbd04ad3d572850775f18bde868c71abcde733ed
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101004
System Description: Debian GNU/Linux bookworm/sid

Configured using:
 'configure 'CFLAGS=-Og -ggdb3' -C --prefix=/home/blc/.local
 --enable-checking=structs --with-file-notification=yes
 --with-x-toolkit=lucid --with-x'

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

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

Major mode: Debugger

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-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
  buffer-read-only: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util 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
help-fns radix-tree cl-print debug backtrace help-mode cl-macs map
byte-opt gv bytecomp byte-compile cconv thingatpt cl-loaddefs cl-lib
find-func rmc iso-transl tooltip eldoc paren electric uniquify
ediff-hook vc-hooks lisp-float-type elisp-mode 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 lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
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 emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs faces cus-face macroexp files window
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget keymap hashtable-print-readable backquote threads dbusbind
inotify lcms2 dynamic-setting system-font-setting font-render-setting
cairo x-toolkit xinput2 x multi-tty make-network-process emacs)

Memory information:
((conses 16 51573 5638)
 (symbols 48 6489 0)
 (strings 32 18774 1872)
 (string-bytes 1 537756)
 (vectors 16 12706)
 (vector-slots 8 181823 10539)
 (floats 8 29 46)
 (intervals 56 370 2)
 (buffers 1000 11))

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

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: 58531 <at> debbugs.gnu.org
Subject: Re: bug#58531: 29.0.50; Wrong predicate used by map-elt gv getter
Date: Sat, 15 Oct 2022 00:54:18 +0300

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

tags 58531 + patch
quit

Basil L. Contovounesios" via "Bug reports for GNU Emacs, the Swiss army knife of text editors [2022-10-15 00:45 +0300] wrote:

> Patch to follow.

Now attached.

In addition to the OP, the patch also addresses:
- The plist-get gv, as discussed in https://bugs.gnu.org/47425#91
- The gv-tests.el no-byte-compile cookie from https://bugs.gnu.org/24402
- The predicate in plist-get & co. being called with flipped arguments
  compared to assoc & alist-get

WDYT?  Thanks,

-- 
Basil

[0001-Audit-some-plist-uses-with-new-predicate-argument.patch (text/x-diff, attachment)]

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 58531 <at> debbugs.gnu.org
Subject: Re: bug#58531: 29.0.50; Wrong predicate used by map-elt gv getter
Date: Sat, 15 Oct 2022 12:33:43 +0200

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

> Now attached.
>
> In addition to the OP, the patch also addresses:
> - The plist-get gv, as discussed in https://bugs.gnu.org/47425#91
> - The gv-tests.el no-byte-compile cookie from https://bugs.gnu.org/24402

Perhaps Stefan has comments; added to the CCs;

> - The predicate in plist-get & co. being called with flipped arguments
>   compared to assoc & alist-get

Hm...  the latter sounds like something that could lead to obscure bugs
in callers out there.

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

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 58531 <at> debbugs.gnu.org
Subject: Re: bug#58531: 29.0.50; Wrong predicate used by map-elt gv getter
Date: Sat, 15 Oct 2022 16:18:54 +0300

Lars Ingebrigtsen [2022-10-15 12:33 +0200] wrote:

> "Basil L. Contovounesios" <contovob <at> tcd.ie> writes:
>
>> - The predicate in plist-get & co. being called with flipped arguments
>>   compared to assoc & alist-get
>
> Hm...  the latter sounds like something that could lead to obscure bugs
> in callers out there.

How so?  The predicate argument is new in Emacs 29, and the order of its
arguments is undocumented, so no-one should be relying on it yet.  I
just didn't see a reason why it should differ between plist-get and
assoc when Emacs 29 is released.

-- 
Basil

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: 58531 <at> debbugs.gnu.org
Subject: Re: bug#58531: 29.0.50; Wrong predicate used by map-elt gv getter
Date: Sat, 15 Oct 2022 11:52:27 -0400

>  (defun map--plist-delete (map key)
>    (let ((tail map) last)
> @@ -346,7 +399,7 @@ map-contains-key
>  If MAP is an alist, TESTFN defaults to `equal'.
>  If MAP is a plist, `plist-member' is used instead."
>    (if (map--plist-p map)
> -      (plist-member map key)
> +      (map--plist-member map key testfn)
>      (let ((v '(nil)))
>        (not (eq v (alist-get key map v nil (or testfn #'equal)))))))

Hmmm looks like we forgot to mark the `testfn` arg obsolete here with
`advertised-calling-convention` like we did for `map-elt`.
Could you fix that oversight in your patch while you're at it?

> -(defun eudc-plist-member (plist prop)
> -  "Return t if PROP has a value specified in PLIST."
> -  (if (not (= 0 (% (length plist) 2)))
> +(defun eudc--plist-member (plist prop &optional predicate)
> +  "Like `plist-member', but signal on invalid PLIST."
> +  ;; Could also use `plistp', but that would change the error.
> +  (or (zerop (% (length plist) 2))
>        (error "Malformed plist"))
> -  (catch 'found
> -    (while plist
> -      (if (eq prop (car plist))
> -	  (throw 'found t))
> -      (setq plist (cdr (cdr plist))))
> -    nil))
> +  (plist-member plist prop predicate))

The current error is poor (it doesn't include the offending plist, for
example), so I think changing it (e.g. using the usual
`wrong-type-argument` error) would be for the better.
I do wonder whether it's worth the trouble keeping the error here, tho,
instead of just using `plist-member` directly.


        Stefan

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

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>, 58531 <at> debbugs.gnu.org
Subject: Re: bug#58531: 29.0.50; Wrong predicate used by map-elt gv getter
Date: Sun, 16 Oct 2022 01:41:54 +0300

Stefan Monnier [2022-10-15 11:52 -0400] wrote:

>>  (defun map--plist-delete (map key)
>>    (let ((tail map) last)
>> @@ -346,7 +399,7 @@ map-contains-key
>>  If MAP is an alist, TESTFN defaults to `equal'.
>>  If MAP is a plist, `plist-member' is used instead."
>>    (if (map--plist-p map)
>> -      (plist-member map key)
>> +      (map--plist-member map key testfn)
>>      (let ((v '(nil)))
>>        (not (eq v (alist-get key map v nil (or testfn #'equal)))))))
>
> Hmmm looks like we forgot to mark the `testfn` arg obsolete here with
> `advertised-calling-convention` like we did for `map-elt`.
> Could you fix that oversight in your patch while you're at it?

Sure, but generic functions don't play well with
advertised-calling-convention: each subsequent cl-defmethod overwrites
the preceding symbol-function, so any existing entry in
advertised-signature-table is no longer found after that.

What would you propose doing?  Call set-advertised-calling-convention
after the last cl-defmethod in map.el and hope no third-party code
defines a new method?  It's not ideal, but it's better than what we
currently have.

Or is there some other trick we can employ that works in Emacs 26+?

>> -(defun eudc-plist-member (plist prop)
>> -  "Return t if PROP has a value specified in PLIST."
>> -  (if (not (= 0 (% (length plist) 2)))
>> +(defun eudc--plist-member (plist prop &optional predicate)
>> +  "Like `plist-member', but signal on invalid PLIST."
>> +  ;; Could also use `plistp', but that would change the error.
>> +  (or (zerop (% (length plist) 2))
>>        (error "Malformed plist"))
>> -  (catch 'found
>> -    (while plist
>> -      (if (eq prop (car plist))
>> -	  (throw 'found t))
>> -      (setq plist (cdr (cdr plist))))
>> -    nil))
>> +  (plist-member plist prop predicate))
>
> The current error is poor (it doesn't include the offending plist, for
> example), so I think changing it (e.g. using the usual
> `wrong-type-argument` error) would be for the better.
> I do wonder whether it's worth the trouble keeping the error here, tho,
> instead of just using `plist-member` directly.

I was just being conservative, because I don't know where EUDC might get
its data from, or how important it is to catch dubious plists
red-handed.

I'd be happy to simplify the code, but let's see if Thomas (CCed) has
any comments.  Thomas, the patch touching eudc.el can be found at:
https://bugs.gnu.org/58531#8.

Thanks,

-- 
Basil

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>, 58531 <at> debbugs.gnu.org
Subject: Re: bug#58531: 29.0.50; Wrong predicate used by map-elt gv getter
Date: Sat, 15 Oct 2022 19:31:20 -0400

>> Hmmm looks like we forgot to mark the `testfn` arg obsolete here with
>> `advertised-calling-convention` like we did for `map-elt`.
>> Could you fix that oversight in your patch while you're at it?
>
> Sure, but generic functions don't play well with
> advertised-calling-convention: each subsequent cl-defmethod overwrites
> the preceding symbol-function, so any existing entry in
> advertised-signature-table is no longer found after that.

Good point.  Not a reason not to add an `advertised-calling-convention`,
but indeed we should fix that.  Could you make a bug report for that?


        Stefan

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 58531 <at> debbugs.gnu.org
Subject: Re: bug#58531: 29.0.50; Wrong predicate used by map-elt gv getter
Date: Sun, 16 Oct 2022 09:41:25 +0200

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

> How so?  The predicate argument is new in Emacs 29, and the order of its
> arguments is undocumented, so no-one should be relying on it yet.

Oh, OK -- I'd forgotten that it was new.  Then flipping the order to
match the other functions makes sense.

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

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 58531 <at> debbugs.gnu.org
Subject: Re: bug#58531: 29.0.50; Wrong predicate used by map-elt gv getter
Date: Sun, 16 Oct 2022 14:15:48 +0300

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

Stefan Monnier [2022-10-15 19:31 -0400] wrote:

>>> Hmmm looks like we forgot to mark the `testfn` arg obsolete here with
>>> `advertised-calling-convention` like we did for `map-elt`.
>>> Could you fix that oversight in your patch while you're at it?
>>
>> Sure, but generic functions don't play well with
>> advertised-calling-convention: each subsequent cl-defmethod overwrites
>> the preceding symbol-function, so any existing entry in
>> advertised-signature-table is no longer found after that.
>
> Good point.  Not a reason not to add an `advertised-calling-convention`,

Updated patch attached.

> but indeed we should fix that.  Could you make a bug report for that?

Done: https://bugs.gnu.org/58563.

Thanks,

-- 
Basil

[0001-Audit-some-plist-uses-with-new-predicate-argument.patch (text/x-diff, attachment)]

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: 58531 <at> debbugs.gnu.org
Subject: Re: bug#58531: 29.0.50; Wrong predicate used by map-elt gv getter
Date: Sun, 16 Oct 2022 12:06:17 -0400

> Updated patch attached.

LGTM, feel free to push, thanks,


        Stefan

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

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 58531 <at> debbugs.gnu.org
Subject: Re: bug#58531: 29.0.50; Wrong predicate used by map-elt gv getter
Date: Sat, 22 Oct 2022 18:01:38 +0300

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

Stefan Monnier [2022-10-16 12:06 -0400] wrote:
>> Updated patch attached.
> LGTM, feel free to push, thanks,

WDYT of the attached additions now that bug#58563 is mostly addressed?

Thanks,

-- 
Basil

[0002-Replace-remaining-map-dispatch-with-cl-defmethod.patch (text/x-diff, attachment)]

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: 58531 <at> debbugs.gnu.org
Subject: Re: bug#58531: 29.0.50; Wrong predicate used by map-elt gv getter
Date: Sat, 22 Oct 2022 11:59:41 -0400

> (map--restore-advertised-signature): New convenience function.

I'm not convinced it's worth the trouble.
This new code will mostly be used in Emacs≥29 anyway (very few users
explicitly install `map` and most packages which depend on `map` don't
depend on a particularly recent version of `map` and hence won't trigger
installation of `map` from GNU ELPA).

The rest is fine by me.


        Stefan

Message #45 received at 58531-done <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 58531-done <at> debbugs.gnu.org
Subject: Re: bug#58531: 29.0.50; Wrong predicate used by map-elt gv getter
Date: Sat, 22 Oct 2022 19:58:33 +0300

close 58531 29.1
quit

Stefan Monnier [2022-10-22 11:59 -0400] wrote:

>> (map--restore-advertised-signature): New convenience function.
>
> I'm not convinced it's worth the trouble.
> This new code will mostly be used in Emacs≥29 anyway (very few users
> explicitly install `map` and most packages which depend on `map` don't
> depend on a particularly recent version of `map` and hence won't trigger
> installation of `map` from GNU ELPA).

OK, I did away with any nondeclarative advertised-calling-convention
choreography.

> The rest is fine by me.

Thanks.  Squashed, pushed, closing.

Audit some plist uses with new predicate argument
9da2efb670 2022-10-22 19:33:12 +0300
https://git.sv.gnu.org/cgit/emacs.git/commit/?id=9da2efb670

-- 
Basil

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

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>, 58531 <at> debbugs.gnu.org
Subject: Re: bug#58531: 29.0.50; Wrong predicate used by map-elt gv getter
Date: Sat, 22 Oct 2022 21:04:27 +0300

Basil L. Contovounesios [2022-10-16 01:41 +0300] wrote:

> Stefan Monnier [2022-10-15 11:52 -0400] wrote:
>
>>> -(defun eudc-plist-member (plist prop)
>>> -  "Return t if PROP has a value specified in PLIST."
>>> -  (if (not (= 0 (% (length plist) 2)))
>>> +(defun eudc--plist-member (plist prop &optional predicate)
>>> +  "Like `plist-member', but signal on invalid PLIST."
>>> +  ;; Could also use `plistp', but that would change the error.
>>> +  (or (zerop (% (length plist) 2))
>>>        (error "Malformed plist"))
>>> -  (catch 'found
>>> -    (while plist
>>> -      (if (eq prop (car plist))
>>> -	  (throw 'found t))
>>> -      (setq plist (cdr (cdr plist))))
>>> -    nil))
>>> +  (plist-member plist prop predicate))
>>
>> The current error is poor (it doesn't include the offending plist, for
>> example), so I think changing it (e.g. using the usual
>> `wrong-type-argument` error) would be for the better.
>> I do wonder whether it's worth the trouble keeping the error here, tho,
>> instead of just using `plist-member` directly.
>
> I was just being conservative, because I don't know where EUDC might get
> its data from, or how important it is to catch dubious plists
> red-handed.
>
> I'd be happy to simplify the code, but let's see if Thomas (CCed) has
> any comments.  Thomas, the patch touching eudc.el can be found at:
> https://bugs.gnu.org/58531#8.

I've now moved this subdiscussion to https://bugs.gnu.org/58720.

-- 
Basil

Previous Next

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