bug#61730: 30.0.50; Compiler warnings for delq and delete

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Michael Heerdegen]

Hello,

I think we should add compiler warnings for `delete' and `delq' function
calls whose return values are unused.  Those are most likely unintended:
still some programmers don't know that the return value must be assigned
back because the destructive operation on the original list structure
does not necessarily lead to the return value.  It's a pitfall and can
lead to bugs that are hard to debug, so having warnings would be an
improvement.

I think the warnings could be added in a similar way as the "mapcar
called for effect" warnings work.

Adding the same kind of warning for `remq' and `remove' would probably
also be useful.  This will probably not occur that often but it still
would be useful I think.

TIA,

Michael.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Richard Stallman]

  > I think we should add compiler warnings for `delete' and `delq' function
  > calls whose return values are unused.

I think that is a good idea.  But before we release such a change, we
should try compiling a lot of code with it and study the warnings produced.
Are nearly all the warnings caused by real bugs?

There are some special cases where the unused value is not a bug.  For
instance, when you know that the elements to be deleted cannot include
the first element, it is safe not to store the result back.  One can
argue that it is cleaner to do so anyway, but that's a matter of
neatness, not a bug.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Mattias Engdegård]

> I think we should add compiler warnings

You rang?

> for `delete' and `delq' function
> calls whose return values are unused.

Let's experiment: warning about for-effect calls to

  mapcar mapcan mapconcat
  delq delete delete-dups delete-consecutive-dups
  cl-delete cl-delete-if cl-delete-if-not cl-delete-duplicates
  sort

results in 34 such calls found on master, most of them about `delq` and `delete`, but `delete-dups`, `cl-delete`, `mapconcat` and `sort` are also represented.

Some of these are no doubt safe, a few of them knowingly so, but it's definitely not obvious from a quick look at the code. It's poor style in any case.

Thus such a warning definitely falls on the beneficial side. Let's do it.

> I think the warnings could be added in a similar way as the "mapcar
> called for effect" warnings work.

That's probably the best place to start (although we prefer warnings to be emitted by the front-end and not in codegen).

We may want to change the warning text from "called for effect" because it's written from the perspective of the compiler; the programmer thinks of it as 'not using the return value'.

For that matter, `with-suppressed-warnings` doesn't work for suppressing this warning very well; we may want to do something about that. This is true for the existing `mapcar` warning as well.

> Adding the same kind of warning for `remq' and `remove' would probably
> also be useful.  This will probably not occur that often but it still
> would be useful I think.

The compiler should already warn about those two since they are declared side-effect-free. Unless somehow `byte-compile-delete-errors` is set during compilation, which can happen if the code messes about with (optimise (safety ...)) -- it's a bit unfortunate. Please tell us if you observe anomalies in this regard.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Eli Zaretskii]

> Cc: 61730@debbugs.gnu.org, Stefan Monnier <monnier@iro.umontreal.ca>
> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Fri, 24 Feb 2023 14:43:34 +0100
> 
> Let's experiment: warning about for-effect calls to
> 
>   mapcar mapcan mapconcat
>   delq delete delete-dups delete-consecutive-dups
>   cl-delete cl-delete-if cl-delete-if-not cl-delete-duplicates
>   sort
> 
> results in 34 such calls found on master, most of them about `delq` and `delete`, but `delete-dups`, `cl-delete`, `mapconcat` and `sort` are also represented.
> 
> Some of these are no doubt safe, a few of them knowingly so, but it's definitely not obvious from a quick look at the code. It's poor style in any case.
> 
> Thus such a warning definitely falls on the beneficial side. Let's do it.

I'd hate to see Emacs's byte compiler becoming the Big Brother who
knows better.  Richard described a couple of situations where the
"problematic" usage is completely legitimate and safe; please try not
to emit the warning in those cases.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> > Thus such a warning definitely falls on the beneficial side. Let's do it.
>
> I'd hate to see Emacs's byte compiler becoming the Big Brother who
> knows better.  Richard described a couple of situations where the
> "problematic" usage is completely legitimate and safe

