master bf47851e08 2/2: Signal error on duplicate key definitions

[Robert Pluim]

branch: master
commit bf47851e0817abcf17eeb4a10d27cf18de2c68a2
Author: Robert Pluim <rpluim@gmail.com>
Commit: Robert Pluim <rpluim@gmail.com>

    Signal error on duplicate key definitions
    
    * lisp/keymap.el (define-keymap, defvar-keymap): Signal error if the
    same key is specified twice.  (Bug#56873)
    
    * doc/lispref/keymaps.texi (Creating Keymaps): Document error
    signaling behaviour.
    
    * test/src/keymap-tests.el (keymap-test-duplicate-definitions): Test
    duplicate definition detection.
---
 doc/lispref/keymaps.texi |  6 ++++--
 lisp/keymap.el           | 16 +++++++++++++++-
 test/src/keymap-tests.el | 12 ++++++++++++
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/doc/lispref/keymaps.texi b/doc/lispref/keymaps.texi
index 5cb5367bc0..2be31d63a6 100644
--- a/doc/lispref/keymaps.texi
+++ b/doc/lispref/keymaps.texi
@@ -374,7 +374,8 @@ number of keys.  Here's a very basic example:
 @end lisp
 
 This function creates a new sparse keymap, defines the keystrokes in
-@var{pairs}, and returns the new keymap.
+@var{pairs}, and returns the new keymap.  It signals an error if there
+are duplicate key bindings in @var{pairs}.
 
 @var{pairs} is a list of alternating key bindings and key definitions,
 as accepted by @code{keymap-set}.  In addition, the key can be the
@@ -438,7 +439,8 @@ variable.  This is what virtually all modes do---a mode 
called
 
 This macro defines @var{name} as a variable, passes @var{options}
 and @var{pairs} to @code{define-keymap}, and uses the result as the
-default value for the variable.
+default value for the variable.  It signals an error if there are
+duplicate key bindings in @var{pairs}.
 
 @var{options} is like the keywords in @code{define-keymap}, but
 there's an additional @code{:doc} keyword that provides the doc
diff --git a/lisp/keymap.el b/lisp/keymap.el
index 376a30f106..107565590c 100644
--- a/lisp/keymap.el
+++ b/lisp/keymap.el
@@ -530,7 +530,8 @@ should be a MENU form as accepted by `easy-menu-define'.
                    (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 @@ should be a MENU form as accepted by `easy-menu-define'.
           (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 @@ as the variable documentation string.
           (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 a/test/src/keymap-tests.el b/test/src/keymap-tests.el
index b0876664ed..ce96be6869 100644
--- a/test/src/keymap-tests.el
+++ b/test/src/keymap-tests.el
@@ -430,6 +430,18 @@ g .. h             foo
   (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