bug#51384: 28.0.60; substitute-command-keys finds global binding for undo instead of in specified keymap

bug#51384: 28.0.60; substitute-command-keys finds global binding for undo instead of in specified keymap

[Robert Pluim]

The \<MAPVAR> construct is documented to set the keymap for subsequent
\[COMMAND] lookups, but for some bindings it doesnʼt seem to work,
finding the global binding instead:


emacs -Q
(require 'repeat)
(substitute-command-keys "\\<undo-repeat-map>\\[undo]")
=> #("C-x u" 0 5 (face help-key-binding font-lock-face help-key-binding))
(substitute-command-keys "\\{undo-repeat-map}")
=> #("key             binding
---             -------

u               undo

" 49 50 (face help-key-binding font-lock-face help-key-binding) 50 52 (font-loc\
k-face nil face nil))

This is a with-native-compilation emacs-28, but emacs-28 without
native compilation behaves the same.

In GNU Emacs 28.0.60 (build 7, x86_64-pc-linux-gnu, GTK+ Version 3.24.24, cairo version 1.16.0)
 of 2021-10-24 built on rltb
Repository revision: b0d64be0bc581958bf3a74152a2cd10172916b03
Repository branch: emacs-28
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Debian GNU/Linux 11 (bullseye)

Configured using:
 'configure --with-native-compilation'

Robert
-- 

bug#51384: 28.0.60; substitute-command-keys finds global binding for undo instead of in specified keymap

[Andreas Schwab]

On Okt 25 2021, Robert Pluim wrote:

> (substitute-command-keys "\\<undo-repeat-map>\\[undo]")
> => #("C-x u" 0 5 (face help-key-binding font-lock-face help-key-binding))

:advertised-binding takes precedence.

Andreas.

-- 
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#51384: 28.0.60; substitute-command-keys finds global binding for undo instead of in specified keymap

[Robert Pluim]

>>>>> On Mon, 25 Oct 2021 12:16:12 +0200, Andreas Schwab <schwab@linux-m68k.org> said:

    Andreas> On Okt 25 2021, Robert Pluim wrote:
    >> (substitute-command-keys "\\<undo-repeat-map>\\[undo]")
    >> => #("C-x u" 0 5 (face help-key-binding font-lock-face help-key-binding))

    Andreas> :advertised-binding takes precedence.

So it does, which is unfortunate, underdocumented, and unnecessary,
since just not specifying the map will get you the
:advertised-binding anyway.

\\(map) anyone, to mean "really only look up in this map"?

Robert
-- 

bug#51384: 28.0.60; substitute-command-keys finds global binding for undo instead of in specified keymap

[Juri Linkov]

>     Andreas> On Okt 25 2021, Robert Pluim wrote:
>     >> (substitute-command-keys "\\<undo-repeat-map>\\[undo]")
>     >> => #("C-x u" 0 5 (face help-key-binding font-lock-face help-key-binding))
>
>     Andreas> :advertised-binding takes precedence.
>
> So it does, which is unfortunate, underdocumented, and unnecessary,
> since just not specifying the map will get you the
> :advertised-binding anyway.
>
> \\(map) anyone, to mean "really only look up in this map"?

For readers of this bug report, here is a link to the discussion:
https://lists.gnu.org/archive/html/emacs-devel/2021-10/msg01845.html

bug#51384: 28.0.60; substitute-command-keys finds global binding for undo instead of in specified keymap

[Robert Pluim]

>>>>> On Mon, 15 Nov 2021 19:25:16 +0200, Juri Linkov <juri@linkov.net> said:

    Andreas> On Okt 25 2021, Robert Pluim wrote:
    >> >> (substitute-command-keys "\\<undo-repeat-map>\\[undo]")
    >> >> => #("C-x u" 0 5 (face help-key-binding font-lock-face help-key-binding))
    >> 
    Andreas> :advertised-binding takes precedence.
    >> 
    >> So it does, which is unfortunate, underdocumented, and unnecessary,
    >> since just not specifying the map will get you the
    >> :advertised-binding anyway.
    >> 
    >> \\(map) anyone, to mean "really only look up in this map"?

    Juri> For readers of this bug report, here is a link to the discussion:
    Juri> https://lists.gnu.org/archive/html/emacs-devel/2021-10/msg01845.html

I have a change that will cause the lookups to happen in the specified
map first, but I still need to audit the 50 or so uses of a command
with an advertised binding in a docstring in the Emacs sources before
I can declare it safe.

Robert
-- 

bug#51384: 28.0.60; substitute-command-keys finds global binding for undo instead of in specified keymap

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> I have a change that will cause the lookups to happen in the specified
> map first, but I still need to audit the 50 or so uses of a command
> with an advertised binding in a docstring in the Emacs sources before
> I can declare it safe.

This was more than half a year ago -- did you get any further here?  (I
agree that the explicitly specified keymap should take precedence over
:advertised-key-binding.)

bug#51384: 28.0.60; substitute-command-keys finds global binding for undo instead of in specified keymap

[Robert Pluim]

>>>>> On Wed, 14 Sep 2022 20:29:52 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:

    Lars> Robert Pluim <rpluim@gmail.com> writes:
    >> I have a change that will cause the lookups to happen in the specified
    >> map first, but I still need to audit the 50 or so uses of a command
    >> with an advertised binding in a docstring in the Emacs sources before
    >> I can declare it safe.

I donʼt think any of the advertised bindings caused a problem, but
Iʼll double check.

    Lars> This was more than half a year ago -- did you get any further here?  (I
    Lars> agree that the explicitly specified keymap should take precedence over
    Lars> :advertised-key-binding.)

The patch has shrunk in the meantime, somebody simplified the code ;-)