He mentioned only one: "when you know that the elements to be deleted
cannot include the first element", and that's the only one I know.  Are
there more?

Even that case is a burden to the reader and the maintainer, because one
has to think about and verify that this condition is fulfilled.  And IMO
this case is more dangerous than the `mapcar' case which just produces
slightly more inefficient code.

But yes, we would warn about some legitimate and safe calls.  The
advantages outweigh the disadvantages in my opinion.

> please try not to emit the warning in those cases.

I think this is impossible.  The compiler can't know or prove whether
the element to delete is different from the first one.


Michael.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: Mattias Engdegård <mattias.engdegard@gmail.com>,
>   61730@debbugs.gnu.org,
>   monnier@iro.umontreal.ca
> Date: Fri, 24 Feb 2023 16:11:51 +0100
> 
> But yes, we would warn about some legitimate and safe calls.  The
> advantages outweigh the disadvantages in my opinion.

Not in my opinion.  I'm sick and tired from seeing compilers emitting
bogus warnings which require one to spend time verifying perfectly
correct code, or, worse, modify the code to shut up the compiler.  Do
we really want to see stuff like

  (setq _ (delq ...))

in our code?

> > please try not to emit the warning in those cases.
> 
> I think this is impossible.  The compiler can't know or prove whether
> the element to delete is different from the first one.

If it's really impossible (and I'm not sure it is), then the better
course of action is to emit the warnings only if the byte compiler was
requested to be more sensitive to potential issues, similar to GCC's
"-W*" options.  IOW, if someone wants to lint their code, let them ask
for a linting compilation.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> Not in my opinion.  I'm sick and tired from seeing compilers emitting
> bogus warnings which require one to spend time verifying perfectly
> correct code, or, worse, modify the code to shut up the compiler.  Do
> we really want to see stuff like
>
>   (setq _ (delq ...))
>
> in our code?

That's a bit of an exaggeration: the code would just look like in the
thousands of other cases where we are not sure whether the element to
delete is not at the head, like

  (setq my-list (delq elt my-list))

which is not worse, even better readable IMO, than a naked `delq'
call.


> If it's really impossible (and I'm not sure it is), then the better
> course of action is to emit the warnings only if the byte compiler was
> requested to be more sensitive to potential issues, similar to GCC's
> "-W*" options.  IOW, if someone wants to lint their code, let them ask
> for a linting compilation.

But I would be okay with that.


Michael.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: mattias.engdegard@gmail.com,  61730@debbugs.gnu.org,
>   monnier@iro.umontreal.ca
> Date: Fri, 24 Feb 2023 16:45:18 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Not in my opinion.  I'm sick and tired from seeing compilers emitting
> > bogus warnings which require one to spend time verifying perfectly
> > correct code, or, worse, modify the code to shut up the compiler.  Do
> > we really want to see stuff like
> >
> >   (setq _ (delq ...))
> >
> > in our code?
> 
> That's a bit of an exaggeration: the code would just look like in the
> thousands of other cases where we are not sure whether the element to
> delete is not at the head, like
> 
>   (setq my-list (delq elt my-list))
> 
> which is not worse, even better readable IMO, than a naked `delq'
> call.

Even though my-list is never used again in the program?  How is this
better than "(setq _ ..."?

> > If it's really impossible (and I'm not sure it is), then the better
> > course of action is to emit the warnings only if the byte compiler was
> > requested to be more sensitive to potential issues, similar to GCC's
> > "-W*" options.  IOW, if someone wants to lint their code, let them ask
> > for a linting compilation.
> 
> But I would be okay with that.

Great, thanks.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 93 bytes --]

As promised, here is a first draft. The false positive rate seems indeed to be quite low.


[-- Attachment #2: warn-ignored-return-value.diff --]
[-- Type: application/octet-stream, Size: 4612 bytes --]

diff --git a/lisp/emacs-lisp/byte-run.el b/lisp/emacs-lisp/byte-run.el
index 9345665eea8..fd9913d1be8 100644
--- a/lisp/emacs-lisp/byte-run.el
+++ b/lisp/emacs-lisp/byte-run.el
@@ -650,11 +650,8 @@ with-suppressed-warnings
 `byte-compile-warnings' for a fuller explanation of the warning
 types.  The types that can be suppressed with this macro are
 `free-vars', `callargs', `redefine', `obsolete',
-`interactive-only', `lexical', `mapcar', `constants',
-`suspicious' and `empty-body'.
-
-For the `mapcar' case, only the `mapcar' function can be used in
-the symbol list."
+`interactive-only', `lexical', `ignored-return-value', `constants',
+`suspicious' and `empty-body'."
   ;; Note: during compilation, this definition is overridden by the one in
   ;; byte-compile-initial-macro-environment.
   (declare (debug (sexp body)) (indent 1))
diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index 095468ad978..5f428b8e1f9 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -317,7 +317,9 @@ byte-compile-warnings
   lexical-dynamic
               lexically bound variable declared dynamic elsewhere
   make-local  calls to `make-variable-buffer-local' that may be incorrect.
-  mapcar      mapcar called for effect.
+  ignored-return-value
+              function called without using the return value where this
+              is likely to be a mistake
   not-unused  warning about using variables with symbol names starting with _.
   constants   let-binding of, or assignment to, constants/nonvariables.
   docstrings  docstrings that are too wide (longer than
@@ -330,7 +332,7 @@ byte-compile-warnings
   empty-body  body argument to a special form or macro is empty.
 
 If the list begins with `not', then the remaining elements specify warnings to
-suppress.  For example, (not mapcar) will suppress warnings about mapcar.
+suppress.  For example, (not free-vars) will suppress the `free-vars' warning.
 
 The t value means \"all non experimental warning types\", and
 excludes the types in `byte-compile--emacs-build-warning-types'.
@@ -3485,11 +3487,35 @@ byte-compile-normal-call
     (byte-compile-callargs-warn form))
   (if byte-compile-generate-call-tree
       (byte-compile-annotate-call-tree form))
-  (when (and byte-compile--for-effect (eq (car form) 'mapcar)
-             (byte-compile-warning-enabled-p 'mapcar 'mapcar))
+
+  ;; We warn about ignored return values for two categories of functions:
+  ;; * Ones like `mapcar' that are side-effect-free themselves but call a
+  ;;   user-supplied function.
+  ;; * Ones like `delq' that mutate a list but whose return argument is
+  ;;   essential to use in case the start of the list has changed.
+  (when (and byte-compile--for-effect
+             (memq (car form)
+                   ;; FIXME: Maybe we should use a property instead of
+                   ;; this list.
+                   '( mapcar mapcan mapconcat
+                      assoc assoc-default
+                      delq delete delete-dups delete-consecutive-dups
+                      nconc sort
+                      cl-delete cl-delete-if cl-delete-if-not
+                      cl-delete-duplicates
+                      cl-nsubst cl-nsubst-if cl-nsubst-if-not
+                      cl-nsubstitute cl-nsubstitute-if cl-nsubstitute-if-not
+                      cl-nunion cl-nintersection
+                      cl-nset-difference cl-nset-exclusive-or
+                      cl-nreconc cl-nsublis
+                      cl-sort))
+             (byte-compile-warning-enabled-p 'ignored-return-value (car form)))
     (byte-compile-warn-x
      (car form)
-     "`mapcar' called for effect; use `mapc' or `dolist' instead"))
+     "value from call to `%s' is unused%s"
+     (car form)
+     (cond ((eq (car form) 'mapcar) "; use `mapc' or `dolist' instead")
+           (t ""))))
   (byte-compile-push-constant (car form))
   (mapc 'byte-compile-form (cdr form))	; wasteful, but faster.
   (byte-compile-out 'byte-call (length (cdr form))))
diff --git a/test/lisp/emacs-lisp/bytecomp-tests.el b/test/lisp/emacs-lisp/bytecomp-tests.el
index 4b0a714e52d..6dce3087e40 100644
--- a/test/lisp/emacs-lisp/bytecomp-tests.el
+++ b/test/lisp/emacs-lisp/bytecomp-tests.el
@@ -1420,8 +1420,8 @@ bytecomp-test--with-suppressed-warnings
    '(defun zot ()
       (mapcar #'list '(1 2 3))
       nil)
-   '((mapcar mapcar))
-   "Warning: .mapcar. called for effect")
+   '((ignored-return-value mapcar))
+   "Warning: value from call to `mapcar' is unused; use `mapc' or `dolist' instead")
 
   (test-suppression
    '(defun zot ()

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> Even though my-list is never used again in the program?  How is this
> better than "(setq _ ..."?

This case is indeed a matter of personal style.

If you modify some list and throw away the return value, the program is
likely to refer to the place again, though, not necessarily in the same
function.

Michael.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Michael Heerdegen]

Mattias Engdegård <mattias.engdegard@gmail.com> writes:

> As promised, here is a first draft.

Cool, thanks.

> The false positive rate seems indeed to be quite low.

If there are more false than correct uses among the reported cases,
enabling this kind of warning all the time might be an improvement
nonetheless.

Michael.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
> > Even though my-list is never used again in the program?  How is this
> > better than "(setq _ ..."?
>
> This case is indeed a matter of personal style.
>
> If you modify some list and throw away the return value, the program is
> likely to refer to the place again, though, not necessarily in the same
> function.

Instead of "(setq _ ..." you could better use "(ignore ..." which would
also serve as a hint to the reader: "the return value is ignored
intentionally, this destructive operation is intended like it is in this
case".  That would not be bad style IMO.

Michael.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 407 bytes --]

24 feb. 2023 kl. 17.45 skrev Michael Heerdegen <michael_heerdegen@web.de>:

> Instead of "(setq _ ..." you could better use "(ignore ..."

I would very much like `ignore` to work that way but it currently doesn't. This is probably a bug. Stefan, what about the attached patch?

(I already pushed a fix for proper for-effect behaviour of with-suppressed-warnings to master; hope that is all right.)


[-- Attachment #2: compile-ignore.diff --]
[-- Type: application/octet-stream, Size: 655 bytes --]

diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index 095468ad978..457efe73886 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -4329,7 +4329,11 @@ byte-compile-goto-if
 
 (defun byte-compile-ignore (form)
   (dolist (arg (cdr form))
-    (byte-compile-form arg t))
+    ;; Compile args for value (to avoid warnings about unused values),
+    ;; emit a discard after each, and trust the LAP peephole optimiser
+    ;; to annihilate useless ops.
+    (byte-compile-form arg)
+    (byte-compile-discard))
   (byte-compile-form nil))
 
 ;; Return the list of items in CONDITION-PARAM that match PRED-LIST.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> -    (byte-compile-form arg t))
> +    ;; Compile args for value (to avoid warnings about unused values),
> +    ;; emit a discard after each, and trust the LAP peephole optimiser
> +    ;; to annihilate useless ops.
> +    (byte-compile-form arg)
> +    (byte-compile-discard))

I doubt that it will always result in the same code :-(


        Stefan

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Richard Stallman]

  > He mentioned only one: "when you know that the elements to be deleted
  > cannot include the first element", and that's the only one I know.  Are
  > there more?

  > Even that case is a burden to the reader and the maintainer, because one
  > has to think about and verify that this condition is fulfilled.

When someone wrote code like that, either person thought about the
question and determined the return value could be ignored, or person
made a mistake and introduced a bug.

How often does each of those two happen?
What fraction of these unused return values are real possible bugs?

People won't mind a rare spurious warning if the warning message
usually indicates a real problem.  But if it is the opposite way,
people will see the warning as annoying bureaucracy and resent it.

Can we come up with a conventional way to indicate you know
you're ignoring the return value and you've concluded it is safe?
For instance, using it as the arg of `ignore'?


-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> Cc: eliz@gnu.org, mattias.engdegard@gmail.com, 61730@debbugs.gnu.org,
> 	monnier@iro.umontreal.ca
> Date: Fri, 24 Feb 2023 23:15:56 -0500
> 
> People won't mind a rare spurious warning if the warning message
> usually indicates a real problem.  But if it is the opposite way,
> people will see the warning as annoying bureaucracy and resent it.

In Emacs maintenance and development, the two cases are actually one.
We rebuild Emacs so frequently that even a "rare" warning appears all
the time and is annoying.  It is not a coincidence that we usually
don't tolerate warnings during the build of Emacs.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Mattias Engdegård]

24 feb. 2023 kl. 21.20 skrev Stefan Monnier <monnier@iro.umontreal.ca>:
> 
>> -    (byte-compile-form arg t))
>> +    ;; Compile args for value (to avoid warnings about unused values),
>> +    ;; emit a discard after each, and trust the LAP peephole optimiser
>> +    ;; to annihilate useless ops.
>> +    (byte-compile-form arg)
>> +    (byte-compile-discard))
> 
> I doubt that it will always result in the same code :-(

Not always indeed, but pretty close. Here is the complete list of .elc files that changed size, with their byte-code growth in bytes:

./lisp/emacs-lisp/pcase.elc 1
./lisp/eshell/esh-ext.elc 1
./lisp/emacs-lisp/bytecomp.elc 2
./lisp/mouse.elc 2
./lisp/org/org-agenda.elc 7
./lisp/emacs-lisp/cconv.elc 9

which is as good as zero. Is there a particular construct that you are worried about?

Most uses of (ignore ...) are for silencing unused variables, and that case should be more or less unchanged with the patch. The same should be true for using `ignore` to discard the return values of functions like mapcar or delete without warning.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> In Emacs maintenance and development, the two cases are actually one.
> We rebuild Emacs so frequently that even a "rare" warning appears all
> the time and is annoying.  It is not a coincidence that we usually
> don't tolerate warnings during the build of Emacs.

What's your opinion about the case of non-destructive functions - is it
ok if we would always warn about thrown away return values of calls of
them?

Michael.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: rms@gnu.org,  mattias.engdegard@gmail.com,  61730@debbugs.gnu.org,
>   monnier@iro.umontreal.ca
> Date: Sat, 25 Feb 2023 13:34:51 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > In Emacs maintenance and development, the two cases are actually one.
> > We rebuild Emacs so frequently that even a "rare" warning appears all
> > the time and is annoying.  It is not a coincidence that we usually
> > don't tolerate warnings during the build of Emacs.
> 
> What's your opinion about the case of non-destructive functions - is it
> ok if we would always warn about thrown away return values of calls of
> them?

What are "non-destructive functions" in this context?

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> > What's your opinion about the case of non-destructive functions - is it
> > ok if we would always warn about thrown away return values of calls of
> > them?
>
> What are "non-destructive functions" in this context?

I mean all functions with either no side effects or such which may alter
original list structures but in unpredictable ways so that only the
return value is ever useful.

Michael.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> I mean all functions with either no side effects or such which may alter
> original list structures but in unpredictable ways so that only the
> return value is ever useful.

With other words: calls where we are sure that they can't make sense.

Michael.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: rms@gnu.org,  mattias.engdegard@gmail.com,  61730@debbugs.gnu.org,
>   monnier@iro.umontreal.ca
> Date: Sat, 25 Feb 2023 16:09:40 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > > What's your opinion about the case of non-destructive functions - is it
> > > ok if we would always warn about thrown away return values of calls of
> > > them?
> >
> > What are "non-destructive functions" in this context?
> 
> I mean all functions with either no side effects or such which may alter
> original list structures but in unpredictable ways so that only the
> return value is ever useful.

Thanks.  I guess this should be okay, but a test run over our sources
would be prudent before the final decision.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Richard Stallman]

  > > I mean all functions with either no side effects or such which may alter
  > > original list structures but in unpredictable ways so that only the
  > > return value is ever useful.

  > Thanks.  I guess this should be okay, but a test run over our sources
  > would be prudent before the final decision.

These sorts of warnings can be annoying.  Whoever actually writes
this, please show us the full list of warnings that you propose to
implement, before putting them into master.

Or, how about having an option to enable these warnings, with "off"
as the default?

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Richard Stallman]

  > In Emacs maintenance and development, the two cases are actually one.
  > We rebuild Emacs so frequently that even a "rare" warning appears all
  > the time and is annoying.  It is not a coincidence that we usually
  > don't tolerate warnings during the build of Emacs.

I am surprised -- I didn't do that when I was the main maintainer.  In
recent years, ISTR seeing warnings in the build often enough.

If you intend to make a change to prevent each such warning, that
implies that each spurious warning is a bigger pain in the neck.  That
makes a stronger argument for making fewer warnings rather than more.

I resent it when a compiler takes up my time pressuring me to prove to
it that I know something isn't a bug, and I usually tell that compiler
(inside my head) where it can take those warnings.

  > What's your opinion about the case of non-destructive functions - is it
  > ok if we would always warn about thrown away return values of calls of
  > them?

Either make it an optional feature (and disabled by default), or do
not implement them.

When I implemented the options that enable such warnings in GCC, I
urged people NOT to use those options by default.  To enable them by
default in a makefile is to impose systematic harassment on every
contributor to the code.  You end up with a program as your
taskmaster, haranguing you continually to insert proof that you didn't
make some mistake.

I never used those options.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Michael Heerdegen]

Richard Stallman <rms@gnu.org> writes:

>   > > I mean all functions with either no side effects or such which
>   > > may alter
>   > > original list structures but in unpredictable ways so that only the
>   > > return value is ever useful.

> These sorts of warnings can be annoying.

How can warning about clear mistakes be annoying?

Michael.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> Cc: michael_heerdegen@web.de, mattias.engdegard@gmail.com,
> 	61730@debbugs.gnu.org, monnier@iro.umontreal.ca
> Date: Sun, 26 Feb 2023 22:22:58 -0500
> 
> Or, how about having an option to enable these warnings, with "off"
> as the default?

That would be nice, yes.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> Cc: 61730@debbugs.gnu.org
> Date: Sun, 26 Feb 2023 22:24:27 -0500
> 
>   > In Emacs maintenance and development, the two cases are actually one.
>   > We rebuild Emacs so frequently that even a "rare" warning appears all
>   > the time and is annoying.  It is not a coincidence that we usually
>   > don't tolerate warnings during the build of Emacs.
> 
> I am surprised -- I didn't do that when I was the main maintainer.

We have more branches than back then.  I routinely build 3 branches
every day -- master, the release branch, and a feature branch for some
long-living feature.  I build 3 more branches weekly.  So I see the
same or similar warnings more than once each day.  The situations
where many Lisp files need to be recompiled are also more frequent
nowadays, due to a much more massive use of macros.  These reasons add
up.

> In recent years, ISTR seeing warnings in the build often enough.

You are tracking the master branch, which is by definition less clean
wrt warnings.

> I resent it when a compiler takes up my time pressuring me to prove to
> it that I know something isn't a bug, and I usually tell that compiler
> (inside my head) where it can take those warnings.

Likewise.  Although the place I use (in my head) is called by a
somewhat different name.  But is similar in nature.

> When I implemented the options that enable such warnings in GCC, I
> urged people NOT to use those options by default.  To enable them by
> default in a makefile is to impose systematic harassment on every
> contributor to the code.  You end up with a program as your
> taskmaster, haranguing you continually to insert proof that you didn't
> make some mistake.

Sadly, that stance is all but gone nowadays: compilers, including GCC,
wine too much, especially if you use "-Wall", and many projects use
"-Wall" by default.  That is called "progress".

End of rant.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Mattias Engdegård]

There is now a consolidated and improved ignored-return-value warning on master. However, it does not yet warn about delq and delete, which this bug originally was about, but keeps a list of other functions for which such a warning makes sense. The false-positive rate seems to be low enough.

The warning can be suppressed using `ignore`, or `with-suppressed-warnings`.

Once the issues indicated by the warnings remaining on master have been dealt with, other functions will be considered for addition, depending on how useful this would be.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Mattias Engdegård]

