smart_holder fixups

[rhaschke]

These are some fixups for the smart_holder branch. See individual commit messages for details. @rwgk

[rhaschke]

@rwgk, given that you have forbidden passing of a std::shared_ptr from C++ to Python before, I guess you also don't yet have roundtrip tests for that use case, i.e. C++ -> Python -> C++ and Python -> C++ -> Python.

[rwgk]

@rwgk, given that you have forbidden passing of a std::shared_ptr from C++ to Python before,

Do you mean "forbidden passing ... for return value policies other than automatic and reference_internal"? What other return value policies are important to support? My thinking was that it's important to throw an exception for the ones that really don't fit the situation, e.g. take_ownership really doesn't make sense in any way I can think of. Similarly, what's the desired semantics of reference, move, or copy? If we accept all those policies without doing anything different or throwing an exception, it's likely misunderstandings or oversights go unnoticed until (much) later (when it can be very costly to debug).

I guess you also don't yet have roundtrip tests for that use case, i.e. C++ -> Python -> C++ and Python -> C++ -> Python.

test_class_sh_basic exercises std::shared_ptr returns and passing, but I'm sure you saw that. I didn't think a round trip test like we have for std::unique_ptr is needed, since there isn't anything tricky.

[rhaschke]

Sorry, obviously it was too late yesterday evening when I wrote this: I neglected that you did handle the cases of automatic and reference_internal. I was particularly missing automatic_reference, which is the default for (C++) arguments passed to python functions from C++.
Regarding copy and move: these are the actual policies automatic would resolve to for non-holder values. For this reason, these two should be definitely supported. Also, if you support reference_internal, you should support reference as well. They don't differ in the handling of the argument itself, but only in the life-time handling of its parent.
I agree that take_ownership doesn't really make sense for a shared pointer. On the other side, it doesn't harm to allow this policy and handle it just like all others. It would be the only one throwing an exception. Allowing it too, we can skip the test completely.

[rwgk]

I just re-read the documentation for the return value policies. The documentation for reference explicitly mentions "do not take ownership", which in the context of shared_ptr is unclear/weird. For copy and move I still feel strongly that those are also best not allowed to avoid guessing and confusion about what they may do. I could be convinced otherwise if those restrictions generate burdensome situations elsewhere, but at the moment I'm thinking simply guiding people to not specify a return value policy that doesn't make a difference anyway is the right thing to do.

If someone really wants to copy or move a value pointed at by a shared_ptr, they can do that easily via a lambda, dereferencing the shared_ptr. Then it's clear and obvious in the bindings code that something unusual is happening, and that it's not different from the behavior for a raw pointer return.

The other changes look good. If you put back the return value policy test I'll merge this PR (after also testing google-internally). We could continue the return value policy discussion separately if needed.

[rhaschke]

@rwgk, I readded the policy check. But I still don't agree with you that copy or move should be rejected.
I see your point that the semantics is not very clear for a shared_ptr.
On the other hand, the return value policy doesn't actually matter for a shared_ptr. So, why would you restrict the allowed values?

[rwgk]

Thanks for the fixes! I'll merge this now.

So, why would you restrict the allowed values?

The main reason is to not lead people into "what does this do?" guessing. Some will just always harbor doubts but never look. That's just wasting a few cognitive cycles. Others (most?) will just copy-paste what they see without understanding. At large scale (our code base has 100s of millions of lines) that copy-paste clutter has a tendency to turn into life-consuming cleanup jobs, when at some point later, it is found that e.g. copy or move actually can have useful semantics. Then those previously meaningless policies scattered around the code base need to be identified and removed. — Coincidentally, I just spent more than a week on something like that. I want to do everything I can to weed out the potential for such a waste of time right at the root.

[jakobandersen]

This fixed my original issue in #3011. However, the second issue (#3011 (comment)) still gives the error RuntimeError: Invalid return_value_policy for shared_ptr..
I think I traced it to https://github.com/pybind/pybind11/blob/smart_holder/include/pybind11/cast.h#L905 where the automatic and automatic_reference policies are prune before going further with the casting.

[rhaschke]

I guess the automatic return value policy is concretized to copy or move in the referenced code snippet and then rejected by the smart_holder_type_caster code. Thus, I think we should allow these policies as well.
@jakobandersen: do you mind preparing a corresponding PR to validate this assumption?

[rwgk]

In the meantime, while working on #3023, I found a combination of std::shared_ptr & return_value_policy::copy here:

pybind11/tests/test_smart_ptr.cpp

Lines 376 to 380 in 4c7697d

	.def_property_readonly("copy", [](const SharedFromThisRef &s) { return s.value; },
	py::return_value_policy::copy)
	.def_readonly("holder_ref", &SharedFromThisRef::shared)
	.def_property_readonly("holder_copy", [](const SharedFromThisRef &s) { return s.shared; },
	py::return_value_policy::copy)

We need to ensure we're compatible with existing behavior, or document the differences if we decide it's better to deviate. — I didn't look enough to be sure what exactly we need to do.