Merge pull request #194 from lutaml/master

Throw an exception when wrapper is not available at addKeepAlive
ruby-rice · Jan 2, 2024 · c2816a0 · c2816a0
2 parents 4e29d09 + 7e34c38
commit c2816a0
Show file tree

Hide file tree

Showing 6 changed files with 146 additions and 8 deletions.
diff --git a/doc/bindings/memory_management.rst b/doc/bindings/memory_management.rst
@@ -198,6 +198,8 @@ Obviously this code could be rewritten to make sure the database object remains
   define_class<Database>("Database")
     .define_method("get_column", &Database::getColumn, Return().keepAlive())
 
+Note that Return().keepAlive() will work with external types only. An attempt to use it with builtin type will result in runtime exception.
+
 C++ Referencing Ruby Objects
 ----------------------------
 

diff --git a/include/rice/rice.hpp b/include/rice/rice.hpp
@@ -1531,7 +1531,7 @@ namespace Rice::detail
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Warray-bounds"
 #endif
-    return static_cast<Wrapper*>(RTYPEDDATA_DATA(value));
+    return RTYPEDDATA_P(value) ? static_cast<Wrapper*>(RTYPEDDATA_DATA(value)) : nullptr;
 #ifdef __GNUC__
 #pragma GCC diagnostic pop
 #endif
@@ -4052,6 +4052,9 @@ namespace Rice::detail
     // Figure out what self is
     Class_T getReceiver(VALUE self);
 
+    // Throw an exception when wrapper cannot be extracted
+    [[noreturn]] static void noWrapper(const VALUE klass, const std::string& wrapper);
+
     // Do we need to keep alive any arguments?
     void checkKeepAlive(VALUE self, VALUE returnValue, std::vector<VALUE>& rubyValues);
 
@@ -4107,7 +4110,7 @@ namespace Rice::detail
   NativeFunction<From_Ruby_T, Function_T, IsMethod>::NativeFunction(VALUE klass, std::string method_name, Function_T function, MethodInfo* methodInfo)
     : klass_(klass), method_name_(method_name), function_(function), methodInfo_(methodInfo)
   {
-   // Create a tuple of NativeArgs that will convert the Ruby values to native values. For 
+    // Create a tuple of NativeArgs that will convert the Ruby values to native values. For 
     // builtin types NativeArgs will keep a copy of the native value so that it 
     // can be passed by reference or pointer to the native function. For non-builtin types
     // it will just pass the value through.
@@ -4233,7 +4236,7 @@ namespace Rice::detail
     {
       // Call the native method and get the result
       Return_T nativeResult = std::apply(this->function_, nativeArgs);
-      
+
       // Return the result
       return this->toRuby_.convert(nativeResult);
     }
@@ -4284,23 +4287,48 @@ namespace Rice::detail
     }
   }
 
+  template<typename From_Ruby_T, typename Function_T, bool IsMethod>
+  void NativeFunction<From_Ruby_T, Function_T, IsMethod>::noWrapper(const VALUE klass, const std::string& wrapper)
+  {
+    std::string message = std::string("Could not find wrapper for '") + rb_obj_classname(klass) + "' " +
+                          wrapper + " type. Did you use keepAlive() on a method that returns a builtin type?";
+    throw std::runtime_error(message);
+  }
+
   template<typename From_Ruby_T, typename Function_T, bool IsMethod>
   void NativeFunction<From_Ruby_T, Function_T, IsMethod>::checkKeepAlive(VALUE self, VALUE returnValue, std::vector<VALUE>& rubyValues)
   {
-    // Check function arguments
+    // selfWrapper will be nullptr if this(self) is a builtin type and not an external(wrapped) type
+    // it is highly unlikely that keepAlive is used in this case but we check anyway
     Wrapper* selfWrapper = getWrapper(self);
+
+    // Check function arguments
     for (const Arg& arg : (*this->methodInfo_))
     {
       if (arg.isKeepAlive())
       {
+        if (selfWrapper == nullptr)
+        {
+          noWrapper(self, "self");
+        }
         selfWrapper->addKeepAlive(rubyValues[arg.position]);
       }
     }
 
     // Check return value
     if (this->methodInfo_->returnInfo.isKeepAlive())
     {
+      if (selfWrapper == nullptr)
+      {
+        noWrapper(self, "self");
+      }
+
+      // returnWrapper will be nullptr if returnValue is a buillt-in type and not an external(wrapped) type
       Wrapper* returnWrapper = getWrapper(returnValue);
+      if (returnWrapper == nullptr)
+      {
+        noWrapper(returnValue, "return");
+      }
       returnWrapper->addKeepAlive(self);
     }
   }

diff --git a/rice/detail/NativeFunction.hpp b/rice/detail/NativeFunction.hpp
@@ -95,6 +95,9 @@ namespace Rice::detail
     // Figure out what self is
     Class_T getReceiver(VALUE self);
 
+    // Throw an exception when wrapper cannot be extracted
+    [[noreturn]] static void noWrapper(const VALUE klass, const std::string& wrapper);
+
     // Do we need to keep alive any arguments?
     void checkKeepAlive(VALUE self, VALUE returnValue, std::vector<VALUE>& rubyValues);
 

