From: Mattias Engdegård <mattiase@acm.org>
Date: Mon, 27 Feb 2023 12:57:48 +0000 (+0100)
Subject: Warn about `condition-case` without handlers
X-Git-Url: http://git.eshelyaron.com/gitweb/?a=commitdiff_plain;h=443c249d85003639512d8d3b6ace184a9ff53bc2;p=emacs.git

Warn about `condition-case` without handlers

Omitting handlers from a `condition-case` form makes it useless
since no errors are caught.

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

diff --git a/etc/NEWS b/etc/NEWS
index 4b0e4e6bd46..9241598f185 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -321,6 +321,21 @@ compared reliably at all.
 This warning can be suppressed using 'with-suppressed-warnings' with
 the warning name 'suspicious'.
 
+---
+*** Warn about 'condition-case' without handlers.
+The compiler now warns when the 'condition-case' form is used without
+any actual handlers, as in
+
+    (condition-case nil (read buffer))
+
+because it has no effect other than the execution of the body form.
+In particular, no errors are caught or suppressed.  If the intention
+was to catch all errors, add an explicit handler for 'error', or use
+'ignore-error' or 'ignore-errors'.
+
+This warning can be suppressed using 'with-suppressed-warnings' with
+the warning name 'suspicious'.
+
 +++
 ** New function 'file-user-uid'.
 This function is like 'user-uid', but is aware of file name handlers,
diff --git a/lisp/emacs-lisp/macroexp.el b/lisp/emacs-lisp/macroexp.el
index c57a27069d6..8cb67c3b8b5 100644
--- a/lisp/emacs-lisp/macroexp.el
+++ b/lisp/emacs-lisp/macroexp.el
@@ -339,14 +339,19 @@ Assumes the caller has bound `macroexpand-all-environment'."
             (`(cond . ,clauses)
              (macroexp--cons fn (macroexp--all-clauses clauses) form))
             (`(condition-case . ,(or `(,err ,body . ,handlers) pcase--dontcare))
-             (macroexp--cons
-              fn
-              (macroexp--cons err
-                              (macroexp--cons (macroexp--expand-all body)
-                                              (macroexp--all-clauses handlers 1)
-                                              (cddr form))
-                              (cdr form))
-              form))
+             (let ((exp-body (macroexp--expand-all body)))
+               (if handlers
+                   (macroexp--cons fn
+                                   (macroexp--cons
+                                    err (macroexp--cons
+                                         exp-body
+                                         (macroexp--all-clauses handlers 1)
+                                         (cddr form))
+                                    (cdr form))
+                                   form)
+                 (macroexp-warn-and-return
+                  (format-message "`condition-case' without handlers")
+                  exp-body (list 'suspicious 'condition-case) t form))))
             (`(,(or 'defvar 'defconst) ,(and name (pred symbolp)) . ,_)
              (push name macroexp--dynvars)
              (macroexp--all-forms form 2))
diff --git a/test/lisp/emacs-lisp/bytecomp-tests.el b/test/lisp/emacs-lisp/bytecomp-tests.el
index 185abaf5c22..b6dcfeedb0c 100644
--- a/test/lisp/emacs-lisp/bytecomp-tests.el
+++ b/test/lisp/emacs-lisp/bytecomp-tests.el
@@ -1446,6 +1446,12 @@ literals (Bug#20852)."
    '((suspicious set-buffer))
    "Warning: Use .with-current-buffer. rather than")
 
+  (test-suppression
+   '(defun zot (x)
+      (condition-case nil (list x)))
+   '((suspicious condition-case))
+   "Warning: `condition-case' without handlers")
+
   (test-suppression
    '(defun zot ()
       (let ((_ 1))