bug#73764: format-kbd-macro returns a key name that keymap-lookup doesn't recognize

bug#73764: format-kbd-macro returns a key name that keymap-lookup doesn't recognize

[Eduardo Ochs]

Hi list,

if we run this

  (format-kbd-macro (read-key-sequence-vector "Type C-M-h:"))

we get "M-C-h". But try:

  (keymap-lookup global-map "M-C-h")
    ;;-> "M-C-h" is not a valid key definition; see `key-valid-p'

  (keymap-lookup global-map "C-M-h")
    ;;-> mark-defun

Tested on GNU Emacs 31.0.50 (build 1, x86_64-pc-linux-gnu,
GTK+ Version 3.24.24, cairo version 1.16.0) of 2024-10-11,
git-pulled and compiled a few hours ago.

  Cheers,
    Eduardo Ochs
    http://anggtwu.net/

bug#73764: format-kbd-macro returns a key name that keymap-lookup doesn't recognize

[Eli Zaretskii]

> From: Eduardo Ochs <eduardoochs@gmail.com>
> Date: Sat, 12 Oct 2024 01:46:26 -0300
> 
> if we run this
> 
>   (format-kbd-macro (read-key-sequence-vector "Type C-M-h:"))
> 
> we get "M-C-h". But try:
> 
>   (keymap-lookup global-map "M-C-h")
>     ;;-> "M-C-h" is not a valid key definition; see `key-valid-p'
> 
>   (keymap-lookup global-map "C-M-h")
>     ;;-> mark-defun

This is because key-valid-p is too restrictive: it only accepts
modifiers in the canonical order A-C-H-M-S-s.  But key-parse, which is
called by keymap-lookup, accepts and correctly processes the modifiers
in any order:

  (key-parse "M-C-h")
   => [134217736]

Should we fix key-valid-p to be more lenient?  Or maybe just remove
the call to keymap--check from keymap-look up?

Stefan, WDYT?

bug#73764: format-kbd-macro returns a key name that keymap-lookup doesn't recognize

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

>>   (keymap-lookup global-map "M-C-h")
>>     ;;-> "M-C-h" is not a valid key definition; see `key-valid-p'
[...]
> Should we fix key-valid-p to be more lenient?

AFAIK the strictness was a conscious choice, so maybe we should simply
improve the error message to say something like "M-C-h uses an invalid
modifier ordering, maybe you meant C-M-h".

>> if we run this
>>
>>   (format-kbd-macro (read-key-sequence-vector "Type C-M-h:"))
>>
>> we get "M-C-h".

I guess we also need to make sure `format-kbd-macro` generates
something that `key-valid-p` accepts.

> Or maybe just remove the call to keymap--check from keymap-lookup?

IIRC we wanted to use `keymap--check` on all input coming from the user,
so I guess the question is whether the second arg of `keymap-lookup` is
expected to be an immediate constant.
That function is still young, so it's hard to tell how it'll turn up,
but my `grep` says that indeed many (most) calls take a literal string
as argument, so `keymap--check` seems appropriate.


        Stefan

bug#73764: format-kbd-macro returns a key name that keymap-lookup doesn't recognize

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eduardo Ochs <eduardoochs@gmail.com>,  73764@debbugs.gnu.org
> Date: Sat, 12 Oct 2024 09:33:59 -0400
> 
> >>   (keymap-lookup global-map "M-C-h")
> >>     ;;-> "M-C-h" is not a valid key definition; see `key-valid-p'
> [...]
> > Should we fix key-valid-p to be more lenient?
> 
> AFAIK the strictness was a conscious choice, so maybe we should simply
> improve the error message to say something like "M-C-h uses an invalid
> modifier ordering, maybe you meant C-M-h".

The general problem is much wider, so it is not easy to intuit "what
you meant".  E.g., what do you say if the key is "S-s-M-H-A-C-b"?  Do
you want to have a function that reorders the modifiers in canonical
order?  And if we have such a function, why not reorder silently and
accept the key, like we do with the order of BEG..END in functions
that accept the region?

> >> if we run this
> >>
> >>   (format-kbd-macro (read-key-sequence-vector "Type C-M-h:"))
> >>
> >> we get "M-C-h".
> 
> I guess we also need to make sure `format-kbd-macro` generates
> something that `key-valid-p` accepts.

AFAICS, that's impossible without completely rewriting its code.  It
proceeds from left to right (i.e. from MSB to LSB).

> > Or maybe just remove the call to keymap--check from keymap-lookup?
> 
> IIRC we wanted to use `keymap--check` on all input coming from the user,
> so I guess the question is whether the second arg of `keymap-lookup` is
> expected to be an immediate constant.

But keymap-look up is not an interactive function.

> That function is still young, so it's hard to tell how it'll turn up,
> but my `grep` says that indeed many (most) calls take a literal string
> as argument, so `keymap--check` seems appropriate.

If we remove the call to keymap--check, won't the other APIs signal an
error instead?  If they will, why do we need to protect them?

bug#73764: format-kbd-macro returns a key name that keymap-lookup doesn't recognize

[Andreas Schwab]

On Okt 12 2024, Eli Zaretskii wrote:

>> I guess we also need to make sure `format-kbd-macro` generates
>> something that `key-valid-p` accepts.
>
> AFAICS, that's impossible without completely rewriting its code.  It
> proceeds from left to right (i.e. from MSB to LSB).

The issue here is that C-h is a character on its own, unlike, say, C-+.

ELISP> (format-kbd-macro [?\M-\C-+])
"C-M-+"

Maybe key-valid-p should accept those control characters as the last
component as a special case.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#73764: format-kbd-macro returns a key name that keymap-lookup doesn't recognize

[Eli Zaretskii]

