bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument

bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument

[Daniel Mendler]

The docstring of `keymap-set-after' says that AFTER=t should be
accepted, but it fails because of the `keymap--check'.

Then I've observed that `keymap-lookup' uses `kbd'. Shouldn't
`key-parse' be used instead?

In GNU Emacs 29.0.60 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.16.0, Xaw scroll bars) of 2023-01-14 built on projects
Repository revision: 8d7ad65665833ae99b7e7119dae37afa438968a4
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Debian GNU/Linux 11 (bullseye)

bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument

[Robert Pluim]

>>>>> On Mon, 16 Jan 2023 19:20:24 +0100, Daniel Mendler <mail@daniel-mendler.de> said:

    Daniel> The docstring of `keymap-set-after' says that AFTER=t should be
    Daniel> accepted, but it fails because of the `keymap--check'.

True. But the problems go deeper than that. `keymap-set-after' does
this:

  (define-key-after keymap (key-parse key) definition
    (and after (key-parse after))))

So passing eg "C-a" as `after' results in

(key-parse "C-a") => [1]

being passed to `define-key-after'. But the events in keymaps for keys
are not vectors, theyʼre integers, so this can never actually work
unless we do something like this:

diff --git c/lisp/keymap.el i/lisp/keymap.el
index 315eaab7560..a0575cd1b9f 100644
--- c/lisp/keymap.el
+++ i/lisp/keymap.el
@@ -186,10 +186,10 @@ keymap-set-after
   (declare (indent defun)
            (compiler-macro (lambda (form) (keymap--compile-check key) form)))
   (keymap--check key)
-  (when after
-    (keymap--check after))
-  (define-key-after keymap (key-parse key) definition
-    (and after (key-parse after))))
+  (when (and after (not (eq after t)))
+    (keymap--check after)
+    (setq after (aref (key-parse after) 0)))
+  (define-key-after keymap (key-parse key) definition after))
 
 (defun key-parse (keys)
   "Convert KEYS to the internal Emacs key representation.

but that makes me wonder what else Iʼm missing, since that feels kind
of wrong. Hmm, maybe we should be taking the last element there, not
the first?

    Daniel> Then I've observed that `keymap-lookup' uses `kbd'. Shouldn't
    Daniel> `key-parse' be used instead?

`kbd' supports a slightly less strict syntax than `key-parse' for
historical reasons. Itʼs not something I think we should change
without good reason. Certainly not in emacs-29

Robert
-- 

bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument

[Daniel Mendler]

On 1/17/23 18:13, Robert Pluim wrote:
> `kbd' supports a slightly less strict syntax than `key-parse' for
> historical reasons. Itʼs not something I think we should change
> without good reason. Certainly not in emacs-29

keymap-lookup was added in 29. So it is still time to use key-parse
instead of kbd?

Daniel

bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument

[Robert Pluim]

>>>>> On Tue, 17 Jan 2023 18:18:17 +0100, Daniel Mendler <mail@daniel-mendler.de> said:

    Daniel> On 1/17/23 18:13, Robert Pluim wrote:
    >> `kbd' supports a slightly less strict syntax than `key-parse' for
    >> historical reasons. Itʼs not something I think we should change
    >> without good reason. Certainly not in emacs-29

    Daniel> keymap-lookup was added in 29. So it is still time to use key-parse
    Daniel> instead of kbd?

Youʼre right about that, and all the related commands talk about
`key-valid-p'. I checked the usage of `keymap-lookup' in emacs-29, and
switching it to `key-parse' should be fine. Care to propose a patch?

Robert
-- 

bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument

[Daniel Mendler]

On 1/18/23 09:36, Robert Pluim wrote:
> Youʼre right about that, and all the related commands talk about
> `key-valid-p'. I checked the usage of `keymap-lookup' in emacs-29, and
> switching it to `key-parse' should be fine. 

Okay, good!

> Care to propose a patch?

No, I am too busy these days maintaining other projects, in particular
Compat from GNU ELPA, thanks to which I found this issue.

Daniel

bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument

[Robert Pluim]

The nice thing about keymaps in Emacs is that everything is so
consistent :-).

The bare minimum to fix this bug:

diff --git c/lisp/keymap.el i/lisp/keymap.el
index 315eaab7560..2caaafabb94 100644
--- c/lisp/keymap.el
+++ i/lisp/keymap.el
@@ -186,6 +186,7 @@ keymap-set-after
   (declare (indent defun)
            (compiler-macro (lambda (form) (keymap--compile-check key) form)))
   (keymap--check key)
+  (when (eq after t) (setq after nil)) ; nil and t are treated the same
   (when after
     (keymap--check after))
   (define-key-after keymap (key-parse key) definition

However, `keymap-set' and `keymap-set-after' donʼt behave the same:

