GNU bug report logs - #78704
[PATCH] Use `seq-do' instead of `seq-map' for side-effects

Previous Next

Full log

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

From: Pip Cet <pipcet <at> protonmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 78704 <at> debbugs.gnu.org,
 Nicolas Petton <nicolas <at> petton.fr>, Zach Shaftel <zach <at> shaf.tel>
Subject: Re: bug#78704: [PATCH] Use `seq-do' instead of `seq-map' for
 side-effects
Date: Tue, 10 Jun 2025 07:22:49 +0000

"Stefan Monnier" <monnier <at> iro.umontreal.ca> writes:

>> I believe it was, but it seems we never complained about discarding,
>> for example, (not (beep)), even though the "not" is superfluous.
>
> FWIW, we don't care about warning for all such cases.  Instead we want
> to care about those cases that often enough reflect errors.

I don't think it's merely about the frequency, it's also about how hard
it is to fix the bug, and how likely it is to do damage.

Replacing mapconcat by mapc when the byte compiler tells you to is
something that can be done without further thought (and maybe should be
done automatically), but a constant or symbol being evaluated for effect
sometimes represents a puzzling bug, in my experiments.

While I realize that the Emacs Elisp code base might not be ideal
(because most bugs that may have cropped up during development have
presumably been fixex), it's what I've looked at so far; there are about
50 places I've changed locally to avoid the additional warnings, most of
them harmless but a little confusing because of the unnecessary
evaluation.

Perhaps this one illustrates how tricky the bugs can be:

diff --git a/lisp/international/ccl.el b/lisp/international/ccl.el
index 5f766d8bef9..37538984df4 100644
--- a/lisp/international/ccl.el
+++ b/lisp/international/ccl.el
@@ -649,8 +649,8 @@ ccl-compile-loop
 	    ;; this loop.
 	    (while ccl-breaks
 	      (ccl-embed-current-address (car ccl-breaks))
-	      (setq ccl-breaks (cdr ccl-breaks))))
-	  nil))))
+	      (setq ccl-breaks (cdr ccl-breaks)))))
+      nil)))
 
 (defun ccl-compile-break (cmd)
   "Compile BREAK statement."

The fix seems obvious (though the modified code is still a little
redundant), but the CCL compiler is well-tested, and there might be
unintended consequences of fixing ccl-compile-loop to return t
sometimes, even if that was the original intent.  Should we live with
this bug instead?

Or this one:

diff --git a/lisp/files.el b/lisp/files.el
index 56d998410d3..a913a6c222b 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -7600,7 +7600,7 @@ recover-session-finish
 			       (condition-case nil
 				   (save-excursion (recover-file file))
 				 (error
-				  "Failed to recover `%s'" file)))
+				  (message "Failed to recover `%s'" file))))
 			     files
 			     '("file" "files" "recover"))
 	    (message "No files can be recovered from this session now")))

Is it right to use "message" here, which may be overwritten immediately
by the next iteration of map-y-or-n-p?  Or we could do without a message
in this case, as we have done for almost 30 years.

Here are the ones in lisp/emacs-lisp, to demonstrate that most of them
are merely cosmetic:

diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index a076012cd30..87a5486404a 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -2260,7 +2260,7 @@ cl--self-tco-on-form
   ;; Apply self-tco to the function returned by FORM, assuming that
   ;; it will be bound to VAR.
   (pcase form
-    (`(function (lambda ,fargs . ,ebody)) form
+    (`(function (lambda ,fargs . ,ebody))
      (pcase-let* ((`(,decls . ,body) (macroexp-parse-body ebody))
                   (`(,ofargs . ,obody) (cl--self-tco var fargs body)))
        `(function (lambda ,ofargs ,@decls . ,obody))))
diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index c7e38015c7e..11bae960887 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -325,8 +325,9 @@ comp-vec--verify-idx
 (defsubst comp-vec-aref (vec idx)
   "Return the element of VEC whose index is IDX."
   (declare (gv-setter (lambda (val)
-                        `(comp-vec--verify-idx ,vec ,idx)
-                        `(puthash ,idx ,val (comp-vec-data ,vec)))))
+                        `(progn
+                           (comp-vec--verify-idx ,vec ,idx)
+                           (puthash ,idx ,val (comp-vec-data ,vec))))))
   (comp-vec--verify-idx vec idx)
   (gethash idx (comp-vec-data vec)))
 
diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index 284e3acd959..e047d4ce1de 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -917,8 +917,8 @@ edebug-read-list
 	(if (eq 'dot (edebug-next-token-class))
 	    (let (dotted-form)
 	      (forward-char 1)		; skip \.
-	      (setq dotted-form (edebug-read-storing-offsets stream))
-		    elements (nconc elements dotted-form)
+	      (setq dotted-form (edebug-read-storing-offsets stream)
+		    elements (nconc elements dotted-form))
 	      (if (not (eq (edebug-next-token-class) 'rparen))
 		  (edebug-syntax-error "Expected `)'"))
 	      (setq edebug-read-dotted-list (listp dotted-form))

> Warnings are always delicate in practice because "pointless" code can
> show up very naturally from perfectly good code, just as the result of
> rewrites like inlining and macro-expansion.  So it's not always
> desirable to be "more thorough" especially when the warning is about
> code that is not optimized enough (as opposed to dubious/invalid code)
> because you can end up with too many false positives.

I agree completely.  Not proposing to add a new warning to the byte
compiler, at this point.

I personally often ignore the byte compiler warnings because code I'm
writing has unused arguments and variables, and those warnings drown out
more serious ones.

Pip

Previous Next

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