Missing cast from const unique_ptr&

[rhaschke]

The issue mentioned in #2901 (the compiler complaining about impossible casting from const unique_ptr& to unique_ptr&&) originates from a missing cast implementation. I added a dummy implementation (69769a9) and the compiler errors are gone.

IMO, casting from a const unique_ptr& should distinguish the following two cases:

1. If the instance is already registered, it should be returned with an increased refcount.
2. Otherwise, a new instance should be created (which holder?), which must not be deleted or moved. Is that possible at all?

[rwgk]

Aha :-)
That makes everything clear immediately, thanks!
Initial thought: This is very similar to static handle cast(const T *src, ...). We could mimic that implementation, or maybe even reuse? The easy case is that an instance exists already and we just incref as you suggest. Or there needs to be return_value_policy::reference_internal or an explicit keep_alive.
Is there a chance you could help with that implementation? It's unlikely that I can prioritize working on this myself anytime soon, unless I hit on a use case in PyCLIF (difficult to predict). But I will review & merge very quickly if you take the lead.
My revised suggestion for a separate new unit test name iff we cannot reuse static handle cast(const T *src, ...): test_class_sh_const_unique_return
Keeping the tests separate helps minimizing merge issues (between master, drake, smart_holder) and is best practice IMO regardless.

[rhaschke]

Done. Thanks for the hint to cast(const T*). Regarding the unit test: I think we should have a unit test in any case.
Do you suggest to move the test into a new file, named test_class_sh_const_unique_return.cpp or to include a test named test_class_sh_const_unique_return into some existing file?

[rhaschke]

Hm. I don't understand why CI fails, while locally it works fine. Need to investigate later...

[rwgk]

Oh, sorry to be ambiguous: I was thinking of test_class_sh_const_unique_return.cpp and .py.
I think I also want to add a few lines to test_class_sh_basic.cpp and .py, but that's a small enough effort I could squeeze that in myself.

[rwgk]

Oh, sorry to be ambiguous: I was thinking of test_class_sh_const_unique_return.cpp and .py.
I think I also want to add a few lines to test_class_sh_basic.cpp and .py, but that's a small enough effort I could squeeze that in myself.

[rwgk]

I looked at one of the CI failures: https://github.com/pybind/pybind11/pull/2904/checks?check_run_id=2120821136
It looks like it's somehow falling back to move_only_holder_caster, which shouldn't be involved at all for types using the smart_holder_type_caster family.

There is a lot going on in test_smart_ptr.cpp, especially with the TYPE_CASTERS macros, which could be easily confusing. If you move the new test to a separate pair of files we have a simpler context to worry about.

About test_smart_ptr in general: it was the most special test for me to deal with. From memory, I think none of the py::class_ in there uses smart_holder, even if you build with ENABLE_SMART_HOLDER_AS_DEFAULT.

My general approach for the existing tests was: if a holder is specified explicitly, keep it that way, with or without ENABLE_SMART_HOLDER_AS_DEFAULT. If a derived py::class_ uses a custom holder, make the std::unique_ptr holder for the base explicit. — That way I could be sure that I'm not trying to run tests designed specifically for the old holder handling for the new smart_holder code. The smart_holder code has independent dedicated tests in the test_class_sh_* files.

[rhaschke]

Thanks for these hints. I will move my tests into test_class_sh_* files. I also noticed that I should use py::classh instead of py::class_ in order to get smart holders. The CI failures are probably due to the fact that CI doesn't use ENABLE_SMART_HOLDER_AS_DEFAULT but requires manual smart_holder triggering via py::classh.

[rwgk]

The CI failures are probably due to the fact that CI doesn't use ENABLE_SMART_HOLDER_AS_DEFAULT but requires manual smart_holder triggering via py::classh.

Oh yes, that's another stumbling stone. I have PR #2879 for that purpose. I keep reusing it, by rebasing it on whatever I want to test at the moment.

[rwgk]

Hi Robert, I just merged #2902 into the smart_holder branch. You may want to rebase this PR.

[rhaschke]

This should be ready for a fresh review. I added several new test cases, one of them still failing. Particularly, this roundtrip test attempts to pass a unique_ptr from python to C++ as a const reference and returning the very same reference to Python again. IMO, this should leave the original python instance alive and just increase the reference count. However, the instance is moved to C++ and thus disowned in Python.

[rwgk]

IMO, this should leave the original python instance alive and just increase the reference count. However, the instance is moved to C++ and thus disowned in Python.

I think (!) you could leverage the dynamic_cast that I just merged (#2902) to achieve that for types that inherit from virtual_overrider_self_life_support, but without something like that I think it's not possible: when you get the same pointer value back, you don't know if it's still the same object, or a different newly allocated one that just happens to have the same address.

[rhaschke]

CI still failing, because windows moves more often... Will fix.

[rhaschke]

CI passes! 🎉

[rwgk]

Thanks Robert! I'll merge this now.