master 7c63b632e4: Add empty-body warning for when, unless etc

[Mattias Engdegård]

branch: master
commit 7c63b632e4e2241a28f08015cc981a72e18d7867
Author: Mattias Engdegård <mattiase@acm.org>
Commit: Mattias Engdegård <mattiase@acm.org>

    Add empty-body warning for when, unless etc
    
    Warn about code like (when SOME-CONDITION) because these may indicate
    bugs.  Warnings currently apply to `when`, `unless`, `ignore-error`,
    `with-suppressed-warnings` and (as before) `let` and `let*`.
    
    * lisp/emacs-lisp/byte-run.el (with-suppressed-warnings):
    Update doc string.
    * lisp/emacs-lisp/bytecomp.el: (byte-compile-warning-types)
    (byte-compile-warnings): Add empty-body.
    (byte-compile-initial-macro-environment):
    Add empty-body warning for with-suppressed-warnings.
    * lisp/emacs-lisp/macroexp.el (macroexp--expand-all):
    Use the empty-body category for let and let*.
    * lisp/subr.el (when, unless, ignore-error): Add empty-body warning.
    * test/lisp/emacs-lisp/bytecomp-tests.el
    (bytecomp-test--with-suppressed-warnings): Add test cases.
---
 lisp/emacs-lisp/byte-run.el            |  4 +--
 lisp/emacs-lisp/bytecomp.el            | 26 ++++++++++++--------
 lisp/emacs-lisp/macroexp.el            |  2 +-
 lisp/subr.el                           | 31 +++++++++++++++--------
 test/lisp/emacs-lisp/bytecomp-tests.el | 45 +++++++++++++++++++++++++++++++++-
 5 files changed, 84 insertions(+), 24 deletions(-)

diff --git a/lisp/emacs-lisp/byte-run.el b/lisp/emacs-lisp/byte-run.el
index b5e887db83..d909395e97 100644
--- a/lisp/emacs-lisp/byte-run.el
+++ b/lisp/emacs-lisp/byte-run.el
@@ -649,8 +649,8 @@ in `byte-compile-warning-types'; see the variable
 `byte-compile-warnings' for a fuller explanation of the warning
 types.  The types that can be suppressed with this macro are
 `free-vars', `callargs', `redefine', `obsolete',
-`interactive-only', `lexical', `mapcar', `constants' and
-`suspicious'.
+`interactive-only', `lexical', `mapcar', `constants',
+`suspicious' and `empty-body'.
 
 For the `mapcar' case, only the `mapcar' function can be used in
 the symbol list.  For `suspicious', only `set-buffer', `lsh' and `eq'
diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index 1a48897739..a41e076f9b 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -295,7 +295,8 @@ The information is logged to `byte-compile-log-buffer'."
   '(redefine callargs free-vars unresolved
              obsolete noruntime interactive-only
              make-local mapcar constants suspicious lexical lexical-dynamic