Robert

diff --git a/lisp/help.el b/lisp/help.el
index 15ab3192ad..263775e64e 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -1204,7 +1204,8 @@ substitute-command-keys
                 (delete-char 2)
                 (let* ((fun (intern (buffer-substring (point) (1- end-point))))
                        (key (with-current-buffer orig-buf
-                              (where-is-internal fun keymap t))))
+                                 (where-is-internal fun (ensure-list keymap) t)))
+
                   (if (not key)
                       ;; Function is not on any key.
                       (let ((op (point)))

bug#51384: 28.0.60; substitute-command-keys finds global binding for undo instead of in specified keymap

[Robert Pluim]

>>>>> On Thu, 15 Sep 2022 09:39:43 +0200, Robert Pluim <rpluim@gmail.com> said:

    Robert> The patch has shrunk in the meantime, somebody simplified the code ;-)

Although, apart from the patch missing a paren, it doesnʼt work,
because Iʼve forgotten how Lisp lists work :-)

diff --git a/lisp/help.el b/lisp/help.el
index 15ab3192ad..90476e4a4f 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -1204,7 +1204,11 @@ substitute-command-keys
                 (delete-char 2)
                 (let* ((fun (intern (buffer-substring (point) (1- end-point))))
                        (key (with-current-buffer orig-buf
-                              (where-is-internal fun keymap t))))
+                              (where-is-internal fun
+                                                 (if keymap
+                                                     (list keymap)
+                                                   keymap)
+                                                 t))))
                   (if (not key)
                       ;; Function is not on any key.
                       (let ((op (point)))

bug#51384: 28.0.60; substitute-command-keys finds global binding for undo instead of in specified keymap

[Andreas Schwab]

On Sep 15 2022, Robert Pluim wrote:

>>>>>> On Thu, 15 Sep 2022 09:39:43 +0200, Robert Pluim <rpluim@gmail.com> said:
>
>     Robert> The patch has shrunk in the meantime, somebody simplified the code ;-)
>
> Although, apart from the patch missing a paren, it doesnʼt work,
> because Iʼve forgotten how Lisp lists work :-)
>
> diff --git a/lisp/help.el b/lisp/help.el
> index 15ab3192ad..90476e4a4f 100644
> --- a/lisp/help.el
> +++ b/lisp/help.el
> @@ -1204,7 +1204,11 @@ substitute-command-keys
>                  (delete-char 2)
>                  (let* ((fun (intern (buffer-substring (point) (1- end-point))))
>                         (key (with-current-buffer orig-buf
> -                              (where-is-internal fun keymap t))))
> +                              (where-is-internal fun
> +                                                 (if keymap
> +                                                     (list keymap)
> +                                                   keymap)

aka (and keymap (list keymap))

-- 
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#51384: 28.0.60; substitute-command-keys finds global binding for undo instead of in specified keymap

[Robert Pluim]

>>>>> On Thu, 15 Sep 2022 11:00:09 +0200, Andreas Schwab <schwab@linux-m68k.org> said:

    Andreas> On Sep 15 2022, Robert Pluim wrote:
    >>>>>>> On Thu, 15 Sep 2022 09:39:43 +0200, Robert Pluim <rpluim@gmail.com> said:
    >> 
    Robert> The patch has shrunk in the meantime, somebody simplified the code ;-)
    >> 
    >> Although, apart from the patch missing a paren, it doesnʼt work,
    >> because Iʼve forgotten how Lisp lists work :-)
    >> 
    >> diff --git a/lisp/help.el b/lisp/help.el
    >> index 15ab3192ad..90476e4a4f 100644
    >> --- a/lisp/help.el
    >> +++ b/lisp/help.el
    >> @@ -1204,7 +1204,11 @@ substitute-command-keys
    >> (delete-char 2)
    >> (let* ((fun (intern (buffer-substring (point) (1- end-point))))
    >> (key (with-current-buffer orig-buf
    >> -                              (where-is-internal fun keymap t))))
    >> +                              (where-is-internal fun
    >> +                                                 (if keymap
    >> +                                                     (list keymap)
    >> +                                                   keymap)

    Andreas> aka (and keymap (list keymap))

Indeed. Either too much C or not enough coffee this morning.

Robert
-- 

bug#51384: 28.0.60; substitute-command-keys finds global binding for undo instead of in specified keymap

[Drew Adams]

> > (if keymap (list keymap) keymap)
> aka (and keymap (list keymap))

+1

bug#51384: 28.0.60; substitute-command-keys finds global binding for undo instead of in specified keymap

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> I donʼt think any of the advertised bindings caused a problem, but
> Iʼll double check.

Thanks; feel free to push after checking.

> -                              (where-is-internal fun keymap t))))
> +                                 (where-is-internal fun (ensure-list keymap) t)))

