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

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

[Stefan Kangas]

Severity: wishlist

It would be useful if `defvar-keymap' could warn on conflicting
bindings, such as in:

    (defvar-keymap foo
      "a" #'next-line
      "a" #'previous-line)

It would also be useful to warn about redundant bindings, such as in:

    (defvar-keymap foo
      "a" #'next-line
      "a" #'next-line)

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

[Robert Pluim]

>>>>> On Mon, 1 Aug 2022 16:47:10 +0000, Stefan Kangas <stefan@marxist.se> said:

    Stefan> Severity: wishlist
    Stefan> It would be useful if `defvar-keymap' could warn on conflicting
    Stefan> bindings, such as in:

    Stefan>     (defvar-keymap foo
    Stefan>       "a" #'next-line
    Stefan>       "a" #'previous-line)

Is that a common occurence?

    Stefan> It would also be useful to warn about redundant bindings, such as in:

    Stefan>     (defvar-keymap foo
    Stefan>       "a" #'next-line
    Stefan>       "a" #'next-line)

Thatʼs just a special case of conflicting bindings. This will do it,
but I wonder if `warn' is overkill. I put it in `define-keymap', but
it could equally well go in `defvar-keymap'.

diff --git a/lisp/keymap.el b/lisp/keymap.el
index 376a30f106..b44a961d73 100644
--- a/lisp/keymap.el
+++ b/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)
+                  (warn "Duplicate definition for key: %S" key)
+                (push key seen-keys))
               (keymap-set keymap key def)))))
       keymap)))
 

Robert
-- 

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

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> Thatʼs just a special case of conflicting bindings. This will do it,
> but I wonder if `warn' is overkill.

I think it should signal an error.

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

[Stefan Kangas]

Robert Pluim <rpluim@gmail.com> writes:

>     Stefan>     (defvar-keymap foo
>     Stefan>       "a" #'next-line
>     Stefan>       "a" #'previous-line)
>
> Is that a common occurence?

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

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

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

[Robert Pluim]

[-- Attachment #1: Type: text/plain, Size: 960 bytes --]

>>>>> 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).


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: duplicate-definitions.patch --]
[-- Type: text/x-diff, Size: 1658 bytes --]

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

[-- Attachment #3: Type: text/plain, Size: 33 bytes --]


The detection looks like this:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: keymap-duplicate-detection.patch --]
[-- Type: text/x-diff, Size: 2266 bytes --]

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

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

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> 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).

Looks good to me; please go ahead and push.

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

[Stefan Kangas]

Robert Pluim <rpluim@gmail.com> writes:

> The detection looks like this:

LGTM.

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

[Robert Pluim]

tags 56873 fixed
close 56873 29.1
quit

>>>>> On Tue, 02 Aug 2022 14:01:18 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:

    Lars> Robert Pluim <rpluim@gmail.com> writes:
    >> 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).

    Lars> Looks good to me; please go ahead and push.

Closing.
Committed as bf47851e08

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

[Drew Adams]

> > but I wonder if `warn' is overkill.
> I think it should signal an error.

Why?  I don't understand what the "error" is.
A warning is one thing - lets you know about
a possible typo or logic mistake.  But an error?

Will you raise an error for these also?  Why not?

(setq a nil
      a t)

(setq a 42
      a 42)

What about generated code?  Is there something
_necessarily wrong_ with the code you want to
raise an error for?  Where's the error beef?