On master there is now a function declaration and property, `important-return-value`, that is used to mark functions that the compiler should warn about when called without their value being used.

I haven't set the property on delq and delete yet; about half of the cases seem to be false positives but it may be worth the effort to clean up those too.
I did fix a correct but somewhat daft use of cl-delete in python.el and enabled the warning for that function.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Michael Heerdegen]

Mattias Engdegård <mattias.engdegard@gmail.com> writes:

> On master there is now a function declaration and property,
> `important-return-value`, that is used to mark functions that the
> compiler should warn about when called without their value being used.
>
> I haven't set the property on delq and delete yet; about half of the
> cases seem to be false positives but it may be worth the effort to
> clean up those too.
> I did fix a correct but somewhat daft use of cl-delete in python.el
> and enabled the warning for that function.

Thanks for that.

The delq and delete warnings are enabled now, and we got a question
about this in emacs-help (by T.V. Raman) that I tried to answer, but it
was about a (slightly doubtful) false positive.

Let's see if there is more outcome.

Michael.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Mattias Engdegård]

20 maj 2023 kl. 03.57 skrev Michael Heerdegen <michael_heerdegen@web.de>:

> The delq and delete warnings are enabled now, and we got a question
> about this in emacs-help (by T.V. Raman) that I tried to answer, but it
> was about a (slightly doubtful) false positive.
> 
> Let's see if there is more outcome.

