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
Message #20 received at 78716 <at> debbugs.gnu.org (full text, mbox):
From: Pip Cet <pipcet <at> protonmail.com> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: 78716 <at> debbugs.gnu.org Subject: Re: bug#78716: 31.0.50; no byte compiler warnings for ignored side-effect-free function calls Date: Sun, 29 Jun 2025 18:25:02 +0000
"Stefan Monnier" <monnier <at> iro.umontreal.ca> writes:
>> 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.
>
> Could be. The "all sub arguments" case is easy to fix (but I'm not
> sure we'd ever need/want it; examples welcome), it's the other one
> that's more interesting.
I was thinking about pcase, cl-loop, and the cconv code; I expected all
three to generate huge and complicated expressions which it might not be
feasible to mix (maybe-ignore ...) calls into.
>>> 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.
>
> The `beep` operation is the parameter to `not`.
But it doesn't generate a warning, so how does the annotation apply to
it?
>> IIUC, you're saying that we need the non-recursive option above, but not
>> the recursive one?
>
> That's right.
Definining a function called maybe-ignore, which returns its argument
but has no compiler macro, seems to work.
>> I'm not sure I agree,
>
> Why? We do want to `beep` here. The only part that we should remove is
> the silly `not` operation.
I wasn't talking about removing the expression recursively (which I
agree doesn't make much sense), just about disabling the warnings in an
entire lexical scope.
>> We already have a recursive option which works, and the non-recursive
>> option would be just like 'ignore'.
>
> Maybe you're right, I simply don't know.
I'll test this some more. Rerunning this on the Emacs tree, I think
things like the which-key.el bug or the ob-tangle.el one are worth
fixing. (However, I'm not quite sure why the ob-tangle.el code
triggered the warning :-) )
I'm not sure they were in the first patch; here they are for
completeness:
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/org/ob-tangle.el b/lisp/org/ob-tangle.el
index 75e7b560848..e3c1d7a45b4 100644
--- a/lisp/org/ob-tangle.el
+++ b/lisp/org/ob-tangle.el
@@ -296,7 +296,8 @@ org-babel-tangle
(when she-bang
(unless tangle-mode (setq tangle-mode #o755)))
(when tangle-mode
- (add-to-list 'modes (org-babel-interpret-file-mode tangle-mode)))
+ (cl-pushnew (org-babel-interpret-file-mode tangle-mode)
+ modes))
;; Possibly create the parent directories for file.
(let ((m (funcall get-spec :mkdirp)))
(and m fnd (not (string= m "no"))
(Tangent: we failed to throw an error for that last one because we were
in a lambda which captured modes; would this be a good idea, maybe?)
diff --git a/lisp/subr.el b/lisp/subr.el
index 69f6e4dbab8..4ab1a1b84b7 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2523,7 +2523,7 @@ add-to-list
(warnfun (lambda ()
;; FIXME: We should also emit a warning for let-bound
;; variables with dynamic binding.
- (when (assq sym byte-compile--lexical-environment)
+ (when (memq sym byte-compile-lexical-variables)
(byte-compile-report-error msg :fill))))
(code
(macroexp-let2 macroexp-copyable-p x element
>
>>>> (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!
>
> We have an endless supply of such code, tho, most of which doesn't have
> the property of being "cosmetically silly".
I don't think all the bugs were cosmetic; the (not (beep)) one certainly
is, though.
>>>> 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?
>
> Could be. I don't know much about elint, other than that it never
> seemed to do anything very useful when I tried it.
>
> In any case, whether it's implement in the compiler or in a separate
> package, the requirement remains to provide a way to silence the
> false-positives.
Absolutely. In my current test runs, 'push' appears to be the only
false positive, and requires a 'maybe-ignore'.
>>> 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 ...
>> )
>
> But if you use it within a macro around code provided by the caller, you
> risk silencing real problems.
Yes, that's always a risk when trying to reduce the number of false
positives.
> So, for the same reason we moved from `with-no-warnings` to
> `with-suppressed-warnings`, I'd rather have something that has a more
> local effect.
I'll have to look at pcase.el to see whether that's feasible.
> This said, maybe we want a general "single-level" version of
> `with-suppressed-warnings` since I've already several times wanted a way
> to suppress a particular kind of warning but only for the top-level of
> an expression and not for all the sub-expressions it may hold.
That sounds like a good idea.
> I think such a ". t" option would be good for a "single-level"
> warning suppressor since in most cases it's obvious which warning it
> would apply to.
> For `with-suppressed-warnings`, OTOH, I'm not sure I want to encourage
> people to suppress all lexical warnings within all the subexpressions of
> a form.
I agree, but the problem isn't the . t, it's the very broad 'lexical
category. Only the "unused" messages should really be disabled. Maybe
it's time to split it up.
Pip
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.