[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
master c137b5195b6 2/2: Add byte-compiler warning about useless trailing
From: |
Mattias Engdegård |
Subject: |
master c137b5195b6 2/2: Add byte-compiler warning about useless trailing cond clauses |
Date: |
Sat, 9 Sep 2023 07:29:07 -0400 (EDT) |
branch: master
commit c137b5195b633c5c931c35385fdb3e75b9ee5f09
Author: Mattias Engdegård <mattiase@acm.org>
Commit: Mattias Engdegård <mattiase@acm.org>
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.
---
etc/NEWS | 15 +++++++++++++++
lisp/emacs-lisp/macroexp.el | 29 ++++++++++++++++++++++++++++-
test/lisp/emacs-lisp/bytecomp-tests.el | 9 +++++++++
3 files changed, 52 insertions(+), 1 deletion(-)
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))