[libc++] Fix incorrect forwarding in tuple's assignment operator

Also, add a bunch of tests for tuple and pair's assignment operators
involving reference types.

Differential Revision: https://reviews.llvm.org/D97419

libcxx/include/tuple

@@ -436,7 +436,7 @@ template<class _Dest, class _Source, class ..._Up, size_t ..._Np>
 _LIBCPP_INLINE_VISIBILITY
 void __memberwise_forward_assign(_Dest& __dest, _Source&& __source, __tuple_types<_Up...>, __tuple_indices<_Np...>) {
     _VSTD::__swallow(((
-        _VSTD::get<_Np>(__dest) = _VSTD::forward<_Up>(_VSTD::get<_Np>(_VSTD::forward<_Source>(__source)))
+        _VSTD::get<_Np>(__dest) = _VSTD::forward<_Up>(_VSTD::get<_Np>(__source))
     ), void(), 0)...);
 }
 
@@ -967,8 +967,8 @@ public:
             is_nothrow_assignable<_SecondType<_Tp...>&, _Up2>
         >::value))
     {
-        _VSTD::get<0>(*this) = _VSTD::move(__pair.first);
+        _VSTD::get<0>(*this) = _VSTD::forward<_Up1>(__pair.first);
-        _VSTD::get<1>(*this) = _VSTD::move(__pair.second);
+        _VSTD::get<1>(*this) = _VSTD::forward<_Up2>(__pair.second);
         return *this;
     }
 

libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/convert_move.pass.cpp

@@ -45,19 +45,51 @@ struct E {
   }
 };
 
+struct NothrowMoveAssignable {
+    NothrowMoveAssignable& operator=(NothrowMoveAssignable&&) noexcept { return *this; }
+};
+
+struct PotentiallyThrowingMoveAssignable {
+    PotentiallyThrowingMoveAssignable& operator=(PotentiallyThrowingMoveAssignable&&) { return *this; }
+};
+
 struct NonAssignable {
   NonAssignable& operator=(NonAssignable const&) = delete;
   NonAssignable& operator=(NonAssignable&&) = delete;
 };
 
-struct NothrowMoveAssignable
+struct MoveAssignable {
-{
+  MoveAssignable& operator=(MoveAssignable const&) = delete;
-    NothrowMoveAssignable& operator=(NothrowMoveAssignable&&) noexcept { return *this; }
+  MoveAssignable& operator=(MoveAssignable&&) = default;
 };
 
-struct PotentiallyThrowingMoveAssignable
+struct CopyAssignable {
+  CopyAssignable& operator=(CopyAssignable const&) = default;
+  CopyAssignable& operator=(CopyAssignable&&) = delete;
+};
+
+struct TrackMove
 {
-    PotentiallyThrowingMoveAssignable& operator=(PotentiallyThrowingMoveAssignable&&) { return *this; }
+    TrackMove() : value(0), moved_from(false) { }
+    explicit TrackMove(int v) : value(v), moved_from(false) { }
+    TrackMove(TrackMove const& other) : value(other.value), moved_from(false) { }
+    TrackMove(TrackMove&& other) : value(other.value), moved_from(false) {
+        other.moved_from = true;
+    }
+    TrackMove& operator=(TrackMove const& other) {
+        value = other.value;
+        moved_from = false;
+        return *this;
+    }
+    TrackMove& operator=(TrackMove&& other) {
+        value = other.value;
+        moved_from = false;
+        other.moved_from = true;
+        return *this;
+    }
+
+    int value;
+    bool moved_from;
 };
 
 int main(int, char**)
@@ -139,6 +171,82 @@ int main(int, char**)
         typedef std::tuple<PotentiallyThrowingMoveAssignable, int> T1;
         static_assert(!std::is_nothrow_assignable<T0&, T1&&>::value, "");
     }