diff --git a/rice/detail/NativeFunction.ipp b/rice/detail/NativeFunction.ipp
@@ -38,7 +38,7 @@ namespace Rice::detail
   NativeFunction<From_Ruby_T, Function_T, IsMethod>::NativeFunction(VALUE klass, std::string method_name, Function_T function, MethodInfo* methodInfo)
     : klass_(klass), method_name_(method_name), function_(function), methodInfo_(methodInfo)
   {
-   // Create a tuple of NativeArgs that will convert the Ruby values to native values. For 
+    // Create a tuple of NativeArgs that will convert the Ruby values to native values. For 
     // builtin types NativeArgs will keep a copy of the native value so that it 
     // can be passed by reference or pointer to the native function. For non-builtin types
     // it will just pass the value through.
@@ -164,7 +164,7 @@ namespace Rice::detail
     {
       // Call the native method and get the result
       Return_T nativeResult = std::apply(this->function_, nativeArgs);
-      
+
       // Return the result
       return this->toRuby_.convert(nativeResult);
     }
@@ -215,23 +215,48 @@ namespace Rice::detail
     }
   }
 
+  template<typename From_Ruby_T, typename Function_T, bool IsMethod>
+  void NativeFunction<From_Ruby_T, Function_T, IsMethod>::noWrapper(const VALUE klass, const std::string& wrapper)
+  {
+    std::string message = std::string("Could not find wrapper for '") + rb_obj_classname(klass) + "' " +
+                          wrapper + " type. Did you use keepAlive() on a method that returns a builtin type?";
+    throw std::runtime_error(message);
+  }
+
   template<typename From_Ruby_T, typename Function_T, bool IsMethod>
   void NativeFunction<From_Ruby_T, Function_T, IsMethod>::checkKeepAlive(VALUE self, VALUE returnValue, std::vector<VALUE>& rubyValues)
   {
-    // Check function arguments
+    // selfWrapper will be nullptr if this(self) is a builtin type and not an external(wrapped) type
+    // it is highly unlikely that keepAlive is used in this case but we check anyway
     Wrapper* selfWrapper = getWrapper(self);
+
+    // Check function arguments
     for (const Arg& arg : (*this->methodInfo_))
     {
       if (arg.isKeepAlive())
       {
+        if (selfWrapper == nullptr)
+        {
+          noWrapper(self, "self");
+        }
         selfWrapper->addKeepAlive(rubyValues[arg.position]);
       }
     }
 
     // Check return value
     if (this->methodInfo_->returnInfo.isKeepAlive())
     {
+      if (selfWrapper == nullptr)
+      {
+        noWrapper(self, "self");
+      }
+
+      // returnWrapper will be nullptr if returnValue is a buillt-in type and not an external(wrapped) type
       Wrapper* returnWrapper = getWrapper(returnValue);
+      if (returnWrapper == nullptr)
+      {
+        noWrapper(returnValue, "return");
+      }
       returnWrapper->addKeepAlive(self);
     }
   }

diff --git a/rice/detail/Wrapper.ipp b/rice/detail/Wrapper.ipp
@@ -193,7 +193,7 @@ namespace Rice::detail
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Warray-bounds"
 #endif
-    return static_cast<Wrapper*>(RTYPEDDATA_DATA(value));
+    return RTYPEDDATA_P(value) ? static_cast<Wrapper*>(RTYPEDDATA_DATA(value)) : nullptr;
 #ifdef __GNUC__
 #pragma GCC diagnostic pop
 #endif

diff --git a/test/test_Keep_Alive_No_Wrapper.cpp b/test/test_Keep_Alive_No_Wrapper.cpp
@@ -0,0 +1,80 @@
+#include "unittest.hpp"
+#include "embed_ruby.hpp"
+#include <rice/rice.hpp>
+#include <rice/stl.hpp>
+
+using namespace Rice;
+
+TESTSUITE(Keep_Alive_No_Wrapper);
+
+namespace
+{
+  class Animal
+  {
+    public:
+      Animal(char const * name) : name_(name) {}
+      char const * getName() { return name_; }
+      virtual ~Animal() = default;
+    private:
+      char const * name_;
+  };
+
+  class Zoo
+  {
+    public:
+      Zoo(void)
+      {
+        pets_.push_back(new Animal("Bear"));
+        pets_.push_back(new Animal("Tiger"));
+        pets_.push_back(new Animal("Lion"));
+      }
+
+      ~Zoo()
+      {
+        for(auto pet : pets_)
+        {
+         delete pet;
+        }
+        pets_.clear();
+      }
+
+    Object getPets(void)    {
+      Array pets;
+      for(auto p: pets_) {
+        pets.push(p);
+      }
+      return pets;
+    }
+
+    private:
+      std::vector<Animal*> pets_;
+  };
+}
+
+SETUP(Keep_Alive_No_Wrapper)
+{
+  embed_ruby();
+}
+
+TESTCASE(test_keep_alive_no_wrapper)
+{
+  define_class<Animal>("Animal")
+    .define_constructor(Constructor<Animal, char const *>())
+    .define_method("get_name", &Animal::getName);
+
+  define_class<Zoo>("Zoo")
+    .define_constructor(Constructor<Zoo>())
+    .define_method("get_pets", &Zoo::getPets, Return().keepAlive());
+
+  Module m = define_module("TestingModule");
+  Object zoo = m.module_eval("@zoo = Zoo.new");
+
+  // get_pets returns an Array (builtin type) so Return().keepAlive()
+  // shall result in std::runtime_error
+  ASSERT_EXCEPTION_CHECK(
+    Exception,
+    m.module_eval("@zoo.get_pets.each do |pet| puts pet.name; end"),
+    ASSERT_EQUAL("Could not find wrapper for 'Array' return type. Did you use keepAlive() on a method that returns a builtin type?",
+                 ex.what())
+  );
+}