You later said that that's not correct -- but in which cases isn't it?

bug#51384: 28.0.60; substitute-command-keys finds global binding for undo instead of in specified keymap

[Robert Pluim]

>>>>> On Fri, 16 Sep 2022 11:29:26 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:

    Lars> Robert Pluim <rpluim@gmail.com> writes:
    >> I donʼt think any of the advertised bindings caused a problem, but
    >> Iʼll double check.

    Lars> Thanks; feel free to push after checking.

Iʼm about half way through. Itʼs one of those situations where maybe
youʼre tempted to code up a solution using el-search, but that might
take longer than doing it manually 😀

    >> -                              (where-is-internal fun keymap t))))
    >> +                                 (where-is-internal fun (ensure-list keymap) t)))

    Lars> You later said that that's not correct -- but in which cases isn't it?

Test case:

(substitute-command-keys "\\<undo-repeat-map>\\[undo]")

My model was wrong. I thought keymap was either nil or bound to a
keymap, but `undo-repeat-map' is a list of value

(keymap (117 . undo))

hence the need to do

(and keymap (list keymap))

Robert
-- 

bug#51384: 28.0.60; substitute-command-keys finds global binding for undo instead of in specified keymap

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> My model was wrong. I thought keymap was either nil or bound to a
> keymap, but `undo-repeat-map' is a list of value
>
> (keymap (117 . undo))
>
> hence the need to do
>
> (and keymap (list keymap))

Ah; of course.  Thanks.

bug#51384: 28.0.60; substitute-command-keys finds global binding for undo instead of in specified keymap

[Robert Pluim]

tags 51384 fixed
close 51384 29.1
quit

So I found one occurence of a missing keymap spec in a docstring, but
that was wrong with or without this change anyway (by some bloke
called Lassie? Larry? something like that 😉). I fixed that, and
pushed to master.

Closing.
Committed as 120ade62cd

Robert
-- 

bug#51384: 28.0.60; substitute-command-keys finds global binding for undo instead of in specified keymap

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> So I found one occurence of a missing keymap spec in a docstring, but
> that was wrong with or without this change anyway (by some bloke
> called Lassie? Larry? something like that 😉).

The VC lies -- the code was written by Per.  🫠

> I fixed that, and pushed to master.

This seems to lead to:

1 unexpected results:
   FAILED  help-tests-substitute-command-keys/keymap-change

bug#51384: 28.0.60; substitute-command-keys finds global binding for undo instead of in specified keymap

[Robert Pluim]

