Skip to content

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Jan 29, 2021

The two new SFINAE hooks are provided to help work around the current lack of support for smart-pointer interoperability. Please consider it an implementation detail that may change in the future, as formal support for smart-pointer interoperability is added into pybind11.

Changelog not needed.

@rwgk rwgk force-pushed the move_only_holder_caster_sfinae branch from 0b8a735 to 9de295f Compare January 30, 2021 19:08
@rwgk rwgk changed the title Adding move_only_holder_caster typename SFINAE = void to enable ext… Adding holder_caster typename SFINAE = void hooks to help work around the current lack of smart-pointer interoperability Jan 30, 2021
@YannickJadoul
Copy link
Collaborator

YannickJadoul commented Jan 30, 2021

For the (literal) record: @rwgk pointed me to these two use cases: https://github.com/pybind/pybind11_protobuf/blob/a451639e0ac6ca66f1ea1bdddb401e264ac27fc8/fast_cpp_proto_casters.h#L321 & https://github.com/pybind/pybind11_protobuf/blob/a451639e0ac6ca66f1ea1bdddb401e264ac27fc8/proto_utils.h#L38:

E.g.:

// All protos use shared pointers as their holder, so automatically convert a
// unique_ptr to a shared pointer. Note that this requires a patch to the core
// pybind11 to add a SFINAE template parameter to move_only_holder_caster.
template <typename ProtoType, typename HolderType>
struct move_only_holder_caster<
    ProtoType, HolderType, std::enable_if_t<google::is_proto_v<ProtoType>>> {
  static handle cast(HolderType&& src, return_value_policy policy,
                     handle handle) {
    return copyable_holder_caster<ProtoType, std::shared_ptr<ProtoType>>::cast(
        std::shared_ptr(std::move(src)), policy, handle);
  }
  static constexpr auto name = type_caster_base<type>::name;
};

And yes, specializing type_caster should still be the only way, but pybind11 already uses the SFINAE of type_caster in the PYBIND11_DECLARE_HOLDER_TYPE macro, so any straightforward attack on this issue would result in an ambiguous specialization, I believe:

#define PYBIND11_DECLARE_HOLDER_TYPE(type, holder_type, ...) \
namespace pybind11 { namespace detail { \
template <typename type> \
struct always_construct_holder<holder_type> : always_construct_holder<void, ##__VA_ARGS__> { }; \
template <typename type> \
class type_caster<holder_type, enable_if_t<!is_shared_ptr<holder_type>::value>> \
: public type_caster_holder<type, holder_type> { }; \
}}

So once again, our interface is fuzzier than it seems. But the addition of this big fat warning should give us the freedom to blame users if they misuse this part of the internal API.

And I do mean that. Don't dare blaming pybind11 if you rely on this and things change.

Copy link
Collaborator

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

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

I'm still not entirely happy, but it does seem to be the simplest way of making these use cases work (and the change is totally innocuous to the good users that keep their hands off of pybind11's internal).

@rwgk
Copy link
Collaborator Author

rwgk commented Jan 30, 2021

Thanks @YannickJadoul!

@rwgk rwgk merged commit 932769b into pybind:master Jan 30, 2021
@rwgk rwgk deleted the move_only_holder_caster_sfinae branch January 30, 2021 20:02
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jan 30, 2021
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants