* bug#56873: Make `defvar-keymap' warn on conflicting and redundant bindings
@ 2022-08-01 16:47 Stefan Kangas
2022-08-02 8:25 ` Robert Pluim
0 siblings, 1 reply; 9+ messages in thread
From: Stefan Kangas @ 2022-08-01 16:47 UTC (permalink / raw)
To: 56873
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)
^ permalink raw reply [flat|nested] 9+ messages in thread* bug#56873: Make `defvar-keymap' warn on conflicting and redundant bindings 2022-08-01 16:47 bug#56873: Make `defvar-keymap' warn on conflicting and redundant bindings Stefan Kangas @ 2022-08-02 8:25 ` Robert Pluim 2022-08-02 9:48 ` Lars Ingebrigtsen 2022-08-02 10:09 ` Stefan Kangas 0 siblings, 2 replies; 9+ messages in thread From: Robert Pluim @ 2022-08-02 8:25 UTC (permalink / raw) To: Stefan Kangas; +Cc: 56873 >>>>> 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 -- ^ permalink raw reply related [flat|nested] 9+ messages in thread
* bug#56873: Make `defvar-keymap' warn on conflicting and redundant bindings 2022-08-02 8:25 ` Robert Pluim @ 2022-08-02 9:48 ` Lars Ingebrigtsen 2022-08-02 15:46 ` Drew Adams 2022-08-02 10:09 ` Stefan Kangas 1 sibling, 1 reply; 9+ messages in thread From: Lars Ingebrigtsen @ 2022-08-02 9:48 UTC (permalink / raw) To: Robert Pluim; +Cc: Stefan Kangas, 56873 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#56873: Make `defvar-keymap' warn on conflicting and redundant bindings 2022-08-02 9:48 ` Lars Ingebrigtsen @ 2022-08-02 15:46 ` Drew Adams 0 siblings, 0 replies; 9+ messages in thread From: Drew Adams @ 2022-08-02 15:46 UTC (permalink / raw) To: Lars Ingebrigtsen, Robert Pluim; +Cc: Stefan Kangas, 56873@debbugs.gnu.org > > 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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#56873: Make `defvar-keymap' warn on conflicting and redundant bindings 2022-08-02 8:25 ` Robert Pluim 2022-08-02 9:48 ` Lars Ingebrigtsen @ 2022-08-02 10:09 ` Stefan Kangas 2022-08-02 11:53 ` Robert Pluim 1 sibling, 1 reply; 9+ messages in thread From: Stefan Kangas @ 2022-08-02 10:09 UTC (permalink / raw) To: Robert Pluim; +Cc: 56873 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.) ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#56873: Make `defvar-keymap' warn on conflicting and redundant bindings 2022-08-02 10:09 ` Stefan Kangas @ 2022-08-02 11:53 ` Robert Pluim 2022-08-02 12:01 ` Lars Ingebrigtsen 2022-08-02 12:17 ` Stefan Kangas 0 siblings, 2 replies; 9+ messages in thread From: Robert Pluim @ 2022-08-02 11:53 UTC (permalink / raw) To: Stefan Kangas; +Cc: Lars Ingebrigtsen, 56873 [-- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* bug#56873: Make `defvar-keymap' warn on conflicting and redundant bindings 2022-08-02 11:53 ` Robert Pluim @ 2022-08-02 12:01 ` Lars Ingebrigtsen 2022-08-02 12:33 ` Robert Pluim 2022-08-02 12:17 ` Stefan Kangas 1 sibling, 1 reply; 9+ messages in thread From: Lars Ingebrigtsen @ 2022-08-02 12:01 UTC (permalink / raw) To: Robert Pluim; +Cc: Stefan Kangas, 56873 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#56873: Make `defvar-keymap' warn on conflicting and redundant bindings 2022-08-02 12:01 ` Lars Ingebrigtsen @ 2022-08-02 12:33 ` Robert Pluim 0 siblings, 0 replies; 9+ messages in thread From: Robert Pluim @ 2022-08-02 12:33 UTC (permalink / raw) To: Lars Ingebrigtsen; +Cc: Stefan Kangas, 56873 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#56873: Make `defvar-keymap' warn on conflicting and redundant bindings 2022-08-02 11:53 ` Robert Pluim 2022-08-02 12:01 ` Lars Ingebrigtsen @ 2022-08-02 12:17 ` Stefan Kangas 1 sibling, 0 replies; 9+ messages in thread From: Stefan Kangas @ 2022-08-02 12:17 UTC (permalink / raw) To: Robert Pluim; +Cc: Lars Ingebrigtsen, 56873 Robert Pluim <rpluim@gmail.com> writes: > The detection looks like this: LGTM. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-08-02 15:46 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-01 16:47 bug#56873: Make `defvar-keymap' warn on conflicting and redundant bindings Stefan Kangas 2022-08-02 8:25 ` Robert Pluim 2022-08-02 9:48 ` Lars Ingebrigtsen 2022-08-02 15:46 ` Drew Adams 2022-08-02 10:09 ` Stefan Kangas 2022-08-02 11:53 ` Robert Pluim 2022-08-02 12:01 ` Lars Ingebrigtsen 2022-08-02 12:33 ` Robert Pluim 2022-08-02 12:17 ` Stefan Kangas
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).