GNU bug report logs - #78716
31.0.50; no byte compiler warnings for ignored side-effect-free function calls

Previous Next

To reply to this bug, email your comments to 78716 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 31.0.50;
 no byte compiler warnings for ignored side-effect-free function calls
Date: Sat, 07 Jun 2025 14:25:47 +0000

This is a spin-off of bug#78704, which is about marking more functions
as having an important return value.

We do not currently warn consistently when discarding the value of a
function call to a side-effect-free function, even though the return
value is usually important.  It's not clear that we should, but I tried
enabling such warnings and found ~5 questionable places in the code in
lisp/.

1. We don't ever warn about side-effect-and-error-free functions like
"not"; this seems to me to be strange behavior on the part of the byte
compiler, but it's probably been that way for a long time.

For example, this "not" in an implicit progn, in vc/vc.el:

    (insert-string (format "Changes to %s since last lock:\n\n"
    		       file))
    (not (beep))
    (yes-or-no-p
          (concat "File has unlocked changes, "
           "claim lock retaining changes? ")))

That's commit bbf97570e5037b2417c9dd6353e1b2a9afadda7c, from 1993, moved
around in 2000 but essentially untouched.  Has the byte compiler been
silent on this situation for all this time?  Is it worth changing?

2. We also don't warn about function calls which we optimize away; this
also seems problematic if the form we optimize away was present in the
original source code, but it's easier to write macros which generate
intermediate forms which can then be optimized, and I don't think we can
currently tell whether we're looking at macro-expanded code or not.

For example, my naive experiment produced false warnings when

(pop list2)

appeared in a context in which its return value was ignored, which is a
common idiom and probably accounts for the majority of the warnings
produced.

OTOH, code like this, in cedet/semantic/find.el:

(defun semantic-brute-find-first-tag-by-name
  (name streamorbuffer &optional search-parts search-include)
  "Find a tag NAME within STREAMORBUFFER.  NAME is a string.
If SEARCH-PARTS is non-nil, search children of tags.
If SEARCH-INCLUDE was never implemented.
Respects `semantic-case-fold'.

Use `semantic-find-first-tag-by-name' instead."
  (let* ((stream (semantic-something-to-tag-table streamorbuffer))
	 (m (assoc-string name stream semantic-case-fold)))
    (if m
	m
      (let ((toklst stream)
	    (children nil))
	(while (and (not m) toklst)
	  (if search-parts
	      (progn
		(setq children (semantic-tag-components-with-overlays
				(car toklst)))
		(if children
		    (setq m (semantic-brute-find-first-tag-by-name
			     name children search-parts search-include)))))
	  (setq toklst (cdr toklst)))
	(if (not m)
	    ;; Go to dependencies, and search there.
	    nil)
	m))))

probably should generate a warning, because the if form seems completely
mysterious to me.

And this code in cc-engine.el:

    (and (c-syntactic-re-search-forward "[;={]" start t t t)
         (eq (char-before) ?=)
         c-overloadable-operators-regexp
         c-opt-op-identifier-prefix
         (save-excursion
           (eq (c-backward-token-2) 0)
           (looking-at c-overloadable-operators-regexp)
           (eq (c-backward-token-2) 0)
           (looking-at c-opt-op-identifier-prefix))))

seems like what might have been intended was another (and ...) in
(save-excursion ...), but I'm not entirely sure.

My suggestion is to fix the first issue and complain about such forms,
by doing something like this:

diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index 5fa65ff71a6..4716e150faf 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -3470,26 +3470,25 @@ byte-compile-form
 
           (when byte-compile--for-effect
             (let ((sef (function-get (car form) 'side-effect-free)))
-              (cond
-               ((and sef (or (eq sef 'error-free)
-                             byte-compile-delete-errors))
-                ;; This transform is normally done in the Lisp optimizer,
-                ;; so maybe we don't need to bother about it here?
-                (setq form (cons 'progn (cdr form)))
-                (setq handler #'byte-compile-progn))
-               ((and (or sef (function-get (car form) 'important-return-value))
-                     ;; Don't warn for arguments to `ignore'.
-                     (not (eq byte-compile--for-effect 'for-effect-no-warn))
-                     (bytecomp--actually-important-return-value-p form)
-                     (byte-compile-warning-enabled-p
-                      'ignored-return-value (car form)))
-                (byte-compile-warn-x
-                 (car form)
-                 "value from call to `%s' is unused%s"
-                 (car form)
-                 (cond ((eq (car form) 'mapcar)
-                        "; use `mapc' or `dolist' instead")
-                       (t "")))))))
+              (and (or sef (function-get (car form) 'important-return-value))
+                   ;; Don't warn for arguments to `ignore'.
+                   (not (eq byte-compile--for-effect 'for-effect-no-warn))
+                   (bytecomp--actually-important-return-value-p form)
+                   (byte-compile-warning-enabled-p
+                    'ignored-return-value (car form))
+                   (byte-compile-warn-x
+                    (car form)
+                    "value from call to `%s' is unused%s"
+                    (car form)
+                    (cond ((eq (car form) 'mapcar)
+                           "; use `mapc' or `dolist' instead")
+                          (t ""))))
+              (and sef (or (eq sef 'error-free)
+                           byte-compile-delete-errors)
+                   ;; This transform is normally done in the Lisp optimizer,
+                   ;; so maybe we don't need to bother about it here?
+                   (setq form (cons 'progn (cdr form)))
+                   (setq handler #'byte-compile-progn))))
 
           (if (and handler
                    ;; Make sure that function exists.

The second class of warnings should not be enabled by default, but maybe
it can become an 'extra warning level which we run once in a while?

diff --git a/lisp/emacs-lisp/byte-opt.el b/lisp/emacs-lisp/byte-opt.el
index 652c79e9c93..4e1e7cd7caf 100644
--- a/lisp/emacs-lisp/byte-opt.el
+++ b/lisp/emacs-lisp/byte-opt.el
@@ -486,6 +486,15 @@ byte-optimize-form-code-walker
 		(if-let* ((tmp (byte-opt--fget fn 'side-effect-free)))
 		    (or byte-compile-delete-errors
 		        (eq tmp 'error-free)))))
+       (and
+        (not (eq for-effect 'for-effect-no-warn))
+        (bytecomp--actually-important-return-value-p form)
+        (byte-compile-warning-enabled-p
+         'ignored-return-value (car form))
+        (byte-compile-warn-x
+         fn
+         "value from call to `%s' is unused"
+         fn))
        (byte-compile-log "  %s called for effect; deleted" fn)
        (byte-optimize-form (cons 'progn (cdr form)) t))
 
But I think to decide that, we need to look at the potential bugs caught
by this code (changed to what I think what was meant):

diff --git a/lisp/which-key.el b/lisp/which-key.el
index 0be0feb542e..3b339a14bf8 100644
--- a/lisp/which-key.el
+++ b/lisp/which-key.el
@@ -2464,7 +2464,7 @@ which-key-dump-bindings
   (let* ((buffer (get-buffer-create buffer-name))
          (keys (which-key--get-bindings (kbd prefix))))
     (with-current-buffer buffer
-      (point-max)
+      (goto-char (point-max))
       (save-excursion
         (dolist (key keys)
           (insert (apply #'format "%s%s%s\n" key)))))
diff --git a/lisp/international/quail.el b/lisp/international/quail.el
index 2b66a0bc0f7..a64a86df91e 100644
--- a/lisp/international/quail.el
+++ b/lisp/international/quail.el
@@ -2113,7 +2113,7 @@ quail-update-guidance
 		nil
 	      ;; We want to make both KEY and STR visible, but if the
 	      ;; window is too short, make at least STR visible.
-	      (setq pos (progn (point) (goto-char pos)))
+	      (setq pos (prog1 (point) (goto-char pos)))
 	      (beginning-of-line)
 	      (set-window-start win (point))
 	      (if (not (pos-visible-in-window-p pos win))
diff --git a/lisp/org/org-table.el b/lisp/org/org-table.el
index 59e1b49f92c..0c358724a39 100644
--- a/lisp/org/org-table.el
+++ b/lisp/org/org-table.el
@@ -1930,8 +1930,8 @@ org-table-copy-down
 	  (setq field-above
 		(let ((f (unless (= beg (line-beginning-position))
 			   (forward-line -1)
-			   (not (org-at-table-hline-p))
-			   (org-table-get-field column))))
+			   (and (not (org-at-table-hline-p))
+			        (org-table-get-field column)))))
 		  (and (org-string-nw-p f)
 		       (org-trim f)))))
 	;; Compute next field.
diff --git a/lisp/progmodes/ruby-mode.el b/lisp/progmodes/ruby-mode.el
index d9044c03aea..5d4137aa7fa 100644
--- a/lisp/progmodes/ruby-mode.el
+++ b/lisp/progmodes/ruby-mode.el
@@ -1782,8 +1782,8 @@ ruby-forward-sexp
                            (nth 1 (setq state (apply #'ruby-parse-partial
                                                      nil state))))
                        (setq expr t)
-                       (skip-chars-forward "<"))
-                     (not expr))))
+                       (skip-chars-forward "<")
+                       (not expr)))))
             (setq i (1- i)))
         ((error) (forward-word-strictly 1)))
       i))))
diff --git a/lisp/progmodes/vhdl-mode.el b/lisp/progmodes/vhdl-mode.el
index 593a83ceffa..0bdb877a10f 100644
--- a/lisp/progmodes/vhdl-mode.el
+++ b/lisp/progmodes/vhdl-mode.el
@@ -7133,8 +7133,9 @@ vhdl-get-syntactic-context
 	   ((and (save-excursion
 		   (goto-char (1+ containing-sexp))
 		   (skip-chars-forward " \t")
-		   (not (eolp))
-		   (not (looking-at "--\\|`")))
+		   (and
+		    (not (eolp))
+		    (not (looking-at "--\\|`"))))
 		 (save-excursion
 		   (vhdl-beginning-of-statement-1 containing-sexp)
 		   (skip-chars-backward " \t(")
 ;; Note: `static-if' can be copied into a package to enable it to be
 ;; used in Emacsen older than Emacs 30.1.  If the package is used in

Plus some unnecessary but apparently harmless code:

diff --git a/lisp/gnus/gnus-score.el b/lisp/gnus/gnus-score.el
index a4102ef0b51..54f07b9d116 100644
--- a/lisp/gnus/gnus-score.el
+++ b/lisp/gnus/gnus-score.el
@@ -1489,7 +1489,6 @@ gnus-score-save
       (setq gnus-score-alist nil)
       (nnheader-set-temp-buffer " *Gnus Scores*")
       (while cache
-	(current-buffer)
 	(setq entry (pop cache)
 	      file (nnheader-translate-file-chars (car entry) t)
 	      score (cdr entry))
diff --git a/lisp/progmodes/gdb-mi.el b/lisp/progmodes/gdb-mi.el
index 74dff3217ff..7e27019bfb4 100644
--- a/lisp/progmodes/gdb-mi.el
+++ b/lisp/progmodes/gdb-mi.el
@@ -1643,7 +1643,6 @@ gdb-buffer-type
 (defun gdb-buffer-shows-main-thread-p ()
   "Return t if current GDB buffer shows main selected thread and
 is not bound to it."
-  (current-buffer)
   (not (local-variable-p 'gdb-thread-number)))
 
 (defun gdb-get-buffer (buffer-type &optional thread)
diff --git a/lisp/textmodes/table.el b/lisp/textmodes/table.el
index 0e024f9261e..3e6954256a6 100644
--- a/lisp/textmodes/table.el
+++ b/lisp/textmodes/table.el
@@ -5360,10 +5360,8 @@ table--goto-coordinate
 	    (if (/= (move-to-column x) x)
 		(if (> (move-to-column x) x)
 		    (if no-tab-expansion
-			(progn
-			  (while (> (move-to-column x) x)
-			    (setq x (1- x)))
-			  (point))
+			(while (> (move-to-column x) x)
+			  (setq x (1- x)))
 		      (throw 'exit (move-to-column x t)))
 		  (throw 'exit nil)))
 	  (move-to-column x t))
diff --git a/lisp/speedbar.el b/lisp/speedbar.el
index b1fd141321c..2b54c456651 100644
--- a/lisp/speedbar.el
+++ b/lisp/speedbar.el
@@ -1232,8 +1232,7 @@ speedbar-mode
 	  dframe-mouse-click-function #'speedbar-click
 	  dframe-mouse-position-function #'speedbar-position-cursor-on-line)
     (setq-local tab-bar-mode nil)
-    (setq tab-line-exclude nil))
-  speedbar-buffer)
+    (setq tab-line-exclude nil)))
 
 (define-obsolete-function-alias 'speedbar-message 'dframe-message "24.4")
 

So five potentially user-visible bugs in the current Emacs code base,
plenty of false positives for the second change, at least.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.