> From: Andreas Schwab <schwab@linux-m68k.org>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  73764@debbugs.gnu.org,
>   eduardoochs@gmail.com
> Date: Sat, 12 Oct 2024 16:51:49 +0200
> 
> On Okt 12 2024, Eli Zaretskii wrote:
> 
> >> I guess we also need to make sure `format-kbd-macro` generates
> >> something that `key-valid-p` accepts.
> >
> > AFAICS, that's impossible without completely rewriting its code.  It
> > proceeds from left to right (i.e. from MSB to LSB).
> 
> The issue here is that C-h is a character on its own, unlike, say, C-+.

Right.  And in that case, the C- prefix is generated when a control
character is seen, which is at the end.

bug#73764: format-kbd-macro returns a key name that keymap-lookup doesn't recognize

[Robert Pluim]

>>>>> On Sat, 12 Oct 2024 18:18:18 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Andreas Schwab <schwab@linux-m68k.org>
    >> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  73764@debbugs.gnu.org,
    >> eduardoochs@gmail.com
    >> Date: Sat, 12 Oct 2024 16:51:49 +0200
    >> 
    >> On Okt 12 2024, Eli Zaretskii wrote:
    >> 
    >> >> I guess we also need to make sure `format-kbd-macro` generates
    >> >> something that `key-valid-p` accepts.
    >> >
    >> > AFAICS, that's impossible without completely rewriting its code.  It
    >> > proceeds from left to right (i.e. from MSB to LSB).
    >> 
    >> The issue here is that C-h is a character on its own, unlike, say, C-+.

    Eli> Right.  And in that case, the C- prefix is generated when a control
    Eli> character is seen, which is at the end.

So if format-kbd-macro generates something with 'C-' in the "wrong"
place, whatʼs to stop us re-ordering the modifiers as a
post-processing step (inside `format-kbd-macro')?

Robert
-- 

bug#73764: format-kbd-macro returns a key name that keymap-lookup doesn't recognize

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: Andreas Schwab <schwab@linux-m68k.org>,  73764@debbugs.gnu.org,
>   monnier@iro.umontreal.ca,  eduardoochs@gmail.com
> Date: Mon, 14 Oct 2024 17:06:05 +0200
> 
> >>>>> On Sat, 12 Oct 2024 18:18:18 +0300, Eli Zaretskii <eliz@gnu.org> said:
> 
>     >> From: Andreas Schwab <schwab@linux-m68k.org>
>     >> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  73764@debbugs.gnu.org,
>     >> eduardoochs@gmail.com
>     >> Date: Sat, 12 Oct 2024 16:51:49 +0200
>     >> 
>     >> On Okt 12 2024, Eli Zaretskii wrote:
>     >> 
>     >> >> I guess we also need to make sure `format-kbd-macro` generates
>     >> >> something that `key-valid-p` accepts.
>     >> >
>     >> > AFAICS, that's impossible without completely rewriting its code.  It
>     >> > proceeds from left to right (i.e. from MSB to LSB).
>     >> 
>     >> The issue here is that C-h is a character on its own, unlike, say, C-+.
> 
>     Eli> Right.  And in that case, the C- prefix is generated when a control
>     Eli> character is seen, which is at the end.
> 
> So if format-kbd-macro generates something with 'C-' in the "wrong"
> place, whatʼs to stop us re-ordering the modifiers as a
> post-processing step (inside `format-kbd-macro')?

If that's the only case where we put C- in the wrong place, then
nothing stops us.  Is it?

bug#73764: format-kbd-macro returns a key name that keymap-lookup doesn't recognize

[Eli Zaretskii]

> Cc: 73764@debbugs.gnu.org, schwab@linux-m68k.org, monnier@iro.umontreal.ca,
>  eduardoochs@gmail.com
> Date: Mon, 14 Oct 2024 18:54:27 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: Robert Pluim <rpluim@gmail.com>
> > Cc: Andreas Schwab <schwab@linux-m68k.org>,  73764@debbugs.gnu.org,
> >   monnier@iro.umontreal.ca,  eduardoochs@gmail.com
> > Date: Mon, 14 Oct 2024 17:06:05 +0200
> > 
> > So if format-kbd-macro generates something with 'C-' in the "wrong"
> > place, whatʼs to stop us re-ordering the modifiers as a
> > post-processing step (inside `format-kbd-macro')?
> 
> If that's the only case where we put C- in the wrong place, then
> nothing stops us.  Is it?

In any case, there's the more general issue at hand here:
keymap--check is (at least in this case) unnecessarily restrictive:
where key-parse will gladly accept and correctly process a key whose
modifiers are in any order, keymap--check insist on the canonical
order.  I'm not sure I like this, at least in non-interactive
functions.

bug#73764: format-kbd-macro returns a key name that keymap-lookup doesn't recognize

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> In any case, there's the more general issue at hand here:
> keymap--check is (at least in this case) unnecessarily restrictive:
> where key-parse will gladly accept and correctly process a key whose
> modifiers are in any order, keymap--check insist on the canonical
> order.  I'm not sure I like this, at least in non-interactive
> functions.

One issue that these new functions tried to resolve was that the old
functions were too lenient.  See, for example, this passage in the
manual, and its docstring:

     The ‘kbd’ function is very permissive, and will try to return
     something sensible even if the syntax used isn't completely
     conforming.  To check whether the syntax is actually valid, use the
     ‘key-valid-p’ function.

So this strict behaviour is not by accident, and we should probably
think twice before going back on this design decision.

At least, we should review the old arguments to make sure we don't bring
back any undesired/buggy behaviour that this was supposed to remedy.
I didn't have the time to do that fully, as there are many relevant
threads from around that period, both on emacs-devel and bug-gnu-emacs.