Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Introduce recursive_container_traits #4623

Merged
merged 16 commits into from
May 5, 2023
Merged

Introduce recursive_container_traits #4623

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
541c360
Testing
franzpoeschel Apr 17, 2023
2c6002c
Similar fix for std::vector
franzpoeschel Apr 17, 2023
4c59860
Fix infinite recursion check:
franzpoeschel Apr 14, 2023
2c48ca9
style: pre-commit fixes
pre-commit-ci[bot] Apr 17, 2023
8d35d80
Optional commit that demonstrates the limitations of this PR
franzpoeschel Apr 17, 2023
79782b5
Fix positioning of container bindings
franzpoeschel Apr 19, 2023
ba0aadf
Suggestions from code review: API side
franzpoeschel Apr 21, 2023
0ecf5f0
Suggestions from code review: Test side
franzpoeschel Apr 21, 2023
9ba38bf
Suggestions from code review
franzpoeschel Apr 24, 2023
531ceb4
Some little fixes
franzpoeschel Apr 25, 2023
1bd1e7e
Reordering of structs
franzpoeschel Apr 25, 2023
a839f8f
Add recursive checks for is_move_constructible
franzpoeschel Apr 25, 2023
eba6239
Static testing for pybind11 type traits
franzpoeschel Apr 28, 2023
77e2c56
More precise checking of recursive types
franzpoeschel May 2, 2023
3ed36b8
Review suggestions
franzpoeschel May 4, 2023
71521c4
Remove leftover include from test
franzpoeschel May 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 72 additions & 9 deletions include/pybind11/detail/type_caster_base.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -822,14 +822,70 @@ 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we generalize this and at the same time make this more uniform?

But a silly thing first: the naming here completely throws me off. It kind-of makes sense with my mind in the context (rare occasion), but imaging myself skimming through this while debugging or working on something completely different (the usual situation), my snap-judgement of what this is doing would go in a completely wrong direction.

I'll use is_container_with_self_referential_mapped_type below.

Currently we have two special cases:

std::is_same<Container, Container::value_type>

is_container_with_self_referential_mapped_type<Container>

Could we turn this into

is_self_referential_container_type<Container>

that

  1. invokes the other two?
  2. can be specialized by users?
struct is_self_referential_container_type<MySelfReferentialContainerType> : std::true_type {};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have done this and I like the way that it turned out, especially since adding custom user specializations now involves overriding a single struct, which just tells Pybind that this type is recursive:

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 {};
} // namespace detail
} // namespace pybind11

// 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 <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
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>> {};

template <typename T, typename SFINAE = void>
struct is_move_constructible : std::is_move_constructible<T> {};

// 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>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

template <typename Container, typename SFINAE = void>

We don't want SFINAE to stop here, users may still need enable_if for their own types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but I've now done it in such a way that the SFINAE parameter of the publically specializable struct defaults to void as is usual for SFINAE.

struct is_move_constructible<
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> {};

// 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 <typename T1, typename T2>
struct is_move_constructible<std::pair<T1, T2>>
: all_of<is_move_constructible<T1>, is_move_constructible<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, typename SFINAE = void>
struct is_move_constructible : std::is_move_constructible<T> {};

// 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.
Expand All @@ -840,7 +896,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<std::is_same<Container, typename Container::value_type>>>::value>>
negation<is_recursive_container<Container>>>::value>>
: is_copy_constructible<typename Container::value_type> {};

// Likewise for std::pair
Expand All @@ -855,14 +911,21 @@ struct is_copy_constructible<std::pair<T1, T2>>
template <typename T, typename SFINAE = void>
struct is_copy_assignable : std::is_copy_assignable<T> {};
template <typename Container>
struct is_copy_assignable<Container,
enable_if_t<all_of<std::is_copy_assignable<Container>,
std::is_same<typename Container::value_type &,
typename Container::reference>>::value>>
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 T1, typename T2>
struct is_copy_assignable<std::pair<T1, T2>>
: all_of<is_copy_assignable<T1>, is_copy_assignable<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>> {};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The asymmetry here seems odd, did you already think this through?

Concretely, would it make sense to apply std::remove_const also to T2?

Because for std::pair by itself (not considering how it's used elsewhere), the order of T1 and T2 doesn't have a function AFAIK.

Which leads me to think: It has to be either none or both.

Or we actually have a trait with subtly different semantics compared to the "pure" is_copy_assignable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on this point, the asymmetry is a bit odd.

Copy link
Contributor Author

@franzpoeschel franzpoeschel May 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PLEASE DO NOT REVIEW YET, I need to go over this on my own again first.

Fixing this required changing the logic of this PR quite a bit.
The trouble of is_recursive_container<T> is that it only tells you that the type is recursive at all and then tells you to deal with it somehow. What I did now instead was replacing that with a recursive_container_traits that has a dependent type recursive_container_traits<Container>::type_to_check_recursively:

/*
 * 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 {};

template <typename Container, typename SFINAE = void>
struct impl_recursive_container_traits {
    using type_to_check_recursively = recursive_bottom;
};

// and its main logic in the specialization

The type_to_check_recursively is defined such:

  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, 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, then const is removed from the first type and the first type is returned.

Having this trait makes the definitions of custom traits such as is_move_constructible very straightforward and no longer requires SFINAE constructs there:

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
// themselves move constructible, but this can not be relied upon when T1 or T2 are themselves
// containers).
template <typename T1, typename T2>
struct is_move_constructible<std::pair<T1, T2>>
    : all_of<is_move_constructible<T1>, is_move_constructible<T2>> {};

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing this required changing the logic of this PR quite a bit.

Oh, wow. Thanks a lot for going the extra mile!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed some comments now, should be ready for review :)


PYBIND11_NAMESPACE_END(detail)

Expand Down
14 changes: 13 additions & 1 deletion include/pybind11/stl_bind.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,22 @@ 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>> {
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;
};

/* 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;
};

/* For pairs, recursively check the two data types */
template <typename T>
struct is_comparable<T, enable_if_t<container_traits<T>::is_pair>> {
Expand Down
Loading