I changed a few spots of murky code that were nevertheless probably (?) false positives, so that the result is unequivocally better or at least no worse, in 0de472e04f.

Some of the remaining warnings point to what appear to be clear bugs. Those should be fixed no matter what.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Michael Heerdegen]

Mattias Engdegård <mattias.engdegard@gmail.com> writes:

> > Let's see if there is more outcome.
>
> I changed a few spots of murky code that were nevertheless probably
> (?) false positives, so that the result is unequivocally better or at
> least no worse, in 0de472e04f.
>
> Some of the remaining warnings point to what appear to be clear
> bugs. Those should be fixed no matter what.

Thanks.  So it was worth it.

I have some doubt about the new property's name
"important-return-value".  It's not bad but it also doesn't really tell
what the warnings are about.  My English is not good enough to come up
with something better though ... "enforce-retval-passing"...hmm,
probably not.

Michael.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> I have some doubt about the new property's name
> "important-return-value".  It's not bad but it also doesn't really tell
> what the warnings are about.  My English is not good enough to come up
> with something better though ... "enforce-retval-passing"...hmm,
> probably not.

Maybe we can take inspirations from other langagues?  In rust we have
the `#[must_use]' attribute [1], and in C++17 and C23 we have the
`[[nodiscard]]' attribute [2] [3].

