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

Previous Next

To reply to this bug, email your comments to 78704 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: Zach Shaftel <zach <at> shaf.tel>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Use `seq-do' instead of `seq-map' for side-effects
Date: Thu, 05 Jun 2025 23:18:26 -0400

[Message part 1 (text/plain, inline)]

Tags: patch

self explanatory i think.



In GNU Emacs 31.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.49, cairo version 1.18.4) of 2025-06-04 built on bigbox
Repository revision: 680fa61b5989b84c0e19ac568be012afd8345f0c
Repository branch: master
System Description: Arch Linux

Configured using:
 'configure --with-modules --without-xwidgets --with-native-compilation
 --with-tree-sitter --without-gsettings --without-gconf --without-gpm
 --with-pgtk --without-compress-install 'CFLAGS=-mtune=native
 -march=native -O2 -g -fuse-ld=mold''

[0001-Use-seq-do-instead-of-seq-map-for-side-effects.patch (text/patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Zach Shaftel <zach <at> shaf.tel>, Nicolas Petton <nicolas <at> petton.fr>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 78704 <at> debbugs.gnu.org
Subject: Re: bug#78704: [PATCH] Use `seq-do' instead of `seq-map' for
 side-effects
Date: Sat, 07 Jun 2025 13:20:24 +0300

> Date: Thu, 05 Jun 2025 23:18:26 -0400
> From:  Zach Shaftel via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> self explanatory i think.

I'm not sure it is, but I added a few people who might have an
opinion.

> >From ba328dd06ebc503d6bc7c1ab1f6c2c45bdf6f375 Mon Sep 17 00:00:00 2001
> From: Zach Shaftel <zach <at> shaf.tel>
> Date: Thu, 5 Jun 2025 00:05:20 -0400
> Subject: [PATCH] Use `seq-do' instead of `seq-map' for side-effects
> 
> * lisp/emacs-lisp/seq.el (seq-reverse): Use `seq-do' instead of
> `seq-map' since it's being used for effect.
> * lisp/emacs-lisp/seq.el (seq-map): Declare `important-return-value' so
> the compiler will complain about it next time.
> ---
>  lisp/emacs-lisp/seq.el | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/lisp/emacs-lisp/seq.el b/lisp/emacs-lisp/seq.el
> index a7954e7614c..af425456fe1 100644
> --- a/lisp/emacs-lisp/seq.el
> +++ b/lisp/emacs-lisp/seq.el
> @@ -195,6 +195,7 @@ seq-subseq
>  
>  (cl-defgeneric seq-map (function sequence)
>    "Return the result of applying FUNCTION to each element of SEQUENCE."
> +  (declare (important-return-value t))
>    (let (result)
>      (seq-do (lambda (elt)
>                (push (funcall function elt) result))
> @@ -295,9 +296,9 @@ seq-sort-by
>  (cl-defgeneric seq-reverse (sequence)
>    "Return a sequence with elements of SEQUENCE in reverse order."
>    (let ((result '()))
> -    (seq-map (lambda (elt)
> -               (push elt result))
> -             sequence)
> +    (seq-do (lambda (elt)
> +              (push elt result))
> +            sequence)
>      (seq-into result (type-of sequence))))
>  
>  ;; faster implementation for sequences (sequencep)
> -- 
> 2.49.0
> 

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

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

"Eli Zaretskii" <eliz <at> gnu.org> writes:

>> Date: Thu, 05 Jun 2025 23:18:26 -0400
>> From:  Zach Shaftel via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>>
>> self explanatory i think.
>
> I'm not sure it is, but I added a few people who might have an
> opinion.

First of all, this is a good catch!  The situation is similar for map-do
/ map-apply, including a usage in radix-tree.el which may not be
reported by the byte compiler.

But I'm not sure adding important-return-value to functions which aren't
usually side-effect-free is a good idea given the warnings it currently
produces.

The main reason is that it's not a helpful warning unless we tell the
user which function to use instead: mapc is a special case which is
handled by an explicit message, but without a "use `seq-do' instead"
in the message, fixing the warning requires looking up docstrings to
find the right alternative, which might not exist.

Some other languages have chosen a different approach and provide a way
for functions to know, at compile time or run time, whether their return
value is used in a particular call.  Maybe we should do that instead?
As an analogy, the byte compiler won't complain about (equal x 3), but
will silently replace it by (eq x 3), so it's not like we always warn
the user about code which can be optimized in a similar fashion.

However, I did notice that we warn about fewer important-return-value
functions than I thought, and opened bug#78716 to discuss whether we
should add more warnings.

Pip

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

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

Pip Cet <pipcet <at> protonmail.com> writes:

> But I'm not sure adding important-return-value to functions which aren't
> usually side-effect-free is a good idea given the warnings it currently
> produces.

my understanding was that important-return-value is meant to instruct
the compiler to warn about discarded values, without also allowing it to
optimize away those calls the way the side-effect-free property does
(for example, `mapcan' is declared important-return-value). so
side-effect-free implies important-return-value. is that not the
intended meaning?

> The main reason is that it's not a helpful warning unless we tell the
> user which function to use instead: mapc is a special case which is
> handled by an explicit message, but without a "use `seq-do' instead"
> in the message, fixing the warning requires looking up docstrings to
> find the right alternative, which might not exist.

very true. maybe important-return-value could accept a string argument
to specify a helpful message? this could also be useful for functions
like `plist-put', which are side-effect-full but whose return value
should still be captured; we could notify the user that
(plist-put plist :prop val) should be wrapped with `setq'.

> Some other languages have chosen a different approach and provide a way
> for functions to know, at compile time or run time, whether their return
> value is used in a particular call.  

interesting, i wasn't aware of that approach. where could i see some
examples?

> Maybe we should do that instead? As an analogy, the byte compiler
> won't complain about (equal x 3), but will silently replace it by (eq
> x 3), so it's not like we always warn the user about code which can be
> optimized in a similar fashion.

that seems appropriate to me. (equal x 3) is still a valid usage of the
`equal' function, and will do what the user intended. on the other hand,
we do complain about (eq x '(t)) in bytecomp--warn-dodgy-eq-arg, because
that almost certainly will not do what the user intended.

> However, I did notice that we warn about fewer important-return-value
> functions than I thought, and opened bug#78716 to discuss whether we
> should add more warnings.
>
> Pip

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

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

"Zach Shaftel" <zach <at> shaf.tel> writes:

> Pip Cet <pipcet <at> protonmail.com> writes:
>
>> But I'm not sure adding important-return-value to functions which aren't
>> usually side-effect-free is a good idea given the warnings it currently
>> produces.
>
> my understanding was that important-return-value is meant to instruct
> the compiler to warn about discarded values, without also allowing it to
> optimize away those calls the way the side-effect-free property does

Usually, the byte compiler only optimizes away calls to functions whose
side-effect-free property is 'error-free, not those for which it is t.
(Bug#63288 was about unexpected failure to do so after evaluating
eieio-core.el, and the same code is causing trouble in bug#78685, so I'm
a bit skeptical that such conditional optimizations are worth it.)

> (for example, `mapcan' is declared important-return-value). so
> side-effect-free implies important-return-value. is that not the
> intended meaning?

I believe it was, but it seems we never complained about discarding, for
example, (not (beep)), even though the "not" is superfluous.  See
bug#78716 for what happens when we enable such warnings (some false
positives, 5 places in the code base that look like potential bugs; and
there are places like x-dnd-handle-unsupported-drop which I cannot make
sense of at all).

>> The main reason is that it's not a helpful warning unless we tell the
>> user which function to use instead: mapc is a special case which is
>> handled by an explicit message, but without a "use `seq-do' instead"
>> in the message, fixing the warning requires looking up docstrings to
>> find the right alternative, which might not exist.
>
> very true. maybe important-return-value could accept a string argument
> to specify a helpful message? this could also be useful for functions

I think that'd be a good idea.

> like `plist-put', which are side-effect-full but whose return value
> should still be captured; we could notify the user that
> (plist-put plist :prop val) should be wrapped with `setq'.

Note that while it's traditional (and documented as a requirement) to
wrap plist-put in setq, Emacs always was very forgiving and modified any
non-empty plist passed to plist-put in a way that ensured the original
cons cell would be returned.  At least on the C side of things, we rely
on this behavior.

plist-put (along with nconc, delq, and delete) is commented out in the
list of important-return-value functions in bytecomp.el, FWIW.
Uncommenting them produces lots of warnings.

>> Some other languages have chosen a different approach and provide a way
>> for functions to know, at compile time or run time, whether their return
>> value is used in a particular call.
>
> interesting, i wasn't aware of that approach. where could i see some
> examples?

Perl 5's wantarray, and the way GCC (incorrectly) rewrites printf
("%s\n", x) to puts (x) in void context.  I'm surprised I haven't been
able to find more, to be honest.

>> Maybe we should do that instead? As an analogy, the byte compiler
>> won't complain about (equal x 3), but will silently replace it by (eq
>> x 3), so it's not like we always warn the user about code which can be
>> optimized in a similar fashion.
>
> that seems appropriate to me. (equal x 3) is still a valid usage of the
> `equal' function, and will do what the user intended.

The same is true of (mapcar #'message strings) and discarding the
result.  It's perfectly valid and does what the user wants, it's just
inefficient.  However, that's an implementation detail.

> on the other hand, we do complain about (eq x '(t)) in
> bytecomp--warn-dodgy-eq-arg, because that almost certainly will not do
> what the user intended.

I think that's a different class of warning altogether, to be honest.

Pip

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Pip Cet <pipcet <at> protonmail.com>
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: Mon, 09 Jun 2025 10:25:38 -0400

> 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.

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.


        Stefan

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

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Pip Cet <pipcet <at> protonmail.com>
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 08:11:22 -0400

> 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.

Thanks.  That kind of data is a good guide, IME.

> @@ -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.

Better a message that gets hidden by a subsequent one than no message at
all, I think.

> 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.

Hmm... I was hoping that an underscore is a lightweight enough way to
indicate when a variable is unused to avoid being drowned in "unused
args" warnings.


        Stefan

Previous Next

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