[Bug c++/111281] New: unhelpful warning output ('nonnull' argument 'v' compared to NULL)

[Bug c++/111281] New: unhelpful warning output ('nonnull' argument 'v' compared to NULL)

[f.heckenbach@fh-soft.de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111281

            Bug ID: 111281
           Summary: unhelpful warning output ('nonnull' argument 'v'
                    compared to NULL)
           Product: gcc
           Version: 12.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: f.heckenbach@fh-soft.de
  Target Milestone: ---

% cat test.cpp
#include <iostream>

void f (const auto &v) { std::cout << v; }

int t ();

int main ()
{
  f (t);
}
% g++ -c -std=c++20 -Wall test.cpp
test.cpp: In function 'void f(const auto:11&) [with auto:11 = int()]':
test.cpp:3:36: warning: 'nonnull' argument 'v' compared to NULL
[-Wnonnull-compare]
    3 | void f (const auto &v) { std::cout << v; }
      |                          ~~~~~~~~~~^~~~

Of course, there is an error in the code (trying to output a function pointer),
but the message given is completely useless:

- There is no "nonnull" in the code.

- There is no comparison (to NULL or anything else for that matter) in the
code.

Most importantly, the message points to an innocent function that doesn't
contain the actual error. While other times (e.g.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109561) GCC likes to give pages
upon pages of questionable context, here it's giving clearly too little for the
message to be of any use.

I guess it's hard to determine a good amount of context to give, but the
following two pieces of information seem essential to me:

- The actual location of the code that the message refers to. Here, it must be
somewhere in the library which is often hard to read, but at least may give a
clue about what's going on. But the message doesn't say where.

- The source code location that ultimately causes the code to be generated
(here, the line in main). Of course, in the case of template instantiations it
must be tracked (but in other cases GCC does this already) and there may be
several places doing the same instantiation (but giving any of them would be
better than none).

As it is, the message doesn't say anything relevant about the code and doesn't
point to any relevant location, so when I got it in my actual code, I wouldn't
even know where to start looking. (Other than, as usual, guessing one of the
most recent changes, but for that we only need "something's wrong" messages. ;)

[Bug c++/111281] unhelpful warning output ('nonnull' argument 'v' compared to NULL)

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111281

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
>- There is no comparison (to NULL or anything else for that matter) in the code.

Actually there is because there is a conversion to bool happening ...

[Bug c++/111281] unhelpful warning output ('nonnull' argument 'v' compared to NULL)

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111281

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
>- There is no "nonnull" in the code.


It is nonnull since it is a reference. References cannot pointer to null
values.

[Bug c++/111281] unhelpful warning output ('nonnull' argument 'v' compared to NULL)

[f.heckenbach@fh-soft.de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111281

--- Comment #3 from Frank Heckenbach <f.heckenbach@fh-soft.de> ---
Thanks for the additional info. I still think it would be useful if the message
told me that, rather than you. ;)

- 'nonnull' is a GCC attribute, and quoting it makes it look like it refers to
that, rather than to being a reference. If you can't easily track the
provenance, a wording like "non-null argument (reference or 'nonnull'
attribute)" might be better.

- 'NULL' is a macro (and very much a deprecated one since we have nullptr), and
both that and the comparison are generated by the compiler and not part of the
code, so a wording like "compared to nullptr or converted to bool" might be
better.

[Bug c++/111281] unhelpful warning output ('nonnull' argument 'v' compared to NULL)

[f.heckenbach@fh-soft.de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111281

--- Comment #4 from Frank Heckenbach <f.heckenbach@fh-soft.de> ---
FWIW, as stated, the lack of context in the message made it hard to find the
actual location of the bug in my code -- in the end even harder than I had
expected since it was well hidden.

Fortunately I was able use concepts to the rescue. Otherwise it would have been
pure guesswork. Here's what I did if anyone cares:

#include <iostream>

template <typename T> concept IsFunction = std::is_function_v <T> ||
(std::is_pointer_v <T> && std::is_function_v <std::remove_pointer_t<T>>);

template <typename T> concept Outputtable = !IsFunction <std::remove_cvref_t
<T>> || std::is_same_v <T, std::ios_base & (std::ios_base &)>;  // the latter
part is for std::left etc.

void f (const Outputtable auto &v) { std::cout << v; }

int t ();

int main ()
{
  f (t);
}

[Bug c++/111281] unhelpful warning output ('nonnull' argument 'v' compared to NULL)

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111281

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Frank Heckenbach from comment #4)
> template <typename T> concept IsFunction = std::is_function_v <T> ||
> (std::is_pointer_v <T> && std::is_function_v <std::remove_pointer_t<T>>);

remove_pointer is a no-op for non-pointers so you'll get the same result with:

template <typename T> concept IsFunction = std::is_function_v
<std::remove_pointer_t<T>>);

[Bug c++/111281] unhelpful warning output ('nonnull' argument 'v' compared to NULL)

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111281

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2023-09-05
     Ever confirmed|0                           |1

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Frank Heckenbach from comment #3)
> - 'nonnull' is a GCC attribute, and quoting it makes it look like it refers
> to that, rather than to being a reference. If you can't easily track the
> provenance, a wording like "non-null argument (reference or 'nonnull'
> attribute)" might be better.

Yes, we shouldn't mention an attribute that is implicitly added by GCC, not
actually present in the code.

> - 'NULL' is a macro (and very much a deprecated one since we have nullptr),
> and both that and the comparison are generated by the compiler and not part
> of the code, so a wording like "compared to nullptr or converted to bool"
> might be better.

Yes, we should not use "NULL" to mean a null pointer.

Using nullptr would be wrong too, because in `if (ptr)` or `if (ptr != 0)`
there's no nullptr keyword or nullptr_t object.

We should say "null" or "null pointer".

Confirmed.

[Bug c++/111281] unhelpful warning output ('nonnull' argument 'v' compared to NULL)

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111281

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
The warning comes from the middle end, where the context of the conversion to
bool might have been lost. But the C++ front end already suppresses those
warnings in several places:

  /* This is a compiler generated comparison, don't emit
     e.g. -Wnonnull-compare warning for it.  */
  else if (TREE_CODE (ifexp) == NE_EXPR)
    suppress_warning (ifexp, OPT_Wnonnull_compare);

Maybe we want that when implicitly converting a function to bool, but then we
wouldn't get any warning at all. It does seem useful to warn about converting a
non-weak function to bool, because it's always true so why not just write
`true` instead. But maybe we want a C++ FE warning instead of the middle end
one.

[Bug c++/111281] unhelpful warning output ('nonnull' argument 'v' compared to NULL)

[f.heckenbach@fh-soft.de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111281

--- Comment #8 from Frank Heckenbach <f.heckenbach@fh-soft.de> ---
I don't suggest to get rid of the warning. As I said in #3, if it's hard to
track, a more inclusive wording seems fine to me.

But my main grief about this message is the lack of context, i.e. the really
relevant source code location (here, where f is called). Without the concept
(which I simplified according to your suggestion, thanks), it would have been
really hard to find.