diff --git a/etc/NEWS b/etc/NEWS index f6be603294e..51e89fc96dd 100644 --- a/etc/NEWS +++ b/etc/NEWS @@ -1084,6 +1084,21 @@ simplified away. This warning can be suppressed using 'with-suppressed-warnings' with the warning name 'suspicious'. +--- +*** Warn about useless trailing 'cond' clauses. +The compiler now warns when a 'cond' form contains clauses following a +default (unconditional) clause. Example: + + (cond ((= x 0) (say "none")) + (t (say "some")) + (say "goodbye")) + +Such a clause will never be executed but is likely to be a mistake, +perhaps due to misplaced brackets. + +This warning can be suppressed using 'with-suppressed-warnings' with +the warning name 'suspicious'. + --- *** Warn about mutation of constant values. The compiler now warns about code that modifies program constants in diff --git a/lisp/emacs-lisp/macroexp.el b/lisp/emacs-lisp/macroexp.el index 6d5cf8723f9..b21f0f0d47f 100644 --- a/lisp/emacs-lisp/macroexp.el +++ b/lisp/emacs-lisp/macroexp.el @@ -330,7 +330,34 @@ Assumes the caller has bound `macroexpand-all-environment'." (let ((fn (car-safe form))) (pcase form (`(cond . ,clauses) - (macroexp--cons fn (macroexp--all-clauses clauses) form)) + ;; Check for rubbish clauses at the end before macro-expansion, + ;; to avoid nuisance warnings from clauses that become + ;; unconditional through that process. + ;; FIXME: this strategy is defeated by forced `macroexpand-all', + ;; such as in `cl-flet'. Haven't seen that in the wild, though. + (let ((default-tail nil) + (n 0) + (rest clauses)) + (while rest + (let ((c (car-safe (car rest)))) + (when (cond ((consp c) (and (memq (car c) '(quote function)) + (cadr c))) + ((symbolp c) (or (eq c t) (keywordp c))) + (t t)) + ;; This is unquestionably a default clause. + (setq default-tail (cdr rest)) + (setq clauses (take (1+ n) clauses)) ; trim the tail + (setq rest nil))) + (setq n (1+ n)) + (setq rest (cdr rest))) + (let ((expanded-form + (macroexp--cons fn (macroexp--all-clauses clauses) form))) + (if default-tail + (macroexp-warn-and-return + (format-message + "Useless clause following default `cond' clause") + expanded-form '(suspicious cond) t default-tail) + expanded-form)))) (`(condition-case . ,(or `(,err ,body . ,handlers) pcase--dontcare)) (let ((exp-body (macroexp--expand-all body))) (if handlers diff --git a/test/lisp/emacs-lisp/bytecomp-tests.el b/test/lisp/emacs-lisp/bytecomp-tests.el index 8bc8a51b04e..cd1bda3ec55 100644 --- a/test/lisp/emacs-lisp/bytecomp-tests.el +++ b/test/lisp/emacs-lisp/bytecomp-tests.el @@ -1512,6 +1512,15 @@ literals (Bug#20852)." '((suspicious unwind-protect)) "Warning: `unwind-protect' without unwind forms") + (test-suppression + '(defun zot (x) + (cond + ((zerop x) 'zero) + (t 'nonzero) + (happy puppy))) + '((suspicious cond)) + "Warning: Useless clause following default `cond' clause") + (test-suppression '(defun zot () (let ((_ 1))