-             docstrings docstrings-non-ascii-quotes not-unused)
+             docstrings docstrings-non-ascii-quotes not-unused
+             empty-body)
   "The list of warning types used when `byte-compile-warnings' is t.")
 (defcustom byte-compile-warnings t
   "List of warnings that the byte-compiler should issue (t for almost all).
@@ -326,6 +327,7 @@ Elements of the list may be:
   docstrings-non-ascii-quotes docstrings that have non-ASCII quotes.
                               This depends on the `docstrings' warning type.
   suspicious  constructs that usually don't do what the coder wanted.
+  empty-body  body argument to a special form or macro is empty.
 
 If the list begins with `not', then the remaining elements specify warnings to
 suppress.  For example, (not mapcar) will suppress warnings about mapcar.
@@ -541,15 +543,19 @@ Return the compile-time value of FORM."
              ;; Later `internal--with-suppressed-warnings' binds it again, this
              ;; time in order to affect warnings emitted during the
              ;; compilation itself.
-             (let ((byte-compile--suppressed-warnings
-                    (append warnings byte-compile--suppressed-warnings)))
-               ;; This function doesn't exist, but is just a placeholder
-               ;; symbol to hook up with the
-               ;; `byte-hunk-handler'/`byte-defop-compiler-1' machinery.
-               `(internal--with-suppressed-warnings
-                 ',warnings
-                 ,(macroexpand-all `(progn ,@body)
-                                   macroexpand-all-environment))))))
+             (if body
+                 (let ((byte-compile--suppressed-warnings
+                        (append warnings byte-compile--suppressed-warnings)))
+                   ;; This function doesn't exist, but is just a placeholder
+                   ;; symbol to hook up with the
+                   ;; `byte-hunk-handler'/`byte-defop-compiler-1' machinery.
+                   `(internal--with-suppressed-warnings
+                     ',warnings
+                     ,(macroexpand-all `(progn ,@body)
+                                       macroexpand-all-environment)))
+               (macroexp-warn-and-return
+                "`with-suppressed-warnings' with empty body"
+                nil '(empty-body with-suppressed-warnings) t warnings)))))
   "The default macro-environment passed to macroexpand by the compiler.
 Placing a macro here will cause a macro to have different semantics when
 expanded by the compiler as when expanded by the interpreter.")
diff --git a/lisp/emacs-lisp/macroexp.el b/lisp/emacs-lisp/macroexp.el
index 8953e5fd01..8aa9cb860c 100644
--- a/lisp/emacs-lisp/macroexp.el
+++ b/lisp/emacs-lisp/macroexp.el
@@ -368,7 +368,7 @@ Assumes the caller has bound `macroexpand-all-environment'."
                      (macroexp-unprogn
                       (macroexp-warn-and-return
                        (format "Empty %s body" fun)
-                       nil nil 'compile-only fun))
+                       nil (list 'empty-body fun) 'compile-only fun))
                    (macroexp--all-forms body))
                  (cdr form))
                 form)))
diff --git a/lisp/subr.el b/lisp/subr.el
index 5e8f3c82a2..69e6198e1b 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -280,14 +280,20 @@ change the list."
 When COND yields non-nil, eval BODY forms sequentially and return
 value of last one, or nil if there are none."
   (declare (indent 1) (debug t))
-  (list 'if cond (cons 'progn body)))
+  (if body
+      (list 'if cond (cons 'progn body))
+    (macroexp-warn-and-return "`when' with empty body"
+                              cond '(empty-body when) t)))
 
 (defmacro unless (cond &rest body)
   "If COND yields nil, do BODY, else return nil.
 When COND yields nil, eval BODY forms sequentially and return
 value of last one, or nil if there are none."
   (declare (indent 1) (debug t))
-  (cons 'if (cons cond (cons nil body))))
+  (if body
+      (cons 'if (cons cond (cons nil body)))
+    (macroexp-warn-and-return "`unless' with empty body"
+                              cond '(empty-body unless) t)))
 
 (defsubst subr-primitive-p (object)
   "Return t if OBJECT is a built-in primitive function."
@@ -383,14 +389,19 @@ Otherwise, return result of last form in BODY.
 CONDITION can also be a list of error conditions.
 The CONDITION argument is not evaluated.  Do not quote it."
   (declare (debug t) (indent 1))
-  (if (and (eq (car-safe condition) 'quote)
-           (cdr condition) (null (cddr condition)))
-      (macroexp-warn-and-return
-       (format "`ignore-error' condition argument should not be quoted: %S"
-               condition)
-       `(condition-case nil (progn ,@body) (,(cadr condition) nil))
-       nil t condition)
-    `(condition-case nil (progn ,@body) (,condition nil))))
+  (cond
+   ((and (eq (car-safe condition) 'quote)
+         (cdr condition) (null (cddr condition)))
+    (macroexp-warn-and-return
+     (format "`ignore-error' condition argument should not be quoted: %S"
+             condition)
+     `(condition-case nil (progn ,@body) (,(cadr condition) nil))
+     nil t condition))
+   (body
+    `(condition-case nil (progn ,@body) (,condition nil)))
+   (t
+    (macroexp-warn-and-return "`ignore-error' with empty body"
+                              nil '(empty-body ignore-error) t condition))))
 
 
 ;;;; Basic Lisp functions.
diff --git a/test/lisp/emacs-lisp/bytecomp-tests.el 
b/test/lisp/emacs-lisp/bytecomp-tests.el
index 61f4998f6b..eec66c9585 100644
--- a/test/lisp/emacs-lisp/bytecomp-tests.el
+++ b/test/lisp/emacs-lisp/bytecomp-tests.el
@@ -1433,7 +1433,50 @@ literals (Bug#20852)."
         (set-buffer (get-buffer-create "foo"))
         nil))
    '((suspicious set-buffer))
-   "Warning: Use .with-current-buffer. rather than"))
+   "Warning: Use .with-current-buffer. rather than")
+
+  (test-suppression
+   '(defun zot ()
+      (let ((_ 1))
+        ))
+   '((empty-body let))
+   "Warning: Empty let body")
+
+  (test-suppression
+   '(defun zot ()
+      (let* ((_ 1))
+        ))
+   '((empty-body let*))
+   "Warning: Empty let\\* body")
+
+  (test-suppression
+   '(defun zot (x)
+      (when x
+        ))
+   '((empty-body when))
+   "Warning: `when' with empty body")
+
+  (test-suppression
+   '(defun zot (x)
+      (unless x
+        ))
+   '((empty-body unless))
+   "Warning: `unless' with empty body")
+
+  (test-suppression
+   '(defun zot (x)
+      (ignore-error arith-error
+        ))
+   '((empty-body ignore-error))
+   "Warning: `ignore-error' with empty body")
+
+  (test-suppression
+   '(defun zot (x)
+      (with-suppressed-warnings ((suspicious eq))
+        ))
+   '((empty-body with-suppressed-warnings))
+   "Warning: `with-suppressed-warnings' with empty body")
+  )
 
 (ert-deftest bytecomp-tests--not-writable-directory ()
   "Test that byte compilation works if the output directory isn't