From c2541a941ed0f3d3e52320a6343c7ca640b8245a Mon Sep 17 00:00:00 2001 From: cfis Date: Thu, 21 Nov 2024 00:11:53 -0800 Subject: [PATCH] It turns out immediate Type verification when calling define_method, define_attr, define_enum, define_iter doesn't work on large C++ code bases because of C++ forwarding support. Therefore instead of immediately failing, this commit updates the TypeRegistry to track undefined types. Then when figure_type is called for the first time, which will happen when Ruby calls a C++ function that returns an object, the TypeRegistry will check the undefined types. If any still remain undefined, because they are not in the list of defined types, it will throw will throw an exception. So if an exception is thrown it should hopefully happen almost immediately at program startup and not at some random point in the future when a method with an undefined type is called. --- rice/Data_Type.ipp | 4 +- rice/detail/Registries.ipp | 18 --------- rice/detail/Type.hpp | 4 +- rice/detail/Type.ipp | 19 +++++++++ rice/detail/TypeRegistry.hpp | 11 ++++- rice/detail/TypeRegistry.ipp | 78 +++++++++++++++++++++++++++++++----- rice/rice.hpp | 1 + rice/traits/rice_traits.hpp | 5 ++- test/test_Attribute.cpp | 25 +++++++++--- test/test_Data_Type.cpp | 65 +++++++++++++++++++++++++----- test/test_Enum.cpp | 33 ++++++++++++--- 11 files changed, 208 insertions(+), 55 deletions(-) diff --git a/rice/Data_Type.ipp b/rice/Data_Type.ipp index 06a640be..78ceca51 100644 --- a/rice/Data_Type.ipp +++ b/rice/Data_Type.ipp @@ -191,8 +191,8 @@ namespace Rice { if (!is_bound()) { - std::string message = "Type " + detail::typeName(typeid(T)) + " is not bound"; - throw std::runtime_error(message.c_str()); + std::string message = "Type is not defined with Rice: " + detail::typeName(typeid(T)); + throw std::invalid_argument(message.c_str()); } } diff --git a/rice/detail/Registries.ipp b/rice/detail/Registries.ipp index a063e7ad..02734aea 100644 --- a/rice/detail/Registries.ipp +++ b/rice/detail/Registries.ipp @@ -2,22 +2,4 @@ namespace Rice::detail { //Initialize static variables here. inline Registries Registries::instance; - - // TODO - Big hack here but this code is dependent on internals - template - bool Type::verify() - { - // Use intrinsic_type so that we don't have to define specializations - // for pointers, references, const, etc. - using Intrinsic_T = intrinsic_type; - - if constexpr (std::is_fundamental_v) - { - return true; - } - else - { - return Registries::instance.types.verifyDefined(); - } - } } diff --git a/rice/detail/Type.hpp b/rice/detail/Type.hpp index 90882abd..c510cabc 100644 --- a/rice/detail/Type.hpp +++ b/rice/detail/Type.hpp @@ -3,6 +3,7 @@ #include #include +#include #include "../traits/rice_traits.hpp" namespace Rice::detail @@ -15,6 +16,7 @@ namespace Rice::detail // Return the name of a type std::string typeName(const std::type_info& typeInfo); + std::string typeName(const std::type_index& typeIndex); std::string makeClassName(const std::type_info& typeInfo); template @@ -24,6 +26,4 @@ namespace Rice::detail void verifyTypes(); } -#include "Type.ipp" - #endif // Rice__Type__hpp_ diff --git a/rice/detail/Type.ipp b/rice/detail/Type.ipp index 4df1372d..2d4729af 100644 --- a/rice/detail/Type.ipp +++ b/rice/detail/Type.ipp @@ -1,4 +1,5 @@ #include "../traits/rice_traits.hpp" +#include "Registries.hpp" #include #include @@ -15,6 +16,19 @@ namespace Rice::detail { + template + bool Type::verify() + { + if constexpr (std::is_fundamental_v) + { + return true; + } + else + { + return Registries::instance.types.verify(); + } + } + template<> struct Type { @@ -89,6 +103,11 @@ namespace Rice::detail return demangle(typeInfo.name()); } + inline std::string typeName(const std::type_index& typeIndex) + { + return demangle(typeIndex.name()); + } + inline std::string makeClassName(const std::type_info& typeInfo) { std::string base = demangle(typeInfo.name()); diff --git a/rice/detail/TypeRegistry.hpp b/rice/detail/TypeRegistry.hpp index 4f7f4c09..8a9bbd3e 100644 --- a/rice/detail/TypeRegistry.hpp +++ b/rice/detail/TypeRegistry.hpp @@ -6,6 +6,7 @@ #include #include #include +#include #include "ruby.hpp" @@ -31,14 +32,22 @@ namespace Rice::detail bool isDefined(); template - bool verifyDefined(); + bool verify(); template std::pair figureType(const T& object); + // Validate unverified types and throw an exception if any exist. This is mostly for unit tests. + void validateUnverifiedTypes(); + // Clear unverified types. This is mostly for unit tests + void clearUnverifiedTypes(); private: std::optional> lookup(const std::type_info& typeInfo); + void raiseUnverifiedType(const std::string& typeName); + std::unordered_map> registry_{}; + std::set unverified_{}; + bool verified_ = true; }; } diff --git a/rice/detail/TypeRegistry.ipp b/rice/detail/TypeRegistry.ipp index 371b9de4..bcd2388f 100644 --- a/rice/detail/TypeRegistry.ipp +++ b/rice/detail/TypeRegistry.ipp @@ -1,4 +1,6 @@ #include +#include +#include #include "ruby.hpp" #include "../traits/rice_traits.hpp" @@ -45,14 +47,20 @@ namespace Rice::detail } template - inline bool TypeRegistry::verifyDefined() + inline bool TypeRegistry::verify() { - if (!isDefined()) + if (isDefined()) { - std::string message = "Type is not defined with Rice: " + detail::typeName(typeid(T)); - throw std::invalid_argument(message); + return true; + } + else + { + const std::type_info& typeInfo = typeid(T); + std::type_index key(typeInfo); + this->unverified_.insert(key); + this->verified_ = false; + return false; } - return true; } inline std::optional> TypeRegistry::lookup(const std::type_info& typeInfo) @@ -73,6 +81,11 @@ namespace Rice::detail template inline std::pair TypeRegistry::figureType(const T& object) { + if (!this->verified_) + { + this->validateUnverifiedTypes(); + } + // First check and see if the actual type of the object is registered std::optional> result = lookup(typeid(object)); @@ -84,14 +97,61 @@ namespace Rice::detail // If not, then we are willing to accept an ancestor class specified by T. This is needed // to support Directors. Classes inherited from Directors are never actually registered // with Rice - and what we really want it to return the C++ class they inherit from. - result = lookup(typeid(T)); + const std::type_info& typeInfo = typeid(T); + result = lookup(typeInfo); if (result) { return result.value(); } - // Give up! - std::string message = "Type " + typeName(typeid(object)) + " is not registered"; - throw std::runtime_error(message.c_str()); + raiseUnverifiedType(detail::typeName(typeInfo)); + // Make the compiler happy + return std::pair(Qnil, nullptr); + } + + inline void TypeRegistry::validateUnverifiedTypes() + { + // Loop over the unverified types and delete each on that is found in the registry + // the registry and raise an exception for the first one that is not + for (auto iter = this->unverified_.begin(); iter != this->unverified_.end(); ) + { + const std::type_index& typeIndex = *iter; + bool isDefined = this->registry_.find(typeIndex) != this->registry_.end(); + if (isDefined) + { + iter = this->unverified_.erase(iter); + } + else + { + iter++; + } + } + + if (this->unverified_.empty()) + { + this->verified_ = true; + return; + } + + std::stringstream stream; + stream << "The following types are not registered with Rice:" << "\n"; + + for (const std::type_index& typeIndex : this->unverified_) + { + stream << " " << typeName(typeIndex) << "\n"; + } + + throw std::invalid_argument(stream.str()); + } + + inline void TypeRegistry::clearUnverifiedTypes() + { + this->unverified_.clear(); + } + + inline void TypeRegistry::raiseUnverifiedType(const std::string& typeName) + { + std::string message = "Type is not registered with Rice: " + typeName; + throw std::invalid_argument(message); } } \ No newline at end of file diff --git a/rice/rice.hpp b/rice/rice.hpp index e59054a4..633fabd5 100644 --- a/rice/rice.hpp +++ b/rice/rice.hpp @@ -29,6 +29,7 @@ #include "detail/HandlerRegistry.hpp" #include "detail/NativeRegistry.hpp" #include "detail/Registries.hpp" +#include "detail/Type.ipp" #include "detail/cpp_protect.hpp" #include "detail/Wrapper.hpp" #include "detail/MethodInfo.hpp" diff --git a/rice/traits/rice_traits.hpp b/rice/traits/rice_traits.hpp index 9ec7d1a4..4f1ce595 100644 --- a/rice/traits/rice_traits.hpp +++ b/rice/traits/rice_traits.hpp @@ -10,9 +10,10 @@ namespace Rice { namespace detail { - // Get the base_type of T - without pointer, reference, const or volatile + // Get the base_type of T - without pointer, reference, const or volatile. We call remove_pointer_t twice + // for T** template - using intrinsic_type = typename std::remove_cv_t>>; + using intrinsic_type = typename std::remove_cv_t>>>; // Recursively remove const/volatile template diff --git a/test/test_Attribute.cpp b/test/test_Attribute.cpp index 5073c82c..f879f6c0 100644 --- a/test/test_Attribute.cpp +++ b/test/test_Attribute.cpp @@ -14,6 +14,11 @@ SETUP(Attribute) embed_ruby(); } +TEARDOWN(Attribute) +{ + Rice::detail::Registries::instance.types.clearUnverifiedTypes(); +} + namespace { class SomeClass @@ -171,20 +176,30 @@ TESTCASE(not_defined) Data_Type c = define_class("DataStruct"); #ifdef _MSC_VER - const char* message = "Type is not defined with Rice: class `anonymous namespace'::SomeClass"; + const char* message = "The following types are not registered with Rice:\n class `anonymous namespace'::SomeClass\n"; #else - const char* message = "Type is not defined with Rice: (anonymous namespace)::SomeClass"; + const char* message = "The following types are not registered with Rice:\n (anonymous namespace)::SomeClass\n"; #endif + c.define_singleton_attr("some_class_static", &DataStruct::someClassStatic); + ASSERT_EXCEPTION_CHECK( std::invalid_argument, - c.define_singleton_attr("some_class_static", &DataStruct::someClassStatic), + Rice::detail::Registries::instance.types.validateUnverifiedTypes(), ASSERT_EQUAL(message, ex.what()) ); ASSERT_EXCEPTION_CHECK( - std::invalid_argument, - c.define_attr("some_class", &DataStruct::someClass), + Rice::Exception, + c.call("some_class_static"), + ASSERT_EQUAL(message, ex.what()) + ); + + c.define_attr("some_class", &DataStruct::someClass); + Object o = c.call("new"); + ASSERT_EXCEPTION_CHECK( + Rice::Exception, + o.call("some_class"), ASSERT_EQUAL(message, ex.what()) ); } \ No newline at end of file diff --git a/test/test_Data_Type.cpp b/test/test_Data_Type.cpp index f6154112..d06ae8fb 100644 --- a/test/test_Data_Type.cpp +++ b/test/test_Data_Type.cpp @@ -14,6 +14,11 @@ SETUP(Data_Type) embed_ruby(); } +TEARDOWN(Data_Type) +{ + Rice::detail::Registries::instance.types.clearUnverifiedTypes(); +} + namespace { class MyClass @@ -618,39 +623,77 @@ TESTCASE(pointerToPointer) namespace { - class SomeClass + class UnknownClass { }; - void undefinedArg(SomeClass& someClass) + void undefinedArg(UnknownClass unknownClass) + { + } + + void undefinedArg(UnknownClass& unknownClass) { } - SomeClass undefinedReturn() + void undefinedArg(UnknownClass* unknownClass) { - return SomeClass(); + } + + UnknownClass undefinedReturn() + { + return UnknownClass(); } } TESTCASE(not_defined) { + Module m = Module(rb_mKernel); + #ifdef _MSC_VER - const char* message = "Type is not defined with Rice: class `anonymous namespace'::SomeClass"; + const char* message = "The following types are not registered with Rice:\n class `anonymous namespace'::UnknownClass\n"; #else - const char* message = "Type is not defined with Rice: (anonymous namespace)::SomeClass"; + const char* message = "The following types are not registered with Rice:\n (anonymous namespace)::UnknownClass\n"; #endif - - ASSERT_EXCEPTION_CHECK( - std::invalid_argument, - define_global_function("undefined_arg", &undefinedArg), + + define_global_function("undefined_return", &undefinedReturn); + ASSERT_EXCEPTION_CHECK( + Rice::Exception, + m.call("undefined_return"), ASSERT_EQUAL(message, ex.what()) ); + define_global_function("undefined_arg_value", &undefinedArg); + ASSERT_EXCEPTION_CHECK( std::invalid_argument, - define_global_function("undefined_return", &undefinedReturn), + Rice::detail::Registries::instance.types.validateUnverifiedTypes(), ASSERT_EQUAL(message, ex.what()) ); + +#ifdef _MSC_VER + message = "Type is not defined with Rice: class `anonymous namespace'::UnknownClass"; +#else + message = "Type is not defined with Rice: (anonymous namespace)::UnknownClass"; +#endif + + ASSERT_EXCEPTION_CHECK( + Rice::Exception, + m.call("undefined_arg_value", nullptr), + ASSERT_EQUAL(message, ex.what()) + ); + + define_global_function("undefined_arg_reference", &undefinedArg); + + ASSERT_EXCEPTION_CHECK( + Rice::Exception, + m.call("undefined_arg_reference", nullptr), + ASSERT_EQUAL(message, ex.what()) + ); + + define_global_function("undefined_arg_pointer", &undefinedArg); + + // This actually works because we pass a nullptr + m.call("undefined_arg_pointer", nullptr); } namespace diff --git a/test/test_Enum.cpp b/test/test_Enum.cpp index e5ae21d2..7d70fa25 100644 --- a/test/test_Enum.cpp +++ b/test/test_Enum.cpp @@ -14,6 +14,12 @@ SETUP(Enum) embed_ruby(); } +TEARDOWN(Enum) +{ + Rice::detail::Registries::instance.types.clearUnverifiedTypes(); +} + + namespace { enum Color { RED, BLACK, GREEN }; @@ -404,21 +410,38 @@ namespace TESTCASE(not_defined) { + Module m = Module(rb_mKernel); + #ifdef _MSC_VER - const char* message = "Type is not defined with Rice: enum `anonymous namespace'::Undefined"; + const char* message = "The following types are not registered with Rice:\n enum `anonymous namespace'::Undefined\n"; #else - const char* message = "Type is not defined with Rice: (anonymous namespace)::Undefined"; + const char* message = "The following types are not registered with Rice:\n (anonymous namespace)::Undefined\n"; #endif + define_global_function("undefined_arg", &undefinedArg); + ASSERT_EXCEPTION_CHECK( std::invalid_argument, - define_global_function("undefined_arg", &undefinedArg), + Rice::detail::Registries::instance.types.validateUnverifiedTypes(), ASSERT_EQUAL(message, ex.what()) ); + define_global_function("undefined_return", &undefinedReturn); ASSERT_EXCEPTION_CHECK( - std::invalid_argument, - define_global_function("undefined_return", &undefinedReturn), + Rice::Exception, + m.call("undefined_return"), + ASSERT_EQUAL(message, ex.what()) + ); + +#ifdef _MSC_VER + message = "Type is not defined with Rice: enum `anonymous namespace'::Undefined"; +#else + message = "Type is not defined with Rice: (anonymous namespace)::Undefined"; +#endif + + ASSERT_EXCEPTION_CHECK( + Rice::Exception, + m.call("undefined_arg", 1), ASSERT_EQUAL(message, ex.what()) ); } \ No newline at end of file