Add tests for virtual_inheritance Issue #1256

[yeganer]

This is a very simple addition to the tests that covers #1256.
When running these tests as part of the pybind11 tests, they work as expected even with (msvc as detailed in the issue) but compiling the same code as part of another project causes the code to crash.

So currently this PR is just to trigger the automated tests I guess?

[jagerman]

Thanks for the test code! I investigated this some more. It was a bit subtle.

The diamond case here actually works properly: It's the non-diamond case, i.e. single inheritance from a virtual base class, where we run into trouble. But in the test case as written in the first commit, registering the diamond case actually fixed both cases!

The core issue is that we aren't detecting virtual inheritance as something special: we treat it as ordinary inheritance, which means when we upcast we just reinterpret_cast the derived pointer to its parent. This isn't valid under virtual inheritance (though it happens to work under gcc/clang), so we need to add some detection here.

We do have a path for proper casting, but it's more complicated, and so we try to only invoke it when necessary: which, currently, means either the class is a base or some ancestor of a base in a registered multiple inheritance class; or you manually specify py::multiple_inheritance{} as an argument to the py::class_<...> constructor (which is designed for telling pybind that multiple inheritance is involved but not being exposed to Python). We need to fix this so that it also triggers when specifying a single, virtual base class.

Now as for your test code, the original commit ended up masking the issue because the diamond inheritance registration marked the Interface base class as non-simple, which then made the non-multiple, virtual inheritance take the slower (but correct) casting path.

The second one by itself, however, already works fine: when the py::class_<Diamond, B_Concrete, C_Concrete>(m, "Diamond") is registered, it recursively marks known ancestors as "non-simple", avoiding the simple path.

I've amended the test code in the PR to duplicate the base Interface (to avoid the second registration accidentally fixing the first one), which let the first case trigger the failure again without having to disable the second test. (I also moved some things around to make the style more consistent with the other pybind test code, but that's mainly cosmetic).

I've also merged a fix into the commit to detect the virtual base, so these tests should pass now. (While I was at it, I squashed and rebased against master).

For current stable, a simple workaround is to add a py::multiple_inheritance{} option to the derived class_ constructor, i.e.:

py::class_<Derived, VirtualBase>(m, "Derived", py::multiple_inheritance{})

Since this commit ends up a bit big (i.e. it required adding a new header, to avoid an unconditional gcc warning in the implementation of the virtual base detection), and because the workaround above is pretty easy, and because this is a fairly rare issue (virtual inheritance is not useful without also having multiple inheritance lurking), I'm leaning against pushing this to to 2.2.x branch.

Edit: Fixes #1256

[oremanj]

I don't think you need to go to the trouble of that is_virtual_base_of implementation if you're willing to restrict yourself to accessible bases; you can just use the fact that static_cast from Base* to Derived* is not permitted if Base is a morally virtual base of Derived.

#include <type_traits>
using namespace std;

template <class Base, class Derived>
struct is_accessible_static_base_of
{
    template <class B = Base, class D = Derived>
    static decltype(static_cast<D*>(declval<B*>()), true_type()) check(B*);
    static false_type check(...);
    static constexpr bool value = is_base_of_v<Base, Derived> && decltype(check((Base*)nullptr))::value;
};

struct B { int x; };
struct D1 : B { int y; };
struct D2 : B { int z; };
struct D : D1, D2 { int w; };

struct VB { int x; };
struct VD1 : virtual B { int y; };
struct VD2 : virtual B { int z; };
struct VD : VD1, VD2 { int w; };
struct VD11 : VD1 {};

static_assert(is_accessible_static_base_of<B, D1>::value);
static_assert(is_accessible_static_base_of<B, D2>::value);
static_assert(is_accessible_static_base_of<D1, D>::value);
static_assert(is_accessible_static_base_of<D2, D>::value);
static_assert(!is_accessible_static_base_of<B, D>::value);
static_assert(!is_accessible_static_base_of<D1, B>::value);
static_assert(!is_accessible_static_base_of<D2, B>::value);
static_assert(!is_accessible_static_base_of<D, D1>::value);
static_assert(!is_accessible_static_base_of<D, D2>::value);
static_assert(!is_accessible_static_base_of<D, B>::value);
static_assert(is_accessible_static_base_of<B, B>::value);
static_assert(is_accessible_static_base_of<D1, D1>::value);
static_assert(is_accessible_static_base_of<D2, D2>::value);
static_assert(is_accessible_static_base_of<D, D>::value);

static_assert(!is_accessible_static_base_of<VB, VD1>::value);
static_assert(!is_accessible_static_base_of<VB, VD11>::value);
static_assert(!is_accessible_static_base_of<VB, VD2>::value);
static_assert(is_accessible_static_base_of<VD1, VD>::value);
static_assert(is_accessible_static_base_of<VD2, VD>::value);
static_assert(!is_accessible_static_base_of<VB, VD>::value);
static_assert(!is_accessible_static_base_of<VD1, VB>::value);
static_assert(!is_accessible_static_base_of<VD2, VB>::value);
static_assert(!is_accessible_static_base_of<VD, VD1>::value);
static_assert(!is_accessible_static_base_of<VD, VD2>::value);
static_assert(!is_accessible_static_base_of<VD, VB>::value);
static_assert(is_accessible_static_base_of<VB, VB>::value);
static_assert(is_accessible_static_base_of<VD1, VD1>::value);
static_assert(is_accessible_static_base_of<VD2, VD2>::value);
static_assert(is_accessible_static_base_of<VD, VD>::value);