Skip to content

Porting/adapting Dustin's PR #2839 to smart_holder branch #2886

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

Merged
merged 3 commits into from
Mar 4, 2021

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Mar 4, 2021

Hi @virtuald, this PR ports your PR #2839 to the smart_holder branch, ready to use if you like to give it a try. The branch is a strict superset of current master, with pybind11/smart_holder.h as an optional-use add-on.

Some remarks:

  • The test you're adding under Attach python lifetime to shared_ptr passed to C++ #2839 was copied over to here, with a different name: test_class_sh_trampoline_shared_ptr_cpp_arg. This way we can merge your PR into master, and then into smart_holder without conflicts. After the merge I will try to consolidate the copies, to remove the duplication.

  • The copy-modified test doubles as an example for how to use smart_holder (include smart_holder.h, add PYBIND11_SMART_HOLDER_TYPE_CASTERS macros, change py::class_ to py::classh).

  • There is one section in the copy-modified test that expects different behavior compared to your original test, prominently marked with comments. I'm currently unclear if the has_alias/is_alias distinction is actually desirable. I tried a little bit to emulate it, but gave up when I couldn't find a straightforward way to make it work. If you feel strongly the distinction is important, I could try harder to make it work also in the smart_holder code.

  • This PR (and the entire smart_holder branch) does NOT make any changes that would require a PYBIND11_INTERNALS_VERSION increment. The trick I'm using is to take a free bit in the smart_holder type, to track pointee_depends_on_holder_owner. Currently the smarter_holder type is not under any version lock policy, because it's still a brand new addition, but after your PR is merged on master, I will look into using your has_alias/is_alias bits instead. I think it makes more sense to keep those bits separate from the rest of the smart_holder logic.

  • Your shared_ptr_deleter appears here as shared_ptr_dec_ref_deleter (I have two other deleters, therefore the more specific name). Note that it works with void operator()(void *), the T isn't actually needed. A small simplification / minor optimization, also makes it easier to reuse between our implementations maybe if we want to make things pretty down the road.

  • Note that the smart_holder also support passing unique_ptr back to C++ if safe (disowning the Python object), but if has_alias is true this feature is disabled; a ValueError is raised.

  • For the record, I tried out my idea outlined previously under Attach python lifetime to shared_ptr passed to C++ #2839: populating the smart_holder immediately with a shared_ptr that has your deleter. It was a bad idea: makes a nice reference cycle right there, undetectable by the gc. Building a new shared_ptr each time one is needed for passing to C++ is the the only way I could find to achieve the desired behavior.

@rwgk
Copy link
Collaborator Author

rwgk commented Mar 4, 2021

The CI is green, also with PYBIND11_USE_SMART_HOLDER_AS_DEFAULT (#2879): Merging for easier Google-internal testing. This change will get reviewed internally. I'll open a new PR if there are requests for changes.

@rwgk rwgk merged commit 97a7fb7 into pybind:smart_holder Mar 4, 2021
@rwgk rwgk deleted the test_class_sh_with_alias branch March 4, 2021 01:58
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Mar 4, 2021
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label Mar 4, 2021
rwgk added a commit to google/clif that referenced this pull request Mar 5, 2021
…CLIF smart_ptrs_test accordingly.

pybind/pybind11#2886

High-level summary of the two critical behavior changes:
* Context: handling of smart pointers for instances of Python-derived classes with pybind11-wrapped bases.
* Keywords: trampoline, also known as alias class, virtual functions with Python overrides.
* Minimal example: `class PyDrvd(m.Abase)` in test_class_sh_with_alias.py
* When a `shared_ptr<base>` is passed from Python to C++ (as a C++ function argument), the `shared_ptr` is built specifically for the function call, using a custom deleter tying the lifetime of the Python instance to the lifetime of the `shared_ptr`. This solves what's sometimes referred to as "inheritance slicing" issue in connection with the pybind11 trampoline feature. Note that there are ~10 open pybind11 issues related to this problem, enumerated in the description of [PR #2839](pybind/pybind11#2839).
* Passing a `unique_ptr<base>` is disabled. A `ValueError` is raised, with message: "Ownership of instance with virtual overrides in Python cannot be transferred to C++."

For full details see the description of PR #2886.

  - 97a7fb722a9842cae4d91be82897dd863f6b607d Porting/adapting Dustin's PR #2839 to smart_holder branch... by Ralf W. Grosse-Kunstleve <rwgk@google.com>

PiperOrigin-RevId: 360966763
@EricCousineau-TRI EricCousineau-TRI added the smart holder See: https://github.com/pybind/pybind11/blob/smart_holder/README_smart_holder.rst label Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
smart holder See: https://github.com/pybind/pybind11/blob/smart_holder/README_smart_holder.rst
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants