[FEAT] Rework casting

[rhaschke]

Recently @rwgk revived the discussion on supporting movable custom types (and holders), see #2583, #2040, #1132.
I want to summarize the discussion so far and structure the problem into several subproblems, in the hope that we can focus our future discussion on the corresponding subproblems.

1. Fix segfaults due to mismatching holder types, i.e. fixing pybind11 wrapped method segfaults if a function returns unique_ptr<T> when holder is shared_ptr<T> #1138 and C++ object is destructed before it can be used, when returned as a shared_ptr and using default holder type #1215.
   pybind11 assumes that holder types are used consistently, i.e. once registered, a custom type needs to stick with its holder type, let it be the default unique_ptr or any custom smart ptr. However, this basic assumption is not validated (yet) and thus can cause severe segfaults if users don't obey this rule.
   I think Detect and fail if using mismatched holders #2644 addresses this issue in a clean and efficient manner (building on the great work Detect and fail if using mismatched holders #1161 from @jagerman) by validating holder compatibility at function/class definition time (runtime).

2. Support moving (rvalue-reference arguments) for custom types, allowing to (easily) wrap functions like this:

   void consume(CustomType&& object) { CustomType sink(std::move(object)); }

   This doesn't work out of the box (yet), but is in principle possible already right now, employing a small wrapper function as pointed out by @YannickJadoul in [WIP] Towards a solution for custom-type rvalue arguments #2047 (comment).
   My PR [WIP] Towards a solution for custom-type rvalue arguments #2047 is an initial attempt to enable this feature. However, I recently detected an open issue with this approach.
   EDIT: In a rework, I hopefully fixed the move/copy semantics. See here for details.

