From 2b86179283f20f2e07cadf4b37304bd9cc263cf6 Mon Sep 17 00:00:00 2001
From: Joshua Oreman <oremanj@hudson-trading.com>
Date: Thu, 11 May 2023 18:29:04 -0600
Subject: [PATCH] Validate our assumption that the base class is a primary
 (offset-zero) base

This is easy to do at ~compile-time if the base is specified as a template argument. (It's not legal to `reinterpret_cast` in a constant expression, so the error is reported at runtime, but the test and error message are expected to be compiled out if the test doesn't fail.)

Also detect accessible virtual bases at compile time. (I'm not aware of a way to detect inaccessible virtual bases either at compile time or runtime, at least not without obtaining an instance pointer somehow.)

It is sort of possible to validate this if the base is specified as a Python object, but much more difficult (requires mucking around with implementation-defined subclasses of `std::type_info`). I can add an implementation of that for libstdc++ on Linux if desired, but it costs about 400 bytes in libnanobind and I don't know how to do it on other platforms. It didn't seem worth it to me.
---
 include/nanobind/nb_class.h  | 17 +++++++++++++++++
 include/nanobind/nb_traits.h |  8 ++++++++
 tests/test_classes.cpp       | 19 +++++++++++++++++++
 tests/test_classes.py        |  7 +++++++
 4 files changed, 51 insertions(+)

diff --git a/include/nanobind/nb_class.h b/include/nanobind/nb_class.h
index 4120f43f..ec4bc013 100644
--- a/include/nanobind/nb_class.h
+++ b/include/nanobind/nb_class.h
@@ -340,6 +340,16 @@ class class_ : public object {
         sizeof...(Ts) == !std::is_same_v<Base, T> + !std::is_same_v<Alias, T>,
         "nanobind::class_<> was invoked with extra arguments that could not be handled");
 
+    // Fail on virtual bases -- they need a this-ptr adjustment, but they're
+    // not amenable to the runtime test in the class_ constructor (because
+    // a C-style cast will do reinterpret_cast if static_cast is invalid).
+    // Primary but inaccessible (ambiguous or private) bases will also
+    // fail is_accessible_static_base_of; the !is_convertible option is to
+    // avoid mis-detecting them as virtual bases.
+    static_assert(!std::is_convertible_v<T*, Base*> ||
+                  detail::is_accessible_static_base_of<Base, T>::value,
+                  "nanobind does not support virtual base classes");
+
     template <typename... Extra>
     NB_INLINE class_(handle scope, const char *name, const Extra &... extra) {
         detail::type_init_data d;
@@ -354,6 +364,13 @@ class class_ : public object {
         if constexpr (!std::is_same_v<Base, T>) {
             d.base = &typeid(Base);
             d.flags |= (uint32_t) detail::type_init_flags::has_base;
+
+            if (uintptr_t offset = (uintptr_t) (Base*) (T*) 0x1000 - 0x1000) {
+                detail::raise("nanobind::class_<>: base class %s is at offset "
+                              "%td within %s! nanobind does not support "
+                              "multiple inheritance", typeid(Base).name(),
+                              offset, typeid(T).name());
+            }
         }
 
         if constexpr (!std::is_same_v<Alias, T>)
diff --git a/include/nanobind/nb_traits.h b/include/nanobind/nb_traits.h
index 66ea2105..83601d3f 100644
--- a/include/nanobind/nb_traits.h
+++ b/include/nanobind/nb_traits.h
@@ -132,6 +132,14 @@ struct detector<std::void_t<Op<Arg>>, Op, Arg>
    avoid redundancy when combined with nb::arg(...).none(). */
 template <typename T> struct remove_opt_mono { using type = T; };
 
+template <typename Base, typename Derived>
+struct is_accessible_static_base_of {
+    template <typename B = Base, typename D = Derived>
+    static decltype(static_cast<D*>(std::declval<B*>()), std::true_type()) check(B*);
+    static std::false_type check(...);
+    static constexpr bool value = decltype(check((Base*)nullptr))::value;
+};
+
 NAMESPACE_END(detail)
 
 template <typename... Args>
diff --git a/tests/test_classes.cpp b/tests/test_classes.cpp
index e54e8261..adcd7c19 100644
--- a/tests/test_classes.cpp
+++ b/tests/test_classes.cpp
@@ -477,4 +477,23 @@ NB_MODULE(test_classes_ext, m) {
     m.def("polymorphic_factory_2", []() { return (PolymorphicBase *) new AnotherPolymorphicSubclass(); });
     m.def("factory", []() { return (Base *) new Subclass(); });
     m.def("factory_2", []() { return (Base *) new AnotherSubclass(); });
+
+    // Test detection of unsupported base/derived relationships
+    // (nanobind requires base subobjects to be at offset zero in the
+    // derived class)
+    struct DerivedBecomesPolymorphic : Struct {
+        virtual ~DerivedBecomesPolymorphic() {}
+    };
+    struct DerivedNonPrimary : Big, Struct {};
+    struct DerivedVirt : virtual Struct {};
+    m.def("bind_newly_polymorphic_subclass", []() {
+        nb::class_<DerivedBecomesPolymorphic, Struct>(
+            nb::handle(), "DerivedBecomesPolymorphic");
+    });
+    m.def("bind_subclass_with_non_primary_base", []() {
+        nb::class_<DerivedNonPrimary, Struct>(nb::handle(), "DerivedNonPrimary");
+    });
+    static_assert(!nb::detail::is_accessible_static_base_of<Struct, DerivedVirt>::value);
+    // this fails at compile time:
+    //nb::class_<DerivedVirt, Struct>(nb::handle(), "DerivedVirt");
 }
diff --git a/tests/test_classes.py b/tests/test_classes.py
index 6fc27ca8..d4305e14 100644
--- a/tests/test_classes.py
+++ b/tests/test_classes.py
@@ -631,3 +631,10 @@ def name(self):
             assert t.go(d2) == 'Rufus says woof'
         finally:
             t.Dog.name = old
+
+
+def test35_non_primary():
+    with pytest.raises(RuntimeError, match="not support multiple inheritance"):
+        t.bind_newly_polymorphic_subclass()
+    with pytest.raises(RuntimeError, match="not support multiple inheritance"):
+        t.bind_subclass_with_non_primary_base()