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.

Author: franzpoeschel

include/pybind11/detail/type_caster_base.h

@@ -822,56 +822,157 @@ using movable_cast_op_type
                                   typename std::add_rvalue_reference<intrinsic_t<T>>::type,
                                   typename std::add_lvalue_reference<intrinsic_t<T>>::type>>;
 
+template <bool Condition, typename Then, typename Else>
+struct if_then_else {};
+
+template <typename Then, typename Else>
+struct if_then_else<true, Then, Else> {
+    using type = Then;
+};
+
+template <typename Then, typename Else>
+struct if_then_else<false, Then, Else> {
+    using type = Else;
+};
+
 // True if Container has a dependent type mapped_type that is equivalent
 // to Container itself
 // Actual implementation in the SFINAE specialization below
-template <typename Container, typename MappedType = Container>
-struct is_container_with_recursive_mapped_type : std::false_type {};
+template <typename Container, typename SFINAE = void>
+struct container_mapped_type_traits {
+    static constexpr bool has_mapped_type = false;
+    static constexpr bool has_recursive_mapped_type = false;
+};
 
 // This specialization is only valid if both conditions are fulfilled:
 // 1) The mapped_type exists
 // 2) And it is equivalent to Container
 template <typename Container>
-struct is_container_with_recursive_mapped_type<Container, typename Container::mapped_type>
-    : std::true_type {};
+struct container_mapped_type_traits<
+    Container,
+    typename std::enable_if<
+        std::is_same<typename Container::mapped_type, Container>::value>::type> {
+    static constexpr bool has_mapped_type = true;
+    static constexpr bool has_recursive_mapped_type = true;
+};
+
+template <typename Container>
+struct container_mapped_type_traits<
+    Container,
+    typename std::enable_if<
+        negation<std::is_same<typename Container::mapped_type, Container>>::value>::type> {
+    static constexpr bool has_mapped_type = true;
+    static constexpr bool has_recursive_mapped_type = false;
+};
 
 // True if Container has a dependent type value_type that is equivalent
 // to Container itself
 // Actual implementation in the SFINAE specialization below
-template <typename Container, typename MappedType = Container>
-struct is_container_with_recursive_value_type : std::false_type {};
+template <typename Container, typename SFINAE = void>
+struct container_value_type_traits : std::false_type {
+    static constexpr bool has_value_type = false;
+    static constexpr bool has_recursive_value_type = false;
+};
 
 // This specialization is only valid if both conditions are fulfilled:
 // 1) The value_type exists
 // 2) And it is equivalent to Container
 template <typename Container>
-struct is_container_with_recursive_value_type<Container, typename Container::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.
-template <typename Container, typename SFINAE = void>
-struct is_recursive_container : any_of<is_container_with_recursive_value_type<Container>,
-                                       is_container_with_recursive_mapped_type<Container>> {};
+struct container_value_type_traits<
+    Container,
+    typename std::enable_if<
+        std::is_same<typename Container::value_type, Container>::value>::type> {
+    static constexpr bool has_value_type = true;
+    static constexpr bool has_recursive_value_type = true;
+};
+
+template <typename Container>
+struct container_value_type_traits<
+    Container,
+    typename std::enable_if<
+        negation<std::is_same<typename Container::value_type, Container>>::value>::type> {
+    static constexpr bool has_value_type = true;
+    static constexpr bool has_recursive_value_type = false;
+};
 
-template <typename T, typename SFINAE = void>
-struct is_move_constructible : std::is_move_constructible<T> {};
+/*
+ * 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 <typename T>
+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, the first type should remove the `const`.
+ * Also, if the container is recursive, then the recursive checking should consider
+ * the first type only.
+ */
+template <typename A, typename B>
+struct impl_type_to_check_recursively<std::pair<A, B>> {
+    using if_recursive = typename std::remove_const<A>::type;
+    using if_not_recursive = std::pair<typename std::remove_const<A>::type, B>;
+};
+
+template <typename Container, typename SFINAE = void>
+struct impl_recursive_container_traits {
+    using type_to_check_recursively = recursive_bottom;
+};
 
-// 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 <typename Container>
-struct is_move_constructible<
+struct impl_recursive_container_traits<
     Container,
