[Bug libstdc++/110102] New: [13 regression] initializer_list ctors of containers skip Allocator_traits::construct, copies move-only type

[Bug libstdc++/110102] New: [13 regression] initializer_list ctors of containers skip Allocator_traits::construct, copies move-only type

[arthur.j.odwyer at gmail dot com]

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

            Bug ID: 110102
           Summary: [13 regression] initializer_list ctors of containers
                    skip Allocator_traits::construct, copies move-only
                    type
           Product: gcc
           Version: 13.1.0
            Status: UNCONFIRMED
          Keywords: accepts-invalid
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: arthur.j.odwyer at gmail dot com
  Target Milestone: ---

// https://godbolt.org/z/4Gq3TWE6M
#include <list>
struct A {
    A(int) {}
    A(const A&) = delete;
    A(A&&) {}
};
int main() {
    std::list<A> v = {1,2,3};
}

This should be ill-formed, but GCC 13.1 accepts it!
GCC 12.3 and earlier correctly reject it.

This is supposed to be constructing a std::initializer_list<A> and calling
`list::list(initializer_list<A>)`, which should then complain because `A(const
A&)` is deleted.

My guess as to what's happening here:
- We're definitely calling list(initializer_list<A>)
- It's calling _M_range_initialize(il.begin(), il.end())
- That's calling __uninitialized_copy_a
- That's probably somehow deciding that because `A` is trivially copyable, we
can just memcpy it. I.e. bug #89164 redux.

Even if it were copyable, we still wouldn't be allowed to bypass
`allocator_traits::construct`. The above snippet uses std::allocator, but I
originally found a more complicated case with pmr::polymorphic_allocator:

// https://godbolt.org/z/ToT6dW5dM
#include <cstdio>
#include <memory_resource>
#include <vector>
struct Widget {
    using allocator_type = std::pmr::polymorphic_allocator<Widget>;
    Widget(int i) : i_(i) {}
    explicit Widget(int i, allocator_type) : i_(i) {}
    explicit Widget(const Widget& rhs, allocator_type) : i_(rhs.i_ + 100) {}
    int i_;
};
static_assert(std::is_trivially_copyable_v<Widget>);
int main() {
    std::pmr::vector<Widget> v = {1,2,3};
    printf("%d %d %d\n", v[0].i_, v[1].i_, v[2].i_);
}

My understanding is that this should print "101 102 103", as GCC 12 does. But
GCC 13.1 prints "1 2 3" instead.

[Bug c++/110102] [13/14 regression] initializer_list ctors of containers skip Allocator_traits::construct, copies move-only type

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
           Keywords|                            |wrong-code
          Component|libstdc++                   |c++
             Status|UNCONFIRMED                 |NEW
      Known to fail|                            |13.1.0, 14.0
      Known to work|                            |12.3.0
            Summary|[13 regression]             |[13/14 regression]
                   |initializer_list ctors of   |initializer_list ctors of
                   |containers skip             |containers skip
                   |Allocator_traits::construct |Allocator_traits::construct
                   |, copies move-only type     |, copies move-only type
   Last reconfirmed|                            |2023-06-03

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Arthur O'Dwyer from comment #0)
> My guess as to what's happening here:
> - We're definitely calling list(initializer_list<A>)

No we aren't.

The compiler transforms std::list{1,2,3} into std::list{il.begin(), il.end()}
where il is std::initializer_list<int>.

I think this changed with r13-4564-gd081807d8d70e3

We shouldn't be doing this transformation here, because A is a program-defined
type and we don't know its properties. The transformation is safe for
{"","",""} when initializing from std::initializer_list<std::string> but not
here.

[Bug c++/110102] [13/14 regression] initializer_list ctors of containers skip Allocator_traits::construct, copies move-only type

[jason at gcc dot gnu.org]

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

--- Comment #2 from Jason Merrill <jason at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #1)
> We shouldn't be doing this transformation here, because A is a
> program-defined type and we don't know its properties.

True, though I wouldn't expect that to matter; for any sensible program I'd
expect it to be a good thing that we only construct the element directly,
rather than construct a temporary A and then copy it into the element. This
seems in the spirit of [class.copy.elision], though I agree that this situation
is not actually covered by that clause.  Perhaps it should be.

[Bug c++/110102] [13/14 regression] initializer_list ctors of containers skip Allocator_traits::construct, copies move-only type

[redi at gcc dot gnu.org]

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

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
In both examples we do still use allocator_traits::construct to initialize the
elements, which is what matters for correct behaviour of sensible programs.

