From: Mattias Engdegård <mattiase@acm.org>
Date: Sat, 9 Sep 2023 11:04:54 +0000 (+0200)
Subject: Add byte-compiler warning about useless trailing cond clauses
X-Git-Url: http://git.eshelyaron.com/gitweb/?a=commitdiff_plain;h=c137b5195b633c5c931c35385fdb3e75b9ee5f09;p=emacs.git

Add byte-compiler warning about useless trailing cond clauses

Warn about clauses after the default clause, as in

    (cond ((= x 0) (say "none"))
          (t (say "some"))
          (say "goodbye"))

because they are very much an indicator of a mistake (such as
misplaced brackets), and since they are deleted by the optimiser, any
other warnings there are lost and the user wouldn't know that
something is wrong otherwise.

* lisp/emacs-lisp/macroexp.el (macroexp--expand-all): Add warning.
* etc/NEWS: Announce.
* test/lisp/emacs-lisp/bytecomp-tests.el
(bytecomp-test--with-suppressed-warnings): Add test case.
---

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