squash-merge smart_holder branch into master

[rwgk]

Description

EDIT: The smart_holder branch was archived here after this PR was merged.

Notes:

• For benchmarking results, see PR [smart_holder] smart_holder / master benchmarking 2025-02-16 #5529.

• The only one existing (on master) test that is changed slightly is test_class.cpp/py. This is only to accommodate testing with PYBIND11_RUN_TESTING_WITH_SMART_HOLDER_AS_DEFAULT_BUT_NEVER_USE_IN_PRODUCTION_PLEASE (this exists primarily to ensure that py::smart_holder is fully compatible with all existing functionality).

• All code was formally reviewed at least once. Much of the code was formally reviewed multiple times (as it was moved around between branches).

• The only changes not reviewed already were merged (into smart_holder) under PR [smart_holder] Remove obsolete detail::type_info::default_holder member. #5541. All other code has been in production use at Google since September 2024. The smart_holder branch has been in production use at Google since February 2021. I.e. this code is extremely thoroughly tested. Additionally, testing via PyCLIF-pybind11 provided thousands of diverse use cases that needed to satisfy many million unit tests (the PyCLIF-pybind11 success was about 99.999%).

• The only significant updates under docs/ are in docs/advanced/smart_ptrs.rst and docs/advanced/classes.rst. The rest of the docs/ updates are mostly mechanical, to replace py::class_ with py::classh (based on this rationale).

• For easy reference, other recent PRs enabling this one:

  • PR Start pybind11v3: Remove all code for PYBIND11_INTERNALS_VERSIONs 4 and 5 #5530
  • PR [smart_holder] Simplification: Enable smart_holder functionality unconditionally. #5531

---

The following is for this PR at commit a4cfd38:

git diff counts under include/:

• 26 lines removed (git diff master include/ | grep '^-' | grep -v '^---' | wc -l)
• 1543 lines added (git diff master include/ | grep '^+' | grep -v '^+++' | wc -l)

I.e., the existing production code is barely touched.

Similarly for tests/:

• 2 lines removed
• 3714 lines added

For more detail, results produced by cloc --diff for include/ between the master branch and this PR:

-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C/C++ Header
 same                           30           1416           3171          15142
 modified                       10              0              1             18
 added                           4            182            159           1182
 removed                         0              0              2              4
-------------------------------------------------------------------------------

I.e., this PR increases the production code size by 7.8%.

---

The squash-commit cd1c9d4 includes these smart_holder PRs not authored by @rwgk, sorted chronologically:

• Missing cast from const unique_ptr& #2904 by @rhaschke was merged on Mar 16, 2021
• smart_holder fixups #3012 by @rhaschke was merged on May 28, 2021
• [smart_holder] return value policies allowed for shared_ptr #3039 by @jakobandersen was merged on Jun 29, 2021
• [SmartHolder] fix(clang-tidy): apply clang-tidy performance fixes  #3048 by @Skylion007 was merged on Jun 18, 2021
• [smart_holder] fix: names of types held by smart holder should be the actual type #3588 by @virtuald was merged on Jan 3, 2022
• [Smart holder] Support void pointer capsules #3633 by @wangxf123456 was merged on Jan 25, 2022
• [smart_holder] fix smart_holder regressions with multiple inheritance #3635 by @virtuald was merged on Jan 26, 2022
• [smart_holder] Add missing test case to CMakeLists.txt #3645 by @wangxf123456 was merged on Jan 25, 2022
• Fixes issue #3788 #3796 by @wangxf123456 was merged on Mar 10, 2022
• Fixes issue #3801 #3807 by @wangxf123456 was merged on Mar 18, 2022
• [smart_holder] Add a new return value policy return_as_bytes #3838 by @wangxf123456 was merged on Apr 15, 2022
• [smart_holder] Fix "extra ';' outside of a function" warning #3929 by @tomba was merged on May 7, 2022
• [smart_holder] Support loading unique_ptr<Derived> as unique_ptr<Base>. #4031 by @wangxf123456 was merged on Jun 27, 2022
• Add a new return value policy _clif_automatic #4343 by @wangxf123456 was merged on Nov 18, 2022
• [smart_holder] Auto select return value policy for clif_automatic #4381 by @wangxf123456 was merged on Dec 5, 2022
• [smart_holder] Implement try_as_void_ptr_capsule as a free function  #4539 by @wangxf123456 was merged on Feb 28, 2023
• [smart_holder] Keep parent alive when returning raw pointers #4609 by @wangxf123456 was merged on Apr 6, 2023
• [smart_holder] Make smart holder type caster of unique_ptr accept automatic_reference #4775 by @wangxf123456 was merged on Aug 3, 2023
• [smart_holder] Unique ptr deleter roundtrip tests and fix #4921 by @iwanders was merged on Nov 7, 2023
• [smart_holder] Smart holder default delete #4924 by @iwanders was merged on Nov 6, 2023
• [smart_holder] Skip GC-dependent tests on GraalPy #5401 by @msimacek was merged on Oct 8, 2024

