From 541c36003197808eb77c7eb5956b7f74c9bb96ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Franz=20P=C3=B6schel?= Date: Mon, 17 Apr 2023 15:25:01 +0200 Subject: [PATCH 01/16] Testing --- tests/test_stl_binders.cpp | 16 ++++++++++++++++ tests/test_stl_binders.py | 18 ++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/tests/test_stl_binders.cpp b/tests/test_stl_binders.cpp index ca9630bd19..6d1f76e95c 100644 --- a/tests/test_stl_binders.cpp +++ b/tests/test_stl_binders.cpp @@ -70,6 +70,19 @@ NestMap *times_hundred(int n) { return m; } +/* + * Recursive data structures as test for issue #4623 + */ +struct RecursiveVector : std::vector { + using Parent = std::vector; + using Parent::Parent; +}; + +struct RecursiveMap : std::map { + using Parent = std::map; + using Parent::Parent; +}; + TEST_SUBMODULE(stl_binders, m) { // test_vector_int py::bind_vector>(m, "VectorInt", py::buffer_protocol()); @@ -149,4 +162,7 @@ TEST_SUBMODULE(stl_binders, m) { m.def("get_vectorstruct", [] { return std::vector{{false, 5, 3.0, true}, {true, 30, -1e4, false}}; }); + + py::bind_vector(m, "RecursiveVector"); + py::bind_map(m, "RecursiveMap"); } diff --git a/tests/test_stl_binders.py b/tests/test_stl_binders.py index 7dca742a9e..47d239a64d 100644 --- a/tests/test_stl_binders.py +++ b/tests/test_stl_binders.py @@ -335,3 +335,21 @@ def test_map_view_types(): assert type(unordered_map_string_double.items()) is items_type assert type(map_string_double_const.items()) is items_type assert type(unordered_map_string_double_const.items()) is items_type + + +def test_recursive_containers(): + recursive_vector = m.RecursiveVector() + recursive_vector.append(m.RecursiveVector()) + recursive_vector[0].append(m.RecursiveVector()) + recursive_vector[0][0].append(m.RecursiveVector()) + recursive_vector[0][0].append(m.RecursiveVector()) + # Can't use len() since test_stl_binders.cpp does not include stl.h, + # so the necessary conversion is missing + assert recursive_vector[0][0].count(m.RecursiveVector()) == 2 + + recursive_map = m.RecursiveMap() + recursive_map[1] = m.RecursiveMap() + recursive_map[1][1] = m.RecursiveMap() + recursive_map[1][1][1] = m.RecursiveMap() + recursive_map[1][1][2] = m.RecursiveMap() + assert list(recursive_map[1][1].keys()) == [1, 2] From 2c6002cf9fef5dd5266bf6b883ca5b3dfdceeee8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Franz=20P=C3=B6schel?= Date: Mon, 17 Apr 2023 15:07:18 +0200 Subject: [PATCH 02/16] Similar fix for std::vector --- include/pybind11/stl_bind.h | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index 7cfd996dda..e1d26bf111 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -61,10 +61,21 @@ struct is_comparable< /* For a vector/map data structure, recursively check the value type (which is std::pair for maps) */ template -struct is_comparable::is_vector>> { +struct is_comparable::is_vector && + // Avoid this instantiation if the type is recursive + negation>::value>> { static constexpr const bool value = is_comparable::value; }; +template +struct is_comparable::is_vector && + // Special case: The vector type is recursive + std::is_same::value>> { + static constexpr const bool value = true; +}; + /* For pairs, recursively check the two data types */ template struct is_comparable::is_pair>> { From 4c59860baafa8a33f9821729e96d733e00c764f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Franz=20P=C3=B6schel?= Date: Fri, 14 Apr 2023 10:37:24 +0200 Subject: [PATCH 03/16] Fix infinite recursion check: 1) Apply to is_copy_assignable additionally 2) Check infinite recursion for map-like types --- include/pybind11/detail/type_caster_base.h | 26 +++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 34dcd26ce5..b98efacfaf 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -830,6 +830,20 @@ struct is_copy_constructible : std::is_copy_constructible {}; template struct is_move_constructible : std::is_move_constructible {}; +// True if Container has a dependent type mapped_type that is equivalent +// to Container itself +template +struct map_self_referential +{ + constexpr static bool value = false; +}; + +template +struct map_self_referential +{ + constexpr static bool value = true; +}; + // Specialization for types that appear to be copy constructible but also look like stl containers // (we specifically check for: has `value_type` and `reference` with `reference = value_type&`): if // so, copy constructability depends on whether the value_type is copy constructible. @@ -839,8 +853,10 @@ struct is_copy_constructible< enable_if_t< all_of, std::is_same, - // Avoid infinite recursion - negation>>::value>> + // Avoid infinite recursion (list types) + negation>, + // Avoid infinite recursion (map types) + negation>>::value>> : is_copy_constructible {}; // Likewise for std::pair @@ -858,7 +874,11 @@ template struct is_copy_assignable, std::is_same>::value>> + typename Container::reference>, + // Avoid infinite recursion (list types) + negation>, + // Avoid infinite recursion (map types) + negation>>::value>> : is_copy_assignable {}; template struct is_copy_assignable> From 2c48ca975cd8f68f929a2464617824d9d9f9beb7 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 17 Apr 2023 13:28:21 +0000 Subject: [PATCH 04/16] style: pre-commit fixes --- include/pybind11/detail/type_caster_base.h | 27 +++++++++++----------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index b98efacfaf..979b07a292 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -832,15 +832,13 @@ struct is_move_constructible : std::is_move_constructible {}; // True if Container has a dependent type mapped_type that is equivalent // to Container itself -template -struct map_self_referential -{ +template +struct map_self_referential { constexpr static bool value = false; }; -template -struct map_self_referential -{ +template +struct map_self_referential { constexpr static bool value = true; }; @@ -871,14 +869,15 @@ struct is_copy_constructible> template struct is_copy_assignable : std::is_copy_assignable {}; template -struct is_copy_assignable, - std::is_same, - // Avoid infinite recursion (list types) - negation>, - // Avoid infinite recursion (map types) - negation>>::value>> +struct is_copy_assignable< + Container, + enable_if_t< + all_of, + std::is_same, + // Avoid infinite recursion (list types) + negation>, + // Avoid infinite recursion (map types) + negation>>::value>> : is_copy_assignable {}; template struct is_copy_assignable> From 8d35d800a3de518335e593668f27b391cd0e77d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Franz=20P=C3=B6schel?= Date: Mon, 17 Apr 2023 15:39:07 +0200 Subject: [PATCH 05/16] Optional commit that demonstrates the limitations of this PR --- tests/test_stl_binders.cpp | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/test_stl_binders.cpp b/tests/test_stl_binders.cpp index 6d1f76e95c..e38ad3df96 100644 --- a/tests/test_stl_binders.cpp +++ b/tests/test_stl_binders.cpp @@ -83,6 +83,32 @@ struct RecursiveMap : std::map { using Parent::Parent; }; +/* + * Pybind11 does not catch more complicated recursion schemes, such as mutual + * recursion. + * In that case, an alternative is to add a custom overload. + */ +struct MutuallyRecursiveMap; +struct MutuallyRecursiveVector; + +struct MutuallyRecursiveMap : std::map {}; +struct MutuallyRecursiveVector : std::vector {}; + +namespace pybind11 { +namespace detail { +template <> +struct is_comparable : std::true_type {}; +template <> +struct is_copy_assignable : std::true_type {}; +template <> +struct is_copy_assignable : std::true_type {}; +template <> +struct is_copy_constructible : std::true_type {}; +template <> +struct is_copy_constructible : std::true_type {}; +} // namespace detail +} // namespace pybind11 + TEST_SUBMODULE(stl_binders, m) { // test_vector_int py::bind_vector>(m, "VectorInt", py::buffer_protocol()); @@ -165,4 +191,6 @@ TEST_SUBMODULE(stl_binders, m) { py::bind_vector(m, "RecursiveVector"); py::bind_map(m, "RecursiveMap"); + py::bind_map(m, "MutuallyRecursiveMap"); + py::bind_vector(m, "MutuallyRecursiveVector"); } From 79782b5f2a31cb49352ef4e872fe56a2b0c333c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Franz=20P=C3=B6schel?= Date: Wed, 19 Apr 2023 10:26:25 +0200 Subject: [PATCH 06/16] Fix positioning of container bindings The bindings were previously in a block that was only activated if numpy was available. --- tests/test_stl_binders.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/test_stl_binders.cpp b/tests/test_stl_binders.cpp index e38ad3df96..28cee06053 100644 --- a/tests/test_stl_binders.cpp +++ b/tests/test_stl_binders.cpp @@ -168,6 +168,12 @@ TEST_SUBMODULE(stl_binders, m) { m, "VectorUndeclStruct", py::buffer_protocol()); }); + // Bind recursive container types + py::bind_vector(m, "RecursiveVector"); + py::bind_map(m, "RecursiveMap"); + py::bind_map(m, "MutuallyRecursiveMap"); + py::bind_vector(m, "MutuallyRecursiveVector"); + // The rest depends on numpy: try { py::module_::import("numpy"); @@ -188,9 +194,4 @@ TEST_SUBMODULE(stl_binders, m) { m.def("get_vectorstruct", [] { return std::vector{{false, 5, 3.0, true}, {true, 30, -1e4, false}}; }); - - py::bind_vector(m, "RecursiveVector"); - py::bind_map(m, "RecursiveMap"); - py::bind_map(m, "MutuallyRecursiveMap"); - py::bind_vector(m, "MutuallyRecursiveVector"); } From ba0aadf99d890c9cd71bb241fffceb6e7aa67a34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Franz=20P=C3=B6schel?= Date: Fri, 21 Apr 2023 12:03:32 +0200 Subject: [PATCH 07/16] Suggestions from code review: API side --- include/pybind11/detail/type_caster_base.h | 39 ++++++++++++++-------- include/pybind11/stl_bind.h | 9 ++--- tests/test_stl_binders.cpp | 10 ++---- 3 files changed, 32 insertions(+), 26 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 979b07a292..675e8622dc 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -832,15 +832,30 @@ struct is_move_constructible : std::is_move_constructible {}; // True if Container has a dependent type mapped_type that is equivalent // to Container itself +// Actual implementation in the SFINAE specializations below template -struct map_self_referential { - constexpr static bool value = false; -}; +struct sfinae_is_container_with_self_referential_mapped_type : std::false_type {}; +// Tie-breaking between the mapped_type and the value_type specializations is trivial: +// The specializations are only valid if both conditions are fulfilled: +// 1) The mapped_type (respectively value_type) exists +// 2) And it is equivalent to Container +// So, in each case, only one specialization will activate. template -struct map_self_referential { - constexpr static bool value = true; -}; +struct sfinae_is_container_with_self_referential_mapped_type + : std::true_type {}; + +template +struct sfinae_is_container_with_self_referential_mapped_type + : std::true_type {}; + +// Use a helper struct in order to give this a nicer public API without helper template parameter. +// This makes this struct nicer to specialize by users. +template +struct is_container_with_self_referential_mapped_type + : sfinae_is_container_with_self_referential_mapped_type {}; // Specialization for types that appear to be copy constructible but also look like stl containers // (we specifically check for: has `value_type` and `reference` with `reference = value_type&`): if @@ -851,10 +866,8 @@ struct is_copy_constructible< enable_if_t< all_of, std::is_same, - // Avoid infinite recursion (list types) - negation>, - // Avoid infinite recursion (map types) - negation>>::value>> + // Avoid infinite recursion + negation>>::value>> : is_copy_constructible {}; // Likewise for std::pair @@ -874,10 +887,8 @@ struct is_copy_assignable< enable_if_t< all_of, std::is_same, - // Avoid infinite recursion (list types) - negation>, - // Avoid infinite recursion (map types) - negation>>::value>> + // Avoid infinite recursion + negation>>::value>> : is_copy_assignable {}; template struct is_copy_assignable> diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index e1d26bf111..069d9dc70e 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -61,10 +61,11 @@ struct is_comparable< /* For a vector/map data structure, recursively check the value type (which is std::pair for maps) */ template -struct is_comparable::is_vector && - // Avoid this instantiation if the type is recursive - negation>::value>> { +struct is_comparable< + T, + enable_if_t::is_vector && + // Avoid this instantiation if the type is recursive + negation>::value>> { static constexpr const bool value = is_comparable::value; }; diff --git a/tests/test_stl_binders.cpp b/tests/test_stl_binders.cpp index 28cee06053..8cf6caf65b 100644 --- a/tests/test_stl_binders.cpp +++ b/tests/test_stl_binders.cpp @@ -97,15 +97,9 @@ struct MutuallyRecursiveVector : std::vector {}; namespace pybind11 { namespace detail { template <> -struct is_comparable : std::true_type {}; +struct is_container_with_self_referential_mapped_type : std::true_type {}; template <> -struct is_copy_assignable : std::true_type {}; -template <> -struct is_copy_assignable : std::true_type {}; -template <> -struct is_copy_constructible : std::true_type {}; -template <> -struct is_copy_constructible : std::true_type {}; +struct is_container_with_self_referential_mapped_type : std::true_type {}; } // namespace detail } // namespace pybind11 From 0ecf5f01f8f956a4cb5a81179b25836639fd5f67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Franz=20P=C3=B6schel?= Date: Fri, 21 Apr 2023 12:09:26 +0200 Subject: [PATCH 08/16] Suggestions from code review: Test side --- tests/test_stl_binders.cpp | 22 +++++++++++++--------- tests/test_stl_binders.py | 18 +++++++++--------- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/tests/test_stl_binders.cpp b/tests/test_stl_binders.cpp index 8cf6caf65b..c921033321 100644 --- a/tests/test_stl_binders.cpp +++ b/tests/test_stl_binders.cpp @@ -86,20 +86,24 @@ struct RecursiveMap : std::map { /* * Pybind11 does not catch more complicated recursion schemes, such as mutual * recursion. - * In that case, an alternative is to add a custom overload. + * In that case, an alternative is to add a custom specialization to + * pybind11::detail::is_container_with_self_referential_mapped_type, thus + * manually telling pybind11 about the recursion. */ -struct MutuallyRecursiveMap; -struct MutuallyRecursiveVector; +struct MutuallyRecursiveContainerPairA; +struct MutuallyRecursiveContainerPairB; -struct MutuallyRecursiveMap : std::map {}; -struct MutuallyRecursiveVector : std::vector {}; +struct MutuallyRecursiveContainerPairA : std::map {}; +struct MutuallyRecursiveContainerPairB : std::vector {}; namespace pybind11 { namespace detail { template <> -struct is_container_with_self_referential_mapped_type : std::true_type {}; +struct is_container_with_self_referential_mapped_type + : std::true_type {}; template <> -struct is_container_with_self_referential_mapped_type : std::true_type {}; +struct is_container_with_self_referential_mapped_type + : std::true_type {}; } // namespace detail } // namespace pybind11 @@ -165,8 +169,8 @@ TEST_SUBMODULE(stl_binders, m) { // Bind recursive container types py::bind_vector(m, "RecursiveVector"); py::bind_map(m, "RecursiveMap"); - py::bind_map(m, "MutuallyRecursiveMap"); - py::bind_vector(m, "MutuallyRecursiveVector"); + py::bind_map(m, "MutuallyRecursiveContainerPairA"); + py::bind_vector(m, "MutuallyRecursiveContainerPairB"); // The rest depends on numpy: try { diff --git a/tests/test_stl_binders.py b/tests/test_stl_binders.py index 47d239a64d..e002f5b678 100644 --- a/tests/test_stl_binders.py +++ b/tests/test_stl_binders.py @@ -337,19 +337,19 @@ def test_map_view_types(): assert type(unordered_map_string_double_const.items()) is items_type -def test_recursive_containers(): +def test_recursive_vector(): recursive_vector = m.RecursiveVector() recursive_vector.append(m.RecursiveVector()) recursive_vector[0].append(m.RecursiveVector()) - recursive_vector[0][0].append(m.RecursiveVector()) - recursive_vector[0][0].append(m.RecursiveVector()) + recursive_vector[0].append(m.RecursiveVector()) # Can't use len() since test_stl_binders.cpp does not include stl.h, # so the necessary conversion is missing - assert recursive_vector[0][0].count(m.RecursiveVector()) == 2 + assert recursive_vector[0].count(m.RecursiveVector()) == 2 + +def test_recursive_map(): recursive_map = m.RecursiveMap() - recursive_map[1] = m.RecursiveMap() - recursive_map[1][1] = m.RecursiveMap() - recursive_map[1][1][1] = m.RecursiveMap() - recursive_map[1][1][2] = m.RecursiveMap() - assert list(recursive_map[1][1].keys()) == [1, 2] + recursive_map[100] = m.RecursiveMap() + recursive_map[100][101] = m.RecursiveMap() + recursive_map[100][102] = m.RecursiveMap() + assert list(recursive_map[100].keys()) == [101, 102] From 9ba38bf9bf144424fa58ac5a60c34f06a74435ba Mon Sep 17 00:00:00 2001 From: Franz Poeschel Date: Mon, 24 Apr 2023 15:22:34 +0200 Subject: [PATCH 09/16] Suggestions from code review 1) Renaming: is_recursive_container and MutuallyRecursiveContainerPair(MV|VM) 2) Avoid ambiguous specializations of is_recursive_container --- include/pybind11/detail/type_caster_base.h | 42 +++++++++++++--------- include/pybind11/stl_bind.h | 9 +++-- tests/test_stl_binders.cpp | 22 ++++++------ 3 files changed, 39 insertions(+), 34 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 675e8622dc..f1cf689a0f 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -832,30 +832,38 @@ struct is_move_constructible : std::is_move_constructible {}; // True if Container has a dependent type mapped_type that is equivalent // to Container itself -// Actual implementation in the SFINAE specializations below +// Actual implementation in the SFINAE specialization below template -struct sfinae_is_container_with_self_referential_mapped_type : std::false_type {}; +struct is_container_with_recursive_mapped_type : std::false_type {}; -// Tie-breaking between the mapped_type and the value_type specializations is trivial: -// The specializations are only valid if both conditions are fulfilled: -// 1) The mapped_type (respectively value_type) exists +// This specialization is only valid if both conditions are fulfilled: +// 1) The mapped_type exists // 2) And it is equivalent to Container -// So, in each case, only one specialization will activate. template -struct sfinae_is_container_with_self_referential_mapped_type +struct is_container_with_recursive_mapped_type : std::true_type {}; +// True if Container has a dependent type value_type that is equivalent +// to Container itself +// Actual implementation in the SFINAE specialization below +template +struct is_container_with_recursive_value_type : std::false_type {}; + +// This specialization is only valid if both conditions are fulfilled: +// 1) The value_type exists +// 2) And it is equivalent to Container template -struct sfinae_is_container_with_self_referential_mapped_type +struct is_container_with_recursive_value_type : std::true_type {}; -// Use a helper struct in order to give this a nicer public API without helper template parameter. -// This makes this struct nicer to specialize by users. -template -struct is_container_with_self_referential_mapped_type - : sfinae_is_container_with_self_referential_mapped_type {}; +// True constant if the type contains itself recursively. +// By default, this will check the mapped_type and value_type dependent types. +// In more complex recursion patterns, users can specialize this struct. +// The second template parameter SFINAE=void is for use of std::enable_if in specializations. +// An example is found in tests/test_stl_binders.cpp. +template +struct is_recursive_container : any_of, + is_container_with_recursive_mapped_type> {}; // Specialization for types that appear to be copy constructible but also look like stl containers // (we specifically check for: has `value_type` and `reference` with `reference = value_type&`): if @@ -867,7 +875,7 @@ struct is_copy_constructible< all_of, std::is_same, // Avoid infinite recursion - negation>>::value>> + negation>>::value>> : is_copy_constructible {}; // Likewise for std::pair @@ -888,7 +896,7 @@ struct is_copy_assignable< all_of, std::is_same, // Avoid infinite recursion - negation>>::value>> + negation>>::value>> : is_copy_assignable {}; template struct is_copy_assignable> diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index 069d9dc70e..82752ecb88 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -61,11 +61,10 @@ struct is_comparable< /* For a vector/map data structure, recursively check the value type (which is std::pair for maps) */ template -struct is_comparable< - T, - enable_if_t::is_vector && - // Avoid this instantiation if the type is recursive - negation>::value>> { +struct is_comparable::is_vector && + // Avoid this instantiation if the type is recursive + negation>::value>> { static constexpr const bool value = is_comparable::value; }; diff --git a/tests/test_stl_binders.cpp b/tests/test_stl_binders.cpp index c921033321..9eca353f00 100644 --- a/tests/test_stl_binders.cpp +++ b/tests/test_stl_binders.cpp @@ -90,20 +90,18 @@ struct RecursiveMap : std::map { * pybind11::detail::is_container_with_self_referential_mapped_type, thus * manually telling pybind11 about the recursion. */ -struct MutuallyRecursiveContainerPairA; -struct MutuallyRecursiveContainerPairB; +struct MutuallyRecursiveContainerPairMV; +struct MutuallyRecursiveContainerPairVM; -struct MutuallyRecursiveContainerPairA : std::map {}; -struct MutuallyRecursiveContainerPairB : std::vector {}; +struct MutuallyRecursiveContainerPairMV : std::map {}; +struct MutuallyRecursiveContainerPairVM : std::vector {}; namespace pybind11 { namespace detail { -template <> -struct is_container_with_self_referential_mapped_type - : std::true_type {}; -template <> -struct is_container_with_self_referential_mapped_type - : std::true_type {}; +template +struct is_recursive_container : std::true_type {}; +template +struct is_recursive_container : std::true_type {}; } // namespace detail } // namespace pybind11 @@ -169,8 +167,8 @@ TEST_SUBMODULE(stl_binders, m) { // Bind recursive container types py::bind_vector(m, "RecursiveVector"); py::bind_map(m, "RecursiveMap"); - py::bind_map(m, "MutuallyRecursiveContainerPairA"); - py::bind_vector(m, "MutuallyRecursiveContainerPairB"); + py::bind_map(m, "MutuallyRecursiveContainerPairMV"); + py::bind_vector(m, "MutuallyRecursiveContainerPairVM"); // The rest depends on numpy: try { From 531ceb4f66120383fd99ba299d110292b05a68ef Mon Sep 17 00:00:00 2001 From: Franz Poeschel Date: Tue, 25 Apr 2023 10:43:52 +0200 Subject: [PATCH 10/16] Some little fixes --- include/pybind11/stl_bind.h | 5 +++-- tests/test_stl_binders.cpp | 5 ++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index 82752ecb88..e90a228937 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -68,12 +68,13 @@ struct is_comparable::value; }; +/* Skip the recursion if the type itself is recursive */ template struct is_comparable::is_vector && // Special case: The vector type is recursive - std::is_same::value>> { - static constexpr const bool value = true; + is_recursive_container::value>> { + static constexpr const bool value = container_traits::is_comparable; }; /* For pairs, recursively check the two data types */ diff --git a/tests/test_stl_binders.cpp b/tests/test_stl_binders.cpp index 9eca353f00..d7c0c4942f 100644 --- a/tests/test_stl_binders.cpp +++ b/tests/test_stl_binders.cpp @@ -86,9 +86,8 @@ struct RecursiveMap : std::map { /* * Pybind11 does not catch more complicated recursion schemes, such as mutual * recursion. - * In that case, an alternative is to add a custom specialization to - * pybind11::detail::is_container_with_self_referential_mapped_type, thus - * manually telling pybind11 about the recursion. + * In that case custom is_recursive_container specializations need to be added, + * thus manually telling pybind11 about the recursion. */ struct MutuallyRecursiveContainerPairMV; struct MutuallyRecursiveContainerPairVM; From 1bd1e7e509912b6d6a15c88af4ebea615b18ff86 Mon Sep 17 00:00:00 2001 From: Franz Poeschel Date: Tue, 25 Apr 2023 10:48:55 +0200 Subject: [PATCH 11/16] Reordering of structs --- include/pybind11/detail/type_caster_base.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index f1cf689a0f..3b42f66c93 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -822,14 +822,6 @@ using movable_cast_op_type typename std::add_rvalue_reference>::type, typename std::add_lvalue_reference>::type>>; -// std::is_copy_constructible isn't quite enough: it lets std::vector (and similar) through when -// T is non-copyable, but code containing such a copy constructor fails to actually compile. -template -struct is_copy_constructible : std::is_copy_constructible {}; - -template -struct is_move_constructible : std::is_move_constructible {}; - // True if Container has a dependent type mapped_type that is equivalent // to Container itself // Actual implementation in the SFINAE specialization below @@ -865,6 +857,14 @@ template struct is_recursive_container : any_of, is_container_with_recursive_mapped_type> {}; +template +struct is_move_constructible : std::is_move_constructible {}; + +// std::is_copy_constructible isn't quite enough: it lets std::vector (and similar) through when +// T is non-copyable, but code containing such a copy constructor fails to actually compile. +template +struct is_copy_constructible : std::is_copy_constructible {}; + // Specialization for types that appear to be copy constructible but also look like stl containers // (we specifically check for: has `value_type` and `reference` with `reference = value_type&`): if // so, copy constructability depends on whether the value_type is copy constructible. From a839f8f7ad1ca035db6bb37e58f74f802740b7e5 Mon Sep 17 00:00:00 2001 From: Franz Poeschel Date: Tue, 25 Apr 2023 10:53:05 +0200 Subject: [PATCH 12/16] Add recursive checks for is_move_constructible --- include/pybind11/detail/type_caster_base.h | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 3b42f66c93..a1edb5522b 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -860,6 +860,27 @@ struct is_recursive_container : any_of struct is_move_constructible : std::is_move_constructible {}; +// Specialization for types that appear to be move constructible but also look like stl containers +// (we specifically check for: has `value_type` and `reference` with `reference = value_type&`): if +// so, move constructability depends on whether the value_type is move constructible. +template +struct is_move_constructible< + Container, + enable_if_t< + all_of, + std::is_same, + // Avoid infinite recursion + negation>>::value>> + : is_move_constructible {}; + +// Likewise for std::pair +// (after C++17 it is mandatory that the move constructor not exist when the two types aren't +// themselves move constructible, but this can not be relied upon when T1 or T2 are themselves +// containers). +template +struct is_move_constructible> + : all_of, is_move_constructible> {}; + // std::is_copy_constructible isn't quite enough: it lets std::vector (and similar) through when // T is non-copyable, but code containing such a copy constructor fails to actually compile. template From eba6239ab62390eed8175a768954b4fac5ea515d Mon Sep 17 00:00:00 2001 From: Franz Poeschel Date: Fri, 28 Apr 2023 11:21:40 +0200 Subject: [PATCH 13/16] Static testing for pybind11 type traits --- include/pybind11/detail/type_caster_base.h | 6 +- tests/test_copy_move.cpp | 238 +++++++++++++++++++++ 2 files changed, 243 insertions(+), 1 deletion(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index a1edb5522b..20e9c40e8e 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -921,7 +921,11 @@ struct is_copy_assignable< : is_copy_assignable {}; template struct is_copy_assignable> - : all_of, is_copy_assignable> {}; + /* + * Need to remove the const qualifier from T1 here since the value_type in + * STL map types is std::pair, and const types are never assignable + */ + : all_of::type>, is_copy_assignable> {}; PYBIND11_NAMESPACE_END(detail) diff --git a/tests/test_copy_move.cpp b/tests/test_copy_move.cpp index 28c2445644..705115f07f 100644 --- a/tests/test_copy_move.cpp +++ b/tests/test_copy_move.cpp @@ -13,6 +13,8 @@ #include "constructor_stats.h" #include "pybind11_tests.h" +#include + template struct empty { static const derived &get_one() { return instance_; } @@ -293,3 +295,239 @@ TEST_SUBMODULE(copy_move_policies, m) { // Make sure that cast from pytype rvalue to other pytype works m.def("get_pytype_rvalue_castissue", [](double i) { return py::float_(i).cast(); }); } + +/* + * Rest of the file: + * static_assert based tests for pybind11 adaptations of of + * std::is_move_constructible, std::is_copy_constructible and + * std::is_copy_assignable (no adaptation of std::is_move_assignable). + * Difference between pybind11 and std traits: pybind11 traits will also check + * the contained value_types. + */ + +struct NotMovable { + NotMovable() = default; + NotMovable(NotMovable const &) = default; + NotMovable(NotMovable &&) = delete; + NotMovable &operator=(NotMovable const &) = default; + NotMovable &operator=(NotMovable &&) = delete; +}; +static_assert(!std::is_move_constructible::value, + "!std::is_move_constructible::value"); +static_assert(std::is_copy_constructible::value, + "std::is_copy_constructible::value"); +static_assert(!pybind11::detail::is_move_constructible::value, + "!pybind11::detail::is_move_constructible::value"); +static_assert(pybind11::detail::is_copy_constructible::value, + "pybind11::detail::is_copy_constructible::value"); +static_assert(!std::is_move_assignable::value, + "!std::is_move_assignable::value"); +static_assert(std::is_copy_assignable::value, + "std::is_copy_assignable::value"); +// pybind11 does not have this +// static_assert(!pybind11::detail::is_move_assignable::value, +// "!pybind11::detail::is_move_assignable::value"); +static_assert(pybind11::detail::is_copy_assignable::value, + "pybind11::detail::is_copy_assignable::value"); + +struct NotCopyable { + NotCopyable() = default; + NotCopyable(NotCopyable const &) = delete; + NotCopyable(NotCopyable &&) = default; + NotCopyable &operator=(NotCopyable const &) = delete; + NotCopyable &operator=(NotCopyable &&) = default; +}; +static_assert(std::is_move_constructible::value, + "std::is_move_constructible::value"); +static_assert(!std::is_copy_constructible::value, + "!std::is_copy_constructible::value"); +static_assert(pybind11::detail::is_move_constructible::value, + "pybind11::detail::is_move_constructible::value"); +static_assert(!pybind11::detail::is_copy_constructible::value, + "!pybind11::detail::is_copy_constructible::value"); +static_assert(std::is_move_assignable::value, + "std::is_move_assignable::value"); +static_assert(!std::is_copy_assignable::value, + "!std::is_copy_assignable::value"); +// pybind11 does not have this +// static_assert(!pybind11::detail::is_move_assignable::value, +// "!pybind11::detail::is_move_assignable::value"); +static_assert(!pybind11::detail::is_copy_assignable::value, + "!pybind11::detail::is_copy_assignable::value"); + +struct NotCopyableNotMovable { + NotCopyableNotMovable() = default; + NotCopyableNotMovable(NotCopyableNotMovable const &) = delete; + NotCopyableNotMovable(NotCopyableNotMovable &&) = delete; + NotCopyableNotMovable &operator=(NotCopyableNotMovable const &) = delete; + NotCopyableNotMovable &operator=(NotCopyableNotMovable &&) = delete; +}; +static_assert(!std::is_move_constructible::value, + "!std::is_move_constructible::value"); +static_assert(!std::is_copy_constructible::value, + "!std::is_copy_constructible::value"); +static_assert(!pybind11::detail::is_move_constructible::value, + "!pybind11::detail::is_move_constructible::value"); +static_assert(!pybind11::detail::is_copy_constructible::value, + "!pybind11::detail::is_copy_constructible::value"); +static_assert(!std::is_move_assignable::value, + "!std::is_move_assignable::value"); +static_assert(!std::is_copy_assignable::value, + "!std::is_copy_assignable::value"); +// pybind11 does not have this +// static_assert(!pybind11::detail::is_move_assignable::value, +// "!pybind11::detail::is_move_assignable::value"); +static_assert(!pybind11::detail::is_copy_assignable::value, + "!pybind11::detail::is_copy_assignable::value"); + +struct NotMovableVector : std::vector {}; +static_assert(std::is_move_constructible::value, + "std::is_move_constructible::value"); +static_assert(std::is_copy_constructible::value, + "std::is_copy_constructible::value"); +static_assert(!pybind11::detail::is_move_constructible::value, + "!pybind11::detail::is_move_constructible::value"); +static_assert(pybind11::detail::is_copy_constructible::value, + "pybind11::detail::is_copy_constructible::value"); +static_assert(std::is_move_assignable::value, + "std::is_move_assignable::value"); +static_assert(std::is_copy_assignable::value, + "std::is_copy_assignable::value"); +// pybind11 does not have this +// static_assert(!pybind11::detail::is_move_assignable::value, +// "!pybind11::detail::is_move_assignable::value"); +static_assert(pybind11::detail::is_copy_assignable::value, + "pybind11::detail::is_copy_assignable::value"); + +struct NotCopyableVector : std::vector {}; +static_assert(std::is_move_constructible::value, + "std::is_move_constructible::value"); +static_assert(std::is_copy_constructible::value, + "std::is_copy_constructible::value"); +static_assert(pybind11::detail::is_move_constructible::value, + "pybind11::detail::is_move_constructible::value"); +static_assert(!pybind11::detail::is_copy_constructible::value, + "!pybind11::detail::is_copy_constructible::value"); +static_assert(std::is_move_assignable::value, + "std::is_move_assignable::value"); +static_assert(std::is_copy_assignable::value, + "std::is_copy_assignable::value"); +// pybind11 does not have this +// static_assert(!pybind11::detail::is_move_assignable::value, +// "!pybind11::detail::is_move_assignable::value"); +static_assert(!pybind11::detail::is_copy_assignable::value, + "!pybind11::detail::is_copy_assignable::value"); + +struct NotCopyableNotMovableVector : std::vector {}; +static_assert(std::is_move_constructible::value, + "std::is_move_constructible::value"); +static_assert(std::is_copy_constructible::value, + "std::is_copy_constructible::value"); +static_assert(!pybind11::detail::is_move_constructible::value, + "!pybind11::detail::is_move_constructible::value"); +static_assert(!pybind11::detail::is_copy_constructible::value, + "!pybind11::detail::is_copy_constructible::value"); +static_assert(std::is_move_assignable::value, + "std::is_move_assignable::value"); +static_assert(std::is_copy_assignable::value, + "std::is_copy_assignable::value"); +// pybind11 does not have this +// static_assert(!pybind11::detail::is_move_assignable::value, +// "!pybind11::detail::is_move_assignable::value"); +static_assert(!pybind11::detail::is_copy_assignable::value, + "!pybind11::detail::is_copy_assignable::value"); + +struct NotMovableMap : std::map {}; +static_assert(std::is_move_constructible::value, + "std::is_move_constructible::value"); +static_assert(std::is_copy_constructible::value, + "std::is_copy_constructible::value"); +static_assert(!pybind11::detail::is_move_constructible::value, + "!pybind11::detail::is_move_constructible::value"); +static_assert(pybind11::detail::is_copy_constructible::value, + "pybind11::detail::is_copy_constructible::value"); +static_assert(std::is_move_assignable::value, + "std::is_move_assignable::value"); +static_assert(std::is_copy_assignable::value, + "std::is_copy_assignable::value"); +// pybind11 does not have this +// static_assert(!pybind11::detail::is_move_assignable::value, +// "!pybind11::detail::is_move_assignable::value"); +static_assert(pybind11::detail::is_copy_assignable::value, + "pybind11::detail::is_copy_assignable::value"); + +struct NotCopyableMap : std::map {}; +static_assert(std::is_move_constructible::value, + "std::is_move_constructible::value"); +static_assert(std::is_copy_constructible::value, + "std::is_copy_constructible::value"); +static_assert(pybind11::detail::is_move_constructible::value, + "pybind11::detail::is_move_constructible::value"); +static_assert(!pybind11::detail::is_copy_constructible::value, + "!pybind11::detail::is_copy_constructible::value"); +static_assert(std::is_move_assignable::value, + "std::is_move_assignable::value"); +static_assert(std::is_copy_assignable::value, + "std::is_copy_assignable::value"); +// pybind11 does not have this +// static_assert(!pybind11::detail::is_move_assignable::value, +// "!pybind11::detail::is_move_assignable::value"); +static_assert(!pybind11::detail::is_copy_assignable::value, + "!pybind11::detail::is_copy_assignable::value"); + +struct NotCopyableNotMovableMap : std::map {}; +static_assert(std::is_move_constructible::value, + "std::is_move_constructible::value"); +static_assert(std::is_copy_constructible::value, + "std::is_copy_constructible::value"); +static_assert(!pybind11::detail::is_move_constructible::value, + "!pybind11::detail::is_move_constructible::value"); +static_assert(!pybind11::detail::is_copy_constructible::value, + "!pybind11::detail::is_copy_constructible::value"); +static_assert(std::is_move_assignable::value, + "std::is_move_assignable::value"); +static_assert(std::is_copy_assignable::value, + "std::is_copy_assignable::value"); +// pybind11 does not have this +// static_assert(!pybind11::detail::is_move_assignable::value, +// "!pybind11::detail::is_move_assignable::value"); +static_assert(!pybind11::detail::is_copy_assignable::value, + "!pybind11::detail::is_copy_assignable::value"); + +struct RecursiveVector : std::vector {}; +static_assert(std::is_move_constructible::value, + "std::is_move_constructible::value"); +static_assert(std::is_copy_constructible::value, + "std::is_copy_constructible::value"); +static_assert(pybind11::detail::is_move_constructible::value, + "pybind11::detail::is_move_constructible::value"); +static_assert(pybind11::detail::is_copy_constructible::value, + "pybind11::detail::is_copy_constructible::value"); +static_assert(std::is_move_assignable::value, + "std::is_move_assignable::value"); +static_assert(std::is_copy_assignable::value, + "std::is_copy_assignable::value"); +// pybind11 does not have this +// static_assert(!pybind11::detail::is_move_assignable::value, +// "!pybind11::detail::is_move_assignable::value"); +static_assert(pybind11::detail::is_copy_assignable::value, + "pybind11::detail::is_copy_assignable::value"); + +struct RecursiveMap : std::map {}; +static_assert(std::is_move_constructible::value, + "std::is_move_constructible::value"); +static_assert(std::is_copy_constructible::value, + "std::is_copy_constructible::value"); +static_assert(pybind11::detail::is_move_constructible::value, + "pybind11::detail::is_move_constructible::value"); +static_assert(pybind11::detail::is_copy_constructible::value, + "pybind11::detail::is_copy_constructible::value"); +static_assert(std::is_move_assignable::value, + "std::is_move_assignable::value"); +static_assert(std::is_copy_assignable::value, + "std::is_copy_assignable::value"); +// pybind11 does not have this +// static_assert(!pybind11::detail::is_move_assignable::value, +// "!pybind11::detail::is_move_assignable::value"); +static_assert(pybind11::detail::is_copy_assignable::value, + "pybind11::detail::is_copy_assignable::value"); From 77e2c56b09319d9d47be6ffc3feabf94d4b6b080 Mon Sep 17 00:00:00 2001 From: Franz Poeschel Date: Tue, 2 May 2023 15:18:55 +0200 Subject: [PATCH 14/16] More precise checking of recursive types Instead of a trait `is_recursive_container`, use a trait `recursive_container_traits` with dependent type `recursive_container_traits::type_to_check_recursively`. So, instead of just checking if a type is recursive and then trying to somehow deal with it, recursively-defined traits such as is_move_constructible can now directly ask this trait where the recursion should proceed. --- include/pybind11/detail/type_caster_base.h | 251 +++++++++++++++------ include/pybind11/stl_bind.h | 18 +- tests/test_stl_binders.cpp | 11 +- 3 files changed, 190 insertions(+), 90 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 20e9c40e8e..6af58ae902 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -822,56 +822,173 @@ using movable_cast_op_type typename std::add_rvalue_reference>::type, typename std::add_lvalue_reference>::type>>; -// True if Container has a dependent type mapped_type that is equivalent -// to Container itself -// Actual implementation in the SFINAE specialization below -template -struct is_container_with_recursive_mapped_type : std::false_type {}; - -// This specialization is only valid if both conditions are fulfilled: -// 1) The mapped_type exists -// 2) And it is equivalent to Container +template +struct if_then_else {}; + +template +struct if_then_else { + using type = Then; +}; + +template +struct if_then_else { + using type = Else; +}; + +// Does the container have a mapped type and is it recursive? +// Implemented by specializations below. +template +struct container_mapped_type_traits { + static constexpr bool has_mapped_type = false; + static constexpr bool has_recursive_mapped_type = false; +}; + template -struct is_container_with_recursive_mapped_type - : std::true_type {}; - -// True if Container has a dependent type value_type that is equivalent -// to Container itself -// Actual implementation in the SFINAE specialization below -template -struct is_container_with_recursive_value_type : std::false_type {}; - -// This specialization is only valid if both conditions are fulfilled: -// 1) The value_type exists -// 2) And it is equivalent to Container +struct container_mapped_type_traits< + Container, + typename std::enable_if< + std::is_same::value>::type> { + static constexpr bool has_mapped_type = true; + static constexpr bool has_recursive_mapped_type = true; +}; + template -struct is_container_with_recursive_value_type - : std::true_type {}; - -// True constant if the type contains itself recursively. -// By default, this will check the mapped_type and value_type dependent types. -// In more complex recursion patterns, users can specialize this struct. -// The second template parameter SFINAE=void is for use of std::enable_if in specializations. -// An example is found in tests/test_stl_binders.cpp. +struct container_mapped_type_traits< + Container, + typename std::enable_if< + negation>::value>::type> { + static constexpr bool has_mapped_type = true; + static constexpr bool has_recursive_mapped_type = false; +}; + +// Does the container have a value type and is it recursive? +// Implemented by specializations below. template -struct is_recursive_container : any_of, - is_container_with_recursive_mapped_type> {}; +struct container_value_type_traits : std::false_type { + static constexpr bool has_value_type = false; + static constexpr bool has_recursive_value_type = false; +}; -template -struct is_move_constructible : std::is_move_constructible {}; +template +struct container_value_type_traits< + Container, + typename std::enable_if< + std::is_same::value>::type> { + static constexpr bool has_value_type = true; + static constexpr bool has_recursive_value_type = true; +}; -// Specialization for types that appear to be move constructible but also look like stl containers -// (we specifically check for: has `value_type` and `reference` with `reference = value_type&`): if -// so, move constructability depends on whether the value_type is move constructible. template -struct is_move_constructible< +struct container_value_type_traits< Container, - enable_if_t< - all_of, - std::is_same, - // Avoid infinite recursion - negation>>::value>> - : is_move_constructible {}; + typename std::enable_if< + negation>::value>::type> { + static constexpr bool has_value_type = true; + static constexpr bool has_recursive_value_type = false; +}; + +/* + * Tag to be used for representing the bottom of recursively defined types. + * Define this tag so we don't have to use void. + */ +struct recursive_bottom {}; + +/* + * Implementation detail of `recursive_container_traits` below. + * `T` is the `value_type` of the container, which might need to be modified to + * avoid recursive types and const types. + */ +template +struct impl_type_to_check_recursively { + /* + * If the container is recursive, then no further recursion should be done. + */ + using if_recursive = recursive_bottom; + /* + * Otherwise yield `T` unchanged. + */ + using if_not_recursive = T; +}; + +/* + * For pairs - only as value type of a map -, the first type should remove the `const`. + * Also, if the map is recursive, then the recursive checking should consider + * the first type only. + */ +template +struct impl_type_to_check_recursively, /* is_this_a_map = */ true> { + using if_recursive = typename std::remove_const::type; + using if_not_recursive = std::pair::type, B>; +}; + +/* + * Implementation of `recursive_container_traits` below. + */ +template +struct impl_recursive_container_traits { + using type_to_check_recursively = recursive_bottom; +}; + +template +struct impl_recursive_container_traits< + Container, + typename std::enable_if::has_value_type>::type> { + static constexpr bool is_recursive + = container_mapped_type_traits::has_recursive_mapped_type + || container_value_type_traits::has_recursive_value_type; + /* + * This member dictates which type Pybind11 should check recursively in traits + * such as `is_move_constructible`, `is_copy_constructible`, `is_move_assignable`, ... + * Direct access to `value_type` should be avoided: + * 1. `value_type` might recursively contain the type again + * 2. `value_type` of STL map types is `std::pair`, the `const` + * should be removed. + * + */ + using type_to_check_recursively = typename if_then_else< + is_recursive, + typename impl_type_to_check_recursively< + typename Container::value_type, + container_mapped_type_traits::has_mapped_type>::if_recursive, + typename impl_type_to_check_recursively< + typename Container::value_type, + container_mapped_type_traits::has_mapped_type>::if_not_recursive>::type; +}; + +/* + * This trait defines the `type_to_check_recursively` which is needed to properly + * handle recursively defined traits such as `is_move_constructible` without going + * into an infinite recursion. + * Should be used instead of directly accessing the `value_type`. + * It cancels the recursion by returning the `recursive_bottom` tag. + * + * The default definition of `type_to_check_recursively` is as follows: + * + * 1. By default, it is `recursive_bottom`, so that the recursion is canceled. + * 2. If the type is non-recursive and defines a `value_type`, then the `value_type` is used. + * If the `value_type` is a pair and a `mapped_type` is defined, + * then the `const` is removed from the first type. + * 3. If the type is recursive and `value_type` is not a pair, then `recursive_bottom` is returned. + * 4. If the type is recursive and `value_type` is a pair and a `mapped_type` is defined, + * then `const` is removed from the first type and the first type is returned. + * + * This behavior can be extended by the user as seen in test_stl_binders.cpp. + * + * This struct is exactly the same as impl_recursive_container_traits. + * The duplication achieves that user-defined specializations don't compete + * with internal specializations, but take precedence. + */ +template +struct recursive_container_traits : impl_recursive_container_traits {}; + +template +struct is_move_constructible + : all_of, + is_move_constructible< + typename recursive_container_traits::type_to_check_recursively>> {}; + +template <> +struct is_move_constructible : std::true_type {}; // Likewise for std::pair // (after C++17 it is mandatory that the move constructor not exist when the two types aren't @@ -883,21 +1000,14 @@ struct is_move_constructible> // std::is_copy_constructible isn't quite enough: it lets std::vector (and similar) through when // T is non-copyable, but code containing such a copy constructor fails to actually compile. -template -struct is_copy_constructible : std::is_copy_constructible {}; +template +struct is_copy_constructible + : all_of, + is_copy_constructible< + typename recursive_container_traits::type_to_check_recursively>> {}; -// Specialization for types that appear to be copy constructible but also look like stl containers -// (we specifically check for: has `value_type` and `reference` with `reference = value_type&`): if -// so, copy constructability depends on whether the value_type is copy constructible. -template -struct is_copy_constructible< - Container, - enable_if_t< - all_of, - std::is_same, - // Avoid infinite recursion - negation>>::value>> - : is_copy_constructible {}; +template <> +struct is_copy_constructible : std::true_type {}; // Likewise for std::pair // (after C++17 it is mandatory that the copy constructor not exist when the two types aren't @@ -908,24 +1018,19 @@ struct is_copy_constructible> : all_of, is_copy_constructible> {}; // The same problems arise with std::is_copy_assignable, so we use the same workaround. -template -struct is_copy_assignable : std::is_copy_assignable {}; -template -struct is_copy_assignable< - Container, - enable_if_t< - all_of, - std::is_same, - // Avoid infinite recursion - negation>>::value>> - : is_copy_assignable {}; +template +struct is_copy_assignable + : all_of< + std::is_copy_assignable, + is_copy_assignable::type_to_check_recursively>> { +}; + +template <> +struct is_copy_assignable : std::true_type {}; + template struct is_copy_assignable> - /* - * Need to remove the const qualifier from T1 here since the value_type in - * STL map types is std::pair, and const types are never assignable - */ - : all_of::type>, is_copy_assignable> {}; + : all_of, is_copy_assignable> {}; PYBIND11_NAMESPACE_END(detail) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index e90a228937..49f1b77821 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -61,21 +61,11 @@ struct is_comparable< /* For a vector/map data structure, recursively check the value type (which is std::pair for maps) */ template -struct is_comparable::is_vector && - // Avoid this instantiation if the type is recursive - negation>::value>> { - static constexpr const bool value = is_comparable::value; -}; +struct is_comparable::is_vector>> + : is_comparable::type_to_check_recursively> {}; -/* Skip the recursion if the type itself is recursive */ -template -struct is_comparable::is_vector && - // Special case: The vector type is recursive - is_recursive_container::value>> { - static constexpr const bool value = container_traits::is_comparable; -}; +template <> +struct is_comparable : std::true_type {}; /* For pairs, recursively check the two data types */ template diff --git a/tests/test_stl_binders.cpp b/tests/test_stl_binders.cpp index d7c0c4942f..3c83655506 100644 --- a/tests/test_stl_binders.cpp +++ b/tests/test_stl_binders.cpp @@ -10,6 +10,7 @@ #include #include +#include "pybind11/detail/type_caster_base.h" #include "pybind11_tests.h" #include @@ -86,7 +87,7 @@ struct RecursiveMap : std::map { /* * Pybind11 does not catch more complicated recursion schemes, such as mutual * recursion. - * In that case custom is_recursive_container specializations need to be added, + * In that case custom recursive_container_traits specializations need to be added, * thus manually telling pybind11 about the recursion. */ struct MutuallyRecursiveContainerPairMV; @@ -98,9 +99,13 @@ struct MutuallyRecursiveContainerPairVM : std::vector -struct is_recursive_container : std::true_type {}; +struct recursive_container_traits { + using type_to_check_recursively = recursive_bottom; +}; template -struct is_recursive_container : std::true_type {}; +struct recursive_container_traits { + using type_to_check_recursively = recursive_bottom; +}; } // namespace detail } // namespace pybind11 From 3ed36b8e30ddb09ba52670b9030ea31e41da6ed5 Mon Sep 17 00:00:00 2001 From: Franz Poeschel Date: Thu, 4 May 2023 14:52:09 +0200 Subject: [PATCH 15/16] Review suggestions 1. Use std::conditional 2. Fix typo --- include/pybind11/detail/type_caster_base.h | 15 +-------------- tests/test_copy_move.cpp | 2 +- 2 files changed, 2 insertions(+), 15 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 6af58ae902..6eca51a746 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -822,19 +822,6 @@ using movable_cast_op_type typename std::add_rvalue_reference>::type, typename std::add_lvalue_reference>::type>>; -template -struct if_then_else {}; - -template -struct if_then_else { - using type = Then; -}; - -template -struct if_then_else { - using type = Else; -}; - // Does the container have a mapped type and is it recursive? // Implemented by specializations below. template @@ -945,7 +932,7 @@ struct impl_recursive_container_traits< * should be removed. * */ - using type_to_check_recursively = typename if_then_else< + using type_to_check_recursively = typename std::conditional< is_recursive, typename impl_type_to_check_recursively< typename Container::value_type, diff --git a/tests/test_copy_move.cpp b/tests/test_copy_move.cpp index 705115f07f..f54733550a 100644 --- a/tests/test_copy_move.cpp +++ b/tests/test_copy_move.cpp @@ -298,7 +298,7 @@ TEST_SUBMODULE(copy_move_policies, m) { /* * Rest of the file: - * static_assert based tests for pybind11 adaptations of of + * static_assert based tests for pybind11 adaptations of * std::is_move_constructible, std::is_copy_constructible and * std::is_copy_assignable (no adaptation of std::is_move_assignable). * Difference between pybind11 and std traits: pybind11 traits will also check From 71521c44787becf5f0896aa868e823efecc5f217 Mon Sep 17 00:00:00 2001 From: Franz Poeschel Date: Thu, 4 May 2023 16:19:38 +0200 Subject: [PATCH 16/16] Remove leftover include from test --- tests/test_stl_binders.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_stl_binders.cpp b/tests/test_stl_binders.cpp index 3c83655506..1681760aa8 100644 --- a/tests/test_stl_binders.cpp +++ b/tests/test_stl_binders.cpp @@ -10,7 +10,6 @@ #include #include -#include "pybind11/detail/type_caster_base.h" #include "pybind11_tests.h" #include