(let ((k (make-sparse-keymap)))
  (keymap-set k "a" "a")
  (keymap-set-after k "b" "b")
  k)

=> (keymap (97 . [97]) (98 . "b"))

so for consistency Iʼd want to put the following in emacs-29:

diff --git c/lisp/keymap.el i/lisp/keymap.el
index 315eaab7560..19faae5c493 100644
--- c/lisp/keymap.el
+++ i/lisp/keymap.el
@@ -188,6 +188,11 @@ keymap-set-after
   (keymap--check key)
   (when after
     (keymap--check after))
+  ;; If we're binding this key to another key, then parse that other
+  ;; key, too.
+  (when (stringp definition)
+    (keymap--check definition)
+    (setq definition (key-parse definition)))
   (define-key-after keymap (key-parse key) definition
     (and after (key-parse after))))

And of course the following for consistency for `keymap-lookup' as
well. I now firmly believe that the new keymap functions should use
`key-parse' and not `kbd'.

diff --git c/lisp/keymap.el i/lisp/keymap.el
index 315eaab7560..da71d48c86e 100644
--- c/lisp/keymap.el
+++ i/lisp/keymap.el
@@ -404,7 +404,7 @@ keymap-lookup
                    (symbolp value))
             (or (command-remapping value) value)
           value))
