[Bug libstdc++/87431] valueless_by_exception() should unconditionally return false if all the constructors are noexcept

[redi at gcc dot gnu.org]

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

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

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

--- Comment #21 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Sigh, there is yet another problem with this patch. Consider:

#include <variant>
#include <vector>

struct DeletedMoves
{
  DeletedMoves() = default;
  DeletedMoves(const DeletedMoves&) = default;
  DeletedMoves(DeletedMoves&&) = delete;
  DeletedMoves& operator=(const DeletedMoves&) = default;
  DeletedMoves& operator=(DeletedMoves&&) = delete;
};

struct ThrowingCopy
{
  ThrowingCopy() = default;
  ThrowingCopy(const ThrowingCopy&) { throw 1; }
  ThrowingCopy& operator=(const ThrowingCopy&) { throw 1; }
};

int main()
{
  using namespace std;
  variant<int, DeletedMoves, vector<ThrowingCopy>> v;
  v.emplace<2>(1);
  v.emplace<2>(1);
}

_Never_valueless<vector<ThrowingCopy>> is true, so we try to provide the
strong-exception guarantee:

      else if constexpr (__detail::__variant::_Never_valueless_alt<type>())
        {
          // This construction might throw:
          variant __tmp(in_place_index<_Np>, __il,
                        std::forward<_Args>(__args)...);
          // But _Never_valueless_alt<type> means this won't:
          *this = std::move(__tmp);
        }

However because is_move_assignable_v<DeletedMoves> is false the variant has no
move assignment operator, only a copy assignment operator. That means the
"move" assignment actually uses the variant's copy assignment operator, which
performs a copy of the vector, and that can throw. If the variant's current
contained value is not the same type as the one being emplaced, the copy
assignment will destroy the previous value, and then if copying the vector
throws we become valueless. But the variant thinks it can never become
valueless, so we have undefined behaviour.

We can restrict  the strong exception safety guarantee to only happen when the
variant has a move assignment operator:

--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -1428,7 +1428,8 @@ namespace __variant
              this->_M_reset();
              __variant_construct_by_index<_Np>(*this, __tmp);
            }
-         else if constexpr (__detail::__variant::_Never_valueless_alt<type>())
+         else if constexpr (__detail::__variant::_Never_valueless_alt<type>()
+             && _Traits::_S_move_assign)
            {
              // This construction might throw:
              variant __tmp(in_place_index<_Np>,
@@ -1474,7 +1475,8 @@ namespace __variant
              __variant_construct_by_index<_Np>(*this, __il,
                  std::forward<_Args>(__args)...);
            }
-         else if constexpr (__detail::__variant::_Never_valueless_alt<type>())
+         else if constexpr (__detail::__variant::_Never_valueless_alt<type>()
+             && _Traits::_S_move_assign)
            {
              // This construction might throw:
              variant __tmp(in_place_index<_Np>, __il,

But this means that branch won't be taken for the variant type in the example
above, and so we don't offer the strong exception safety guarantee for that
type, so we also need to consider the extra condition in the
__never_valueless() function:

@@ -352,7 +352,8 @@ namespace __variant
   template <typename... _Types>
     constexpr bool __never_valueless()
     {
-      return (_Never_valueless_alt<_Types>::value && ...);
+      return _Traits<_Types...>::_S_move_assign
+       && (_Never_valueless_alt<_Types>::value && ...);
     }

   // Defines index and the dtor, possibly trivial.