[Bug c++/102482] Winit-list-lifetime false positive for temporaries with std::initializer_list

["redi at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dmalcolm at gcc dot gnu.org
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2021-09-29
     Ever confirmed|0                           |1

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Federico Kircheis from comment #4)
> Note that following equivalent snippet
> 
> ----
> #include <initializer_list>
> 
> struct span {
>     span(std::initializer_list<int> il) noexcept : begin(nullptr),
> size(il.size()) { begin = il.begin();}
>     const int* begin;
>     std::size_t size;
> };
> ----
> 
> does not trigger the warning.

Ideally it would warn, but I hope the reason it doesn't its fairly obvious. The
C++ front end doesn't perform arbitrarily complex data flow analysis for the
constructor body, it just looks for the most common pattern of storing a
pointer to the temporary array in a mem-initializer. Using the array in the
constructor body /without storing it/ is less problematic, because only the
non-static data members persist beyond the end of the constructor e.g. this is
fine:

span(std::initializer_list<int> il)
{ for (auto i : il) std::cout << i << ' '; }


As a simple heuristic to warn about the majority of cases, GCC checks for
storing the pointer in the mem-initializer-list only.

Maybe you're misunderstanding what the warning is telling you. It's not saying
that the statement `return foo(span(std::initializer_list<int>{}));` has a bug,
because that's not the location of the warning. It's telling you that your
constructor definition is potentially dangerous. Which is totally true. How is
the compiler supposed to know that you never plan to use that constructor for
lvalues? (C++ doesn't allow a && ref-qualifier on a constructor, which would
enforce that at the language level). Should it re-compile the constructor every
time it's called, passing in whether it's constructing an rvalue or lvalue, so
that only some uses of it warn? That would slow down compilation for every
class with an initializer-list constructor. What if the constructor is not
defined inline, so is compiled completely separate from its callers?

If **you** know that nobody will never use that constructor for lvalues, then
just disable the warning around the constructor:

struct span {
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Winit-list-lifetime"
    span(std::initializer_list<int> il) noexcept : begin(il.begin()),
size(il.size()) {}
#pragma GCC diagnostic pop
    const int* begin;
    std::size_t size;
};

Or if you don't think the warning is useful, just disable it on the command
line via -Wno-init-list-lifetime. But the compiler can't know how you plan to
use that constructor, so it tells you it's got a potential bug.

> > And the fabulous manual [...]
> 
> Mhm, I should have done a little more research before opening this ticket.

Reading the documentation for the warning you're reporting a bug for is
probably a good start :-)

> > *   When a list constructor stores the "begin" pointer from the
> >     "initializer_list" argument, this doesn't extend the lifetime of
> >     the array, so if a class variable is constructed from a temporary
> >     "initializer_list", the pointer is left dangling by the end of the
> >     variable declaration statement.
> 
> The first statement is correct (does not extend lifetime), but the second is
> not always (the pointer is not always left dangling).

If the pointer's lifetime has not ended, it dangles. The manual seems correct
as written (it could be made more verbose to talk about special cases, but
approximately nobody reads the manual now, adding more words isn't going to
help).


> Also following snippet generates the warning:
> 
> ----
> #include <initializer_list>
> 
> struct span {
>     span(std::initializer_list<int>& il) noexcept : begin(il.begin()),
> size(il.size()) {}
>     const int* begin;
>     std::size_t size;
> };
> ----
> 
> but I currently cannot imagine a scenario where this would dangle, as one
> cannot write "span({1,2,3})"

True, but approximately nobody writes constructors taking a non-const lvalue
reference to std::initializer_list, so this corner case doesn't seem very
important. This does seem like a concrete enhancement that could be made easily
though, even if low value in practice.

The point is, this is a warning that warns about **potentially** problematic
code in the constructor. All warnings like this have false positives (otherwise
they would be errors not warnings).

If you have concrete suggestions for improving the heuristics then it might be
possible to reduce the false positive rate, but otherwise this amounts to "this
warning has false positives" and the response is "yes".

The current -Winit-list-lifetime is a simple, heuristic-based warning done in
the front end, so is going to have false positives. Pushing the warning into
the middle end (where flow analysis and inlining happens) would make it
smarter, but that would require teaching the language-independent middle end
about C++ std::initializer_list semantics, and would be a completely different
warning to the current simple heiristic in the front end. So a lot of work.

I'll confirm this as a diagnostic bug, but I would be surprised if anything can
be done about it.

Maybe Dave Malcolm's -fanalyzer could be taught to track std::initializer_list
lifetimes and check if the pointer escapes past the array's lifetime. Then if
you know you are going to check your code with -fanalyzer you could use
-Wno-init-list-lifetime during normal compilation, and rely on -fanalyzer
builds to find these problems (without false positives). But that doesn't exist
yet, and not everybody is going to use -fanalyzer, so the current warning is
still useful and working as designed.