3. Support moving of holder types, i.e. ownership transfer from python to C++, allowing to wrap functions like this:

   void consume(unique_ptr<CustomType>&& object) { CustomType sink(std::move(*holder.release()); }

   There are two possible implementations for this: WIP: Support ownership transfer between C++ and Python with shared_ptr<T> and unique_ptr<T> for pure C++ instances and single-inheritance instances #1146 (rather huge) and Towards a solution for rvalue holder arguments in wrapped functions #2046. However, to quote @wjakob:

   Transferring ownership from Python to C++ is a super-dangerous and rather unnatural (non-pythonic) operation and intentionally not supported in pybind11.

   If implementing this, we need to ensure that all python object references of the moved holder are validated before loading. For an open issues on this, see Towards a solution for rvalue holder arguments in wrapped functions #2046 (comment).

4. A feature related to both 1. and 3. is the (implicit) conversion between different holder types as requested e.g. in Detect and fail if using mismatched holders #1161 (comment) and proposed in Detect and fail if using mismatched holders #1161 (comment): Instead of complaining about incompatible holder types, pybind11 should "just" auto-magically convert between them - if possible. For example, std::shared_ptr can be implicitly move-constructed from a std::unique_ptr, which indeed would make sense for function return values, i.e. auto-converting a std::unique_ptr return value into a std::shared_ptr holder.
   But, in my opinion, this is the only valid use case. If 3. is in place, argument conversion can be easily handled explicitly with corresponding wrapper functions.

   Instead of a silently auto-converting between holders, maybe a new return_value_policy could be used that specifies the target holder type and then - at compile time - just adds another wrapping layer to convert the return value?

5. As pointed out by @YannickJadoul in [WIP] Towards a solution for custom-type rvalue arguments #2047 (comment), there are some more open issues with casters, e.g. passing containers of char * (e.g. std::vector<char *>) does not work #2245/Fix casters of STL containers with char* #2303/[BUG] Keep pybind11 type casters alive recursively when needed #2527, or Casting from an unregistered type does not throw. #2336.

6. EDIT(eric): pybind11 inheritance slicing; see [BUG] Problem when creating derived Python objects from C++ (inheritance slicing) #1333 for an example. Including it here due to current coupling to holder setup.

[YannickJadoul]

Yes, we're aware. Hopefully, this could be some project for pybind11 3.0.

Thanks for the summary, though!

[YannickJadoul]

To clarify: fixing bugs and adding minor features seems definitely possible before, ofc. But a full redesign/rework will take some time be breaking changes.

[YannickJadoul]

I've created a new "holders" label, for everything related to this type of thing. This should make it easier to find back issues and PRs (as you can search for everything with this label: holders Issues and PRs regarding holders ).

Please apply it (or ping me or send me a message or ...), if you find anything related!

EDIT: The distinction with the already existing "casters" label isn't always obvious. Let's be careful, but in case of doubt, better have the two labels than none?

[rwgk]

Boost.Python observations (also relevant to the issues reported under #2672):

• Boost.Python has this function to convert void* pointers for polymorphic types:
  https://github.com/boostorg/python/blob/5e4f6278e01f018c32cebc4ffc3b75a743fb1042/src/object/inheritance.cpp#L390
  This functionality is completely missing from pybind11: the reinterpret and explicit casts pointed out under Adding py::smart_holder (for smart-pointer interoperability). #2672 (comment) and Adding py::smart_holder (for smart-pointer interoperability). #2672 (comment) implicitly assume that the offset as computed in the Boost.Python code is always 0.

• When passing to shared_ptr arguments, Boost.Python always constructs a new shared_ptr, using a properly cast raw pointer, and a custom deleter keeping the PyObject owning the raw pointer alive:
  https://github.com/boostorg/python/blob/5e4f6278e01f018c32cebc4ffc3b75a743fb1042/include/boost/python/converter/shared_ptr_from_python.hpp#L52
  Note that this code uses the shared_ptr aliasing constructor.
  In this way shared_ptr upcasts and downcasts work correctly, although the argument shared_ptr::use_count results tend to surprise users. Note that this mechanism is independent of the HeldType, in other words, no matter what the HeldType, it is possible to pass to shared_ptr arguments.

• With auto_ptr<T> as HeldType, Boost.Python does support passing to a auto_ptr<T> argument: the PyObject is properly disowned, although there is no dedicated error message for this situation, when methods of the disowned object are called: https://github.com/rwgk/rwgk_tbx/blob/01fd6692dbc11eb121a9aa93595b10e36f31a86f/tst_auto_ptr_holder.py#L46

• With auto_ptr as HeldType, Boost.Python does NOT support passing, e.g., a held auto_ptr<derived> to a auto_ptr<base> argument: https://github.com/rwgk/rwgk_tbx/blob/01fd6692dbc11eb121a9aa93595b10e36f31a86f/tst_auto_ptr_holder.py#L98
  When inserting prints in convert_type (object/inheritance.cpp link above), src_t shows as drvd as expected, but dst_t shows as auto_ptr<base>. Apparently the functionality to extract the raw pointer, cast it, and put it back into a new auto_ptr object, is missing from Boost.Python.

[EricCousineau-TRI]

One question: Would we consider slicing to be under this umbrella?
If so, I'd like to add Item 5, to address slicing (#1333).

As we discussed in VC, in an ideal world, this would be decoupled. However, the mechanisms for handling single and multiple inheritance are coupled due to how casting works in conjunction with object lifetime.

EDIT (@YannickJadoul): another issue about "slicing" or however we're going to call this, to be mentioned in this item: #1941

[rwgk]

One question: Would we consider slicing to be under this umbrella?

My take: it's a very important use case. It will be great if we can capture this in the redesign, too. Let's try how far we get, and drop it only if we cannot find a reasonable way to handle it?

"Slicing" is a very overloaded term. It could help the discussion if we had a more descriptive / less ambiguous term, especially to attract contributions or opinions from outsiders.

[EricCousineau-TRI]

Aye, gotcha, I assume in the context of slice() type and the syntax to support it. For now, I'll just call it "pybind11 inheritance slicing", and will ponder some more succinct terms.
FWIW, here's the Wikipedia article on general object slicing (not exactly what happens here, but akin to it): https://en.wikipedia.org/wiki/Object_slicing

[dabrahams]

@rwgk suggestion: port each Boost.Python test to PyBind11. That should flush out anything we took care to get right in Boost.Python but that isn't handled by PyBind11.

[rwgk]

Hi @EricCousineau-TRI , @rhaschke , @YannickJadoul

I wrote proof-of-concept code & tests for a "smart holder" (under #2672). It's a different kind of holder compared to current pybind11 semantics, but the purpose is very similar. Please see the comment at the top of the header file.

I hope the links below work permanently (beyond the next force push to my fork).
Question mainly for Eric & Robert: do you think this could address all the needs for your projects?
I'm hoping to arrive at a solution that works for everybody, so that none of us has to maintain forks in the future.

EDIT 2021-01-14: WARNING: this version of smart_holder_poc.h has a serious bug: it is neither copyable nor movable, but those mechanism are not disabled. I'm working on a fix: vptr_deleter_guard_flag need to live on the heap.

include/pybind11/smart_holder_poc.h:
https://github.com/pybind/pybind11/pull/2672/files#diff-27833a8008c9cd24dc1e2dcb96af0bd0fb1cbf68eaefde67593bea0661c5eb6b

tests/core/smart_holder_poc_test.cpp:
https://github.com/pybind/pybind11/pull/2672/files#diff-668a098dc0b0e4d156d0624330162259e39c8ae816ccc93d88f1520612097dbb

I tested the code with this command:

clang++ -fsanitize=address -std=c++11 -O0 -g -Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wnon-virtual-dtor -I$HOME/forked/pybind11/include -I$HOME/clone/Catch2/single_include/catch2 -I/usr/include/python3.8 $HOME/forked/pybind11/tests/core/smart_holder_poc_test.cpp && a.out

The tests systematically cover all combinations of from and as, which you can list like this ([S] is for Success expected, [E] for Exception expected):

$ grep 'TEST_CASE' tests/core/smart_holder_poc_test.cpp
TEST_CASE("from_raw_ptr_unowned+as_raw_ptr_unowned", "[S]") {
TEST_CASE("from_raw_ptr_unowned+const_value_ref", "[S]") {
TEST_CASE("from_raw_ptr_unowned+as_raw_ptr_release_ownership", "[E]") {
TEST_CASE("from_raw_ptr_unowned+as_unique_ptr", "[E]") {
TEST_CASE("from_raw_ptr_unowned+as_unique_ptr_with_deleter", "[E]") {
TEST_CASE("from_raw_ptr_unowned+as_shared_ptr", "[S]") {
TEST_CASE("from_raw_ptr_take_ownership+const_value_ref", "[S]") {
TEST_CASE("from_raw_ptr_take_ownership+as_raw_ptr_release_ownership1", "[S]") {
TEST_CASE("from_raw_ptr_take_ownership+as_raw_ptr_release_ownership2", "[E]") {
TEST_CASE("from_raw_ptr_take_ownership+as_unique_ptr1", "[S]") {
TEST_CASE("from_raw_ptr_take_ownership+as_unique_ptr2", "[E]") {
TEST_CASE("from_raw_ptr_take_ownership+as_unique_ptr_with_deleter", "[E]") {
TEST_CASE("from_raw_ptr_take_ownership+as_shared_ptr", "[S]") {
TEST_CASE("from_unique_ptr+const_value_ref", "[S]") {
TEST_CASE("from_unique_ptr+as_raw_ptr_release_ownership1", "[S]") {
TEST_CASE("from_unique_ptr+as_raw_ptr_release_ownership2", "[E]") {
TEST_CASE("from_unique_ptr+as_unique_ptr1", "[S]") {
TEST_CASE("from_unique_ptr+as_unique_ptr2", "[E]") {
TEST_CASE("from_unique_ptr+as_unique_ptr_with_deleter", "[E]") {
TEST_CASE("from_unique_ptr+as_shared_ptr", "[S]") {
TEST_CASE("from_unique_ptr_with_deleter+const_value_ref", "[S]") {
TEST_CASE("from_unique_ptr_with_deleter+as_raw_ptr_release_ownership", "[E]") {
TEST_CASE("from_unique_ptr_with_deleter+as_unique_ptr", "[E]") {
TEST_CASE("from_unique_ptr_with_deleter+as_unique_ptr_with_deleter1", "[S]") {
TEST_CASE("from_unique_ptr_with_deleter+as_unique_ptr_with_deleter2", "[E]") {
TEST_CASE("from_unique_ptr_with_deleter+as_shared_ptr", "[S]") {
TEST_CASE("from_shared_ptr+const_value_ref", "[S]") {
TEST_CASE("from_shared_ptr+as_raw_ptr_release_ownership", "[E]") {
TEST_CASE("from_shared_ptr+as_unique_ptr", "[E]") {
TEST_CASE("from_shared_ptr+as_unique_ptr_with_deleter", "[E]") {
TEST_CASE("from_shared_ptr+as_shared_ptr", "[S]") {
TEST_CASE("error_unpopulated_holder", "[E]") {
TEST_CASE("error_incompatible_type", "[E]") {
TEST_CASE("error_disowned_holder", "[E]") {

[rwgk]

Hi again @EricCousineau-TRI , @rhaschke , @YannickJadoul

Under my experimental PR #2672 I added a "type_caster bare interface demo" (thanks a lot @laramiel for the FreezableInt test, it was my starting point for this). The demo exercises all kinds of returning/passing a type T, including shared_ptr<T> and unique_ptr<T>, fully const-preserving:

tests/test_type_caster_bare_interface_demo.cpp:
https://github.com/pybind/pybind11/pull/2672/files#diff-311f60a15aa4e63e4bd116b584ceaa5b2d1afd392d69c42dfb539eff459e4071

tests/test_type_caster_bare_interface_demo.py:
https://github.com/pybind/pybind11/pull/2672/files#diff-ee26183242cbaf0ecbf6c3c38d0f43f157f954e1257af7227a0ff6323e152926

Bigger picture:

• The smart_holder proof-of-concept (pervious comment) is meant to become the bottom part of what I need for the desired smart-pointer interoperability.
• The type_caster_bare_interface_demo here is the top part.
• Note that the type_caster_bare_interface_demo works with current pybind11 master as-is, while the smart_holder doesn't depend on pybind11 at all.
• The connection between them is currently completely missing, but I feel with the two ends in hand that's mostly a lot of leg work (which will also include porting the Boost.Python inheritance convert_type code).
• My idea for the next step: copy class class_ to class classh (for class smart holder, bumped together), with class_ and classh co-existing in the pybind11 namespace. Then refactor everything underneath in a separate sub-namespace to use smart_holder at the core. That way we can continue to be very conservative with the class_ implementation, but freely develop the new classh implementation, co-existing for as long as we're still figuring out how to unify.

[rwgk]

Under my experimental PR #2672 I added a "test_classh_wip" (wip = work in progress) that combines test_type_caster_bare_interface_demo & smart_holder_poc. All load functionality is working already, including clean disowning, but the static handle cast functions are still only stubs:

tests/test_classh_wip.cpp: https://github.com/pybind/pybind11/pull/2672/files#diff-6e1ab7ea62c6e8b556380e2c3288f905bf4a06633238c37088ea6be0ad8c5a79

tests/test_classh_wip.py: https://github.com/pybind/pybind11/pull/2672/files#diff-9a69dff06135fd95cd59536a893fa3f9e9367517590f84fe249d047029189b4a

Thus far, the required changes between class_ and classh are surprisingly (to me) small: 8ef1255
(The renaming from class_ to classh is in a separate commit.)

[rwgk]

test_classh_wip & classh.h under my experimental PR #2672 are now "almost feature complete", and pass ASAN, MSAN, UBSAN (after several hours of debugging ... thanks @YannickJadoul for making it possible via your #2756).
Still missing for correctness and feature parity with class_:

• Deregistering unowned instances.
• Implementation for enable_shared_from_this special case.

[rwgk]

test_classh_wip now also implements calling deregister_instance after disowning via unique_ptr. Tests pass ASAN, MSAN, UBSAN, and I also ran an embarrassingly simple manual leak check by running disowning cycles in an infinite loop, while monitoring the memory usage with the top command.