SFINAE on pair/tuple assignment operators 2729

[zoecarver]

Summary

This patch ensures that SFINAE is used to delete assignment operators in pair and tuple based on issue 2729.

[EricWF]

We should already have tests for this.

And you should only write fail.cpp tests when there is absolutely no other way to test the behavior.
In this case it can be tested with a passing test and type traits.

[zoecarver]

• remove fail tests
• update pass tests

[ldionne]

test/std/utilities/tuple/tuple.tuple/tuple.assign/copy.pass.cpp

115 ↗

(On Diff #203178)

I think you'd be testing the same thing if you said !std::is_assignable<T, P const>::value, but the intent might be clearer. What do you think?

You could also use T& instead to clear up any impression that this matters to the test (I had to think about it).

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

40 ↗

(On Diff #203178)

I think it would be clearer to say NonMoveAssignable here, since this is really the property we're using.

[zoecarver]

• change name of helper struct
• update constness and value type of type trait

[zoecarver]

@ldionne updated based on your review. Does this look good now?

[ldionne]

test/std/utilities/utility/pairs/pairs.pair/assign_pair.pass.cpp

76 ↗

(On Diff #204401)

Why don't you keep the old check for static_assert(!std::is_copy_assignable<P>::value, ""); too?

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

25 ↗

(On Diff #204401)

You should be able to add this to make it copy-assignable:

NonMoveAssignable& operator=(NonMoveAssignable const&) = default;

[zoecarver]

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

25 ↗

(On Diff #204401)

Hmm you're right but that unfortunatly fails the test !std::is_move_assignable<std::pair<int, NonMoveAssignable>>::value. I'll look into fixing it.

[zoecarver]

• Rebase off main
• Remove changes to unrelated tests

[ldionne]

So you added tests for std::tuple::operator=(std::pair const&) and std::tuple::operator=(std::pair&&), but not for:

• pair& operator=(const pair& p);
• template<class U, class V> pair& operator=(const pair<U, V>& p);
• pair& operator=(pair&& p) noexcept(see below);
• template<class U, class V> pair& operator=(pair<U, V>&& p);
• tuple& operator=(const tuple& u);
• tuple& operator=(tuple&& u) noexcept(see below);
• template <class... UTypes> tuple& operator=(const tuple<UTypes...>& u);
• template <class... UTypes> tuple& operator=(tuple<UTypes...>&& u);

The list is taken from the issue you linked. Is it because those constructors are already tested in these cases?

Also, the LWG issue says:

std::is_copy_assignable<std::pair<int, std::unique_ptr<int>>>::value is true, and should be false.

Can you please add a test with unique_ptr to the tests for the constructors that should support it? I'm a big fan of just copy-pasting simple things that we know should work properly and test that they do indeed work as intended. This avoids bad surprises.

[zoecarver]

• Rebase off main
• Add tests for the remaining overloads.
• Format changes with git-clang-format.

[zoecarver]

• Add test in const_pair.pass.cpp (missed in original pass).

[zoecarver]

@ldionne sorry about that. I don't know how I overlooked the majority of the issue. Anyway, I've made sure that all the overloads have a test now.

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

108–110

This is just to mark where we have existing test coverage for this issue (to make it easier to review). I'll remove these comments when I commit.

[ldionne]

In D62454#2575490, @zoecarver wrote:

@ldionne sorry about that. I don't know how I overlooked the majority of the issue. Anyway, I've made sure that all the overloads have a test now.

Meh, no worries! Thanks a lot for adding the missing tests.

Shiiiiip it!

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

108–110

Oh, ok, thanks a lot :-).

libcxx/docs/Cxx1zStatusIssuesStatus.csv

 "`2681 <https://wg21.link/LWG2681>`__","filesystem::copy() cannot copy symlinks","Issaquah","|Complete|",""
 "`2682 <https://wg21.link/LWG2682>`__","filesystem::copy() won't create a symlink to a directory","Issaquah","|Complete|",""
 "`2686 <https://wg21.link/LWG2686>`__","Why is std::hash specialized for error_code, but not error_condition?","Issaquah","|Complete|",""
 "`2694 <https://wg21.link/LWG2694>`__","Application of LWG 436 accidentally deleted definition of ""facet""","Issaquah","|Complete|",""
 "`2696 <https://wg21.link/LWG2696>`__","Interaction between make_shared and enable_shared_from_this is underspecified","Issaquah","|Nothing To Do|",""
 "`2699 <https://wg21.link/LWG2699>`__","Missing restriction in [numeric.requirements]","Issaquah","|Complete|",""
 "`2712 <https://wg21.link/LWG2712>`__","copy_file(from, to, ...) has a number of unspecified error conditions","Issaquah","|Complete|",""
 "`2722 <https://wg21.link/LWG2722>`__","equivalent incorrectly specifies throws clause","Issaquah","|Complete|",""
-"`2729 <https://wg21.link/LWG2729>`__","Missing SFINAE on std::pair::operator=","Issaquah","",""
+"`2729 <https://wg21.link/LWG2729>`__","Missing SFINAE on std::pair::operator=","Issaquah","|Complete|",""
 "`2732 <https://wg21.link/LWG2732>`__","Questionable specification of path::operator/= and path::append","Issaquah","|Complete|",""
 "`2733 <https://wg21.link/LWG2733>`__","[fund.ts.v2] gcd / lcm and bool","Issaquah","|Complete|",""
 "`2735 <https://wg21.link/LWG2735>`__","std::abs(short), std::abs(signed char) and others should return int instead of double in order to be compatible with C++98 and C","Issaquah","|Complete|",""
 "`2736 <https://wg21.link/LWG2736>`__","nullopt_t insufficiently constrained","Issaquah","|Complete|",""
 "`2738 <https://wg21.link/LWG2738>`__","``is_constructible``\  with void types","Issaquah","|Complete|",""
 "`2739 <https://wg21.link/LWG2739>`__","Issue with time_point non-member subtraction with an unsigned duration","Issaquah","|Complete|",""
 "`2740 <https://wg21.link/LWG2740>`__","constexpr optional<T>::operator->","Issaquah","|Complete|",""
 "`2742 <https://wg21.link/LWG2742>`__","Inconsistent string interface taking string_view","Issaquah","|Complete|",""

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

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // <tuple>
 
 // template <class... Types> class tuple;
 
 // template <class U1, class U2>
 //   tuple& operator=(const pair<U1, U2>& u);
 
 // UNSUPPORTED: c++03
 
 #include <tuple>
 #include <utility>
+#include <memory>
 #include <cassert>
 
 #include "test_macros.h"
 
 int main(int, char**)
 {
     {
         typedef std::pair<long, char> T0;
         typedef std::tuple<long long, short> T1;
         T0 t0(2, 'a');
         T1 t1;
         t1 = t0;
         assert(std::get<0>(t1) == 2);
         assert(std::get<1>(t1) == short('a'));
     }
+    {
+        // test that the implicitly generated copy assignment operator
+        // is properly deleted
+        using T = std::tuple<int, int>;
+        using P = std::tuple<std::unique_ptr<int>, std::unique_ptr<int>>;
+        static_assert(!std::is_assignable<T, const P &>::value, "");
+    }
 
   return 0;
 }

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

 };
 
 struct D
     : B
 {
     explicit D(int i = 0) : B(i) {}
 };
 
+struct NonAssignable {
+  NonAssignable& operator=(NonAssignable const&) = delete;
+  NonAssignable& operator=(NonAssignable&&) = delete;
+};
+
 int main(int, char**)
 {
     {
         typedef std::tuple<long> T0;
         typedef std::tuple<long long> T1;
         T0 t0(2);
         T1 t1;
         t1 = t0;
int main(int, char**)
         int x = 42;
         int y = 43;
         std::tuple<int&&> t(std::move(x));
         std::tuple<int&> t2(y);
         t = t2;
         assert(std::get<0>(t) == 43);
         assert(&std::get<0>(t) == &x);
     }
+    {
+      using T = std::tuple<int, NonAssignable>;
+      using U = std::tuple<NonAssignable, int>;
+      static_assert(!std::is_assignable<T, U const&>::value, "");
+      static_assert(!std::is_assignable<U, T const&>::value, "");
+    }
 
   return 0;
 }

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

 
 struct E {
   E() = default;
   E& operator=(int) {
       return *this;
   }
 };
 
+struct NonAssignable {
+  NonAssignable& operator=(NonAssignable const&) = delete;
+  NonAssignable& operator=(NonAssignable&&) = delete;
+};
+
 int main(int, char**)
 {
     {
         typedef std::tuple<long> T0;
         typedef std::tuple<long long> T1;
         T0 t0(2);
         T1 t1;
         t1 = std::move(t0);
int main(int, char**)
         int x = 42;
         int y = 43;
         std::tuple<int&&, E> t(std::move(x), E{});
         std::tuple<int&&, int> t2(std::move(y), 44);
         t = std::move(t2);
         assert(std::get<0>(t) == 43);
         assert(&std::get<0>(t) == &x);
     }
+    {
+      using T = std::tuple<int, NonAssignable>;
+      using U = std::tuple<NonAssignable, int>;
+      static_assert(!std::is_assignable<T, U&&>::value, "");
+      static_assert(!std::is_assignable<U, T&&>::value, "");
+    }
 
   return 0;
 }

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

struct CopyAssignable {
   CopyAssignable& operator=(CopyAssignable &&) = delete;
 };
 static_assert(std::is_copy_assignable<CopyAssignable>::value, "");
 struct MoveAssignable {
   MoveAssignable& operator=(MoveAssignable const&) = delete;
   MoveAssignable& operator=(MoveAssignable&&) = default;
 };
 
+struct CopyAssignableInt {
+  CopyAssignableInt& operator=(int&) { return *this; }
+};
+
 int main(int, char**)
 {
     {
         typedef std::tuple<> T;
         T t0;
         T t;
         t = t0;
     }
int main(int, char**)
     }
     {
         // test that the implicitly generated copy assignment operator
         // is properly deleted
         using T = std::tuple<std::unique_ptr<int>>;
         static_assert(!std::is_copy_assignable<T>::value, "");
     }
     {
-        using T = std::tuple<int, NonAssignable>;
-        static_assert(!std::is_copy_assignable<T>::value, "");
+      // HERE (and below/above)
     }
     {
         using T = std::tuple<int, CopyAssignable>;
         static_assert(std::is_copy_assignable<T>::value, "");
     }
     {
         using T = std::tuple<int, MoveAssignable>;
         static_assert(!std::is_copy_assignable<T>::value, "");
     }
+    {
+      using T = std::tuple<int, int, int>;
+      using P = std::pair<int, int>;
+      static_assert(!std::is_assignable<T, P>::value, "");
+    }
+    { // test const requirement
+      using T = std::tuple<CopyAssignableInt, CopyAssignableInt>;
+      using P = std::pair<int, int>;
+      static_assert(!std::is_assignable<T&, P const>::value, "");
+    }
+    {
+      using T = std::tuple<int, MoveAssignable>;
+      using P = std::pair<int, MoveAssignable>;
+      static_assert(!std::is_assignable<T&, P&>::value, "");
+    }
 
   return 0;
 }

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

int main(int, char**)
         // test that the implicitly generated move assignment operator
         // is properly deleted
         using T = std::tuple<std::unique_ptr<int>>;
         static_assert(std::is_move_assignable<T>::value, "");
         static_assert(!std::is_copy_assignable<T>::value, "");
 
     }
     {
-        using T = std::tuple<int, NonAssignable>;
-        static_assert(!std::is_move_assignable<T>::value, "");
+      // HERE (and below/above)
     }
     {
         using T = std::tuple<int, MoveAssignable>;
         static_assert(std::is_move_assignable<T>::value, "");
     }
     {
         // The move should decay to a copy.
         CountAssign::reset();

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

 };
 
 struct D
     : B
 {
     explicit D(int i) : B(i) {}
 };
 
+struct NonMoveAssignable {
+  NonMoveAssignable& operator=(NonMoveAssignable const&) = default;
+  NonMoveAssignable& operator=(NonMoveAssignable&&) = delete;
+};
+
 int main(int, char**)
 {
     {
         typedef std::pair<long, std::unique_ptr<D>> T0;
         typedef std::tuple<long long, std::unique_ptr<B>> T1;
         T0 t0(2, std::unique_ptr<D>(new D(3)));
         T1 t1;
         t1 = std::move(t0);
         assert(std::get<0>(t1) == 2);
         assert(std::get<1>(t1)->id_ == 3);
     }
+    {
+      using T = std::tuple<int, NonMoveAssignable>;
+      using P = std::pair<int, NonMoveAssignable>;
+      static_assert(!std::is_assignable<T&, P&&>::value, "");
+    }
+    {
+      using T = std::tuple<int, int, int>;
+      using P = std::pair<int, int>;
+      static_assert(!std::is_assignable<T&, P&&>::value, "");
+    }
 
   return 0;
 }

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

 #include <utility>
 #include <cassert>
 
 #include "test_macros.h"
 #if TEST_STD_VER >= 11
 #include "archetypes.h"
 #endif
 
+struct CopyAssignableInt {
+  CopyAssignableInt& operator=(int&) { return *this; }
+};
+
+struct Unrelated {};
+
 TEST_CONSTEXPR_CXX20 bool test() {
   {
     typedef std::pair<int, short> P1;
     typedef std::pair<double, long> P2;
     P1 p1(3, static_cast<short>(4));
     P2 p2;
     p2 = p1;
     assert(p2.first == 3);
#elif TEST_STD_VER >= 11
     assert(C::constructed == 0);
     assert(C::assigned == 1);
     assert(C::copy_assigned == 1);
     assert(C::move_assigned == 0);
     assert(p.first == 42);
     assert(p.second.value == -42);
   }
 #endif
+
+  { // test const requirement
+    using T = std::pair<CopyAssignableInt, CopyAssignableInt>;
+    using P = std::pair<int, int>;
+    static_assert(!std::is_assignable<T&, P const>::value, "");
+  }
+  {
+    using T = std::pair<int, Unrelated>;
+    using P = std::pair<Unrelated, int>;
+    static_assert(!std::is_assignable<T&, P&>::value, "");
+    static_assert(!std::is_assignable<P&, T&>::value, "");
+  }
   return true;
 }
 
 int main(int, char**) {
   test();
 #if TEST_STD_VER >= 20
   static_assert(test());
 #endif
 
   return 0;
 }

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

TEST_CONSTEXPR_CXX20 bool test() {
     assert(p2.first.copied == 0);
     assert(p2.first.moved == 0);
   }
   {
     using P = std::pair<int, ConstexprTestTypes::MoveAssignOnly>;
     static_assert(!std::is_copy_assignable<P>::value, "");
   }
   {
+    using P = std::pair<int, std::unique_ptr<int> >;
+    static_assert(!std::is_copy_assignable<P>::value, "");
+  }
+  {
     using P = std::pair<int, Incomplete&>;
     static_assert(!std::is_copy_assignable<P>::value, "");
     P p(42, inc_obj);
     assert(&p.second == &inc_obj);
   }
 
   return true;
 }

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

