diff --git a/rice/detail/Wrapper.ipp b/rice/detail/Wrapper.ipp index 64489cc4..718189ea 100644 --- a/rice/detail/Wrapper.ipp +++ b/rice/detail/Wrapper.ipp @@ -122,8 +122,9 @@ namespace Rice::detail if constexpr (is_complete_v) { // is_abstract_v requires a complete type, so nest inside is_complete_v. - // Deleting an abstract class through a non-virtual destructor is UB. - if constexpr (std::is_destructible_v && !std::is_abstract_v) + // Deleting an abstract class through a non-virtual destructor is UB, + // but it is safe if the destructor is virtual. + if constexpr (std::is_destructible_v && (!std::is_abstract_v || std::has_virtual_destructor_v)) { if (this->isOwner_) { diff --git a/test/test_Ownership.cpp b/test/test_Ownership.cpp index 6846017e..e7363e77 100644 --- a/test/test_Ownership.cpp +++ b/test/test_Ownership.cpp @@ -188,6 +188,36 @@ namespace static inline MyCopyableClass* instance_ = nullptr; }; + // Abstract class with virtual destructor - deletion through base pointer is safe + class AbstractVirtualDtor + { + public: + static inline int destructorCalls = 0; + + static void reset() + { + destructorCalls = 0; + } + + virtual ~AbstractVirtualDtor() + { + destructorCalls++; + } + + virtual int compute() const = 0; + }; + + class ConcreteChild : public AbstractVirtualDtor + { + public: + int compute() const override { return 42; } + }; + + AbstractVirtualDtor* createChild() + { + return new ConcreteChild(); + } + class OwnerBox; class AliasBox @@ -327,6 +357,14 @@ SETUP(Ownership) .define_constructor(Constructor()) .define_method("create_owned", &SharedFactory::createOwned, Return().takeOwnership()) .define_method("get_borrowed", &SharedFactory::getBorrowed); + + define_class("AbstractVirtualDtor") + .define_method("compute", &AbstractVirtualDtor::compute); + + define_class("ConcreteChild"); + + define_module("AbstractVirtualDtorTest") + .define_module_function("create_child", &createChild, Return().takeOwnership()); } TEARDOWN(Ownership) @@ -541,3 +579,23 @@ TESTCASE(MultipleOwnerReferences) result = m.module_eval(code); ASSERT_EQUAL(Qfalse, result.value()); } + +TESTCASE(AbstractVirtualDestructor) +{ + AbstractVirtualDtor::reset(); + + Module m = define_module("TestingModule"); + + std::string code = R"(child = AbstractVirtualDtorTest.create_child + child.compute)"; + + Object result = m.module_eval(code); + ASSERT_EQUAL(detail::To_Ruby().convert(42), result.value()); + + // Force GC to clean up the object - destructor should be called + // since AbstractVirtualDtor has a virtual destructor + rb_gc_start(); + rb_gc_start(); + + ASSERT_EQUAL(1, AbstractVirtualDtor::destructorCalls); +}