Merge pull request #225 from ruby-rice/type_verification

Improve Type Verification

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());
     }
   }
 

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<typename T>
-  bool Type<T>::verify()
-  {
-    // Use intrinsic_type so that we don't have to define specializations
-    // for pointers, references, const, etc.
-    using Intrinsic_T = intrinsic_type<T>;
-
-    if constexpr (std::is_fundamental_v<Intrinsic_T>)
-    {
-      return true;
-    }
-    else
-    {
-      return Registries::instance.types.verifyDefined<Intrinsic_T>();
-    }
-  }
 }

rice/detail/Type.hpp

@@ -3,6 +3,7 @@
 
 #include <string>
 #include <typeinfo>
+#include <typeindex>
 #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<typename T>
@@ -24,6 +26,4 @@ namespace Rice::detail
   void verifyTypes();
 }
 
-#include "Type.ipp"
-
 #endif // Rice__Type__hpp_

rice/detail/Type.ipp

@@ -1,4 +1,5 @@
 #include "../traits/rice_traits.hpp"
+#include "Registries.hpp"
 
 #include <iosfwd>
 #include <iterator>
@@ -15,6 +16,19 @@
 
 namespace Rice::detail
 {
+  template<typename T>
+  bool Type<T>::verify()
+  {
+    if constexpr (std::is_fundamental_v<T>)
+    {
+      return true;
+    }
+    else
+    {
+      return Registries::instance.types.verify<T>();
+    }
+  }
+
   template<>
   struct Type<void>
   {
@@ -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());

rice/detail/TypeRegistry.hpp

@@ -6,6 +6,7 @@
 #include <typeindex>
 #include <typeinfo>
 #include <unordered_map>
+#include <set>
 
 #include "ruby.hpp"
 
@@ -31,14 +32,22 @@ namespace Rice::detail
     bool isDefined();
 
     template <typename T>
-    bool verifyDefined();
+    bool verify();
 
     template <typename T>
     std::pair<VALUE, rb_data_type_t*> 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<std::pair<VALUE, rb_data_type_t*>> lookup(const std::type_info& typeInfo);
+    void raiseUnverifiedType(const std::string& typeName);
+
     std::unordered_map<std::type_index, std::pair<VALUE, rb_data_type_t*>> registry_{};
+    std::set<std::type_index> unverified_{};
+    bool verified_ = true;
   };
 }
 

rice/detail/TypeRegistry.ipp

@@ -1,4 +1,6 @@
 #include <stdexcept>
+#include <sstream>
+#include <typeindex>
 
 #include "ruby.hpp"
 #include "../traits/rice_traits.hpp"
@@ -45,14 +47,20 @@ namespace Rice::detail
   }
 
   template <typename T>
-  inline bool TypeRegistry::verifyDefined()
+  inline bool TypeRegistry::verify()
   {
-    if (!isDefined<T>())
+    if (isDefined<T>())
     {
-      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<std::pair<VALUE, rb_data_type_t*>> TypeRegistry::lookup(const std::type_info& typeInfo)
@@ -73,6 +81,11 @@ namespace Rice::detail
   template <typename T>
   inline std::pair<VALUE, rb_data_type_t*> 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<std::pair<VALUE, rb_data_type_t*>> 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<VALUE, rb_data_type_t*>(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);
   }
 }

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"

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<typename T>
-    using intrinsic_type = typename std::remove_cv_t<std::remove_pointer_t<std::remove_reference_t<T>>>;
+    using intrinsic_type = typename std::remove_cv_t<std::remove_pointer_t<std::remove_pointer_t<std::remove_reference_t<T>>>>;
 
     // Recursively remove const/volatile
     template<typename T>

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<DataStruct> c = define_class<DataStruct>("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())
   );
 }