bug#56873: Make `defvar-keymap' warn on conflicting and redundant bindings

[Robert Pluim]

>>>>> On Tue, 2 Aug 2022 10:09:43 +0000, Stefan Kangas <stefan@marxist.se> said:

    Stefan> Robert Pluim <rpluim@gmail.com> writes:
    Stefan> (defvar-keymap foo
    Stefan> "a" #'next-line
    Stefan> "a" #'previous-line)
    >> 
    >> Is that a common occurence?

    Stefan> I don't know.  I've only seen such a mistake once, but flagging it 
might
    Stefan> help us find more.

There are in fact 4 instances in Emacsʼ sources.

    Stefan> Your patch looks fine to me, but I agree with Lars that it should 
be an
    Stefan> error instead of a warning.  (And putting it in `define-keymap' is
    Stefan> indeed better.)

I put it in both, and it turns out we have errors in both, which I
propose to fix like this (this preserves current behaviour, due to the
'last definition wins' nature of define-keymap). I decided that for
coherence, the gnus-summary-up-thread binding should go as well (itʼs
available on "T-u" anyway).

diff --git c/lisp/gnus/gnus-srvr.el i/lisp/gnus/gnus-srvr.el
index a520bfcd8b..54be0f8e6a 100644
--- c/lisp/gnus/gnus-srvr.el
+++ i/lisp/gnus/gnus-srvr.el
@@ -699,7 +699,6 @@ gnus-browse-mode-map
   "n" #'gnus-browse-next-group
   "p" #'gnus-browse-prev-group
   "DEL" #'gnus-browse-prev-group
-  "<delete>" #'gnus-browse-prev-group
   "N" #'gnus-browse-next-group
   "P" #'gnus-browse-prev-group
   "M-n" #'gnus-browse-next-group
diff --git c/lisp/gnus/gnus-sum.el i/lisp/gnus/gnus-sum.el
index bf2a083fec..90b57695c5 100644
--- c/lisp/gnus/gnus-sum.el
+++ i/lisp/gnus/gnus-sum.el
@@ -1958,8 +1958,6 @@ :keymap
   "C-M-b" #'gnus-summary-prev-thread
   "M-<down>" #'gnus-summary-next-thread
   "M-<up>" #'gnus-summary-prev-thread
-  "C-M-u" #'gnus-summary-up-thread
-  "C-M-d" #'gnus-summary-down-thread
   "&" #'gnus-summary-execute-command
   "c" #'gnus-summary-catchup-and-exit
   "C-w" #'gnus-summary-mark-region-as-read
diff --git c/lisp/ibuffer.el i/lisp/ibuffer.el
index 742d21d0b0..65430d7d11 100644
--- c/lisp/ibuffer.el
+++ i/lisp/ibuffer.el
@@ -447,7 +447,6 @@ ibuffer-mode-map
 
   "d"           #'ibuffer-mark-for-delete
   "C-d"         #'ibuffer-mark-for-delete-backwards
-  "k"           #'ibuffer-mark-for-delete
   "x"           #'ibuffer-do-kill-on-deletion-marks
 
   ;; immediate operations
diff --git c/lisp/wdired.el i/lisp/wdired.el
index a5858ed190..106d57174d 100644
--- c/lisp/wdired.el
+++ i/lisp/wdired.el
@@ -902,7 +902,6 @@ wdired-perm-mode-map
   "x"   #'wdired-set-bit
   "-"   #'wdired-set-bit
   "S"   #'wdired-set-bit
-  "s"   #'wdired-set-bit
   "T"   #'wdired-set-bit
   "t"   #'wdired-set-bit
   "s"   #'wdired-set-bit

The detection looks like this:

diff --git i/lisp/keymap.el w/lisp/keymap.el
index 376a30f106..107565590c 100644
--- i/lisp/keymap.el
+++ w/lisp/keymap.el
@@ -530,7 +530,8 @@ define-keymap
                    (keymap keymap)
                    (prefix (define-prefix-command prefix nil name))
                    (full (make-keymap name))
-                   (t (make-sparse-keymap name)))))
+                   (t (make-sparse-keymap name))))
+          seen-keys)
       (when suppress
         (suppress-keymap keymap (eq suppress 'nodigits)))
       (when parent
@@ -544,6 +545,9 @@ define-keymap
           (let ((def (pop definitions)))
             (if (eq key :menu)
                 (easy-menu-define nil keymap "" def)
+              (if (member key seen-keys)
+                  (error "Duplicate definition for key: %S %s" key keymap)
+                (push key seen-keys))
               (keymap-set keymap key def)))))
       keymap)))
 
@@ -571,6 +575,16 @@ defvar-keymap
           (push (pop defs) opts))))
     (unless (zerop (% (length defs) 2))
       (error "Uneven number of key/definition pairs: %s" defs))
+    (let ((defs defs)
+          key seen-keys)
+      (while defs
+        (setq key (pop defs))
+        (pop defs)
+        (when (not (eq key :menu))
+          (if (member key seen-keys)
+              (error "Duplicate definition for key '%s' in keymap '%s'"
+                     key variable-name)
+            (push key seen-keys)))))
     `(defvar ,variable-name
        (define-keymap ,@(nreverse opts) ,@defs)
        ,@(and doc (list doc)))))
diff --git i/test/src/keymap-tests.el w/test/src/keymap-tests.el
index b0876664ed..ce96be6869 100644
--- i/test/src/keymap-tests.el
+++ w/test/src/keymap-tests.el
@@ -430,6 +430,18 @@ test-non-key-events
   (make-non-key-event 'keymap-tests-event)
   (should (equal (where-is-internal 'keymap-tests-command) '([3 103]))))
 
+(ert-deftest keymap-test-duplicate-definitions ()
+  "Check that defvar-keymap rejects duplicate key definitions."
+  (should-error
+   (defvar-keymap
+       ert-keymap-duplicate
+       "a" #'next-line
+       "a" #'previous-line))
+  (should-error
+   (define-keymap
+       "a" #'next-line
+       "a" #'previous-line)))
+
 (provide 'keymap-tests)
 
 ;;; keymap-tests.el ends here