Allow references to objects held by smart pointers

[dean0x7d]

The documentation rightfully warns against mixing smart pointers and raw pointers in interfaces. However, it's easy to forget that const& has a similar issue and can result in a double free (I was bitten by this recently).

This PR makes it possible to return references to types which are wrapped with a std::shared_ptr or similar copyable smart pointer. Here, 'references' mean & or * with RVP reference or reference_internal. Returning a & or * with RVP take_ownership is still bad as described in the docs.

In the added tests the values returned from shared_ref and from_this_bad_wp would usually result in a double free. This is resolved by not constructing the holder in cases where a non-owning reference is returned.

Because the holder isn't always constructed it's also not possible to cast that Python object back to the holder type. A check is added for this, which accounts for the expected TypeError when casting ref -> holder and bad_wp -> holder. Both ref and bad_wp are still perfectly valid references and can be used per usual. I think that having this case usable with just this limitation is preferable to a crash due to a double free.

The new test also covers every way of initializing a copyable holder type and casting it back to C++.

[wjakob]

Wouldn't it be more sensible to just add a static_assert somewhere that produces an error message in this case? These kinds of function signatures seem problematic as a design pattern, so I'm hesitant to merge a somewhat intrusive patch to support them..

Thoughts?

[dean0x7d]

A static_assert isn't possible because it all depends on the return value policy. It would be possible to pybind11_fail at runtime, but that would require keeping the new conditions and also adding an else with the failure -- even more code. At that point, might as well just support it.

In C++, even if a type is usually held by a std::shared_ptr it's not uncommon to still have just a simple const& accessor because we know in C++ that the reference never implies ownership. This is in contrast to a raw pointer, where I would definitely consider it problematic in conjunction with std::shared_ptr. Taking a non-owning reference seems like a perfectly valid use case that should not segfault.

[wjakob]

I feel like there's something essential that I don't get about this proposal. Currently, if you bind a function returning const std::shared_ptr<MyType>&, the return value will be converted by type_caster_holder::cast (https://github.com/pybind/pybind11/blob/master/include/pybind11/cast.h#L933), which will invoke type_caster_generic::cast with a pointer to the current shared pointer in the existing_holder parameter (https://github.com/pybind/pybind11/blob/master/include/pybind11/cast.h#L262).

This function will in turn call detail::generic_type:: init_holder and thendetail::generic_type:: init_holder_helper (https://github.com/pybind/pybind11/blob/master/include/pybind11/pybind11.h#L1152), which will initialize the shared pointer from the existing one.

And that is exactly what we want. The shared pointers know about each other, and the object won't be deallocated before all the shared pointers go out of scope. In other words, no double frees.

[dean0x7d]

I think I have obscured the issue in the test because I wanted to pack a lot of test coverage in a small amount of code.

The basic issue looks like this:

struct Widget {
    struct A {};

    A a;
};

py::class_<Widget::A, std::shared_ptr<Widget::A>>(m, "WidgetA");
py::class_<Widget>(m, "Widget")
    .def(py::init<>())
    .def_readonly("a", &Widget::a);

>>> w = Widget()
>>> a = w.a
>>> del a, w
Crash: pointer freed twice

This is because Widget::A is held by std::shared_ptr. It hits this branch which essentially creates a std::shared_ptr to a member variable which it shouldn't ever try to free.

With the changes in this PR, the std::shared_ptr will not be created at all for simple references where owned == false.

[wjakob]

Ok, I see -- so the issue is that the fancier holder type disables things like reference and reference_internal.

[dean0x7d]

Yeah, exactly. The docs do warn about this kind of thing, but mostly because of pointers and rvp::take_ownership -- which is quite reasonable. The issue with & and rvp::reference and rvp::reference_internal is surprising behavior IMO.

[wjakob]

The downside of this being of course that you can get into quite bizarre situations where a Widget::A instance suddenly can't be used to call a function expecting a std::shared_ptr<A>. Also not very nice.

I wonder if returning false in the caster is the right thing to do here, versus throwing an exception that informs the user that a non-held instance can't be cast into a held one, and that their API is crap =P

[wjakob]

One more question: I assume that in the case that the class derives from enable_shared_from_this, we still create a holder?

[dean0x7d]

You're right, an exception is more appropriate there. Overloading on that would be insanity. An informative error is better. I'll add it. (The message is also better than a double free crash.)

enable_shared_from_this still creates a holder as before. The only change here is in the case where bad_weak_ptr was thrown. This had the same double free issue, but works now. I'll see about making the test code a bit clearer.

[jagerman]

and that their API is crap

Careful about putting that in the error message: you'll only invite commentary about how judging code by its API is the wrong metric, and about Stroustrup being an idiot and therefore anything he ever suggested should be avoided. :)

[wjakob]

Let me know when this is good to merge.

[dean0x7d]

OK, that should be it. There's now an error message when trying to load Holder<T> from T&.

The tests are a bit longer now, but hopefully also clearer. The comments indicate which path of init_holder_helper is being tested.

I also removed an unnecessary PYBIND11_DECLARE_HOLDER_TYPE for std::shared_ptr<T> from the docs. This was accidentally left in before. (The macro is explained later for generic smart pointers.)

[wjakob]

Great, thank you!