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,173 @@ using movable_cast_op_type
                                   typename std::add_rvalue_reference<intrinsic_t<T>>::type,
                                   typename std::add_lvalue_reference<intrinsic_t<T>>::type>>;
 
-// 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 {};
-
-// This specialization is only valid if both conditions are fulfilled:
-// 1) The mapped_type exists
-// 2) And it is equivalent to Container
+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;
+};
+
+// Does the container have a mapped type and is it recursive?
+// Implemented by specializations below.
+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;
+};
+
 template <typename Container>
-struct is_container_with_recursive_mapped_type<Container, typename Container::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 <typename Container, typename MappedType = Container>
-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<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 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.
+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;
+};
+
+// Does the container have a value type and is it recursive?
+// Implemented by specializations below.
 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 : std::false_type {
+    static constexpr bool has_value_type = false;
+    static constexpr bool has_recursive_value_type = false;
+};
 
-template <typename T, typename SFINAE = void>
-struct is_move_constructible : std::is_move_constructible<T> {};
+template <typename 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;
+};
 
-// 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 container_value_type_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<
+        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;
+};
+
+/*
+ * 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, bool is_this_a_map>
+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 <typename A, typename B>
+struct impl_type_to_check_recursively<std::pair<A, B>, /* is_this_a_map = */ true> {
+    using if_recursive = typename std::remove_const<A>::type;
+    using if_not_recursive = std::pair<typename std::remove_const<A>::type, B>;
+};
+
+/*
+ * Implementation of `recursive_container_traits` below.
+ */
+template <typename Container, typename SFINAE = void>
+struct impl_recursive_container_traits {
+    using type_to_check_recursively = recursive_bottom;
+};
+
+template <typename Container>
+struct impl_recursive_container_traits<
+    Container,
+    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,
+            container_mapped_type_traits<Container>::has_mapped_type>::if_recursive,
+        typename impl_type_to_check_recursively<
+            typename Container::value_type,
+            container_mapped_type_traits<Container>::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 <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 +1000,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 +1018,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

@@ -10,6 +10,7 @@
 #include <pybind11/numpy.h>
 #include <pybind11/stl_bind.h>
 
+#include "pybind11/detail/type_caster_base.h"
 #include "pybind11_tests.h"
 
 #include <deque>
@@ -86,7 +87,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 +99,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