Suggested changelog entry:

Placeholder: The smart_holder branch was merged with this PR.

To be covered in the upgrade guide:

• Tim's suggestions: squash-merge smart_holder branch into master #5542 (review)
• Explain holder caster backwards compatibility traits:
  • copyable_holder_caster_shared_ptr_with_smart_holder_support_enabled
  • move_only_holder_caster_unique_ptr_with_smart_holder_support_enabled
  • Example: https://github.com/pybind/pybind11_protobuf/blob/f02a2b7653bc50eb5119d125842a3870db95d251/pybind11_protobuf/native_proto_caster.h#L89-L101

[virtuald]

Only reviewed documentation.

I note that the documentation doesn't mention PYBIND11_USE_SMART_HOLDER_AS_DEFAULT at all. In my usage of pybind11, I enable that macro because (a) I'm lazy and (b) I assumed that because smart holder is awesome that eventually when it got merged it would become the default and then we could get rid of the macro.

I don't understand the backwards compatibility argument for not making smart holder the default, but if there really are known backwards compatibility issues and it's not going to be the default I think I lean towards getting rid of the macro? When reading pybind11 code it would be good to know "this is definitely using smart holder" and "this is definitely not" without having to know which compile flags are set.

Edit: lol clearly I didn't read the PR description carefully enough

[virtuald]

Maybe it's another PR, but since it's the recommended default, we probably should change all of the documentation to use it, since users often just copy/paste from the documentation without thinking about it.

[rwgk]

I was on the fence, thanks for the feedback! I'll change it around.

[rwgk]

Done in commit ac9d31e.

(I made a fast pass, followed by a careful slow pass reviewing the generated html.)

[rwgk]

@virtuald As someone who's used the smart_holder branch for a long time, what's you opinion about this question?

Commit 4d8973e removes pybind11/smart_holder.h. Would it be better to keep it?

It only makes a difference to people who have been using the smart_holder branch before (probably a relative small number?). When they switch to pbyind11v3, they'll be forced to adjust their code (remove pybind11/smart_holder.h includes). Is that a hardship, or just a minor inconvenience?

[virtuald]

Maybe I missed it, but is this a new section that has nothing to do with smart holder? Maybe move it to a separate PR.

[rwgk]

Yes, it's definitely odd here, but it's based on this existing part:

pybind11/docs/advanced/smart_ptrs.rst

Lines 50 to 113 in d8565ac

	One potential stumbling block when using holder types is that they need to be
	applied consistently. Can you guess what's broken about the following binding
	code?
	.. code-block:: cpp
	class Child { };
	class Parent {
	public:
	Parent() : child(std::make_shared<Child>()) { }
	Child *get_child() { return child.get(); } /* Hint: ** DON'T DO THIS ** */
	private:
	std::shared_ptr<Child> child;
	};
	PYBIND11_MODULE(example, m) {
	py::class_<Child, std::shared_ptr<Child>>(m, "Child");
	py::class_<Parent, std::shared_ptr<Parent>>(m, "Parent")
	.def(py::init<>())
	.def("get_child", &Parent::get_child);
	}
	The following Python code will cause undefined behavior (and likely a
	segmentation fault).
	.. code-block:: python
	from example import Parent
	print(Parent().get_child())
	The problem is that ``Parent::get_child()`` returns a pointer to an instance of
	``Child``, but the fact that this instance is already managed by
	``std::shared_ptr<...>`` is lost when passing raw pointers. In this case,
	pybind11 will create a second independent ``std::shared_ptr<...>`` that also
	claims ownership of the pointer. In the end, the object will be freed **twice**
	since these shared pointers have no way of knowing about each other.
	There are two ways to resolve this issue:
	1. For types that are managed by a smart pointer class, never use raw pointers
	in function arguments or return values. In other words: always consistently
	wrap pointers into their designated holder types (such as
	``std::shared_ptr<...>``). In this case, the signature of ``get_child()``
	should be modified as follows:
	.. code-block:: cpp
	std::shared_ptr<Child> get_child() { return child; }
	2. Adjust the definition of ``Child`` by specifying
	``std::enable_shared_from_this<T>`` (see cppreference_ for details) as a
	base class. This adds a small bit of information to ``Child`` that allows
	pybind11 to realize that there is already an existing
	``std::shared_ptr<...>`` and communicate with it. In this case, the
	declaration of ``Child`` should look as follows:
	.. _cppreference: http://en.cppreference.com/w/cpp/memory/enable_shared_from_this
	.. code-block:: cpp
	class Child : public std::enable_shared_from_this<Child> { };