Also, is there any definitive relation between this
'important-return-value property and the existing "pure" function
declaration?  Like the `(declare (pure t))' thing in the front of a
defun.

[1]: https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-must_use-attribute
[2]: https://en.cppreference.com/w/cpp/language/attributes/nodiscard
[3]: https://en.cppreference.com/w/c/language/attributes/nodiscard

-- 
Best,


RY

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Michael Heerdegen]

Ruijie Yu <ruijie@netyu.xyz> writes:

> Maybe we can take inspirations from other langagues?  In rust we have
> the `#[must_use]' attribute [1], and in C++17 and C23 we have the
> `[[nodiscard]]' attribute [2] [3].

These are not bad.  I like "nodiscard".

> Also, is there any definitive relation between this
> 'important-return-value property and the existing "pure" function
> declaration?  Like the `(declare (pure t))' thing in the front of a
> defun.

Interesting question.  Does 'pure' (or 'side-effect-free') imply
'important-return-value'?

Michael.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Eli Zaretskii]

> Cc: Mattias Engdegård <mattias.engdegard@gmail.com>,
>  61730@debbugs.gnu.org, Stefan Monnier <monnier@iro.umontreal.ca>
> From: Michael Heerdegen <michael_heerdegen@web.de>
> Date: Sun, 21 May 2023 05:57:47 +0200
> 
> Ruijie Yu <ruijie@netyu.xyz> writes:
> 
> > Maybe we can take inspirations from other langagues?  In rust we have
> > the `#[must_use]' attribute [1], and in C++17 and C23 we have the
> > `[[nodiscard]]' attribute [2] [3].
> 
> These are not bad.  I like "nodiscard".