-    enable_if_t<
-        all_of<std::is_move_constructible<Container>,
-               std::is_same<typename Container::value_type &, typename Container::reference>,
-               // Avoid infinite recursion
-               negation<is_recursive_container<Container>>>::value>>
-    : is_move_constructible<typename Container::value_type> {};
+    typename std::enable_if<container_value_type_traits<Container>::has_value_type>::type> {
+    static constexpr bool is_recursive
+        = container_mapped_type_traits<Container>::has_recursive_mapped_type
+          || container_value_type_traits<Container>::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<A const, B>`, 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>::if_recursive,
+        typename impl_type_to_check_recursively<
+            typename Container::value_type>::if_not_recursive>::type;
+};
+
+/*
+ * This is exactly the same as impl_recursive_container_traits.
+ * This duplication achieves that user-defined specializations don't compete
+ * with internal specializations, but take precedence.
+ */
+template <typename Container, typename SFINAE = void>
+struct recursive_container_traits : impl_recursive_container_traits<Container> {};
+
+template <typename T>
+struct is_move_constructible
+    : all_of<std::is_move_constructible<T>,
+             is_move_constructible<
+                 typename recursive_container_traits<T>::type_to_check_recursively>> {};
+
+template <>
+struct is_move_constructible<recursive_bottom> : 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 +984,14 @@ struct is_move_constructible<std::pair<T1, T2>>
 
 // std::is_copy_constructible isn't quite enough: it lets std::vector<T> (and similar) through when
 // T is non-copyable, but code containing such a copy constructor fails to actually compile.
-template <typename T, typename SFINAE = void>
-struct is_copy_constructible : std::is_copy_constructible<T> {};
+template <typename T>
+struct is_copy_constructible
+    : all_of<std::is_copy_constructible<T>,
+             is_copy_constructible<
+                 typename recursive_container_traits<T>::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 <typename Container>
-struct is_copy_constructible<
-    Container,
-    enable_if_t<
-        all_of<std::is_copy_constructible<Container>,
-               std::is_same<typename Container::value_type &, typename Container::reference>,
-               // Avoid infinite recursion
-               negation<is_recursive_container<Container>>>::value>>
-    : is_copy_constructible<typename Container::value_type> {};
+template <>
+struct is_copy_constructible<recursive_bottom> : 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 +1002,19 @@ struct is_copy_constructible<std::pair<T1, T2>>
     : all_of<is_copy_constructible<T1>, is_copy_constructible<T2>> {};
 
 // 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>,
-               // Avoid infinite recursion
-               negation<is_recursive_container<Container>>>::value>>
-    : is_copy_assignable<typename Container::value_type> {};
+template <typename T>
+struct is_copy_assignable
+    : all_of<
+          std::is_copy_assignable<T>,
+          is_copy_assignable<typename recursive_container_traits<T>::type_to_check_recursively>> {
+};
+
+template <>
+struct is_copy_assignable<recursive_bottom> : std::true_type {};
+
 template <typename T1, typename T2>
 struct is_copy_assignable<std::pair<T1, T2>>
-    /*
-     * Need to remove the const qualifier from T1 here since the value_type in
-     * STL map types is std::pair<const Key, T>, and const types are never assignable
-     */
-    : all_of<is_copy_assignable<typename std::remove_const<T1>::type>, is_copy_assignable<T2>> {};
+    : all_of<is_copy_assignable<T1>, is_copy_assignable<T2>> {};
 
 PYBIND11_NAMESPACE_END(detail)
 

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 <typename T>
-struct is_comparable<T,
-                     enable_if_t<container_traits<T>::is_vector &&
-                                 // Avoid this instantiation if the type is recursive
-                                 negation<is_recursive_container<T>>::value>> {
-    static constexpr const bool value = is_comparable<typename T::value_type>::value;
-};
+struct is_comparable<T, enable_if_t<container_traits<T>::is_vector>>
+    : is_comparable<typename recursive_container_traits<T>::type_to_check_recursively> {};
 
-/* Skip the recursion if the type itself is recursive */
-template <typename T>
-struct is_comparable<T,
-                     enable_if_t<container_traits<T>::is_vector &&
-                                 // Special case: The vector type is recursive
-                                 is_recursive_container<T>::value>> {
-    static constexpr const bool value = container_traits<T>::is_comparable;
-};
+template <>
+struct is_comparable<recursive_bottom> : std::true_type {};
 
 /* For pairs, recursively check the two data types */
 template <typename T>

tests/test_stl_binders.cpp

@@ -86,7 +86,7 @@ struct RecursiveMap : std::map<int, RecursiveMap> {
 /*
  * 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 +98,13 @@ struct MutuallyRecursiveContainerPairVM : std::vector<MutuallyRecursiveContainer
 namespace pybind11 {
 namespace detail {
 template <typename SFINAE>
-struct is_recursive_container<MutuallyRecursiveContainerPairMV, SFINAE> : std::true_type {};
+struct recursive_container_traits<MutuallyRecursiveContainerPairMV, SFINAE> {
+    using type_to_check_recursively = recursive_bottom;
+};
 template <typename SFINAE>
-struct is_recursive_container<MutuallyRecursiveContainerPairVM, SFINAE> : std::true_type {};
+struct recursive_container_traits<MutuallyRecursiveContainerPairVM, SFINAE> {
+    using type_to_check_recursively = recursive_bottom;
+};
 } // namespace detail
 } // namespace pybind11
 
@@ -166,8 +170,8 @@ TEST_SUBMODULE(stl_binders, m) {
     // Bind recursive container types
     py::bind_vector<RecursiveVector>(m, "RecursiveVector");
     py::bind_map<RecursiveMap>(m, "RecursiveMap");
-    py::bind_map<MutuallyRecursiveContainerPairMV>(m, "MutuallyRecursiveContainerPairMV");
-    py::bind_vector<MutuallyRecursiveContainerPairVM>(m, "MutuallyRecursiveContainerPairVM");
+    // py::bind_map<MutuallyRecursiveContainerPairMV>(m, "MutuallyRecursiveContainerPairMV");
+    // py::bind_vector<MutuallyRecursiveContainerPairVM>(m, "MutuallyRecursiveContainerPairVM");
 
     // The rest depends on numpy:
     try {