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.

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

Towards a solution for rvalue holder arguments in wrapped functions #2046

Closed
wants to merge 9 commits into from

Conversation

rhaschke
Copy link
Contributor

In #2040 I was complaining about a fundamental issue in pybind11, namely that rvalue function arguments (of custom types and particularly of std::unique_ptr) are not supported, i.e. the following function cannot be wrapped:

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

As this is a major blocker for my project to transition from boost::python to pybind11, I tried to investigate the issue in more detail.
It looks like, that a major design decision was to explicitly forbid transferring ownership from python to C++. This allows maintaining a raw value pointer (in addition to the holder) for fast and direct access to the raw pointer. (I noticed that the holder's get() is only rarely called.)

If you are not willing to support this important use case, you should explicitly state this fact very prominently on some main documentation page(s). However, I hope that you will add this feature in the future as pybind11 is a great replacement for boost::python, providing many benefits and exhibiting much better code readability.

The above-mentioned design decision becomes noticeable - amongst other things - by the fact that move_only_holder_casters do not support loading of arguments and thus conversion from python to C++ is not possible for move-only holder types.
The commits of this PR replace the existing implementation of move_only_holder_caster with that of copyable_holder_caster (with some minor tweaks), which essentially enables the requested feature.
However, a major caveat remains: The holder pointer remains accessible after moving, while it should become None and thus raise an exception if accessed again. I tried to accomplish this here, but it didn't have the desired effect - I guess because pybind11 mainly accesses object instances by their raw value pointers instead of going through the holder.

Another major caveat comes from the overload-resolution process: During this process, several function overloads are tried to match in sequence, each one requiring to load the python arguments into the type_caster. However, this loading process already moves the holder into the caster and thus is only possible once.

@rhaschke rhaschke force-pushed the rvalue-argument-fix2 branch from dd69ad2 to b78e8e3 Compare December 30, 2019 19:54
@rhaschke
Copy link
Contributor Author

I noticed that the holder's get() is only rarely called.

Trying to understand why this is the case, I learned that the default caster (type_caster_base<T>) has no notion of the expected holder type and never goes through the holder's get()!

@rhaschke rhaschke changed the title Towards a solution for rvalue function arguments Towards a solution for rvalue holder arguments in wrapped functions Dec 30, 2019
@rhaschke
Copy link
Contributor Author

I think I fixed one remaining issue: Instead of moving the holder potentially multiple times during loading in overload resolution, I just "load" a pointer to the holder and perform actual moving only when actually casting to holder_type&&.
However, the main issue of invalidating the python instance remains.

@rhaschke
Copy link
Contributor Author

rhaschke commented Jan 1, 2020

I might have solved the 2nd remaining issue, namely invalidating the PyObject instance after moving, as well. In addition to invalidating and (re)checking the instance pointer, we probably need to check that there are no other python object instances accessing (member) elements of the to-be-moved object instance. Is has_patients a way to do so?

@wjakob
Copy link
Member

wjakob commented Apr 26, 2020

Transferring ownership from Python to C++ is a super-dangerous and rather unnatural (non-pythonic) operation and intentionally not supported in pybind11. It would be very hard to convince me to make a change here, and the current discussion & tentative patch don't make a strong enough case to support a very narrow use case IMO. Happy to discuss further, but I'm closing this for now. Sorry that it's taken me a while to respond.

@wjakob wjakob closed this Apr 26, 2020
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Nov 18, 2020
See also:

* pybind#2046
* pybind#2583

test_unique_ptr_member builds and runs successfully.
Several other unit tests fail.
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