I think it'll be best to move it only after this PR is merged, to not make this PR even more difficult to review.

[virtuald]

Commit 4d8973e removes pybind11/smart_holder.h. Would it be better to keep it?

No promises were really made about smart_holder and if one is using (what is effectively) a fork of a project then you just accept whatever comes with it, so I think whatever makes sense would be good. Since everything in that file is marked as deprecated, it seems good to get rid of it.

Looking at my own usage, I used -DPYBIND11_USE_SMART_HOLDER_AS_DEFAULT and never explicitly included that header.

[rwgk]

Commit 4d8973e removes pybind11/smart_holder.h. Would it be better to keep it?

No promises were really made about smart_holder and if one is using (what is effectively) a fork of a project then you just accept whatever comes with it, so I think whatever makes sense would be good. Since everything in that file is marked as deprecated, it seems good to get rid of it.

Thanks! I'm glad to get rid of it completely. I'll remove my note from the PR description.

Looking at my own usage, I used -DPYBIND11_USE_SMART_HOLDER_AS_DEFAULT and never explicitly included that header.

I want to nudge people towards not ever using that macro in production situations, with this in mind:

• Using it in one translation unit but not another could lead to ODR violations.

• When bindings code is moved from one repo or project to another, and one of them uses the macro but not the other, people might get very surprised by the subtle behavior differences.

One idea is to rename the macro, .e.g.: PYBIND11_RUN_TESTING_WITH_SMART_HOLDER_AS_DEFAULT_BUT_NEVER_USE_IN_PRODUCTION_PLEASE

So in your case the old name would become a no-op, some tests would break, which you'd then fix by changing your class_ to classh.

I'm trying to think ahead, please chime in if I'm going off the tracks:

You wouldn't be able to go back to an older pybind11 version easily. But then again, if your code requires the smart_holder feature, that wouldn't work anyway. So all good?

But what will people do if I'm overlooking something and they have a reason to use py::classh when available, but need to stay compatible with py::class_ for some period of time?

They could go through their code to change py::class_ to e.g. PY_CLASSH_ (same number of characters as py::class_, but also hints that it might be one or the other), then define that from the command line or a project-specific config-kind-of header as needed. — Is that a good situation, or should we do something to support them more?

[rwgk]

@virtuald I also made these changes after you reviewed:

• commit 7cae21f — Change macro name to PYBIND11_RUN_TESTING_WITH_SMART_HOLDER_AS_DEFAULT_BUT_NEVER_USE_IN_PRODUCTION_PLEASE
• commit 388fa99 — Add a note pointing to the holder reinterpret_cast.

[rwgk]

@timohl, could you help with a second set of eyes reviewing the documentation changes and the PR description?

[rwgk]

@henryiii, can you think of anything that I might be overlooking?

[rwgk]

@lanctot, could you help reviewing the documentation changes and the PR description? Will this work for https://github.com/google-deepmind/open_spiel?

[rwgk]

@laramiel for awareness mainly. — But it'd be awesome if you get a chance to glance through the PR description and documentation updates.

For context: Laramie was a/the main reviewer of the smart_holder code, in particular #5257.

[rwgk]

