Suggestions from code review

1) Renaming: is_recursive_container and
   MutuallyRecursiveContainerPair(MV|VM)
2) Avoid ambiguous specializations of is_recursive_container

Author: franzpoeschel

include/pybind11/detail/type_caster_base.h

@@ -829,30 +829,38 @@ struct is_copy_constructible : std::is_copy_constructible<T> {};
 
 // 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 <typename Container, typename MappedType = Container>
-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 <typename Container>
-struct sfinae_is_container_with_self_referential_mapped_type<Container,
-                                                             typename Container::mapped_type>
+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
 template <typename Container>
-struct sfinae_is_container_with_self_referential_mapped_type<Container,
-                                                             typename Container::value_type>
+struct is_container_with_recursive_value_type<Container, typename Container::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 <typename Container>
-struct is_container_with_self_referential_mapped_type
-    : sfinae_is_container_with_self_referential_mapped_type<Container> {};
+// 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>> {};
 
 // 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
@@ -864,7 +872,7 @@ struct is_copy_constructible<
         all_of<std::is_copy_constructible<Container>,
                std::is_same<typename Container::value_type &, typename Container::reference>,
                // Avoid infinite recursion
-               negation<is_container_with_self_referential_mapped_type<Container>>>::value>>
+               negation<is_recursive_container<Container>>>::value>>
     : is_copy_constructible<typename Container::value_type> {};
 
 // Likewise for std::pair
@@ -885,7 +893,7 @@ struct is_copy_assignable<
         all_of<std::is_copy_assignable<Container>,
                std::is_same<typename Container::value_type &, typename Container::reference>,
                // Avoid infinite recursion
-               negation<is_container_with_self_referential_mapped_type<Container>>>::value>>
+               negation<is_recursive_container<Container>>>::value>>
     : is_copy_assignable<typename Container::value_type> {};
 template <typename T1, typename T2>
 struct is_copy_assignable<std::pair<T1, T2>>

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 <typename T>
-struct is_comparable<
-    T,
-    enable_if_t<container_traits<T>::is_vector &&
-                // Avoid this instantiation if the type is recursive
-                negation<is_container_with_self_referential_mapped_type<T>>::value>> {
+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;
 };
 

tests/test_stl_binders.cpp

@@ -90,20 +90,18 @@ struct RecursiveMap : std::map<int, RecursiveMap> {
  * 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<int, MutuallyRecursiveContainerPairB> {};
-struct MutuallyRecursiveContainerPairB : std::vector<MutuallyRecursiveContainerPairA> {};
+struct MutuallyRecursiveContainerPairMV : std::map<int, MutuallyRecursiveContainerPairVM> {};
+struct MutuallyRecursiveContainerPairVM : std::vector<MutuallyRecursiveContainerPairMV> {};
 
 namespace pybind11 {
 namespace detail {
-template <>
-struct is_container_with_self_referential_mapped_type<MutuallyRecursiveContainerPairB>
-    : std::true_type {};
-template <>
-struct is_container_with_self_referential_mapped_type<MutuallyRecursiveContainerPairA>
-    : std::true_type {};
+template <typename SFINAE>
+struct is_recursive_container<MutuallyRecursiveContainerPairMV, SFINAE> : std::true_type {};
+template <typename SFINAE>
+struct is_recursive_container<MutuallyRecursiveContainerPairVM, SFINAE> : std::true_type {};
 } // namespace detail
 } // namespace pybind11
 
@@ -169,8 +167,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<MutuallyRecursiveContainerPairA>(m, "MutuallyRecursiveContainerPairA");
-    py::bind_vector<MutuallyRecursiveContainerPairB>(m, "MutuallyRecursiveContainerPairB");
+    py::bind_map<MutuallyRecursiveContainerPairMV>(m, "MutuallyRecursiveContainerPairMV");
+    py::bind_vector<MutuallyRecursiveContainerPairVM>(m, "MutuallyRecursiveContainerPairVM");
 
     // The rest depends on numpy:
     try {