bug#64417: 30.0.50; keymap-unset :REMOVE keyword not working

bug#64417: 30.0.50; keymap-unset :REMOVE keyword not working

[German Pacenza]

When I evaluate:
(keymap-unset diff-mode-map "M-SPC" :REMOVE)
M-SPC still runs scroll-up-command

(keymap-unset diff-mode-map "M-SPC")
M-SPC is removed from diff-mode-map and cycle-spacing (global-mode-map)
is activated

This contradicts the keymap-unset docstring:

If REMOVE, remove the binding instead of unsetting it.  This only
makes a difference when there’s a parent keymap.  When unsetting
a key in a child map, it will still shadow the same key in the
parent keymap.  Removing the binding will allow the key in the
parent keymap to be used.



In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.38, cairo version 1.17.8) of 2023-07-01 built on KRONOS
Repository revision: 2195935870ca173f8b16c4821816f77ecb2d96c3
Repository branch: master
System Description: Manjaro Linux

Configured using:
 'configure --with-pgtk --with-xwidgets --with-native-compilation
 --without-libsystemd --without-compress-install
 --prefix=/home/german/.local/emacs'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBXML2 MODULES NATIVE_COMP NOTIFY INOTIFY PDUMPER PGTK PNG
RSVG SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS TREE_SITTER
WEBP XIM XWIDGETS GTK3 ZLIB

Important settings:
  value of $LC_MONETARY: es_AR.UTF-8
  value of $LC_NUMERIC: es_AR.UTF-8
  value of $LC_TIME: es_AR.UTF-8
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  vertico-mode: t
  savehist-mode: t
  popper-mode: t
  minibuffer-depth-indicate-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  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
  window-divider-mode: t
  line-number-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
/home/german/.emacs.d/elpa/embark-consult-0.7/embark-org hides /home/german/.emacs.d/elpa/embark-0.22.1/embark-org
/home/german/.emacs.d/elpa/transient-0.4.1/transient hides /home/german/.local/emacs/share/emacs/30.0.50/lisp/transient

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 time-date mm-decode mm-bodies mm-encode mail-parse
rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045
ietf-drums mm-util mail-prsvr mail-utils orderless find-func shortdoc
text-property-search thingatpt help-fns radix-tree comp comp-cstr rx
vertico savehist popper subr-x transient cl-extra help-mode cl-seq
format-spec edmacro kmacro eieio byte-opt bytecomp byte-compile
eieio-core cl-macs gv compat mb-depth g3r-light-theme info
doom-themes-autoloads ef-themes-autoloads embark-consult-autoloads
consult-autoloads embark-autoloads helpful-autoloads
elisp-refs-autoloads f-autoloads magit-autoloads git-commit-autoloads
magit-section-autoloads dash-autoloads nushell-mode-autoloads
orderless-autoloads popper-autoloads rainbow-mode-autoloads s-autoloads
transient-autoloads vertico-autoloads with-editor-autoloads
compat-autoloads yuck-mode-autoloads warnings icons cl-loaddefs cl-lib
rmc iso-transl tooltip cconv eldoc paren electric uniquify ediff-hook
vc-hooks lisp-float-type elisp-mode mwheel term/pgtk-win pgtk-win
term/common-win pgtk-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 theme-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 xwidget-internal dbusbind inotify dynamic-setting
system-font-setting font-render-setting cairo gtk pgtk lcms2 multi-tty
make-network-process native-compile emacs)

Memory information:
((conses 16 129895 170977) (symbols 48 9654 10)
 (strings 32 34252 11318) (string-bytes 1 1016743) (vectors 16 19874)
 (vector-slots 8 402554 139039) (floats 8 112 219)
 (intervals 56 733 184) (buffers 984 13))

-- 
German Pacenza

bug#64417: 30.0.50; keymap-unset :REMOVE keyword not working

[Eli Zaretskii]

> From: German Pacenza <germanp82@hotmail.com>
> Date: Sun, 02 Jul 2023 08:46:28 -0300
> 
> 
> When I evaluate:
> (keymap-unset diff-mode-map "M-SPC" :REMOVE)
> M-SPC still runs scroll-up-command
> 
> (keymap-unset diff-mode-map "M-SPC")
> M-SPC is removed from diff-mode-map and cycle-spacing (global-mode-map)
> is activated
> 
> This contradicts the keymap-unset docstring:
> 
> If REMOVE, remove the binding instead of unsetting it.  This only
> makes a difference when there’s a parent keymap.  When unsetting
> a key in a child map, it will still shadow the same key in the
> parent keymap.  Removing the binding will allow the key in the
> parent keymap to be used.

But:

  (keymap-parent diff-mode-map) => nil

bug#64417: 30.0.50; keymap-unset :REMOVE keyword not working

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