@isuruf for awareness mainly. — But please let me know if you believe this PR will cause issues for Conda. (I don't think so; I believe any adjustments that may be needed on the Conda side (for migrating to pybind11 v3.0.0) will be for changes that exist on pybind11 master already.)

[timohl]

@timohl, could you help with a second set of eyes reviewing the documentation changes and the PR description?

Sure. I can check tomorrow.

[rwgk]

I added 4 commits. The first two restore or add mechanical fixes:

• commit 8409b19
• commit 85cc92d

This one clarifies why the number of characters in classh is the same as in class_:

• commit b73f430

This one strengthens the case for py::smart_holder slightly:

• commit d3f9f93

I feel the pendulum has swung a little too far in the opposite direction. Previously the emphasis on py::smart_holder was too strong, now it's too weak. It's quite likely that a fair number of people will miss the quite subtle notes, fall into one of the pitfalls, and then have to painstakingly debug themselves out again. I'm convinced, safety first is the far better choice; there isn't even a noteworthy performance hit for being safe (I believe). Similarly, the issues around trampolines are definitely underemphasized now.

I'm OK, though, to merge this PR as it is now.

[rwgk]

FWIW, ChatGPT is giving passing marks to the docs/ changes:

https://chatgpt.com/share/67c33999-7d70-8008-a58d-522886466b1c

The production code changes (under include/) relative to the smart_holder branch are minimal.

[rwgk]

I added a commit to add back the PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT define:

• 4b05643

Copying the commit message here for easy reference:

---

Add back PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT

This #define was

• introduced with [smart_holder] Fix HAVE vs HAS naming mishap (PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT) #5286
• removed with [smart_holder] Simplification: Enable smart_holder functionality unconditionally. #5531

It is has been in use here:

• https://github.com/pybind/pybind11_protobuf/blob/f02a2b7653bc50eb5119d125842a3870db95d251/pybind11_protobuf/native_proto_caster.h#L89-L101

Currently pybind11 unit tests for the two holder caster backwards compatibility traits

• copyable_holder_caster_shared_ptr_with_smart_holder_support_enabled
• move_only_holder_caster_unique_ptr_with_smart_holder_support_enabled

are missing.

---

I'll add tests after this PR is merged.

See also: https://github.com/duckdb/duckdb/blob/052b3dc20091ed4ee71f7e3218687b502bb328ea/tools/pythonpkg/src/include/duckdb_python/pybind11/conversions/pyconnection_default.hpp#L14-L37

That code only compiles with a copyable_holder_caster_shared_ptr_with_smart_holder_support_enabled specialization (false_type) and will be a good starting point for adding a unit test.

Thanks @itamaro for testing this PR and pointing out that duckdb code!

[Skylion007]

What is the motivation of a squash merge? Why nuke all the history of smart holder?

[rwgk]

What is the motivation of a squash merge?

git bisect — that would become a total mess without squash merging, especially because the branch existed for over 4 years.

Why nuke all the history of smart holder?

The history will stay around indefinitely: I will rename the branch to archive/smart_holder, but I will not delete it.

This explains how the history will stay around (also touches on git bisect):

• https://chatgpt.com/share/67c36022-5fac-8008-9d85-cd39d072938b

[laramiel]

FWIW, I think https://github.com/pybind/pybind11_protobuf/blob/main/pybind11_protobuf/tests/regression_wrappers_test.py failed before 4b05643

[rwgk]

Thanks @henryiii!

I'll merge this asap, but I want to do this first:

ChatGPT prompt:

I have a git feature branch that lived for a long time. Many contributions were made to the branch, and master was merged many times, too. I now want to squash-merge the feature branch into master. I'm looking for ways to give proper credit.

My mind is going to:

• I need to identify all commits to the feature branch (that didn't come from the master branch).
• Ideally I'd like all contributors to appear as as contributors of the squash-merge into master.

Do you have suggestions for how to achieve that?

Response: https://chatgpt.com/share/67c77bef-c554-8008-b23d-3ebab2b1572b

[rwgk]

Not rerunning GitHub Actions before merging:

The last three commits (9bb8d06, b9bf98a, 8e432e9) only update the top-level README.rst

I added Co-authored-by: to the last commit (8e432e9), listing all contributors to the smart_holder branch. In retrospect, I wish I had done that for the squash-commit cd1c9d4, but changing that commit would mean invalidating all other existing commit hashes on this PR as well, which would break all references from the PR comments.

For visibility:
@EthanSteinberg
@iwanders
@jakobandersen
@msimacek
@rhaschke
@Skylion007
@tomba
@virtuald
@wangxf123456

[rwgk]

I want to say thank you very much to:

• @dewitt for creating a role in May-Sep 2024 that allowed me to clean up the smart_holder branch and google/pybind11clif.
• @laramiel and @rainwoodman for the many helpful reviews.
• @jrhemstad, @kkraus14, @leofang for giving me the space to close out this massive unfinished job.

My main goal was to reunify the entire pybind11 ecosystem, and I hope this work brings lasting benefits to everyone.

[petersteneteg]

Huge thanks to everyone involved here, everyone in the pybind11 team and rwgk especially! This branch and the pybind11 project are providing huge value to us, making many things possible which would otherwise be far out of reach for us!

Huge thanks!!!!