For the first one we call construct(A*, int&&) and for the second one we call
construct(Widget*, int&, const Widget::allocator_type&) and so the scoped
allocator is still used correctly for the elements.

[Bug c++/110102] [13/14 regression] initializer_list ctors of containers skip Allocator_traits::construct, copies move-only type

[arthur.j.odwyer at gmail dot com]

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

--- Comment #4 from Arthur O'Dwyer <arthur.j.odwyer at gmail dot com> ---
I came across the `Widget` bug in the course of writing (and it's now mentioned
in) this blog post on value semantics and PMR:
https://quuxplusone.github.io/blog/2023/06/03/p1144-pmr-koans/#the-evil-allocator-aware-type
FWIW I'm highly sympathetic to the idea that `Widget` is "not a sensible
program" and I would love to see CWG or LWG make it UB. ;) It would make what
I'm trying to do with value semantics in P1144 so much easier if some more of
these "unreasonable programs" were less well-defined. But that seems like a
*gigantic* task.

My first, reduced, example, where `std::list<A> v = {1,2,3}` is accepted for
move-only type `A`, is 100% a bug and should be fixed. But that is easy to fix.

Jason, *I* don't know by what heuristic the optimizer decides to do this
optimization or not... but are *you* at all worried that it might be a
pessimization for some types? For example, suppose we had
    struct NthPrime { constexpr NthPrime(int); ... };
    std::vector<NthPrime> primes = {1,2,3,4,5,6};
Here we *want* to make an initializer_list<NthPrime> backed by a static array
of NthPrime at compile time. If we make an initializer_list<int>, then we'll
have to run the NthPrime(int) constructor at runtime, and that's expensive.
I tried out this example on Godbolt and the optimizer made the correct choice
in this specific case, so that's good. But I figured I should mention the
scenario in case it brings something to your mind.

[Bug c++/110102] [13/14 regression] initializer_list ctors of containers skip Allocator_traits::construct, copies move-only type

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |13.2

[Bug c++/110102] [13/14 regression] initializer_list ctors of containers skip Allocator_traits::construct, copies move-only type

[jason at gcc dot gnu.org]

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

Jason Merrill <jason at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|13.2                        |---

