Detect and fail if using mismatched holders

[jagerman]

This adds a check when registering a class or a function with a holder return that the same wrapped type hasn't been previously seen using a different holder type.

This fixes#1138 by detecting the failure; currently attempting to use two different holder types (e.g. a unique_ptr and shared_ptr) in difference places can segfault because we don't have any type safety on the holder instances.

This an alternative to #1139, which I don't think will work for pybind as is (I'll comment in that PR).

[EricCousineau-TRI]

(Follow up from #1138)

Just a couple of comments / questions (quoting from this PR and your comment in the other PR):

Conversion between holder types--e.g. being able to convert an rvalue unique_ptr into a shared_ptr seems feasible, but I think it's going to need a different path than stealing the pointer and stashing it in the new type. std::shared_ptr has a std::unique_ptr && constructor for doing exactly this promotion

I have covered a constrained case of this under #1146 - by permitting moveing an rvalue unique_ptr<T> = unique_ptr<T, default_deleter<T>> holder (from class_) into a shared_ptr<T> casted return type:
https://github.com/pybind/pybind11/pull/1146/files#diff-2443092219154d8d844074fa320e45f7R1134

This also adds in a layer to check type mismatches when erasing the holders (via void* passing, and const-casting stuff) in the internal code (for additional checks):
https://github.com/pybind/pybind11/pull/1146/files#diff-caf3c493ee8ade1a606a667ccd7378d5R397
The caveat is that this only supports a specific conversion (only default_deleter<> for unique_ptr<>) due to type erasure - but I am not sure how this can be side-stepped.

Is there a way something like this could be incorporated into this new PR that you have?
(I could then shave it off of that WIP PR)

There are some other cases this PR doesn't catch that the other one does:

To follow up, sorry, I should have followed up on the old PR. The newer PR, #1146 (which is unfortunately large, so it got lost in the details), had a mechanism to handle this; however, this setup (using typeids) does seem much better!

and ideally, detect it at compile time rather than run-time

Can I ask if you have an idea of how this would work? I cannot think of anything at the moment.

[jagerman]

Can I ask if you have an idea of how this would work? I cannot think of anything at the moment.

Well, just brainstorming a bit (because I haven't really thought this through): One possible way is with a declaration along the lines of py::implicitly_convertible<A, B>(), but for holders. Maybe something like: py::holder_convertible<A, std::unique_ptr<A>, std::shared_ptr<A>>(); which could then stash a lambda in the type information that could then be used to allow the conversion.

Or perhaps more flexible, a py::holder_conversion<A>(f) where f is a function taking a holder by rvalue reference and returning another holder, e.g.:

py::holder_conversion<A>([](std::unique_ptr<A> &&a) {
returnstd::shared_ptr(std::move(a)); });

That would also allow you to do things like go the other way (albeit with a required run-time check):

py::holder_conversion<B>([](std::shared_ptr<B> &&b) {
if (b.use_count() > 1)
throwstd::runtime_error("Cannot convert a shared_ptr with multiple refs");
return std::unique_ptr<B>(b.release());
});

[EricCousineau-TRI]

Maybe something like: py::holder_convertible<A, std::unique_ptr, std::shared_ptr>();

That makes sense, but this seems like it would be strictly runtime rather than compile-time.
Also, for common holder types (shared_ptr, unique_ptr), would it be possible have these commonly registered when class_ is constructed?

a py::holder_conversion(f) where f is a function taking a holder

That makes sense too, and seems like it could ultimately be the underlying implementation for the first alternative.

That would also allow you to do things like go the other way

AFAIK, it is not possible to convert a shared_ptr to a unique_ptr in STL, as there is no thread-safe release mechanism :(

That being said, could some sort of mechanism like this be used to more intuitively release unique_ptr-held instances to C++?

(If this is too off-topic to this PR, I can move these comments to another thread.)

[wjakob]

This strikes me as fairly heavy weight (code added to every function dispatcher). Can't we just point out in the docs that you're not supposed to mix holder types? It strikes me as a fairly rare design pattern to use unique_ptr and shared_ptr interchangeably for a type.

[EricCousineau-TRI]

True - but if it doesn't add too much weight, could there still be a way to raise an alarm if attempting to convert holder types?
Could the conversion check be delegated strictly to the holder type converters, copyable_holder_caster and move_only_holder_caster? It seems like it should be viable there (I believe).

(And just to check, are you talking about the suggestions for conversions? Or are you talking about the original PR?)

[jagerman]

This strikes me as fairly heavy weight (code added to every function dispatcher).

I somewhat agree and am a bit ambivalent about this PR, but it's not that heavy: the code is only added to function and class initialization, not function dispatching.

[jagerman]

Oh, I think you were referring to the holder conversion stuff in the comments above rather than the mismatched holder detection.

[EricCousineau-TRI]

Sorry for late-follow-up, but it seems like @tadeu may have encountered this in #1215.

Once concern with the current PR is that it may only check Return values but ignore input arguments (and possibly casting).
Could this logic be moved into the holder casters, copyable_holder_caster and move_only_holder_caster, and could the holder type info be stored directly in a type's record?

I'd be happy to make those changes if you'd like, as it'd be a nice step to incorporate bits of #1146.

[oremanj]

I spent a while today debugging a segfault that would've given a clear error message in the presence of this PR - are there any plans to merge it?

[jagerman]

@wjakob - thoughts on this PR? (As it is now -- there are other comments here that went off on a bit of a tangent). The basic protection here seems useful and relatively cheap—a check when registering classs and function, and yet another container in the internals struct.

[TallJimbo]

@pschella and I just encountered this, and disallowing mixed holder types is a big problem for our codebase (regardless of whether the failure mode is a segfault or a friendlier exception). We consider it good practice to use unique_ptr when you can and shared_ptr when you need to (on the same class), and having to inform downstream callers which holder was used so they can know when to wrap a smart-pointer-converting lambda instead of the original function is a workaround we'd like to avoid.

Our organization may be able to contribute some effort towards fixing this, and the holder_conversion proposal above sounds quite reasonable - I think it would have to be a runtime-only operation, since code returning a *_ptr<T> doesn't know anything about the py::class_<T...> instantiation that was used. But it sounds like that would also have to involve adding some new weight to possibly performance-critical places, and I'd like to have some agreement that a change in that direction would not be dead-on-arrival before diving in. It'd also be good to know whether to try to build on top of this PR or not.

[rhaschke]

I came across the same issue and suggested another fix in #2052. However, the one here is more elegant as it detects the conflicting holder types during function/class definition. I think this PR should be merged to avoid potential segfaults and establish a warning mechanism to ensure consistent holder types.
For your convenience, I already rebased this PR to master: https://github.com/rhaschke/pybind11/tree/holder-type-checks.

@EricCousineau-TRI's original aim, on the other hand, was to automatically convert between different holder types.
As pointed out in #1161 (comment), it's a common pattern to use unique_ptr were possible (e.g. returning from factory functions) and switch to shared_ptr only when needed. It would be great if a pattern as suggested in #1161 (comment) could be realized in a future PR.

[rhaschke]

Hm. As pointed out in #1161 (comment), this PR doesn't handle mismatching function arguments (only return types).
I added another (failing) test in my copy of this PR, which indicates that this PR should be augmented by corresponding checks for function arguments as well.

[rhaschke]

I was puzzled why you use a global map holders_seen here, instead of storing the unique holder type of a registered type in its typeinfo as I did in #2052. However, your approach just considers the first occurrence of a holder type as the gold standard. 👍

[YannickJadoul]

Adding this to the 2.7 milestone. Not per se to have this included as-is, but to make sure we discuss this old, but probably still relevant PR.