+    {
+        // We assign through the reference and don't move out of the incoming ref,
+        // so this doesn't work (but would if the type were CopyAssignable).
+        {
+            using T1 = std::tuple<MoveAssignable&, long>;
+            using T2 = std::tuple<MoveAssignable&, int>;
+            static_assert(!std::is_assignable<T1&, T2&&>::value, "");
+        }
+
+        // ... works if it's CopyAssignable
+        {
+            using T1 = std::tuple<CopyAssignable&, long>;
+            using T2 = std::tuple<CopyAssignable&, int>;
+            static_assert(std::is_assignable<T1&, T2&&>::value, "");
+        }
+
+        // For rvalue-references, we can move-assign if the type is MoveAssignable
+        // or CopyAssignable (since in the worst case the move will decay into a copy).
+        {
+            using T1 = std::tuple<MoveAssignable&&, long>;
+            using T2 = std::tuple<MoveAssignable&&, int>;
+            static_assert(std::is_assignable<T1&, T2&&>::value, "");
+
+            using T3 = std::tuple<CopyAssignable&&, long>;
+            using T4 = std::tuple<CopyAssignable&&, int>;
+            static_assert(std::is_assignable<T3&, T4&&>::value, "");
+        }
+
+        // In all cases, we can't move-assign if the types are not assignable,
+        // since we assign through the reference.
+        {
+            using T1 = std::tuple<NonAssignable&, long>;
+            using T2 = std::tuple<NonAssignable&, int>;
+            static_assert(!std::is_assignable<T1&, T2&&>::value, "");
+
+            using T3 = std::tuple<NonAssignable&&, long>;
+            using T4 = std::tuple<NonAssignable&&, int>;
+            static_assert(!std::is_assignable<T3&, T4&&>::value, "");
+        }
+    }
+    {
+        // Make sure that we don't incorrectly move out of the source's reference.
+        using Dest = std::tuple<TrackMove, long>;
+        using Source = std::tuple<TrackMove&, int>;
+        TrackMove track{3};
+        Source src(track, 4);
+        assert(!track.moved_from);
+
+        Dest dst;
+        dst = std::move(src); // here we should make a copy
+        assert(!track.moved_from);
+        assert(std::get<0>(dst).value == 3);
+    }
+    {
+        // But we do move out of the source's reference if it's a rvalue ref
+        using Dest = std::tuple<TrackMove, long>;
+        using Source = std::tuple<TrackMove&&, int>;
+        TrackMove track{3};
+        Source src(std::move(track), 4);
+        assert(!track.moved_from); // we just took a reference
+
+        Dest dst;
+        dst = std::move(src);
+        assert(track.moved_from);
+        assert(std::get<0>(dst).value == 3);
+    }
+    {
+        // If the source holds a value, then we move out of it too
+        using Dest = std::tuple<TrackMove, long>;
+        using Source = std::tuple<TrackMove, int>;
+        Source src(TrackMove{3}, 4);
+        Dest dst;
+        dst = std::move(src);
+        assert(std::get<0>(src).moved_from);
+        assert(std::get<0>(dst).value == 3);
+    }
 
     return 0;
 }

libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/move.pass.cpp

@@ -142,6 +142,30 @@ int main(int, char**)
         using T = std::tuple<PotentiallyThrowingMoveAssignable, int>;
         static_assert(!std::is_nothrow_move_assignable<T>::value, "");
     }
+    {
+        // We assign through the reference and don't move out of the incoming ref,
+        // so this doesn't work (but would if the type were CopyAssignable).
+        using T1 = std::tuple<MoveAssignable&, int>;
+        static_assert(!std::is_move_assignable<T1>::value, "");
+
+        // ... works if it's CopyAssignable
+        using T2 = std::tuple<CopyAssignable&, int>;
+        static_assert(std::is_move_assignable<T2>::value, "");
+
+        // For rvalue-references, we can move-assign if the type is MoveAssignable
+        // or CopyAssignable (since in the worst case the move will decay into a copy).
+        using T3 = std::tuple<MoveAssignable&&, int>;
+        using T4 = std::tuple<CopyAssignable&&, int>;
+        static_assert(std::is_move_assignable<T3>::value, "");
+        static_assert(std::is_move_assignable<T4>::value, "");
+
+        // In all cases, we can't move-assign if the types are not assignable,
+        // since we assign through the reference.
+        using T5 = std::tuple<NonAssignable&, int>;
+        using T6 = std::tuple<NonAssignable&&, int>;
+        static_assert(!std::is_move_assignable<T5>::value, "");
+        static_assert(!std::is_move_assignable<T6>::value, "");
+    }
 
     return 0;
 }

libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/move_pair.pass.cpp

@@ -37,12 +37,48 @@ struct D
     explicit D(int i) : B(i) {}
 };
 
+struct TrackMove
+{
+    TrackMove() : value(0), moved_from(false) { }
+    explicit TrackMove(int v) : value(v), moved_from(false) { }
+    TrackMove(TrackMove const& other) : value(other.value), moved_from(false) { }
+    TrackMove(TrackMove&& other) : value(other.value), moved_from(false) {
+        other.moved_from = true;
+    }
+    TrackMove& operator=(TrackMove const& other) {
+        value = other.value;
+        moved_from = false;
+        return *this;
+    }
+    TrackMove& operator=(TrackMove&& other) {
+        value = other.value;
+        moved_from = false;
+        other.moved_from = true;
+        return *this;
+    }
+
+    int value;
+    bool moved_from;
+};
+
 struct NonAssignable
 {
   NonAssignable& operator=(NonAssignable const&) = delete;
   NonAssignable& operator=(NonAssignable&&) = delete;
 };
 
+struct MoveAssignable
+{
+  MoveAssignable& operator=(MoveAssignable const&) = delete;
+  MoveAssignable& operator=(MoveAssignable&&) = default;
+};
+
+struct CopyAssignable
+{
+  CopyAssignable& operator=(CopyAssignable const&) = default;
+  CopyAssignable& operator=(CopyAssignable&&) = delete;
+};
+
 struct NothrowMoveAssignable
 {
     NothrowMoveAssignable& operator=(NothrowMoveAssignable&&) noexcept { return *this; }
@@ -87,6 +123,82 @@ int main(int, char**)
         static_assert(!std::is_nothrow_assignable<Tuple&, Pair&&>::value, "");
         static_assert(!std::is_assignable<Tuple&, Pair const&&>::value, "");
     }
+    {
+        // We assign through the reference and don't move out of the incoming ref,
+        // so this doesn't work (but would if the type were CopyAssignable).
+        {
+            using T = std::tuple<MoveAssignable&, int>;
+            using P = std::pair<MoveAssignable&, int>;
+            static_assert(!std::is_assignable<T&, P&&>::value, "");
+        }
+
+        // ... works if it's CopyAssignable
+        {
+            using T = std::tuple<CopyAssignable&, int>;
+            using P = std::pair<CopyAssignable&, int>;
+            static_assert(std::is_assignable<T&, P&&>::value, "");
+        }
+
+        // For rvalue-references, we can move-assign if the type is MoveAssignable
+        // or CopyAssignable (since in the worst case the move will decay into a copy).
+        {
+            using T1 = std::tuple<MoveAssignable&&, int>;
+            using P1 = std::pair<MoveAssignable&&, int>;
+            static_assert(std::is_assignable<T1&, P1&&>::value, "");
+
+            using T2 = std::tuple<CopyAssignable&&, int>;
+            using P2 = std::pair<CopyAssignable&&, int>;
+            static_assert(std::is_assignable<T2&, P2&&>::value, "");
+        }
+
+        // In all cases, we can't move-assign if the types are not assignable,
+        // since we assign through the reference.
+        {
+            using T1 = std::tuple<NonAssignable&, int>;
+            using P1 = std::pair<NonAssignable&, int>;
+            static_assert(!std::is_assignable<T1&, P1&&>::value, "");
+
+            using T2 = std::tuple<NonAssignable&&, int>;
+            using P2 = std::pair<NonAssignable&&, int>;
+            static_assert(!std::is_assignable<T2&, P2&&>::value, "");
+        }
+    }
+    {
+        // Make sure that we don't incorrectly move out of the source's reference.
+        using Dest = std::tuple<TrackMove, int>;
+        using Source = std::pair<TrackMove&, int>;
+        TrackMove track{3};
+        Source src(track, 4);
+        assert(!track.moved_from);
+
+        Dest dst;
+        dst = std::move(src); // here we should make a copy
+        assert(!track.moved_from);
+        assert(std::get<0>(dst).value == 3);
+    }
+    {
+        // But we do move out of the source's reference if it's a rvalue ref
+        using Dest = std::tuple<TrackMove, int>;
+        using Source = std::pair<TrackMove&&, int>;
+        TrackMove track{3};
+        Source src(std::move(track), 4);
+        assert(!track.moved_from); // we just took a reference
+
+        Dest dst;
+        dst = std::move(src);
+        assert(track.moved_from);
+        assert(std::get<0>(dst).value == 3);
+    }
+    {
+        // If the pair holds a value, then we move out of it too
+        using Dest = std::tuple<TrackMove, int>;
+        using Source = std::pair<TrackMove, int>;
+        Source src(TrackMove{3}, 4);
+        Dest dst;
+        dst = std::move(src);
+        assert(src.first.moved_from);
+        assert(std::get<0>(dst).value == 3);
+    }
 
     return 0;
 }

libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_rv_pair.pass.cpp

@@ -40,6 +40,16 @@ struct NotAssignable {
   NotAssignable& operator=(NotAssignable&&) = delete;
 };
 
+struct MoveAssignable {
+  MoveAssignable& operator=(MoveAssignable const&) = delete;
+  MoveAssignable& operator=(MoveAssignable&&) = default;
+};
+
+struct CopyAssignable {
+  CopyAssignable& operator=(CopyAssignable const&) = default;
+  CopyAssignable& operator=(CopyAssignable&&) = delete;
+};
+
 TEST_CONSTEXPR_CXX20 bool test() {
   {
     typedef std::pair<ConstexprTestTypes::MoveOnly, int> P;
@@ -89,9 +99,36 @@ TEST_CONSTEXPR_CXX20 bool test() {
     assert(p2.first.copied == 0);
   }
   {
-    using T = std::pair<int, NotAssignable>;
+    using P1 = std::pair<int, NotAssignable>;
-    using P = std::pair<int, NotAssignable>;
+    using P2 = std::pair<NotAssignable, int>;
-    static_assert(!std::is_assignable<T&, P&&>::value, "");
+    using P3 = std::pair<NotAssignable, NotAssignable>;
+    static_assert(!std::is_move_assignable<P1>::value, "");
+    static_assert(!std::is_move_assignable<P2>::value, "");
+    static_assert(!std::is_move_assignable<P3>::value, "");
+  }
+  {
+    // We assign through the reference and don't move out of the incoming ref,
+    // so this doesn't work (but would if the type were CopyAssignable).
+    using P1 = std::pair<MoveAssignable&, int>;
+    static_assert(!std::is_move_assignable<P1>::value, "");
+
+    // ... works if it's CopyAssignable
+    using P2 = std::pair<CopyAssignable&, int>;
+    static_assert(std::is_move_assignable<P2>::value, "");
+
+    // For rvalue-references, we can move-assign if the type is MoveAssignable
+    // or CopyAssignable (since in the worst case the move will decay into a copy).
+    using P3 = std::pair<MoveAssignable&&, int>;
+    using P4 = std::pair<CopyAssignable&&, int>;
+    static_assert(std::is_move_assignable<P3>::value, "");
+    static_assert(std::is_move_assignable<P4>::value, "");
+
+    // In all cases, we can't move-assign if the types are not assignable,
+    // since we assign through the reference.
+    using P5 = std::pair<NotAssignable&, int>;
+    using P6 = std::pair<NotAssignable&&, int>;
+    static_assert(!std::is_move_assignable<P5>::value, "");
+    static_assert(!std::is_move_assignable<P6>::value, "");
   }
   return true;
 }

libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_rv_pair_U_V.pass.cpp

@@ -44,6 +44,21 @@ struct CopyAssignableInt {
   CopyAssignableInt& operator=(int&) { return *this; }
 };
 
+struct NotAssignable {
+  NotAssignable& operator=(NotAssignable const&) = delete;
+  NotAssignable& operator=(NotAssignable&&) = delete;
+};
+
+struct MoveAssignable {
+  MoveAssignable& operator=(MoveAssignable const&) = delete;
+  MoveAssignable& operator=(MoveAssignable&&) = default;
+};
+
+struct CopyAssignable {
+  CopyAssignable& operator=(CopyAssignable const&) = default;
+  CopyAssignable& operator=(CopyAssignable&&) = delete;
+};
+
 TEST_CONSTEXPR_CXX20 bool test() {
   {
     typedef std::pair<Derived, short> P1;
@@ -71,6 +86,60 @@ TEST_CONSTEXPR_CXX20 bool test() {
     static_assert(!std::is_assignable<T&, P&&>::value, "");
     static_assert(!std::is_assignable<P&, T&&>::value, "");
   }
+  {
+    // Make sure we can't move-assign from a pair containing a reference
+    // if that type isn't copy-constructible (since otherwise we'd be
+    // stealing the object through the reference).
+    using P1 = std::pair<MoveAssignable, long>;
+    using P2 = std::pair<MoveAssignable&, int>;
+    static_assert(!std::is_assignable<P1&, P2&&>::value, "");
+
+    // ... but this should work since we're going to steal out of the
+    // incoming rvalue reference.
+    using P3 = std::pair<MoveAssignable, long>;
+    using P4 = std::pair<MoveAssignable&&, int>;
+    static_assert(std::is_assignable<P3&, P4&&>::value, "");
+  }
+  {
+    // We assign through the reference and don't move out of the incoming ref,
+    // so this doesn't work (but would if the type were CopyAssignable).
+    {
+      using P1 = std::pair<MoveAssignable&, long>;
+      using P2 = std::pair<MoveAssignable&, int>;
+      static_assert(!std::is_assignable<P1&, P2&&>::value, "");
+    }
+
+    // ... works if it's CopyAssignable
+    {
+      using P1 = std::pair<CopyAssignable&, long>;
+      using P2 = std::pair<CopyAssignable&, int>;
+      static_assert(std::is_assignable<P1&, P2&&>::value, "");
+    }
+
+    // For rvalue-references, we can move-assign if the type is MoveAssignable,
+    // or CopyAssignable (since in the worst case the move will decay into a copy).
+    {
+      using P1 = std::pair<MoveAssignable&&, long>;
+      using P2 = std::pair<MoveAssignable&&, int>;
+      static_assert(std::is_assignable<P1&, P2&&>::value, "");
+
+      using P3 = std::pair<CopyAssignable&&, long>;
+      using P4 = std::pair<CopyAssignable&&, int>;
+      static_assert(std::is_assignable<P3&, P4&&>::value, "");
+    }
+
+    // In all cases, we can't move-assign if the types are not assignable,
+    // since we assign through the reference.
+    {
+      using P1 = std::pair<NotAssignable&, long>;
+      using P2 = std::pair<NotAssignable&, int>;
+      static_assert(!std::is_assignable<P1&, P2&&>::value, "");
+
+      using P3 = std::pair<NotAssignable&&, long>;
+      using P4 = std::pair<NotAssignable&&, int>;
+      static_assert(!std::is_assignable<P3&, P4&&>::value, "");
+    }
+  }
   return true;
 }
 

libcxx/test/std/utilities/utility/pairs/pairs.pair/move_ctor.pass.cpp

@@ -25,6 +25,12 @@ struct Dummy {
   Dummy(Dummy &&) = default;
 };
 
+struct NotCopyOrMoveConstructible {
+  NotCopyOrMoveConstructible() = default;
+  NotCopyOrMoveConstructible(NotCopyOrMoveConstructible const&) = delete;
+  NotCopyOrMoveConstructible(NotCopyOrMoveConstructible&&) = delete;
+};
+
 int main(int, char**)
 {
     {
@@ -40,6 +46,31 @@ int main(int, char**)
         static_assert(!std::is_copy_constructible<P>::value, "");
         static_assert(std::is_move_constructible<P>::value, "");
     }
+    {
+        // When constructing a pair containing a reference, we only bind the
+        // reference, so it doesn't matter whether the type is or isn't
+        // copy/move constructible.
+        {
+            using P = std::pair<NotCopyOrMoveConstructible&, int>;
+            static_assert(std::is_move_constructible<P>::value, "");
 
-  return 0;
+            NotCopyOrMoveConstructible obj;
+            P p2{obj, 3};
+            P p1(std::move(p2));
+            assert(&p1.first == &obj);
+            assert(&p2.first == &obj);
+        }
+        {
+            using P = std::pair<NotCopyOrMoveConstructible&&, int>;
+            static_assert(std::is_move_constructible<P>::value, "");
+
+            NotCopyOrMoveConstructible obj;
+            P p2{std::move(obj), 3};
+            P p1(std::move(p2));
+            assert(&p1.first == &obj);
+            assert(&p2.first == &obj);
+        }
+    }
+
+    return 0;
 }

libcxx/test/std/utilities/utility/pairs/pairs.pair/rv_pair_U_V.pass.cpp

@@ -65,6 +65,17 @@ struct ImplicitT {
   int value;
 };
 
+struct NotCopyOrMoveConstructible {
+  NotCopyOrMoveConstructible() = default;
+  NotCopyOrMoveConstructible(NotCopyOrMoveConstructible const&) = delete;
+  NotCopyOrMoveConstructible(NotCopyOrMoveConstructible&&) = delete;
+};
+
+struct NonCopyConstructible {
+    NonCopyConstructible(NonCopyConstructible const&) = delete;
+    NonCopyConstructible(NonCopyConstructible&&) = default;
+};
+
 int main(int, char**)
 {
     {
@@ -161,6 +172,41 @@ int main(int, char**)
         test_pair_rv<ExplicitTypes::ConvertingType, ExplicitTypes::ConvertingType&, true, false>();
         test_pair_rv<ExplicitTypes::ConvertingType, ExplicitTypes::ConvertingType&&, true, false>();
     }
+    {
+        // When constructing a pair containing a reference, we only bind the
+        // reference, so it doesn't matter whether the type is or isn't
+        // copy/move constructible.
+        {
+            using P1 = std::pair<NotCopyOrMoveConstructible&, long>;
+            using P2 = std::pair<NotCopyOrMoveConstructible&, int>;
+            static_assert(std::is_constructible<P1, P2&&>::value, "");
+
+            NotCopyOrMoveConstructible obj;
+            P2 p2{obj, 3};
+            P1 p1(std::move(p2));
+            assert(&p1.first == &obj);
+            assert(&p2.first == &obj);
+        }
+        {
+            using P1 = std::pair<NotCopyOrMoveConstructible&&, long>;
+            using P2 = std::pair<NotCopyOrMoveConstructible&&, int>;
+            static_assert(std::is_constructible<P1, P2&&>::value, "");
+
+            NotCopyOrMoveConstructible obj;
+            P2 p2{std::move(obj), 3};
+            P1 p1(std::move(p2));
+            assert(&p1.first == &obj);
+            assert(&p2.first == &obj);
+        }
+    }
+    {
+        // Make sure we can't move-construct from a pair containing a reference
+        // if that type isn't copy-constructible (since otherwise we'd be stealing
+        // the object through the reference).
+        using P1 = std::pair<NonCopyConstructible, long>;
+        using P2 = std::pair<NonCopyConstructible&, int>;
+        static_assert(!std::is_constructible<P1, P2&&>::value, "");
+    }
 #if TEST_STD_VER > 11
     { // explicit constexpr test
         constexpr std::pair<int, int> p1(42, 43);