detail::is_accessible_base_of seems to fail on union types

[lyskov]

Not sure if this is a bug or intended behavior: it looks like detail::is_accessible_base_of seems give false negative for union types. Here the minimal example to reproduce and an error.

Thanks,

#include <pybind11/pybind11.h>
union A
{
	int i;
	float f;
};
PYBIND11_MODULE(example, m) {
	A a, b;
	a = b;  // work fine!

	pybind11::class_< A > (m, "A")
        .def(pybind11::init<>() )
		.def("assign", (A & (A::*)(const A &)) &A::operator=); // trigger static assert
		;
}

Error:

In file included from union.cpp:1:
/home/benchmark/src/pybind11/include/pybind11/pybind11.h:1000:5: error: static_assert failed "Cannot bind an inaccessible base class method; use a lambda definition instead"
    static_assert(detail::is_accessible_base_of<Class, Derived>::value,
    ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/benchmark/src/pybind11/include/pybind11/pybind11.h:1085:25: note: in instantiation of function template specialization 'pybind11::method_adaptor<A, A &, A, const A &>' requested here
        cpp_function cf(method_adaptor<type>(std::forward<Func>(f)), name(name_), is_method(*this),
                        ^
union.cpp:16:4: note: in instantiation of function template specialization 'pybind11::class_<A>::def<A &(A::*)(const A &)>' requested here
                .def("assign", (A & (A::*)(const A &)) &A::operator=); // trigger static assert
                 ^
1 error generated.

[bstaletic]

This seems to solve the problem:

diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h
index 33805e0..ec081fa 100644
--- a/include/pybind11/detail/common.h
+++ b/include/pybind11/detail/common.h
@@ -610,7 +610,7 @@ template <typename Base, typename Derived> using is_strict_base_of = bool_consta
 /// Like is_base_of, but also requires that the base type is accessible (i.e. that a Derived pointer
 /// can be converted to a Base pointer)
 template <typename Base, typename Derived> using is_accessible_base_of = bool_constant<
-    std::is_base_of<Base, Derived>::value && std::is_convertible<Derived *, Base *>::value>;
+    (std::is_same<Base, Derived>::value || std::is_base_of<Base, Derived>::value) && std::is_convertible<Derived *, Base *>::value>;

 template <template<typename...> class Base>
 struct is_template_base_of_impl {

I'm just not sure how to write a proper test.

[YannickJadoul]

I'm just not sure how to write a proper test.

Can't you just take the example from above and add it to the test suite?

Also, a quick comment would be nice, maybe quoting cppreference.com:

If Derived is derived from Base or if both are the same non-union class (in both cases ignoring cv-qualification), provides the member constant value equal to true. Otherwise value is false.

So normally, is_same is included in is_base_of. Just not in the case of a union. Question then is: do we actually support binding unions?

[bstaletic]

Can't you just take the example from above and add it to the test suite?

I can, but my question was more about what to assert on the python side. Do I just not assert anything and say "if it compiles, ship it"?

So normally, is_same is included in is_base_of. Just not in the case of a union. Question then is: do we actually support binding unions?

This is also why I'm not sure if I should keep the parenthesis in the expression. We do support binding std::variant and variant-like types. I guess it's not too far fetched to support unions?

[YannickJadoul]

I can, but my question was more about what to assert on the python side. Do I just not assert anything and say "if it compiles, ship it"?

Myeah; or just get the standard set of tests that the other classes would have? Set a value, and see if the value you get is the same?

This is also why I'm not sure if I should keep the parenthesis in the expression.

Guess it doesn't matter, because if is_same is true, is_convertible will be true. But this is closes to what's meant, I suppose?

We do support binding std::variant and variant-like types. I guess it's not too far fetched to support unions?

Myeah. Just ... I don't know. Raw unions (without being embedded in a class and some safe-access methods) are very non-Pythonic? So just curious if they were ever meant to be supported.

[bstaletic]

Guess it doesn't matter, because if is_same is true, is_convertible will be true.

That's true.

But this is closes to what's meant, I suppose?

That's the part I'm not sure about.

Myeah. Just ... I don't know. Raw unions (without being embedded in a class and some safe-access methods) are very non-Pythonic? So just curious if they were ever meant to be supported.

That's a valid point. I don't have the answer. I could submit a PR and see what wjakob says.

[YannickJadoul]

That's a valid point. I don't have the answer. I could submit a PR and see what wjakob says.

Maybe that's a good plan? I guess you already have the patch, anyway. Tests can wait until we know whether it's worth adding them or not.

[bstaletic]

#2320