From a83d69e78ffe58bbd410aa7503384a312e08963b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sebastian=20Gs=C3=A4nger?=
 <8004308+sgsaenger@users.noreply.github.com>
Date: Thu, 31 Oct 2019 12:38:24 +0100
Subject: [PATCH] test pair-copyability on C++17 upwards (#1886)

* test pair-copyability on C++17 upwards

The stdlib falsely detects containers like M=std::map<T, U>
as copyable, even when one of T and U is not copyable.
Therefore we cannot rely on the stdlib dismissing std::pair<T, M>
by itself, even on C++17.

* fix is_copy_assignable

bind_map used std::is_copy_assignable which suffers from the same problems
as std::is_copy_constructible, therefore the same fix has been applied.

* created tests for copyability
---
 include/pybind11/cast.h     | 16 ++++++++++++----
 include/pybind11/stl_bind.h |  4 ++--
 tests/test_stl_binders.cpp  | 22 +++++++++++++++++++++
 tests/test_stl_binders.py   | 38 +++++++++++++++++++++++++++++++++++++
 4 files changed, 74 insertions(+), 6 deletions(-)

diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h
index e1bbf03aa..90407eb98 100644
--- a/include/pybind11/cast.h
+++ b/include/pybind11/cast.h
@@ -797,12 +797,20 @@ template <typename Container> struct is_copy_constructible<Container, enable_if_
         negation<std::is_same<Container, typename Container::value_type>>
     >::value>> : is_copy_constructible<typename Container::value_type> {};
 
-#if !defined(PYBIND11_CPP17)
-// Likewise for std::pair before C++17 (which mandates that the copy constructor not exist when the
-// two types aren't themselves copy constructible).
+// Likewise for std::pair
+// (after C++17 it is mandatory that the copy constructor not exist when the two types aren't themselves
+// copy constructible, but this can not be relied upon when T1 or T2 are themselves containers).
 template <typename T1, typename T2> struct is_copy_constructible<std::pair<T1, T2>>
     : all_of<is_copy_constructible<T1>, is_copy_constructible<T2>> {};
-#endif
+
+// The same problems arise with std::is_copy_assignable, so we use the same workaround.
+template <typename T, typename SFINAE = void> struct is_copy_assignable : std::is_copy_assignable<T> {};
+template <typename Container> struct is_copy_assignable<Container, enable_if_t<all_of<
+        std::is_copy_assignable<Container>,
+        std::is_same<typename Container::value_type &, typename Container::reference>
+    >::value>> : is_copy_assignable<typename Container::value_type> {};
+template <typename T1, typename T2> struct is_copy_assignable<std::pair<T1, T2>>
+    : all_of<is_copy_assignable<T1>, is_copy_assignable<T2>> {};
 
 NAMESPACE_END(detail)
 
diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h
index d3adaed3a..62bd90819 100644
--- a/include/pybind11/stl_bind.h
+++ b/include/pybind11/stl_bind.h
@@ -512,7 +512,7 @@ template <typename, typename, typename... Args> void map_assignment(const Args &
 
 // Map assignment when copy-assignable: just copy the value
 template <typename Map, typename Class_>
-void map_assignment(enable_if_t<std::is_copy_assignable<typename Map::mapped_type>::value, Class_> &cl) {
+void map_assignment(enable_if_t<is_copy_assignable<typename Map::mapped_type>::value, Class_> &cl) {
     using KeyType = typename Map::key_type;
     using MappedType = typename Map::mapped_type;
 
@@ -528,7 +528,7 @@ void map_assignment(enable_if_t<std::is_copy_assignable<typename Map::mapped_typ
 // Not copy-assignable, but still copy-constructible: we can update the value by erasing and reinserting
 template<typename Map, typename Class_>
 void map_assignment(enable_if_t<
-        !std::is_copy_assignable<typename Map::mapped_type>::value &&
+        !is_copy_assignable<typename Map::mapped_type>::value &&
         is_copy_constructible<typename Map::mapped_type>::value,
         Class_> &cl) {
     using KeyType = typename Map::key_type;
diff --git a/tests/test_stl_binders.cpp b/tests/test_stl_binders.cpp
index a88b589e1..868887409 100644
--- a/tests/test_stl_binders.cpp
+++ b/tests/test_stl_binders.cpp
@@ -54,6 +54,14 @@ template <class Map> Map *times_ten(int n) {
     return m;
 }
 
+template <class NestMap> NestMap *times_hundred(int n) {
+    auto m = new NestMap();
+    for (int i = 1; i <= n; i++)
+        for (int j = 1; j <= n; j++)
+            (*m)[i].emplace(int(j*10), E_nc(100*j));
+    return m;
+}
+
 TEST_SUBMODULE(stl_binders, m) {
     // test_vector_int
     py::bind_vector<std::vector<unsigned int>>(m, "VectorInt", py::buffer_protocol());
@@ -85,6 +93,20 @@ TEST_SUBMODULE(stl_binders, m) {
     m.def("get_mnc", &times_ten<std::map<int, E_nc>>, py::return_value_policy::reference);
     py::bind_map<std::unordered_map<int, E_nc>>(m, "UmapENC");
     m.def("get_umnc", &times_ten<std::unordered_map<int, E_nc>>, py::return_value_policy::reference);
+    // Issue #1885: binding nested std::map<X, Container<E>> with E non-copyable
+    py::bind_map<std::map<int, std::vector<E_nc>>>(m, "MapVecENC");
+    m.def("get_nvnc", [](int n)
+        {
+            auto m = new std::map<int, std::vector<E_nc>>();
+            for (int i = 1; i <= n; i++)
+                for (int j = 1; j <= n; j++)
+                    (*m)[i].emplace_back(j);
+            return m;
+        }, py::return_value_policy::reference);
+    py::bind_map<std::map<int, std::map<int, E_nc>>>(m, "MapMapENC");
+    m.def("get_nmnc", &times_hundred<std::map<int, std::map<int, E_nc>>>, py::return_value_policy::reference);
+    py::bind_map<std::unordered_map<int, std::unordered_map<int, E_nc>>>(m, "UmapUmapENC");
+    m.def("get_numnc", &times_hundred<std::unordered_map<int, std::unordered_map<int, E_nc>>>, py::return_value_policy::reference);
 
     // test_vector_buffer
     py::bind_vector<std::vector<unsigned char>>(m, "VectorUChar", py::buffer_protocol());
diff --git a/tests/test_stl_binders.py b/tests/test_stl_binders.py
index 6d5a15983..b83a587f2 100644
--- a/tests/test_stl_binders.py
+++ b/tests/test_stl_binders.py
@@ -212,6 +212,44 @@ def test_noncopyable_containers():
 
     assert vsum == 150
 
+    # nested std::map<std::vector>
+    nvnc = m.get_nvnc(5)
+    for i in range(1, 6):
+        for j in range(0, 5):
+            assert nvnc[i][j].value == j + 1
+
+    for k, v in nvnc.items():
+        for i, j in enumerate(v, start=1):
+            assert j.value == i
+
+    # nested std::map<std::map>
+    nmnc = m.get_nmnc(5)
+    for i in range(1, 6):
+        for j in range(10, 60, 10):
+            assert nmnc[i][j].value == 10 * j
+
+    vsum = 0
+    for k_o, v_o in nmnc.items():
+        for k_i, v_i in v_o.items():
+            assert v_i.value == 10 * k_i
+            vsum += v_i.value
+
+    assert vsum == 7500
+
+    # nested std::unordered_map<std::unordered_map>
+    numnc = m.get_numnc(5)
+    for i in range(1, 6):
+        for j in range(10, 60, 10):
+            assert numnc[i][j].value == 10 * j
+
+    vsum = 0
+    for k_o, v_o in numnc.items():
+        for k_i, v_i in v_o.items():
+            assert v_i.value == 10 * k_i
+            vsum += v_i.value
+
+    assert vsum == 7500
+
 
 def test_map_delitem():
     mm = m.MapStringDouble()