-    (key-binding (kbd key) accept-default no-remap position)))
+    (key-binding (key-parse key) accept-default no-remap position)))
 
 (defun keymap-local-lookup (keys &optional accept-default)
   "Return the binding for command KEYS in current local keymap only.

Eli, is this too much for emacs-29?

Robert
-- 

bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument

[Daniel Mendler]

On 1/19/23 10:55, Robert Pluim wrote:
> And of course the following for consistency for `keymap-lookup' as
> well. I now firmly believe that the new keymap functions should use
> `key-parse' and not `kbd'.

Robert, speaking of `key-parse', I wonder why this function accepts
invalid key strings. Why don't we move the `key-valid-p' check to
`key-parse'? This would alleviate the need for many additional
`key-valid-p' checks across keymap.el. One could maybe even get rid of
the `keymap--check' helper.

The function `key-parse' is publicly exposed to the user and as such it
would be good if it were as strict as the rest of the keymap.el API.
However `kbd' has been reimplemented based on `key-parse' which prevents
`key-parse' from being more strict. One could either introduce a
`key--parse-lax` function which is used by `kbd' and `key-parse' (plus a
`key-valid-p' check) or add a NOERROR/NOVALIDATE argument to
`key-parse', which is then used by `kbd'. What do you think?

> Eli, is this too much for emacs-29?

While it is late for 29, these APIs were newly and I believe it would be
better to improve them now, such that they don't have to change again in
the next release.

Daniel

bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument

[Robert Pluim]

>>>>> On Thu, 19 Jan 2023 11:08:07 +0100, Daniel Mendler <mail@daniel-mendler.de> said:

    Daniel> On 1/19/23 10:55, Robert Pluim wrote:
    >> And of course the following for consistency for `keymap-lookup' as
    >> well. I now firmly believe that the new keymap functions should use
    >> `key-parse' and not `kbd'.

    Daniel> Robert, speaking of `key-parse', I wonder why this function accepts
    Daniel> invalid key strings. Why don't we move the `key-valid-p' check to
    Daniel> `key-parse'? This would alleviate the need for many additional
    Daniel> `key-valid-p' checks across keymap.el. One could maybe even get rid of
    Daniel> the `keymap--check' helper.

Do you have an example of such an invalid string?

    Daniel> The function `key-parse' is publicly exposed to the user and as such it
    Daniel> would be good if it were as strict as the rest of the keymap.el API.
    Daniel> However `kbd' has been reimplemented based on `key-parse' which prevents
    Daniel> `key-parse' from being more strict. One could either introduce a
    Daniel> `key--parse-lax` function which is used by `kbd' and `key-parse' (plus a
    Daniel> `key-valid-p' check) or add a NOERROR/NOVALIDATE argument to
    Daniel> `key-parse', which is then used by `kbd'. What do you think?

    >> Eli, is this too much for emacs-29?

    Daniel> While it is late for 29, these APIs were newly and I believe it would be
    Daniel> better to improve them now, such that they don't have to change again in
    Daniel> the next release.

I think itʼs too late to make such changes for emacs-29. Iʼm not even
sure that the minimal changes I proposed will be accepted :-)

Robert
-- 

bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument

[Daniel Mendler]

On 1/19/23 11:16, Robert Pluim wrote:
>     Daniel> Robert, speaking of `key-parse', I wonder why this function accepts
>     Daniel> invalid key strings. Why don't we move the `key-valid-p' check to
>     Daniel> `key-parse'? This would alleviate the need for many additional
>     Daniel> `key-valid-p' checks across keymap.el. One could maybe even get rid of
>     Daniel> the `keymap--check' helper.
> 
> Do you have an example of such an invalid string?

Just try something like this:

(key-valid-p "bug")
(key-parse "bug")
>     Daniel> The function `key-parse' is publicly exposed to the user and as such it
>     Daniel> would be good if it were as strict as the rest of the keymap.el API.
>     Daniel> However `kbd' has been reimplemented based on `key-parse' which prevents
>     Daniel> `key-parse' from being more strict. One could either introduce a
>     Daniel> `key--parse-lax` function which is used by `kbd' and `key-parse' (plus a
>     Daniel> `key-valid-p' check) or add a NOERROR/NOVALIDATE argument to
>     Daniel> `key-parse', which is then used by `kbd'. What do you think?
> 
>     >> Eli, is this too much for emacs-29?
> 
>     Daniel> While it is late for 29, these APIs were newly and I believe it would be
>     Daniel> better to improve them now, such that they don't have to change again in
>     Daniel> the next release.
> 
> I think itʼs too late to make such changes for emacs-29. Iʼm not even
> sure that the minimal changes I proposed will be accepted :-)

I don't think so since of all these functions are new. These seem like
simple internal refactorings. Adding a NOERROR argument to key-parse
seems like the least intrusive approach.

Daniel

bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument

[Robert Pluim]

>>>>> On Thu, 19 Jan 2023 11:39:34 +0100, Daniel Mendler <mail@daniel-mendler.de> said:

    Daniel> On 1/19/23 11:16, Robert Pluim wrote:
    Daniel> Robert, speaking of `key-parse', I wonder why this function accepts
    Daniel> invalid key strings. Why don't we move the `key-valid-p' check to
    Daniel> `key-parse'? This would alleviate the need for many additional
    Daniel> `key-valid-p' checks across keymap.el. One could maybe even get rid of
    Daniel> the `keymap--check' helper.
    >> 
    >> Do you have an example of such an invalid string?

    Daniel> Just try something like this:

    Daniel> (key-valid-p "bug")
    Daniel> (key-parse "bug")

Ah, the old "do we have to put spaces between keys" issue. Iʼm not
sure how best to deal with that, Iʼll have to read through keymap.el
some more. This:

(keymap-set k "a" "bug")

is already invalid, you have to write either

(keymap-set k "a" "b u g")

or

(keymap-set k "a" (kmacro "b u g"))

(Iʼm still tempted to change `kbd' to *always* return a vector, even
if all the characters are ASCII)

    >> I think itʼs too late to make such changes for emacs-29. Iʼm not even
    >> sure that the minimal changes I proposed will be accepted :-)

    Daniel> I don't think so since of all these functions are new. These seem like
    Daniel> simple internal refactorings. Adding a NOERROR argument to key-parse
    Daniel> seems like the least intrusive approach.

I know emacs-29 hasnʼt been released yet, but changing a function to
error by default when it didnʼt do previously seems risky. Iʼll make
that change locally and see what happens. (Update: it did not go well,
there are test-suite failures).

Robert
-- 

bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument

[Daniel Mendler]

On 1/19/23 12:05, Robert Pluim wrote:
>>>>>> On Thu, 19 Jan 2023 11:39:34 +0100, Daniel Mendler <mail@daniel-mendler.de> said:
> 
>     Daniel> On 1/19/23 11:16, Robert Pluim wrote:
>     Daniel> Robert, speaking of `key-parse', I wonder why this function accepts
>     Daniel> invalid key strings. Why don't we move the `key-valid-p' check to
>     Daniel> `key-parse'? This would alleviate the need for many additional
>     Daniel> `key-valid-p' checks across keymap.el. One could maybe even get rid of
>     Daniel> the `keymap--check' helper.
>     >> 
>     >> Do you have an example of such an invalid string?
> 
>     Daniel> Just try something like this:
> 
>     Daniel> (key-valid-p "bug")
>     Daniel> (key-parse "bug")
> 
> Ah, the old "do we have to put spaces between keys" issue. Iʼm not
> sure how best to deal with that, Iʼll have to read through keymap.el
> some more. This:

My point is that it would be expected from `key-parse' that it is
equally strict as the other keymap functions, otherwise we miss bugs
where `key-parse' wasn't used properly. Furthermore we would avoid all
these `key-valid-p` and `keymap--check' calls, as I mentioned.

