Subject: bug#76700: 31.0.50; find-func.el: incorrect use of minor mode

From: Madhu <[email-masked]>
Date: Mon, 03 Mar 2025 08:36:32 +0530 (IST)

I brought up this issue on emacs-devel but the discussion did not
proceed beyond
https://lists.gnu.org/archive/html/emacs-devel/2025-02/msg01054.html

for decades find-func.el had used `find-function-setup-keys' to set up
global bindings to functions in find-func.el.  Since these are global
bindings these did not require a minor mode.

The minor-mode was introduced in c3e989ca9d7, "New minor mode
find-function-mode replaces find-function-setup-keys" which introduced
a keymap via new features.

This was objected to on the emacs-devel list by Eshel in
+message-id:<[email-masked]> (sorry lists.gnu.org
namazu apparently websearch by mid does not work or provide 
a link to the html mailing list) on "Fri, 04 Oct 2024 11:56:17 +0200"

and the commit 1094c3f914 "find-function-mode: Define keys at a low
precedence level" was introduced to restore "backward compatibility".

However this is an incorrect use of define-minor-mode, The use of
define-minor-mode is to explicitly use a keymap to shadow global
bindings and using it to modify global bindings is a basic category
error a thinko and should not be promoted in the code base.

as the functions in find-func.el are always available for binding, a
minor mode does not make sense, if only to mutate global bindings.

global bindings set by the user when the mode is in effect are undone
when the mode is removed. Emacs should not be doing this.

I believe The original function find-function-setup-keys' introduced
by rms explicitly avoided using a minor mode precisely for these reasons. Introducing a minor mode is a conceptual regression.

If a local keymap is not desired, the minor mode should be removed and
the original function undeprecated.

The attached patch however restores the local keymap but sets up global
bindings via the original find-function-setup-keys.
>From 1279279580f7f15e63da9624cdefa4c842f8b24f Mon Sep 17 00:00:00 2001
From: Madhu <[email-masked]>
Date: Mon, 24 Feb 2025 07:48:54 +0530
Subject: [PATCH] lisp/emacs-lisp/find-func.el: restore local keymap

* lisp/emacs-lisp/find-func.el: find-function-mode-map. (restored from
commit 1094c3f9147), (find-function-setup-keys), restored from commit
c3e989ca9, but modified to operate on global-keymap to maintain
compatibility).

 commit message 1094c9147 uses "low precedence level" instead of "global"
---
 lisp/emacs-lisp/find-func.el | 60 +++++++++++++++++++++---------------
 1 file changed, 36 insertions(+), 24 deletions(-)

diff --git a/lisp/emacs-lisp/find-func.el b/lisp/emacs-lisp/find-func.el
index 6b7b5083620..b08b8c41aa2 100644
--- a/lisp/emacs-lisp/find-func.el
+++ b/lisp/emacs-lisp/find-func.el
@@ -812,36 +812,48 @@ find-variable-at-point
     (when (and symb (not (equal symb 0)))
       (find-variable-other-window symb))))
 
+(defvar-keymap find-function-mode-map
+  "C-x F"   #'find-function
+  "C-x 4 F" #'find-function-other-window
+  "C-x 5 F" #'find-function-other-frame
+
+  "C-x K"   #'find-function-on-key
+  "C-x 4 K" #'find-function-on-key-other-window
+  "C-x 5 K" #'find-function-on-key-other-frame
+
+  "C-x V"   #'find-variable
+  "C-x 4 V" #'find-variable-other-window
+  "C-x 5 V" #'find-variable-other-frame
+
+  "C-x L"   #'find-library
+  "C-x 4 L" #'find-library-other-window
+  "C-x 5 L" #'find-library-other-frame)
+
 ;;;###autoload
 (define-minor-mode find-function-mode
   "Enable some key bindings for the `find-function' family of functions."
-  :group 'find-function :version "31.1" :global t :lighter nil
-  ;; For compatibility with the historical behavior of the old
-  ;; `find-function-setup-keys', define our bindings at the precedence
-  ;; level of the global map.
-  :keymap nil
-  (pcase-dolist (`(,map ,key ,cmd)
-                 `((,ctl-x-map   "F" find-function)
-                   (,ctl-x-4-map "F" find-function-other-window)
-                   (,ctl-x-5-map "F" find-function-other-frame)
-                   (,ctl-x-map   "K" find-function-on-key)
-                   (,ctl-x-4-map "K" find-function-on-key-other-window)
-                   (,ctl-x-5-map "K" find-function-on-key-other-frame)
-                   (,ctl-x-map   "V" find-variable)
-                   (,ctl-x-4-map "V" find-variable-other-window)
-                   (,ctl-x-5-map "V" find-variable-other-frame)
-                   (,ctl-x-map   "L" find-library)
-                   (,ctl-x-4-map "L" find-library-other-window)
-                   (,ctl-x-5-map "L" find-library-other-frame)))
-    (if find-function-mode
-        (keymap-set map key cmd)
-      (keymap-unset map key t))))
+  :global t :lighter nil   ; compat. with old `find-function-setup-keys'
+  :group 'find-function :version "31.1")
 
 ;;;###autoload
 (defun find-function-setup-keys ()
-  "Turn on `find-function-mode', which see."
-  (find-function-mode 1))
-(make-obsolete 'find-function-setup-keys 'find-function-mode "31.1")
+  "Define some key bindings for the `find-function' family of functions.
+and also define the keys globally"
+  (find-function-mode 1)
+  (define-key ctl-x-map "F" 'find-function)
+  (define-key ctl-x-4-map "F" 'find-function-other-window)
+  (define-key ctl-x-5-map "F" 'find-function-other-frame)
+  (define-key ctl-x-map "K" 'find-function-on-key)
+  (define-key ctl-x-4-map "K" 'find-function-on-key-other-window)
+  (define-key ctl-x-5-map "K" 'find-function-on-key-other-frame)
+  (define-key ctl-x-map "V" 'find-variable)
+  (define-key ctl-x-4-map "V" 'find-variable-other-window)
+  (define-key ctl-x-5-map "V" 'find-variable-other-frame)
+  (define-key ctl-x-map "L" 'find-library)
+  (define-key ctl-x-4-map "L" 'find-library-other-window)
+  (define-key ctl-x-5-map "L" 'find-library-other-frame))
+
+;;(make-obsolete 'find-function-setup-keys 'find-function-mode "31.1")
 
 (provide 'find-func)
 
-- 
2.46.0.27.gfa3b914457