Subject: bug#78704: [PATCH] Use `seq-do' instead of `seq-map' for side-effects

From: Pip Cet <[email-masked]>
Date: Mon, 09 Jun 2025 10:55:00 +0000

"Zach Shaftel" <[email-masked]> writes:

> Pip Cet <[email-masked]> 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