TEST_CONSTEXPR_CXX20 CountAssign& operator=(CountAssign const&) {
     return *this;
   }
   TEST_CONSTEXPR_CXX20 CountAssign& operator=(CountAssign&&) {
     ++moved;
     return *this;
   }
 };
 
+struct NotAssignable {
+  NotAssignable& operator=(NotAssignable const&) = delete;
+  NotAssignable& operator=(NotAssignable&&) = delete;
+};
+
 TEST_CONSTEXPR_CXX20 bool test() {
   {
     typedef std::pair<ConstexprTestTypes::MoveOnly, int> P;
     P p1(3, 4);
     P p2;
     p2 = std::move(p1);
     assert(p2.first.value == 3);
     assert(p2.second == 4);
TEST_CONSTEXPR_CXX20 bool test() {
     P p;
     P p2;
     p = std::move(p2);
     assert(p.first.moved == 1);
     assert(p.first.copied == 0);
     assert(p2.first.moved == 0);
     assert(p2.first.copied == 0);
   }
+  {
+    using T = std::pair<int, NotAssignable>;
+    using P = std::pair<int, NotAssignable>;
+    static_assert(!std::is_assignable<T, P&&>::value, "");
+  }
   return true;
 }
 
 int main(int, char**) {
   test();
 #if TEST_STD_VER >= 20
   static_assert(test());
 #endif
 
   return 0;
 }

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

TEST_CONSTEXPR_CXX20 CountAssign& operator=(CountAssign const&) {
     return *this;
   }
   TEST_CONSTEXPR_CXX20 CountAssign& operator=(CountAssign&&) {
     ++moved;
     return *this;
   }
 };
 
