squash-merge smart_holder branch into master

[rwgk]

Description

Notes:

• For easy reference: readthedocs preview for this PR.

• 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%.

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)
• classh().def() chaining and py::classh& gotcha (Peter's problem)

[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.

[lanctot]

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

Hi @rwgk,

My short-term solution for OpenSpiel will be to pin our checkout to a commit: (see google-deepmind/open_spiel#1315). Then I'll help whoever pulls in v3 at Google. Will this work for us.. i.e. will the smart_holder branch continue to exist post-merge with master, or should I fork it?

[rwgk]

@lanctot wrote:

Will this work for us.. i.e. will the smart_holder branch continue to exist post-merge with master, or should I fork it?

I'm happy to keep it around indefinitely, although I'd want to rename it, e.g. archive/smart_holder. I'll be sure to coordinate with you before doing that. And I could hold off for a few months if the renaming causes hardships.

[lanctot]

And I could hold off for a few months if the renaming causes hardships.

A simple renaming will be an easy fix for us, so happy to coordinate with you to change our script when the name changes. I'll update that PR when it's renamed. Thanks!

[rwgk]

We have a dedicated pybind11 Slack workspace, and this morning there were discussions regarding the choice of classh as the name for the class_<T, smart_holder> convenience shortcut. I’d like to share my original rationale here for broader visibility:

1. Maximizing adoption of pybind11 v3.0.0 by minimizing breaking changes

   • A key goal is to ensure a smooth transition for users. This is why smart_holder is not the default holder. As it stands, this PR introduces no breaking changes upon merging smart_holder (to the best of my knowledge). The only breaking change so far is the PYBIND11_INTERNALS_ID modification, which has already been merged into master and only affects backward compatibility in cross-extension interoperability.

2. Maximizing adoption of class_<T, smart_holder> by minimizing whitespace changes

   • When users switch to smart_holder, avoiding unnecessary whitespace changes ensures that meaningful modifications remain clear in diffs. This makes debugging easier down the line — users won’t have to sift through whitespace noise when reviewing changes weeks or months later.

3. Facilitating adoption at scale, particularly in large organizations

   • From my experience at Google, migrating large codebases is often hindered by even small breaking changes and whitespace clutter. Any disruption can significantly slow down adoption. A flood of trivial formatting changes can delay reviews by days, as code owners across a company may deprioritize them. In many cases, I had to follow up individually to get migration steps unstuck after prolonged delays due to minor issues.

Given these priorities, the specific name choice (classh) is far less critical than ensuring a frictionless transition.

I'm open to changing the name — now is the time — but only to one with the same number of characters to maintain these benefits.

If anyone has an alternative solution that meets all three goals above, I’d be happy to discuss it.

[henryiii]

A bad name is a debt we carry to eternity; it makes it harder to learn, harder to use, and harder to understand. "PyClash" is what I see when I read this, and it's not clear what it is supposed to mean.

Idea 1: Change the default

What breaking changes would be introduced by making the default holder py::smart_holder instead of std::unique_ptr? From what I understand, our entire test suite works if we swap it out entirely. If we talking about a really small amount of breakage, maybe we should just change the default, and allow people to set std::unique_ptr if they need it.

IMO, this is a 3.0 release, so we are allowed to make changes like these if it generally an improvement and doesn't break most users. Maybe we could even add a define that restores the 2.x style default for emergencies.

py::class_<Thing>(...)  // Smart holder
py::class_<Thing, std::unique_ptr<Thing>>(...)  // Classic

So what scale breakages are we talking about? 10%? 1%? 0.1%? Do we know?

Idea 2: Give up on the "number of characters" requirement

This seems like a really trivial requirement. When we replaced py::module with py::module_, we didn't worry that it was one extra character. If someone wants to move py::class_<T> -> py::class_h<T>, then they are making a change to their code, we shouldn't be forced to come up with a bad name just to keep the number of characters the same. People will be using and reading the name for years after the transition. No where else in pybind11 do we have really cryptic names trying to keep them short.

And people don't have to transition! Is there a benefit to replacing std::unique_ptr with py::smart_holder other than the interaction with std::shared_ptr? Many users don't need it.

We already tell all our users to do namespace py = pybind11;. Why can't a user shorten this themselves if they need a specific number of characters?

Some ideas: py::cls, py::cls_sh, py::class_h, py::class_sh, py::class_smart_holder.

py::cls<Thing>(...)  // Smart holder
py::class_<Thing>(...)  // Classic

Idea 3: Don't recommend the shortcut or don't provide the shortcut

This is, I think, my favorite one, assuming there are downsides to Idea 1, and the one most inline with the way pybind11 has been designed in the past: use py::class_<T, py::smart_holder> in the docs, add an example for how to set up a shortcut, and leave it for large code bases that are willing to change characters but not add more characters. It's clear, it shows users exactly how to use a different holder (like std::shared_ptr), and it's inline with our tag system (pybind11 has a lot of these).

(We could also secretly add this, or mention it once, but I think just showing users how to set it up, just like namespace py = pybind11, would be fine).

py::class_<Thing, py::smart_holder>(...)  // Smart holder
py::class_<Thing>(...)  // Classic

// Or, user setup shortcut:
template <typename type_, typename... options>
using py_class_h = class_<type_, smart_holder, options...>;

py_class_h<Thing>(...)  // Smart holder

---

If none of those works, then py::sclass isn't terrible. But it in contradiction to the rest of the pybind11 design of mirroring the Python name (with *_ if the name is reserved). Someone would have to know to that class ... is actually better as py::sclass.

[henryiii]

Also, why not this?

template <typename type_, typename... options>
using classh = class_<type_, smart_holder, options...>;

Why is this physically a different class?

[rwgk]

Also, why not this?

template <typename type_, typename... options>
using classh = class_<type_, smart_holder, options...>;

Why is this physically a different class?

Thanks for asking that question!

I'm testing commit ff7c087.

The other day (when @petersteneteg reported his problem) I was thinking about using using, but I got myself confused believing it's not possible.

(I totally don't remember anymore why I decided to use inheritance.)

[rwgk]

I'm happy with "sclass"! Especially since someone other than me suggested it. 😁

To all Germans, "S class" is this. — Not a bad association to evoke.

I'll make that change a little later.

---

A bad name is a debt we carry to eternity;

There are so many existing and much worse naming accidents in this world ... if I balance that with the countless hours lost dealing with the fallout from not holding up the "three goals", I feel very sad.

For entertainment, here is my all-time favorite naming accident:

• https://datatracker.ietf.org/doc/html/rfc7231#section-5.5.2

It was never corrected! I always thought that's the smartest thing they could do. It's just a name, but changing it would wipe out so many hours of human time.

[rwgk]

Why is this physically a different class?

Thanks for asking that question!

I'm testing commit ff7c087.

The other day (when @petersteneteg reported his problem) I was thinking about using using, but I got myself confused believing it's not possible.

(I totally don't remember anymore why I decided to use inheritance.)

Testing passes! — @petersteneteg I think that resolves your issue, excellent.

[henryiii]

What about Idea 1 and Idea 3? Do we know what would break if we changed the default? Everything would just get a little larger and slower (I think you measured this? Or was it just the effect of having smart_holder present, but not using it)? Trying to get a feel of how much it's "let's not be risky" vs. a real risk. Do we have any large codebases where it's been tried? Also, if it is a bit slower and larger (looks like py::smart_holder has a shared_ptr inside? Those have a mutex on access!), do we really need to push everyone toward it if they don't need custom access? Everyone using released pybind11 today doesn't need use this ability yet. Trying to get a feel, since I've not used this branch before.

And idea 3 was just to not provide the shortcut at all, and let users write the using, like we do today with py = pybind11. It's just two lines.

[henryiii]

It was never corrected!

I'm sure if they had seen it before shipping it, they would have corrected it. That's why I want to get this right, we haven't shipped it yet!

py::sclass is better than py::classh, as long as that's the direction we want to go (see above).

[virtuald]

Updated robotpy-build branch to see if tests pass ... and they do, and the migration diff is really small.

Interestingly enough, I found it simpler to do py::class_<T, py::smart_holder> because my autogenerator has one instance where it needs to use unique_ptr. It just ended up being easier to say "if special case do unique_ptr, otherwise do py::smart_holder". I'm not so sure that would be the case for someone writing bindings by hand, probably not.

I haven't really looked at the code for this branch yet, but I assume that if it's effectively the same thing I've been using for the last few years then it's pretty solid.

As the initial person who suggested py::sclass, I like that better than classh.

[rwgk]

It was never corrected!

I'm sure if they had seen it before shipping it, they would have corrected it. That's why I want to get this right, we haven't shipped it yet!

It's not about spelling, the point is: What are the consequences of a move we can make, for the ecosystem at large?

Two (alternative) choices we have:

1. Spelling:

classh or cls_smh or sclass or class_sh, in isolation, there is absolutely no doubt IMO that class_sh is the best. But, the choice of spelling is really far on the inconsequential side of the spectrum, people will get used to whatever we pick after a minute or two and be fine with it; it's really not something worth spending brain cycles on, emotional or rational. Having to navigate whitespace noise is obviously much worse. I'm convinced that'll cause costly accidents for some.

2. Make smart_holder the default holder:

That idea has far more value IMO:

• Safe by default.
• Estimated >95% of all use cases will just work.
• Nothing new to learn for most, except maybe that we have to explain that it's best to remove std::shared_ptr holders.

But, in my last days at Google, I ran global testing with smart_holder as the default holder. That's how I found this (broke some tests):

• https://github.com/rwgk/stuff/blob/68dcdcb1b1bad05dd736f1fd54684320f515dfd6/museum_of_codiosities/GetPyWrappedObjectPtr.hpp

There are a bunch of wild things like that in the wild. I also had to patch on the order of ten (from memory) source files around the codebase to insert explicit std::unique_ptr holders (to not have to get into more intrusive changes).

That's not a lot of changes, but I translate that to: Making smart_holder the default holder will definitely slow down the adoption of pybind11v3, which has undesirable consequences, like people having to support legacy code longer, delayed migrations and deprecations, etc. — How much negative value has that exactly: I don't know; and it's also not my problem anymore. The real stakeholders that come to mind are Google, Meta, the pytorch ecosystem, and maybe a few other large organizations.

How can we get their input?

[henryiii]

Two years from now, no one will care about the number of characters in the name. They will care about what "classh" is and why they are supposed to use it instead of class_. Many people will use the simpler name because they see it in their editor and thousands of examples of code, even if our docs only show the new one. Everything else in pybind11 follows a clear, explicit naming scheme. Also they might be annoyed that their editor keeps correcting it to classy, like mine keeps doing ;). The number of characters in the name doesn't matter. You can always rename it if you want to keep it the same as some old code.

Unless we really want to push smart_holder as the default, I'd be fine to not provide a shortcut at all, and just document how to set up a shortcut. Then everyone who really wants a short name can pick whatever name they want. We can document the two lines required to provide a shortcut name. If we really wanted to provide our own name later, we can always add it later, maybe based on what people seem to be using. This also gives people a better mental model of how this works.

If we do really want to push smart_holder as the default, let's make it the default.

To be clear, 90% of our users don't care if they can convert things to a shared_ptr or not. They are happy with what we have provided for years. Having the ability to do this is fantastic for certain use cases, but I'm not convinced it needs to be the default over unique_ptr. The fact there's a shared_ptr inside worries me, that's generally really bad for multithreading (which is now possible in 3.13+!).

I'll have to play with my own benchmarking and code a bit before I can form my own opinion here, I've been going off of what I'm told so far. I'm really curious to see a very hot multithreaded loop compared.

[rwgk]

If we do really want to push smart_holder as the default, let's make it the default.

I don't. — I only want to recommend it, as I explained originally. I was responding to your "Idea 1" and your follow-on question "Do we know what would break if we changed the default?" — What I quoted above just sends us into circles.

I cannot spend a lot more time on this. TBH, suggestions to not provide an alias at all just leave me shaking my head. How much harm can it do to provide one simple alias that makes it possible for people to experiment easily? And if they like it, the alias used will be uniform across the ecosystem.

I'll backtrack, so that this PR merely adds the option to easily use the smart_holder feature, with a really tiny compile-time overhead and binary size overhead, but I'll remove most classh from the documentation again.

To be clear, 90% of our users don't care if they can convert things to a shared_ptr or not.

I agree, the other 10% (a couple thousand people I guess) will probably find the recommendation and figure it out from there.

[petersteneteg]

(looks like py::smart_holder has a shared_ptr inside? Those have a mutex on access!),

Not sure what is meant by "access", I assume you mean to dereference it or getting the raw pointer, and that is most definitely not guarded by any mutex what so ever. Copying or assigning the shared_ptr will have to do some atomic operations on the refcounts in the control block.

[petersteneteg]

I don't care about what the default is as long as I can continue to use it. I would like to still have the classh shortcut in there, I don't really care about the spelling, but classh has been around for a long time in the branch that I have now been using for years.

What I really want is to have this merged now! And please stop having @rwgk running in circles to get this merged, he has already done monumental effort which I am truly grateful for!

[henryiii]

It will get merged.

I'm worried only about what we are asking users to use. I'm just trying to review this for the long-term benefit of pybind11. And expecting 0 code changes when a branch (even one that's been around for a while) is merged into a project is not reasonable. There are no released versions of this, so no one will be surprised by the upgrade - every single branch user will need to switch to master or a released version of pybind11. We are only talking about two lines that would need to be added! Regardless of what we do, I am fine to have a "hidden" (undocumented) py::classh shortcut to ease transition, though.

What I really want is to have this merged now!

Comments like this are not helpful. Stating this is something you want is fine (and I'm completely aware there's a significant number of users for whom this is vital, that's why this branch exists. That's why I suggested this be prepared for merging for 3.0 in the first place.) But demanding that something be merged because you really want it and it was a lot of work will get nowhere on any reputable project.

I think @rwgk already knows this, but I'm always completely happy to make any change I suggest. I can update the docs if needed.

I only want to recommend it

That's what I meant by "push it as the default". If we update all examples to use it, that's "pushing it as the default".

---

For this PR, I think having an undocumented py::classh shortcut and explicit-only docs is the safest thing. We can follow it up later with a smaller dedicated PR if something needs changing. (That's also why I liked not having any shortcut, we can always add one later!)

[henryiii]

I left one mention of the shortcut, but focused on the explicit version. I think that's better for this PR, and maybe we can think about followups.

[henryiii]

Left over? (also on other CI files)

[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.