Add check for matching holder_type when inheriting

[pschella]

One frequently encountered source of errors are mismatching holder types between bases and derived classes. If either the base has unique_ptr holder type and the derived class shared_ptr (or vice versa) a segfault results.

Tracking holder types for all classes in a large codebase can be cumbersome and error prone.

This pull request is an example of how import-time checks for matching holder types can be added. I welcome your comments.

[dean0x7d]

I think the test could be simplified so it doesn't require a separate module. It should be possible to pack up everything into a function within the existing test module. Something like:

m.def("test_matching_holder_type", []() {
    struct Base { };
    struct Derived : Base { };

    try {
        py::class_<Base>(m, "Base");
        py::class_<Derived, std::shared_ptr<Derived>, Base>(m, "Derived")
            .def(py::init<>());
    } catch (std::runtime_error &) {
        return true;
    }
    return false;
});

assert test_matching_holder_type() is True

It might be possible to simplify void_holder to a pure type metafunction:

template <typename... Ts>
struct void_holder<std::unique_ptr<Ts...>> {
    using type = std::unique_ptr<void>;
};

where the typeid just moves outside:

tinfo->void_holder_type = &typeid(detail::void_holder<holder_type>::type);

A more generic approach for void_holder might also be worth considering so that users don't need to provide specializations in most cases:

template <typename T, typename SFINAE = void>
struct void_holder {
    using type = void;
}

template <typename...> using void_t = void;

template <template<typename...> class Holder, typename... Ts>
struct void_holder<Holder<Ts...>, void_t<Holder<void>>> {
    using type = Holder<void>;
};

The void_t SFINAE would make sure Holder<void> is a valid type. But I'm just thinking out loud here and this might not even compile properly.

[wjakob]

+1 for the single module approach proposed by @dean0x7d.

FWIW I think that storing a full type ID is overkill. I suggest to record a single bool which records whether a non-standard holder (i.e. something other than std::unique_ptr<Type>) is used.

[pschella]

Thanks for the feedback. I pushed a simplified version.
(left the previous commit intact, so will have to be squashed before potential merge)

[pschella]

That is why it was variadic, but I agree this is better. I'll make the change.

[pschella]

Wondering if we should split the error reporting telling which one has non-default holder type?

[pschella]

I split it up. Easier to debug.

[dean0x7d]

Why is the check_holder_type parameter needed?

[dean0x7d]

Unless I'm missing something, the second test is going to be skipped completely because the first one throws. Consider separate functions.

Also, .def(py::init<>()) is probably not needed for the test.

[dean0x7d]

Sorry, I need to revise my original suggestion a bit. It might be nicer to catch the exception in Python in order to also check that the correct exception was thrown.

m.def("test_matching_holder_type", []() {
    struct Base { };
    struct Derived : Base { };

    py::class_<Base>(m, "Base");
    py::class_<Derived, std::shared_ptr<Derived>, Base>(m, "Derived");
});

with pytest.raises(RuntimeError) as excinfo:
    test_matching_holder_type()
assert str(excinfo.value) == "generic_type: ..."

[pschella]

I followed the suggestions from @dean0x7d to clean things up a bit more. Please let me know if I should squash the commits into one.

[dean0x7d]

There are some weird failures on AppVeyor. It may have to do with the uninitialized py::module m;. Try initializing it with a name or maybe auto m = py::module::import("__main__"); and see if that clears it.

There are also some minor style issues. See the log on Travis.

[pschella]

Changes implemented and commits squashed.

[wjakob]

Looks great -- thank you!