> When I evaluate:
> (keymap-unset diff-mode-map "M-SPC" :REMOVE)
> M-SPC still runs scroll-up-command
>
> (keymap-unset diff-mode-map "M-SPC")
> M-SPC is removed from diff-mode-map and cycle-spacing (global-mode-map)
> is activated
>
> This contradicts the keymap-unset docstring:
>
> If REMOVE, remove the binding instead of unsetting it.  This only
> makes a difference when there’s a parent keymap.  When unsetting
> a key in a child map, it will still shadow the same key in the
> parent keymap.  Removing the binding will allow the key in the
> parent keymap to be used.

Thanks for providing a great example of why the `remove` arg of
`keymap-unset` is a problematic feature :-)

The technical reason for the above: while `diff-mode-map` has no parent,
the keymap bound to ESC in `diff-mode-map` has `diff-mode-shared-map`
as parent, and `M-SPC` is the same (for keymap purposes) as `ESC SPC`.
Of curse, there's no binding of anything to `scroll-up-command` itself
in `diff-mode-shared-map` itself: the binding is to be found in
`diff-mode-shared-map`s parent, `special-mode-map`.

So we can't *remove* the binding from `M-SPC` to `scroll-up-command`
without modifying some other keymap.

So, the code is doing The Right Thing :-(


        Stefan

bug#64417: 30.0.50; keymap-unset :REMOVE keyword not working

[German Pacenza]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Thanks for providing a great example of why the `remove` arg of
> `keymap-unset` is a problematic feature :-)
>
> The technical reason for the above: while `diff-mode-map` has no parent,
> the keymap bound to ESC in `diff-mode-map` has `diff-mode-shared-map`
> as parent, and `M-SPC` is the same (for keymap purposes) as `ESC SPC`.
> Of curse, there's no binding of anything to `scroll-up-command` itself
> in `diff-mode-shared-map` itself: the binding is to be found in
> `diff-mode-shared-map`s parent, `special-mode-map`.
>
> So we can't *remove* the binding from `M-SPC` to `scroll-up-command`
> without modifying some other keymap.
>
> So, the code is doing The Right Thing :-(
>
>
>         Stefan
>

I added that line to my init in October last year and it worked fine. I
git bisect and found the commit that broke it:

commit bb3e0ded9eba71596b34806b302d63977259c3dd
Author: Robert Pluim <rpluim@gmail.com>
Date:   Fri Mar 17 09:50:38 2023 +0100

    Don't add a key binding when REMOVE is non-nil
    
    * src/keymap.c (store_in_keymap): Don't add a nil keybinding if we've
    been asked to remove a non-existent binding.  (Bug#62207)

What I do is setting M-SPC in global-map to 'execute-extended-command'
That gets shadowed by diff-mode-map to run 'scroll-up-command', so I use
keymap-unset with :REMOVE to remove it.
Currently keymap-unset removes the binding _without_ :REMOVE and doesn't
do anything _with_ :REMOVE.

-- 
German Pacenza

bug#64417: 30.0.50; keymap-unset :REMOVE keyword not working

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

>     Don't add a key binding when REMOVE is non-nil
>     
>     * src/keymap.c (store_in_keymap): Don't add a nil keybinding if we've
>     been asked to remove a non-existent binding.  (Bug#62207)
>
> What I do is setting M-SPC in global-map to 'execute-extended-command'
> That gets shadowed by diff-mode-map to run 'scroll-up-command', so I use
> keymap-unset with :REMOVE to remove it.

`remove` is for the case where you want to unhide the keybinding of
a *parent* map (that's an unusual use-case).  Here OTOH you want to
unhide the keybinding of a lower-precedence map, so you shouldn't use
`remove`.

> Currently keymap-unset removes the binding _without_ :REMOVE and doesn't
> do anything _with_ :REMOVE.

If it hurts, don't do that :-)


        Stefan

bug#64417: 30.0.50; keymap-unset :REMOVE keyword not working

[German Pacenza]

> `remove` is for the case where you want to unhide the keybinding of
> a *parent* map (that's an unusual use-case).  Here OTOH you want to
> unhide the keybinding of a lower-precedence map, so you shouldn't use
> `remove`.

Ok. That makes sense, I thought global-mode was the parent keymap.

>> Currently keymap-unset removes the binding _without_ :REMOVE and doesn't
>> do anything _with_ :REMOVE.
>
> If it hurts, don't do that :-)

Ok, thanks for the info.

-- 
German Pacenza

bug#64417: 30.0.50; keymap-unset :REMOVE keyword not working

[Stefan Kangas]

German Pacenza <germanp82@hotmail.com> writes:

>> `remove` is for the case where you want to unhide the keybinding of
>> a *parent* map (that's an unusual use-case).  Here OTOH you want to
>> unhide the keybinding of a lower-precedence map, so you shouldn't use
>> `remove`.
>
> Ok. That makes sense, I thought global-mode was the parent keymap.
>
>>> Currently keymap-unset removes the binding _without_ :REMOVE and doesn't
>>> do anything _with_ :REMOVE.
>>
>> If it hurts, don't do that :-)
>
> Ok, thanks for the info.

So it seems like there is no bug here.  Closing.