Enable unique_ptr holder with mixed Deleters between base and derived types

[trelau]

Recognize "std::unique_ptr<T, D>" as a default holder even if "D" doesn't match between base and derived types. Currently, if "D" doesn't match you get a "type does not have a non-default holder type while its base does" error.

Reference this issue: #1317

And this solution: #1317 (comment)

An example where this will remove some boilerplate code is described here.

[jagerman]

LGTM. As I mentioned in #1317, this better matches the intentions of the check (which is really just meant to keep you from accidentally mixing unique_ptrs and shared_ptrs).

[EricCousineau-TRI]

One potential concern: What happens when you have a non-empty deleter that actually changes the memory layout of unique_ptr? It seems like this now enables you to mix unique_ptrs along the inheritance chain, which could cause a segfault without some more typeid checks (e.g. #1161)?

(It also seems like your holder could get misinterpreted, e.g. between unique_ptr<T> and unique_ptr<T, no_delete>, depending on which py::class_ instantiation is handling the instance.)

[jagerman]

I don't think it's a problem since unique_ptr<T>s aren't castable to unique_ptr<Base>/unique_ptr<Derived> in the first place. The check here is to make sure that you don't mix holders that can cast to base/derived holders up and down the inheritance chain (i.e. shared_ptr) with holders that can't (i.e. unique_ptr).

[EricCousineau-TRI]

I don't think it's a problem since unique_ptrs aren't castable to unique_ptr/unique_ptr in the first place. [...]

I'm not sure if I completely understand; can I ask which casting you're referring to, specifically between T and Base? From a quick test, casting in C++ from unique_ptr<T> to unique_ptr<Base> is valid (via std::move, that is).

(Asking in the context of #1237 where, at PR's present state, pybind11::cast() / pyinbd11::move() can be used to convert Python objects to unique_ptr arguments.)

[EricCousineau-TRI]

Just did a quick test on this branch:
https://github.com/EricCousineau-TRI/pybind11/pull/1/files#diff-2afbeba333b4cc339dabd6a379ea943cR155

Takeaways:

• Generally, static compilation checks for castability between the Deleters of unique_ptr will ensure users don't carelessly cast unique_ptrs.
• If a user starts mixing unique_ptrs and, for whatever reason, uses a nontrivial deleter (something that takes up memory, e.g. std::function<> for type-erased destructors) at somewhere along the inheritance chain, they may be in for a nasty surprise if they are not consistent with their API.

The above caveat seems like a rare edge case, and would seem like a general design mistake, so I think this PR is fine as-is; could be addressed at a later point, perhaps with #1161.

[trelau]

Rebased on master. Any idea why a single configuration is failing for xcode?

[trelau]

Rebased on master and passed all builds. Ready for review @jagerman @wjakob

[justbuchanan]

+1 for this PR. What else needs to be done before it can be merged? I'de be happy to help if possible.

[trelau]

anything in particular holding back this PR? happy to help if there are any requests.

[wjakob]

It looks good to me. The reason for the delay is my (very) limited time budget for maintaining this project.