Of course `kbd' should stay as lax as it has always been.

>     >> I think itʼs too late to make such changes for emacs-29. Iʼm not even
>     >> sure that the minimal changes I proposed will be accepted :-)
> 
>     Daniel> I don't think so since of all these functions are new. These seem like
>     Daniel> simple internal refactorings. Adding a NOERROR argument to key-parse
>     Daniel> seems like the least intrusive approach.
> 
> I know emacs-29 hasnʼt been released yet, but changing a function to
> error by default when it didnʼt do previously seems risky. 

It is mostly used internally. There are only 9 call sites in the Emacs code.

> Iʼll make
> that change locally and see what happens. (Update: it did not go well,
> there are test-suite failures).

This is hardly an argument. You should check all the call sites and
adjust accordingly. In particular `kbd' must pass 'noerror. I would
expect this to be a pretty small patch given the small number of call sites.

Daniel

bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument

[Robert Pluim]

>>>>> On Thu, 19 Jan 2023 12:19:19 +0100, Daniel Mendler <mail@daniel-mendler.de> said:

    Daniel> On 1/19/23 12:05, Robert Pluim wrote:

    Daniel> My point is that it would be expected from `key-parse' that it is
    Daniel> equally strict as the other keymap functions, otherwise we miss bugs
    Daniel> where `key-parse' wasn't used properly. Furthermore we would avoid all
    Daniel> these `key-valid-p` and `keymap--check' calls, as I mentioned.

`key-parse' should perhaps be renamed to `key--parse' as itʼs very
much internal.

    Daniel> Of course `kbd' should stay as lax as it has always been.

    Daniel> It is mostly used internally. There are only 9 call sites in the Emacs code.

    >> Iʼll make
    >> that change locally and see what happens. (Update: it did not go well,
    >> there are test-suite failures).

    Daniel> This is hardly an argument. You should check all the call sites and
    Daniel> adjust accordingly. In particular `kbd' must pass 'noerror. I would
    Daniel> expect this to be a pretty small patch given the small number of call sites.

The test suite shows what peopleʼs assumptions are, so such failures
are valuable. It may be enough to use 'noerror in `kbd', indeed, but I
havenʼt checked that yet. Or maybe the test suite needs adjusting.

Robert
-- 

bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument

[Daniel Mendler]

On 1/19/23 16:27, Robert Pluim wrote:
>>>>>> On Thu, 19 Jan 2023 12:19:19 +0100, Daniel Mendler <mail@daniel-mendler.de> said:
> 
>     Daniel> On 1/19/23 12:05, Robert Pluim wrote:
> 
>     Daniel> My point is that it would be expected from `key-parse' that it is
>     Daniel> equally strict as the other keymap functions, otherwise we miss bugs
>     Daniel> where `key-parse' wasn't used properly. Furthermore we would avoid all
>     Daniel> these `key-valid-p` and `keymap--check' calls, as I mentioned.
> 
> `key-parse' should perhaps be renamed to `key--parse' as itʼs very
> much internal.

Yes, that would be another possibility. I also thought about that, but
having key-parse public seems useful for packages. There should be an
official way to convert from the string to the internal key representation.

My only gripe with key-parse in its current form is really that it is
not strict enough. Another possibility in case we don't want to
introduce a NOERROR/LAX argument:

key-parse (strict) calls key-valid-p and key--parse-lax
kbd calls key--parse-lax

>     Daniel> Of course `kbd' should stay as lax as it has always been.
> 
>     Daniel> It is mostly used internally. There are only 9 call sites in the Emacs code.
> 
>     >> Iʼll make
>     >> that change locally and see what happens. (Update: it did not go well,
>     >> there are test-suite failures).
> 
>     Daniel> This is hardly an argument. You should check all the call sites and
>     Daniel> adjust accordingly. In particular `kbd' must pass 'noerror. I would
>     Daniel> expect this to be a pretty small patch given the small number of call sites.
> 
> The test suite shows what peopleʼs assumptions are, so such failures
> are valuable. It may be enough to use 'noerror in `kbd', indeed, but I
> havenʼt checked that yet. Or maybe the test suite needs adjusting.

Yes, of course. But I assume that in this case the failure is not coming
from direct calls to key-parse but from calls to kbd etc. The key-parse
itself doesn't seem to be really tested in the Emacs test suite.

