Package: emacs;
Reported by: Pip Cet <pipcet <at> protonmail.com>
Date: Sat, 7 Jun 2025 14:27:02 UTC
Severity: normal
Found in version 31.0.50
View this message in rfc822 format
From: Pip Cet <pipcet <at> protonmail.com> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: 78716 <at> debbugs.gnu.org Subject: bug#78716: 31.0.50; no byte compiler warnings for ignored side-effect-free function calls Date: Sat, 28 Jun 2025 21:55:33 +0000
"Stefan Monnier" <monnier <at> iro.umontreal.ca> writes:
>> 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.
>
> There are several macros which rely on the byte-compiler silently
> erasing such "side-effect-and-error-free" if it's not used.
I understand that the macro case is important and should be solved;
there are many ways to do so, such as introducing a maybe-ignore
function and using it in macros (analogously to ignore suppressing
for-effect warnings).
(I think we could also keep track of which cons cells were in the
original program and which ones arose through macroexpansion, but this
failed to work right away...).
So let's ignore that problem for now, and decide whether a warning is
desirable assuming it can be suppressed for certain (or, even better,
all) macros.
> There is a warning for those that can signal errors *because* we can't
> safely erase them.
(Erasing the other ones hides bugs, so it's "safe" only for correct
code).
And there are several places where we have to work around the warning we
have, because we're intentionally evaluating a side-effect-free function
for effect (to see if they throw). Introducing 'maybe-ignore' in a few
select places doesn't seem to me to be too much of a problem.
>> 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?
>
> Probably.
>> Is it worth changing?
>
> Often, it's hard for macros to know if a value will be used or not, so
> if we want to change it, we need a convenient way for macros to let the
> compiler erase those operations silently.
I suspect we'd need two ways: one which suppresses the warning
recursively (lexical scope, of course, not dynamic), for all
sub^n-arguments, and one which suppresses it only for one function call.
My suggestion would be to use with-suppressed-warnings for the first
case, and 'maybe-ignore' for the second case, and to apply this:
diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index 7c5991cb633..ad447656e38 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -380,7 +380,8 @@ byte-compile-warning-enabled-p
(let ((suppress nil))
(dolist (elem byte-compile--suppressed-warnings)
(when (and (eq (car elem) warning)
- (memq symbol (cdr elem)))
+ (or (eq (cdr elem) t)
+ (memq symbol (cdr elem))))
(setq suppress t)))
(and (not suppress)
;; During an Emacs build, we want all warnings.
to allow
(with-suppressed-warnings ((ignored-return-value . t))
BODY)
> It's not completely trivial because we can't just wrap them like (FOO
> (not (beep))) since the annotation should apply to the `not` operation
> but not to its argument (the `beep` operation).
I'm not sure I understand; the beep operation has no parameters, and
wouldn't ignore its parameter if it had one.
IIUC, you're saying that we need the non-recursive option above, but not
the recursive one?
I'm not sure I agree, but would that really be hard? We already have a
recursive option which works, and the non-recursive option would be just
like 'ignore'.
>> (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.
>
> At the same time: is it an actual problem?
Mysterious code which no one can now determine the intention of? Sounds
like a problem to me!
> Other than clean up the code, what would be the benefit?
Other than cleaning up existing code, and much more importantly, it'd
help people write new code :-)
> I'm not sure it's worth the trouble here.
I'm not sure either; TBH, I wrote this mostly in case there was an
unmanageable third category of cases where we don't want the warnings to
appear, in addition to macro expansions (which I think are solvable) and
very special corner cases like benchmarks. So far, there doesn't appear
to be, so maybe this is worth thinking about more.
>> 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))))
>
> IIUC you're swapping the two tests such that an `important-return-value`
> is not missed just because we optimized it away according to its
> `error-free` annotation?
Basically, yes. We still perform the deletion in what becomes a
fall-through case, but as the comment says, either the byte optimizer
was disabled and we probably shouldn't optimize the code (but still
complain!), or the byte optimizer missed an optimization and we should
complain about that instead.
> I can go along with that.
>> 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?
>
> Adding a notion of "level" for warnings that aren't super-important
> might make sense, yes.
OTOH, wasn't that what elint was for?
> I'd still welcome some way to silence those
> warnings locally rather than via some global setting.
(with-suppressed-warnings ((lexical . t))
... various incomplete defuns ...
)
is certainly something I'm going to use :-)
Pip
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.