>>>>> On Tue, 20 Sep 2022 17:51:37 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:


    Lars> 1 unexpected results:
    Lars>    FAILED  help-tests-substitute-command-keys/keymap-change

Thatʼs doing

   (test "\\<minibuffer-local-must-match-map>\\[abort-recursive-edit]"
   "C-]")

and abort-recursive-edit is in the global-map, and not
minibuffer-local-must-match-map. Oops. This fixes it. If that looks ok
Iʼll push later tonight.

diff --git a/lisp/help.el b/lisp/help.el
index 0ec5b9c85b..b4b9120da3 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -1208,6 +1208,12 @@ substitute-command-keys
                                                  (and keymap
                                                       (list keymap))
                                                  t))))
+                  ;; If we're looking in a particular keymap which has
+                  ;; no binding, then we need to redo the lookup, with
+                  ;; the global map as well this time.
+                  (when (and (not key) keymap)
+                    (setq key (with-current-buffer orig-buf
+                                (where-is-internal fun keymap t))))
                   (if (not key)
                       ;; Function is not on any key.
                       (let ((op (point)))

bug#51384: 28.0.60; substitute-command-keys finds global binding for undo instead of in specified keymap

[Stefan Kangas]

Robert Pluim <rpluim@gmail.com> writes:

> diff --git a/lisp/help.el b/lisp/help.el
> index 0ec5b9c85b..b4b9120da3 100644
> --- a/lisp/help.el
> +++ b/lisp/help.el
> @@ -1208,6 +1208,12 @@ substitute-command-keys
>                                                   (and keymap
>                                                        (list keymap))
>                                                   t))))
> +                  ;; If we're looking in a particular keymap which has
> +                  ;; no binding, then we need to redo the lookup, with
> +                  ;; the global map as well this time.
> +                  (when (and (not key) keymap)
> +                    (setq key (with-current-buffer orig-buf
> +                                (where-is-internal fun keymap t))))
>                    (if (not key)
>                        ;; Function is not on any key.
>                        (let ((op (point)))

This looks eerily similar to the code I removed here:

    commit ac0027f6a5480bd4739fdf71413a19012f400483
    Author: Stefan Kangas <stefan@marxist.se>
    Date:   Mon Jul 11 17:21:23 2022 +0200

        Remove dead branch from substitute-command-keys

        * lisp/help.el (substitute-command-keys): Remove dead branch;
        where-is-internal will follow any remaps for us.  Note also that the
        test case for remapping still pass.

Do we have a test case that reverting that commit would fix?

(I can't see Lars' message yet, so the delays with the gnu.org mail
systems seem to be ongoing...)

bug#51384: 28.0.60; substitute-command-keys finds global binding for undo instead of in specified keymap

[Robert Pluim]

>>>>> On Tue, 20 Sep 2022 14:03:53 -0400, Stefan Kangas <stefankangas@gmail.com> said:
    Stefan> This looks eerily similar to the code I removed here:

    Stefan>     commit ac0027f6a5480bd4739fdf71413a19012f400483
    Stefan>     Author: Stefan Kangas <stefan@marxist.se>
    Stefan>     Date:   Mon Jul 11 17:21:23 2022 +0200

    Stefan>         Remove dead branch from substitute-command-keys

    Stefan>         * lisp/help.el (substitute-command-keys): Remove dead branch;
    Stefan>         where-is-internal will follow any remaps for us.  Note also that the
    Stefan>         test case for remapping still pass.

Similar, but not the same. That dead branch followed remaps, this code
redoes the lookup in the global map.

    Stefan> Do we have a test case that reverting that commit would fix?

No, and I donʼt see why youʼd want to revert it, it seems correct to
me.

    Stefan> (I can't see Lars' message yet, so the delays with the gnu.org mail
    Stefan> systems seem to be ongoing...)

Itʼs not jut gnu.org for me, but maybe thatʼs a local issue.

Anyway, Iʼve pushed the fix as cee9a2cbe0

Robert
-- 

bug#51384: 28.0.60; substitute-command-keys finds global binding for undo instead of in specified keymap

[Stefan Kangas]

Robert Pluim <rpluim@gmail.com> writes:

> Similar, but not the same. That dead branch followed remaps, this code
> redoes the lookup in the global map.
>
>     Stefan> Do we have a test case that reverting that commit would fix?
>
> No, and I donʼt see why youʼd want to revert it, it seems correct to
> me.

Right, thanks for confirming.