Subject: Re: bug#78716: 31.0.50; no byte compiler warnings for ignored side-effect-free function calls

From: Pip Cet <[email-masked]>
Date: Sun, 29 Jun 2025 18:25:02 +0000

"Stefan Monnier" <[email-masked]> 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