no-discard-value, I hope.  Just "nodiscard" is too terse (discard
what?).

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Mattias Engdegård]

21 maj 2023 kl. 05.57 skrev Michael Heerdegen <michael_heerdegen@web.de>:

>> Maybe we can take inspirations from other langagues?  In rust we have
>> the `#[must_use]' attribute [1], and in C++17 and C23 we have the
>> `[[nodiscard]]' attribute [2] [3].
> 
> These are not bad.  I like "nodiscard".

There are probably better names than the chosen one but I went back and forth on it for a while so we should let it sink in for a bit before switching again. There is still some time before the release of Emacs 30.

>> Also, is there any definitive relation between this
>> 'important-return-value property and the existing "pure" function
>> declaration?  Like the `(declare (pure t))' thing in the front of a
>> defun.
> 
> Interesting question.  Does 'pure' (or 'side-effect-free') imply
> 'important-return-value'?

Not `pure`, but (almost) all functions declared `pure` are also `side-effect-free` (possibly error-free).

`side-effect-free` often produces the same warning as `important-return-value` for calls that don't use the return value, but the entire call can be deleted silently under some circumstances.


21 maj 2023 kl. 07.55 skrev Eli Zaretskii <eliz@gnu.org>:

> no-discard-value, I hope.  Just "nodiscard" is too terse (discard
> what?).

I agree, we should prefer more descriptive names. Let's see what we can come up with.

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Mattias Engdegård]

20 maj 2023 kl. 11.14 skrev Mattias Engdegård <mattias.engdegard@gmail.com>:

> I changed a few spots of murky code that were nevertheless probably (?) false positives, so that the result is unequivocally better or at least no worse, in 0de472e04f.
> 
> Some of the remaining warnings point to what appear to be clear bugs. Those should be fixed no matter what.

The truly bad ones have been fixed, but warnings remain in:

lisp/mouse.el:2772:10
lisp/dom.el:165:8
lisp/emacs-lisp/package.el:2556:15
lisp/gnus/gnus-art.el:6489:14
lisp/gnus/gnus-art.el:6500:12
lisp/gnus/gnus-cus.el:414:14
lisp/gnus/gnus-topic.el:328:14
lisp/gnus/gnus-sum.el:4656:12
lisp/net/newst-treeview.el:1835:17
lisp/org/ob-lua.el:388:19
lisp/org/ob-ruby.el:239:13
lisp/org/ob-ruby.el:261:10
lisp/use-package/bind-key.el:254:8

Except for the ones in Org, these are probably false positives (although I'm not sure about all of them). I could go around wrapping them in (ignore ...) to make the warnings go away but I don't want to do that unless I can vouch for them being false positives and explain why.

So I'm going to disable the warning for `delq` and `delete` again, because seeing those warnings at every build is not an option and they aren't going away by themselves. (Sorry, Michael!)

(The ones in Org have nothing to do with delq and delete per se and are easy to silence but given that the warning is going away I won't bother.)

bug#61730: 30.0.50; Compiler warnings for delq and delete

[Michael Heerdegen]

Mattias Engdegård <mattias.engdegard@gmail.com> writes:

> Except for the ones in Org, these are probably false positives
> (although I'm not sure about all of them). I could go around wrapping
> them in (ignore ...) to make the warnings go away but I don't want to
> do that unless I can vouch for them being false positives and explain
> why.
>
> So I'm going to disable the warning for `delq` and `delete` again,
> because seeing those warnings at every build is not an option and they
> aren't going away by themselves. (Sorry, Michael!)

It's fine.  I didn't expect a significant number of false positives, but
if there is, we shouldn't keep the warning.

Thanks for your work here.

Michael.