--- Comment #5 from Jason Merrill <jason at gcc dot gnu.org> ---
(In reply to Arthur O'Dwyer from comment #4)
> My first, reduced, example, where `std::list<A> v = {1,2,3}` is accepted for
> move-only type `A`, is 100% a bug and should be fixed. But that is easy to
> fix.

It's pedantically a bug, but only because of the suboptimal specified
initializer_list semantics.  Just looking at that line I expect a list of As
initialized from integers; I don't expect that initialization to involve an
unnecessary copy.

It's also unfortunate that initialization from a prvalue initializer_list can't
use the A move constructor.

>     struct NthPrime { constexpr NthPrime(int); ... };
>     std::vector<NthPrime> primes = {1,2,3,4,5,6};
> Here we *want* to make an initializer_list<NthPrime> backed by a static
> array of NthPrime at compile time. If we make an initializer_list<int>, then
> we'll have to run the NthPrime(int) constructor at runtime, and that's
> expensive.
> I tried out this example on Godbolt and the optimizer made the correct
> choice in this specific case, so that's good.

Right, we don't do this transformation if the initializer_list backing array
would be constant.

[Bug c++/110102] [13/14 regression] initializer_list ctors of containers skip Allocator_traits::construct, copies move-only type

[arthur.j.odwyer at gmail dot com]

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

--- Comment #6 from Arthur O'Dwyer <arthur.j.odwyer at gmail dot com> ---
(In reply to Jason Merrill from comment #5)
> (In reply to Arthur O'Dwyer from comment #4)
> > My first, reduced, example, where `std::list<A> v = {1,2,3}` is accepted for
> > move-only type `A`, is 100% a bug and should be fixed. But that is easy to
> > fix.
> 
> It's pedantically a bug, but only because of the suboptimal specified
> initializer_list semantics.  Just looking at that line I expect a list of As
> initialized from integers [...] It's also unfortunate that initialization from a prvalue initializer_list can't use A's move constructor.

Yes, but since you just implemented P2752, you know why we can't do *that*. :)

I strongly disagree that this is [only] "pedantically" a bug. The behavior of
std::initializer_list is super fundamental to "Intro C++". If GCC continues
with this behavior, I guarantee there will be tons of newbies asking (me, other
instructors, StackOverflow, etc) "If initializer lists are so immutable, then
why does this code compile? You told me it wouldn't!"

Before C++20, I got tons of the same question for GCC's acceptance of
    int f(auto x) { return x; }
I still think it would be nice if GCC stopped supporting
    int f(std::vector<auto> v) { return v[0]; }
, at least silently by default.

Similarly here: I wouldn't actually mind if GCC supported
    std::vector<std::move_only_function<int()>> v = { f, g, h };
with an on-by-default warning. It's that it accepts the extension *silently*,
and the extension is *so visible to newbies* and *so critical to their
understanding of the actual language*, that has me worried.

Here's a sketch of the `vector<move_only_function>` example:
https://godbolt.org/z/dPnfbnhsf

[Bug c++/110102] [13/14 regression] initializer_list ctors of containers skip Allocator_traits::construct, copies move-only type

[cvs-commit at gcc dot gnu.org]

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

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The trunk branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:35d2c40e4ac9ba57ae82e4722e557a2028d0cf13

commit r14-1658-g35d2c40e4ac9ba57ae82e4722e557a2028d0cf13
Author: Jason Merrill <jason@redhat.com>
Date:   Thu Jun 8 16:21:38 2023 -0400

    c++: init-list of uncopyable type [PR110102]

    The maybe_init_list_as_range optimization is a form of copy elision, but we
    can only elide well-formed copies.

            PR c++/110102

    gcc/cp/ChangeLog:

            * call.cc (maybe_init_list_as_array): Check that the element type
is
            copyable.

    gcc/testsuite/ChangeLog:

            * g++.dg/cpp0x/initlist-opt1.C: New test.

[Bug c++/110102] [13 regression] initializer_list ctors of containers skip Allocator_traits::construct, copies move-only type

[jason at gcc dot gnu.org]

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

Jason Merrill <jason at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
      Known to work|                            |14.0
      Known to fail|14.0                        |
            Summary|[13/14 regression]          |[13 regression]
                   |initializer_list ctors of   |initializer_list ctors of
                   |containers skip             |containers skip
                   |Allocator_traits::construct |Allocator_traits::construct
                   |, copies move-only type     |, copies move-only type
           Assignee|unassigned at gcc dot gnu.org      |jason at gcc dot gnu.org

--- Comment #8 from Jason Merrill <jason at gcc dot gnu.org> ---
Fixed on trunk so far.

(In reply to Arthur O'Dwyer from comment #6)
> Before C++20, I got tons of the same question for GCC's acceptance of
>     int f(auto x) { return x; }

Looks like that was fixed in GCC 8.

> I still think it would be nice if GCC stopped supporting
>     int f(std::vector<auto> v) { return v[0]; }
> , at least silently by default.

Fixed (to require -fconcepts-ts) on trunk.

[Bug c++/110102] [13 regression] initializer_list ctors of containers skip Allocator_traits::construct, copies move-only type

[arthur.j.odwyer at gmail dot com]

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

--- Comment #9 from Arthur O'Dwyer <arthur.j.odwyer at gmail dot com> ---
(In reply to Jason Merrill from comment #8)
> (In reply to Arthur O'Dwyer from comment #6)
> > I still think it would be nice if GCC stopped supporting
> >     int f(std::vector<auto> v) { return v[0]; }
> > , at least silently by default.
> 
> Fixed (to require -fconcepts-ts) on trunk.

Nice! Thank you!

[Bug c++/110102] [13 regression] initializer_list ctors of containers skip Allocator_traits::construct, copies move-only type

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |13.2

[Bug c++/110102] [13 regression] initializer_list ctors of containers skip Allocator_traits::construct, copies move-only type

[cvs-commit at gcc dot gnu.org]

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

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-13 branch has been updated by Jason Merrill
<jason@gcc.gnu.org>:

https://gcc.gnu.org/g:be1e122bd20c17aa0b57fc40cbd64f9e9a889aa2

commit r13-7471-gbe1e122bd20c17aa0b57fc40cbd64f9e9a889aa2
Author: Jason Merrill <jason@redhat.com>
Date:   Thu Jun 8 16:21:38 2023 -0400

    c++: init-list of uncopyable type [PR110102]

    The maybe_init_list_as_range optimization is a form of copy elision, but we
    can only elide well-formed copies.

            PR c++/110102

    gcc/cp/ChangeLog:

            * call.cc (maybe_init_list_as_array): Check that the element type
is
            copyable.

    gcc/testsuite/ChangeLog:

            * g++.dg/cpp0x/initlist-opt1.C: New test.

[Bug c++/110102] [13 regression] initializer_list ctors of containers skip Allocator_traits::construct, copies move-only type

[jason at gcc dot gnu.org]

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

Jason Merrill <jason at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #11 from Jason Merrill <jason at gcc dot gnu.org> ---
Fixed for 13.2