[Bug libstdc++/113007] New: `std::variant` converting constructor and `operator=` compile while the C++ Standard says they must not

[Bug libstdc++/113007] New: `std::variant` converting constructor and `operator=` compile while the C++ Standard says they must not

[paul.skeptic at yandex dot ru]

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

            Bug ID: 113007
           Summary: `std::variant` converting constructor and `operator=`
                    compile while the C++ Standard says they must not
           Product: gcc
           Version: 13.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: paul.skeptic at yandex dot ru
  Target Milestone: ---

According to [variant.ctor] (https://eel.is/c++draft/variant.ctor, also see
https://en.cppreference.com/w/cpp/utility/variant/variant) converting
constructor

    template<class T>
    constexpr variant(T&& t) noexcept(/*...*/);

determines the alternative to construct as if by selecting an overload for
`F(std​::​forward<T>(​t))` from an overload set `F(T_j)` where `T_j` are types
of the alternatives,
e.g. for `std::variant<std::monostate, bool, int64_t, double>` the overload set
would be

    void F(std::monostate) {}
    void F(bool) {}
    void F(int64_t) {}
    void F(double) {}


In violation of the standard this code compiles with GCC and libstdc++
(https://godbolt.org/z/fd8xa1bqM):

    std::variant<std::monostate, bool, int64_t, double> v = 42;

while overload resolution of `F()` fails:

    F(42); // error: ambiguous confused_john_travolta.gif

Compiler output:

<source>: In function 'int main()':
<source>:14:4: error: call of overloaded 'F(int)' is ambiguous
   14 |   F(42);
      |   ~^~~~
<source>:6:6: note: candidate: 'void F(bool)'
    6 | void F(bool) {}
      |      ^
<source>:7:6: note: candidate: 'void F(int64_t)'
    7 | void F(int64_t) {}
      |      ^
<source>:8:6: note: candidate: 'void F(double)'
    8 | void F(double) {}
      |      ^


Also the same applies to converting assignment operator

    template<class T>
    constexpr variant& operator=(T&& t) noexcept(/*...*/);

see [variant.assign] (https://eel.is/c++draft/variant.assign, also
https://en.cppreference.com/w/cpp/utility/variant/operator%3D).

Among libc++, libstdc++ and MSVC standard libraries only MSVC standard library
gets it according to the standard so you can look at it for reference.

See corresponding bug for libc++:
https://github.com/llvm/llvm-project/issues/75323

[Bug libstdc++/113007] `std::variant` converting constructor and `operator=` compile while the C++ Standard says they must not

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Created attachment 56874
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=56874&action=edit
Full testcase

Please next time attach the full testcase (or place it inline) instead of just
linking to godbolt.

[Bug libstdc++/113007] `std::variant` converting constructor and `operator=` compile while the C++ Standard says they must not

[redi at gcc dot gnu.org]

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

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Pavel Novikov from comment #0)
> e.g. for `std::variant<std::monostate, bool, int64_t, double>` the overload
> set would be
> 
>     void F(std::monostate) {}
>     void F(bool) {}
>     void F(int64_t) {}
>     void F(double) {}

No, because it says:

"build an imaginary function FUN(Ti) for each alternative type Ti for which Ti
x[] = {std​::​forward<T>(t)}; is well-formed for some invented variable x."

That array initializer would be ill-formed for all except int64_t.
std::monostate just can't be initialized from an int, and for double and bool
it's a narrowing conversion (because of the braces).

I think libstdc++ is correct.

[Bug libstdc++/113007] `std::variant` converting constructor and `operator=` compile while the C++ Standard says they must not

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |INVALID

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
#include <variant>

int main()
{
  int i = 42;
  std::monostate x_0[] = { i };
  bool x_1[] = { i };
  long long x_2[] = { i };
  double x_3[] = { i };
}

var.cc: In function ‘int main()’:
var.cc:6:30: error: initializer for ‘std::monostate’ must be brace-enclosed
    6 |   std::monostate x_0[] = { i };
      |                              ^
var.cc:7:18: warning: narrowing conversion of ‘i’ from ‘int’ to ‘bool’
[-Wnarrowing]
    7 |   bool x_1[] = { i };
      |                  ^
var.cc:9:20: warning: narrowing conversion of ‘i’ from ‘int’ to ‘double’
[-Wnarrowing]
    9 |   double x_3[] = { i };
      |                    ^

Narrowing conversions are hard errors in SFINAE contexts, even though they're
just a warning in other contexts.

Only x_2 is well-formed, and so the overload set is just F(int64_t).

[Bug libstdc++/113007] `std::variant` converting constructor and `operator=` compile while the C++ Standard says they must not

[paul.skeptic at yandex dot ru]

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

--- Comment #4 from Pavel Novikov <paul.skeptic at yandex dot ru> ---
Interesting. Thank you for explanation.

Indeed the standard says all over that narrowing conversion in initialization
is prohibited, though this code compiles:

    int i = 42;
    bool b[] = {i}; // narrowing
    int64_t i64[] = {i};
    double d[] = {i}; // narrowing*

(*putting aside that any `int` can be _exactly_ represented by a double
precision floating point value)

As an approximation for `T x[] = {t}` requirement I came up with this code:

    template<typename T, typename U>
    std::enable_if_t<sizeof(decltype(T{std::declval<U>()}))>
    test() {}

and indeed it fails to compile when narrwoing conversion is required, i.e.

    test<bool, int>(); // error: narrowing
    test<int64_t, int>();
    test<double, int>(); // error: narrowing

So turning

> an imaginary function FUN(Ti) for each alternative type Ti for which Ti x[] = {std​::​forward<T>(t)}; is well-formed for some invented variable x.

into code using the approximation above we have the following code

    template<typename T, typename U>
    std::enable_if_t<sizeof(decltype(T{std::declval<U>()}))>
    F(U) {}

    F<bool>(42); // error: deduction/substitution failed due to narrowing
    F<int64_t>(42);
    F<double>(42); // error: deduction/substitution failed due to narrowing

And if we consider `F<T_j>()` functions as an overload set, then indeed only
`F<int64_t>()` is well formed considering (non)narrowing requirement and is
ultimately selected.
(See https://godbolt.org/z/sxof45Eeh)

Did I get it right?

[Bug libstdc++/113007] `std::variant` converting constructor and `operator=` compile while the C++ Standard says they must not

[redi at gcc dot gnu.org]

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

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Pavel Novikov from comment #4)
> Indeed the standard says all over that narrowing conversion in
> initialization is prohibited, though this code compiles:
> 
>     int i = 42;
>     bool b[] = {i}; // narrowing
>     int64_t i64[] = {i};
>     double d[] = {i}; // narrowing*

They compile *with a diagnostic* (a warning, specifically). That is what the
standard requires for ill-formed code, and those narrowing conversions are
ill-formed. If you want them to be errors not warnings, you can use
-Werror=narrowing. In a SFINAE context, they result in substitution failure,
because "warn and continue" is not an option for substitution failures.

> (*putting aside that any `int` can be _exactly_ represented by a double
> precision floating point value)

Assuming 32-bit int and 64-bit double, yes. That isn't guaranteed by the
standard though: int could have 64 bits, for example. To avoid unportable code,
it's considered to be narrowing "except where the source is a constant
expression and the actual value after conversion will fit into the target type
and will produce the original value when converted back to the original type".

So double d{42} is not a narrowing conversion, but double d{i} is.

> As an approximation for `T x[] = {t}` requirement I came up with this code:
> 
>     template<typename T, typename U>
>     std::enable_if_t<sizeof(decltype(T{std::declval<U>()}))>
>     test() {}
> 
> and indeed it fails to compile when narrwoing conversion is required, i.e.
> 
>     test<bool, int>(); // error: narrowing
>     test<int64_t, int>();
>     test<double, int>(); // error: narrowing
> 
> So turning
> 
> > an imaginary function FUN(Ti) for each alternative type Ti for which Ti x[] = {std​::​forward<T>(t)}; is well-formed for some invented variable x.
> 
> into code using the approximation above we have the following code
> 
>     template<typename T, typename U>
>     std::enable_if_t<sizeof(decltype(T{std::declval<U>()}))>
>     F(U) {}
> 
>     F<bool>(42); // error: deduction/substitution failed due to narrowing
>     F<int64_t>(42);
>     F<double>(42); // error: deduction/substitution failed due to narrowing
> 
> And if we consider `F<T_j>()` functions as an overload set, then indeed only
> `F<int64_t>()` is well formed considering (non)narrowing requirement and is
> ultimately selected.
> (See https://godbolt.org/z/sxof45Eeh)
> 
> Did I get it right?

Yes. Libstdc++ uses something similar to your enable_if, but using void_t
instead:

  template<typename _Ti> struct _Arr { _Ti _M_x[1]; };

  template<size_t _Ind, typename _Tp, typename _Ti>
    struct _Build_FUN<_Ind, _Tp, _Ti,
                      void_t<decltype(_Arr<_Ti>{{std::declval<_Tp>()}})>>

The _Arr struct is needed to ensure that we're trying to convert to T{1} not
just T, which is what the standard wants.

[Bug libstdc++/113007] `std::variant` converting constructor and `operator=` compile while the C++ Standard says they must not

[redi at gcc dot gnu.org]

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

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #5)
> The _Arr struct is needed to ensure that we're trying to convert to T{1} not

Oops, I mean T[1] not T{1} of course!

[Bug libstdc++/113007] `std::variant` converting constructor and `operator=` compile while the C++ Standard says they must not

[paul.skeptic at yandex dot ru]

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

--- Comment #7 from Pavel Novikov <paul.skeptic at yandex dot ru> ---
(In reply to Jonathan Wakely from comment #5&6)

Got it.

Thank you for taking the time to expansively explain what I left out, and
confirming my understanding.

Now it's time to file a bug in MSVC's standard library.

[Bug libstdc++/113007] `std::variant` converting constructor and `operator=` compile while the C++ Standard says they must not

[de34 at live dot cn]

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

Jiang An <de34 at live dot cn> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |de34 at live dot cn

--- Comment #8 from Jiang An <de34 at live dot cn> ---
(In reply to Pavel Novikov from comment #7)

> Now it's time to file a bug in MSVC's standard library.

This was by design. MSVC STL intendedly only enabled changes in P0608R3 since
C++20 (https://github.com/microsoft/STL/pull/1629#issuecomment-778895630).

But given the author of P0608R3 applied the changes to libc++ in C++17 mode, I
think it makes more sense to treat it as a DR.

I've opened this issue:
https://github.com/microsoft/STL/issues/4412