I also maintain a test suite as part of the Compat package for newly
introduced APIs. There I also have tests, which could be upstreamed to
the Emacs test suite if there is interest, since some functions lack
tests there. See
https://github.com/emacs-compat/compat/blob/master/compat-tests.el

Daniel

bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: 60867@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>
> Date: Thu, 19 Jan 2023 10:55:49 +0100
> 
> Eli, is this too much for emacs-29?

I think this is fine, as all of those functions are new in Emacs 29,
right?

bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument

[Robert Pluim]

>>>>> On Thu, 19 Jan 2023 12:23:17 +0200, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com>
    >> Cc: 60867@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>
    >> Date: Thu, 19 Jan 2023 10:55:49 +0100
    >> 
    >> Eli, is this too much for emacs-29?

    Eli> I think this is fine, as all of those functions are new in Emacs 29,
    Eli> right?

Yes, theyʼre all new for emacs-29.

Robert
-- 

bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: mail@daniel-mendler.de,  60867@debbugs.gnu.org
> Date: Thu, 19 Jan 2023 11:40:50 +0100
> 
> >>>>> On Thu, 19 Jan 2023 12:23:17 +0200, Eli Zaretskii <eliz@gnu.org> said:
> 
>     >> From: Robert Pluim <rpluim@gmail.com>
>     >> Cc: 60867@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>
>     >> Date: Thu, 19 Jan 2023 10:55:49 +0100
>     >> 
>     >> Eli, is this too much for emacs-29?
> 
>     Eli> I think this is fine, as all of those functions are new in Emacs 29,
>     Eli> right?
> 
> Yes, theyʼre all new for emacs-29.

Then patches to make them more consistent are okay, but please make
sure you don't need to update the documentation (doc strings, NEWS,
and the manuals).

Thanks.

bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument

[Robert Pluim]

>>>>> On Thu, 19 Jan 2023 13:27:02 +0200, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com>
    >> Cc: mail@daniel-mendler.de,  60867@debbugs.gnu.org
    >> Date: Thu, 19 Jan 2023 11:40:50 +0100
    >> 
    >> >>>>> On Thu, 19 Jan 2023 12:23:17 +0200, Eli Zaretskii <eliz@gnu.org> said:
    >> 
    >> >> From: Robert Pluim <rpluim@gmail.com>
    >> >> Cc: 60867@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>
    >> >> Date: Thu, 19 Jan 2023 10:55:49 +0100
    >> >> 
    >> >> Eli, is this too much for emacs-29?
    >> 
    Eli> I think this is fine, as all of those functions are new in Emacs 29,
    Eli> right?
    >> 
    >> Yes, theyʼre all new for emacs-29.

    Eli> Then patches to make them more consistent are okay, but please make
    Eli> sure you don't need to update the documentation (doc strings, NEWS,
    Eli> and the manuals).

The elisp manual needed adjusting, but not because of this change, the
description of `keymap-set-after' was inaccurate.

Robert
-- 

bug#60867: 29.0.60; keymap-set-after does not accept the AFTER=t argument

[Robert Pluim]

tags 60867 fixed
close 60867 29.1
quit

>>>>> On Thu, 19 Jan 2023 16:20:48 +0100, Robert Pluim <rpluim@gmail.com> said:

>>>>> On Thu, 19 Jan 2023 13:27:02 +0200, Eli Zaretskii <eliz@gnu.org> said:
    >>> From: Robert Pluim <rpluim@gmail.com>
    >>> Cc: mail@daniel-mendler.de,  60867@debbugs.gnu.org
    >>> Date: Thu, 19 Jan 2023 11:40:50 +0100
    >>> 
    >>> >>>>> On Thu, 19 Jan 2023 12:23:17 +0200, Eli Zaretskii <eliz@gnu.org> said:
    >>> 
    >>> >> From: Robert Pluim <rpluim@gmail.com>
    >>> >> Cc: 60867@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>
    >>> >> Date: Thu, 19 Jan 2023 10:55:49 +0100
    >>> >> 
    >>> >> Eli, is this too much for emacs-29?
    >>> 
    Eli> I think this is fine, as all of those functions are new in Emacs 29,
    Eli> right?
    >>> 
    >>> Yes, theyʼre all new for emacs-29.

    Eli> Then patches to make them more consistent are okay, but please make
    Eli> sure you don't need to update the documentation (doc strings, NEWS,
    Eli> and the manuals).

    Robert> The elisp manual needed adjusting, but not because of this change, the
    Robert> description of `keymap-set-after' was inaccurate.

Anyway, Iʼve pushed the fix for this issue whilst I think about
Danielʼs points about `key-parse'.

Robert
-- 
Closing.
Committed as c7e02eaa3d