GNU bug report logs - #56873
Make `defvar-keymap' warn on conflicting and redundant bindings

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 56873 in the body.
You can then email your comments to 56873 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: bug-gnu-emacs <at> gnu.org
Subject: Make `defvar-keymap' warn on conflicting and redundant bindings
Date: Mon, 1 Aug 2022 16:47:10 +0000

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)

Message #8 received at 56873 <at> debbugs.gnu.org (full text, mbox):

From: Robert Pluim <rpluim <at> gmail.com>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 56873 <at> debbugs.gnu.org
Subject: Re: bug#56873: Make `defvar-keymap' warn on conflicting and
 redundant bindings
Date: Tue, 02 Aug 2022 10:25:46 +0200

>>>>> On Mon, 1 Aug 2022 16:47:10 +0000, Stefan Kangas <stefan <at> 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
-- 

Message #11 received at 56873 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: Stefan Kangas <stefan <at> marxist.se>, 56873 <at> debbugs.gnu.org
Subject: Re: bug#56873: Make `defvar-keymap' warn on conflicting and
 redundant bindings
Date: Tue, 02 Aug 2022 11:48:09 +0200

Robert Pluim <rpluim <at> 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.

Message #14 received at 56873 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: 56873 <at> debbugs.gnu.org
Subject: Re: bug#56873: Make `defvar-keymap' warn on conflicting and redundant
 bindings
Date: Tue, 2 Aug 2022 10:09:43 +0000

Robert Pluim <rpluim <at> 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.)

Message #19 received at 56873 <at> debbugs.gnu.org (full text, mbox):

From: Robert Pluim <rpluim <at> gmail.com>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 56873 <at> debbugs.gnu.org
Subject: Re: bug#56873: Make `defvar-keymap' warn on conflicting and
 redundant bindings
Date: Tue, 02 Aug 2022 13:53:32 +0200

[Message part 1 (text/plain, inline)]

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

    Stefan> Robert Pluim <rpluim <at> 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).

[duplicate-definitions.patch (text/x-diff, inline)]

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

[Message part 3 (text/plain, inline)]

The detection looks like this:

[keymap-duplicate-detection.patch (text/x-diff, inline)]

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

Message #22 received at 56873 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: Stefan Kangas <stefan <at> marxist.se>, 56873 <at> debbugs.gnu.org
Subject: Re: bug#56873: Make `defvar-keymap' warn on conflicting and
 redundant bindings
Date: Tue, 02 Aug 2022 14:01:18 +0200

Robert Pluim <rpluim <at> 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.

Message #25 received at 56873 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 56873 <at> debbugs.gnu.org
Subject: Re: bug#56873: Make `defvar-keymap' warn on conflicting and redundant
 bindings
Date: Tue, 2 Aug 2022 12:17:34 +0000

Robert Pluim <rpluim <at> gmail.com> writes:

> The detection looks like this:

LGTM.

Message #28 received at 56873 <at> debbugs.gnu.org (full text, mbox):

From: Robert Pluim <rpluim <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Stefan Kangas <stefan <at> marxist.se>, 56873 <at> debbugs.gnu.org
Subject: Re: bug#56873: Make `defvar-keymap' warn on conflicting and
 redundant bindings
Date: Tue, 02 Aug 2022 14:33:30 +0200

tags 56873 fixed
close 56873 29.1
quit

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

    Lars> Robert Pluim <rpluim <at> 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

Message #35 received at 56873 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>, Robert Pluim <rpluim <at> gmail.com>
Cc: Stefan Kangas <stefan <at> marxist.se>,
 "56873 <at> debbugs.gnu.org" <56873 <at> debbugs.gnu.org>
Subject: RE: [External] : bug#56873: Make `defvar-keymap' warn on conflicting
 and redundant bindings
Date: Tue, 2 Aug 2022 15:46:09 +0000

> > 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?

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.