+struct CopyAssignableInt {
+  CopyAssignableInt& operator=(int&) { return *this; }
+};
+
 TEST_CONSTEXPR_CXX20 bool test() {
   {
     typedef std::pair<Derived, short> P1;
     typedef std::pair<ConstexprTestTypes::MoveOnly, long> P2;
     P1 p1(Derived(), static_cast<short>(4));
     P2 p2;
     p2 = std::move(p1);
     assert(p2.second == 4);
   }
   {
     using P = std::pair<int, CountAssign>;
     using T = std::pair<long, CountAssign>;
     T t(42, -42);
     P p(101, 101);
     p = std::move(t);
     assert(p.first == 42);
     assert(p.second.moved == 1);
     assert(p.second.copied == 0);
     assert(t.second.moved == 0);
     assert(t.second.copied == 0);
   }
+  { // test const requirement
+    using T = std::pair<CopyAssignableInt, CopyAssignableInt>;
+    using P = std::pair<int, int>;
+    static_assert(!std::is_assignable<T, P&&>::value, "");
+    static_assert(!std::is_assignable<P, T&&>::value, "");
+  }
   return true;
 }
 
 int main(int, char**) {
   test();
 #if TEST_STD_VER >= 20
   static_assert(test());
 #endif
 
   return 0;
 }