fixes #69 - deepcopy struct default values on init. Also includes these other fixes that popped up along the way:

* Fix move semantics on python DialectGeneric impl
* Fix deepcopy call when passing a tuple argument ( was only extracting first element )
* Fix extremely subtle deepcopy bug where partially created PyStruct was getting GC cleaned

Signed-off-by: Ambalu, Robert <robert.ambalu@point72.com>

Author: robambalu

cpp/csp/engine/Struct.cpp

@@ -5,7 +5,7 @@
 namespace csp
 {
 
-StructField::StructField( CspTypePtr type, const std::string & fieldname,
+StructField::StructField( CspTypePtr type, const std::string & fieldname, 
                           size_t size, size_t alignment ) :
     m_fieldname( fieldname ),
     m_offset( 0 ),
@@ -18,15 +18,15 @@ StructField::StructField( CspTypePtr type, const std::string & fieldname,
 {
 }
 
-/*  StructMeta
+/*  StructMeta  
 
 A note on member layout.  Meta will order objects in the following order:
 - non-native fields ( ie PyObjectPtr for the python dialect )
 - native fields sorted in order
 - set/unset bitmask bytes ( 1 byte per 8 fields )
 
 Derived structs will simply append to the layout of the base struct, properly padding between classes to align
-its fields properly.
+its fields properly.  
 This layout is imposed on Struct instances.  Since Struct needs refcount and meta * fields, for convenience they are stored
 *before* a Struct's "this" pointer as hidden data.  This way struct ptrs can be passed into StructMeta without
 and adjustments required for the hidden fields
@@ -47,7 +47,7 @@ StructMeta::StructMeta( const std::string & name, const Fields & fields,
     //decided to place them at the start cause they are most likely size of ptr or greater
 
     m_fieldnames.reserve( m_fields.size() );
-    for( size_t i = 0; i < m_fields.size(); i++ )
+    for( size_t i = 0; i < m_fields.size(); i++ ) 
         m_fieldnames.emplace_back( m_fields[i] -> fieldname() );
 
     std::sort( m_fields.begin(), m_fields.end(), []( auto && a, auto && b )
@@ -93,7 +93,7 @@ StructMeta::StructMeta( const std::string & name, const Fields & fields,
     m_isFullyNative = m_isPartialNative && ( m_base ? m_base -> isNative() : true );
 
     //Setup masking bits for our fields
-    //NOTE we can be more efficient by sticking masks into any potential alignment gaps, dont want to spend time on it
+    //NOTE we can be more efficient by sticking masks into any potential alignment gaps, dont want to spend time on it 
     //at this point
     m_maskSize     = !m_fields.empty() ? 1 + ( ( m_fields.size() - 1 ) / 8 ) : 0;
     m_size         = offset + m_maskSize;
@@ -116,7 +116,7 @@ StructMeta::StructMeta( const std::string & name, const Fields & fields,
     {
         m_fields.insert( m_fields.begin(), m_base -> m_fields.begin(), m_base -> m_fields.end() );
         m_fieldnames.insert( m_fieldnames.begin(), m_base -> m_fieldnames.begin(), m_base -> m_fieldnames.end() );
-
+        
         m_firstPartialField        = m_base -> m_fields.size();
         m_firstNativePartialField += m_base -> m_fields.size();
         m_fieldMap = m_base -> m_fieldMap;
@@ -145,7 +145,7 @@ Struct * StructMeta::createRaw() const
     initialize( s );
 
     if( m_default )
-        s -> copyFrom( m_default.get() ); //TODO change to deepcopy after this fix is released
+        s -> deepcopyFrom( m_default.get() );
 
     return s;
 }
@@ -212,7 +212,7 @@ void StructMeta::initialize( Struct * s ) const
     }
 
     memset( reinterpret_cast<std::byte*>(s) + m_nativeStart, 0, partialNativeSize() );
-
+    
     if( !m_isPartialNative )
     {
         for( size_t idx = m_firstPartialField; idx < m_firstNativePartialField; ++idx )
@@ -221,7 +221,7 @@ void StructMeta::initialize( Struct * s ) const
             static_cast<NonNativeStructField*>( field ) -> initialize( s );
         }
     }
-
+    
     if( m_base )
         m_base -> initialize( s );
 }
@@ -231,9 +231,9 @@ void StructMeta::copyFrom( const Struct * src, Struct * dest )
     if( unlikely( src == dest ) )
         return;
 
-    if( dest -> meta() != src -> meta() &&
+    if( dest -> meta() != src -> meta() && 
         !StructMeta::isDerivedType( src -> meta(), dest -> meta() ) )
-            CSP_THROW( TypeError, "Attempting to copy from struct type '" << src -> meta() -> name() << "' to struct type '" << dest -> meta() -> name()
+            CSP_THROW( TypeError, "Attempting to copy from struct type '" << src -> meta() -> name() << "' to struct type '" << dest -> meta() -> name() 
                        << "'. copy_from may only be used to copy from same type or derived types" );
 
     dest -> meta() -> copyFromImpl( src, dest, false );
@@ -280,7 +280,7 @@ void StructMeta::copyFromImpl( const Struct * src, Struct * dest, bool deepcopy
         //note that partialNative will include the mask bytes - this sets the native part and the mask
         memcpy( reinterpret_cast<std::byte*>(dest) + m_nativeStart, reinterpret_cast<const std::byte*>(src) + m_nativeStart,
                 partialNativeSize() );
-
+     
         if( m_base )
             m_base -> copyFromImpl( src, dest, deepcopy );
     }
@@ -291,13 +291,13 @@ void StructMeta::updateFrom( const Struct * src, Struct * dest )
     if( unlikely( src == dest ) )
         return;
 
-    if( dest -> meta() != src -> meta() &&
+    if( dest -> meta() != src -> meta() && 
         !StructMeta::isDerivedType( src -> meta(), dest -> meta() ) )
-            CSP_THROW( TypeError, "Attempting to update from struct type '" << src -> meta() -> name() << "' to struct type '" << dest -> meta() -> name()
+            CSP_THROW( TypeError, "Attempting to update from struct type '" << src -> meta() -> name() << "' to struct type '" << dest -> meta() -> name() 
                        << "'. update_from may only be used to update from same type or derived types" );
 
     dest -> meta() -> updateFromImpl( src, dest );
-}
+}    
 
 void StructMeta::updateFromImpl( const Struct * src, Struct * dest ) const
 {
@@ -328,7 +328,7 @@ bool StructMeta::isEqual( const Struct * x, const Struct * y ) const
 
     //Note the curent use of memcpy for native types.  This can cause issues on double comparisons
     //esp if expecting NaN == NaN to be false, and when comparing -0.0 to +0.0.. may want to revisit
-    //We we do we may as well remove the basepadding copy
+    //We we do we may as well remove the basepadding copy 
     if( isNative() )
         return memcmp( x, y, size() ) == 0;
 
@@ -341,7 +341,7 @@ bool StructMeta::isEqual( const Struct * x, const Struct * y ) const
         for( size_t idx = m_firstPartialField; idx < m_firstNativePartialField; ++idx )
         {
             auto * field = m_fields[ idx ].get();
-
+                
             if( field -> isSet( x ) != field -> isSet( y ) )
                 return false;
 
@@ -386,19 +386,19 @@ size_t StructMeta::hash( const Struct * x ) const
             "Exceeded max recursion depth of " << MAX_RECURSION_DEPTH << " in " << name() << "::hash(), cannot hash cyclic data structure" );
 
     hash ^= csp::hash::hash_bytes( x + m_nativeStart, partialNativeSize() );
-
+    
     if( !m_isPartialNative )
     {
         for( size_t idx = m_firstPartialField; idx < m_firstNativePartialField; ++idx )
         {
             auto * field = m_fields[ idx ].get();
-
+            
             //we dont incorporate unset fields, bitmask will cover them
             if( field -> isSet( x ) )
                 hash ^= static_cast<NonNativeStructField*>( field ) -> hash( x );
         }
     }
-
+    
     if( m_base )
         hash ^= std::hash<uint64_t>()( (uint64_t ) m_base.get() ) ^ m_base -> hash( x );
 
@@ -414,18 +414,18 @@ void StructMeta::clear( Struct * s ) const
     }
 
     memset( reinterpret_cast<std::byte*>(s) + m_nativeStart, 0, partialNativeSize() );
-
+    
     if( !m_isPartialNative )
     {
         for( size_t idx = m_firstPartialField; idx < m_firstNativePartialField; ++idx )
         {
             auto * field = m_fields[ idx ].get();
-
+            
             if( field -> isSet( s ) )
                 static_cast<NonNativeStructField*>( field ) -> clearValue( s );
         }
     }
-
+    
     if( m_base )
         m_base -> clear( s );
 }
@@ -466,7 +466,7 @@ void StructMeta::destroy( Struct * s ) const
             static_cast<NonNativeStructField*>( field ) -> destroy( s );
         }
     }
-
+    
     if( m_base )
         m_base -> destroy( s );
 }

cpp/csp/python/PyCspType.cpp

@@ -1,4 +1,5 @@
 #include <csp/python/PyCspType.h>
+#include <csp/python/PyStruct.h>
 #include <csp/python/Conversions.h>
 #include <Python.h>
 
@@ -25,14 +26,14 @@ DialectGenericType::DialectGenericType( const DialectGenericType &rhs )
 
 DialectGenericType::DialectGenericType( DialectGenericType &&rhs )
 {
-    new( this ) csp::python::PyObjectPtr( reinterpret_cast<const csp::python::PyObjectPtr &&>(rhs) );
+    new( this ) csp::python::PyObjectPtr( reinterpret_cast<csp::python::PyObjectPtr &&>(rhs) );
 }
 
 DialectGenericType DialectGenericType::deepcopy() const
 {
-    static PyObject *pyDeepcopy = PyObject_GetAttrString( PyImport_ImportModule( "copy" ), "deepcopy" );
-    PyObject * pyVal = PyObject_CallFunction( pyDeepcopy, "O", python::toPythonBorrowed( *this ) );
-    return DialectGenericType( reinterpret_cast<const DialectGenericType &&>( std::move( csp::python::PyObjectPtr::own( pyVal ) ) ) );
+    static PyObject * pyDeepcopy = PyObject_GetAttrString( PyImport_ImportModule( "copy" ), "deepcopy" );
+    PyObject * pyVal = PyObject_CallFunction( pyDeepcopy, "(O)", python::toPythonBorrowed( *this ) );
+    return DialectGenericType( reinterpret_cast<DialectGenericType &&>( std::move( csp::python::PyObjectPtr::check( pyVal ) ) ) );
 }
 
 DialectGenericType &DialectGenericType::operator=( const DialectGenericType &rhs )
@@ -43,7 +44,7 @@ DialectGenericType &DialectGenericType::operator=( const DialectGenericType &rhs
 
 DialectGenericType &DialectGenericType::operator=( DialectGenericType &&rhs )
 {
-    *reinterpret_cast<csp::python::PyObjectPtr *>(this) = reinterpret_cast<const csp::python::PyObjectPtr &&>(rhs);
+    *reinterpret_cast<csp::python::PyObjectPtr *>(this) = std::move( reinterpret_cast<csp::python::PyObjectPtr &&>(rhs) );
     return *this;
 }
 

cpp/csp/python/PyStruct.cpp

@@ -169,9 +169,9 @@ static PyObject * PyStructMeta_new( PyTypeObject *subtype, PyObject *args, PyObj
 
     /*back reference to the struct type that will be accessible on the csp struct -> meta()
       DialectStructMeta will hold a borrowed reference to the type to avoid a circular dep
-
+      
       This is the layout of references between all these types
-                              StructMeta (shared_ptr) <-------- strong ref
+                              StructMeta (shared_ptr) <-------- strong ref 
                                   |                              |
                            DialectStructMeta ---> weak ref to PyStructMeta ( the PyType )
                                  /\                              /\
@@ -671,10 +671,10 @@ PyObject * PyStruct_new( PyTypeObject * type, PyObject *args, PyObject *kwds )
     if( ! ( (PyStructMeta * ) type ) -> structMeta )
         CSP_THROW( TypeError, "csp.Struct cannot be instantiated" );
 
-    PyStruct * pystruct = ( PyStruct * ) type -> tp_alloc( type, 0 );
-
     StructPtr struct_ =  ( (PyStructMeta * ) type ) -> structMeta -> create();
 
+    PyStruct * pystruct = ( PyStruct * ) type -> tp_alloc( type, 0 );
+
     //assign dialectptr, but we DO NOT incref the instance on the struct
     struct_ -> setDialectPtr( pystruct );
 
@@ -845,16 +845,24 @@ int PyStruct_setattro( PyStruct * self, PyObject * attr, PyObject * value )
 
 PyObject * PyStruct_copy( PyStruct * self )
 {
+    CSP_BEGIN_METHOD;
     PyObject * copy = self -> ob_type -> tp_alloc( self -> ob_type, 0 );
     new ( copy ) PyStruct( self -> struct_ -> copy() );
     return copy;
+    CSP_RETURN_NULL;
 }
 
 PyObject * PyStruct_deepcopy( PyStruct * self )
 {
-    PyObject * deepcopy = self -> ob_type -> tp_alloc( self -> ob_type, 0 );
-    new ( deepcopy ) PyStruct( self -> struct_ -> deepcopy() );
-    return deepcopy;
+    CSP_BEGIN_METHOD;
+    //Note that once tp_alloc is called, the object will get added to GC
+    //deepcopy traversal may kick in a GC collect, so we have to call that first before the PyStruct is created
+    //of it may traverse a partially consturcted object and crash 
+    auto deepcopy = self -> struct_ -> deepcopy();
+    PyObject * pyDeepcopy = self -> ob_type -> tp_alloc( self -> ob_type, 0 );
+    new ( pyDeepcopy ) PyStruct( deepcopy );
+    return pyDeepcopy;
+    CSP_RETURN_NULL;
 }
 
 PyObject * PyStruct_clear( PyStruct * self )

csp/impl/struct.py

@@ -195,6 +195,9 @@ def __getstate__(self):
     def __setstate__(self, state):
         self.update(**state)
 
+    def __deepcopy__(self, memodict={}):
+        return self.deepcopy()
+
     def __dir__(self):
         return self.__full_metadata_typed__.keys()
 

csp/impl/wiring/numba_node.py

@@ -67,17 +67,17 @@ def _create(self, engine, memo):
         exposed_utility_values = {}
         cache_key = [self._impl]
         for (ts_idx, basket_idx), input in self.ts_inputs():
-            exposed_utility_values[NumbaNodeParser.get_ts_input_value_getter_name(ts_idx)] = (
-                NumbaTSTypedFunctionResolver.get_value_getter_function(input.tstype.typ)
-            )
+            exposed_utility_values[
+                NumbaNodeParser.get_ts_input_value_getter_name(ts_idx)
+            ] = NumbaTSTypedFunctionResolver.get_value_getter_function(input.tstype.typ)
             cache_key.append(input.tstype)
 
         for output in self._outputs:
             assert output.kind.is_single_ts()
             ts_idx = output.ts_idx
-            exposed_utility_values[NumbaNodeParser.get_ts_out_value_return_name(ts_idx)] = (
-                NumbaTSTypedFunctionResolver.get_value_returner_function(output.typ.typ)
-            )
+            exposed_utility_values[
+                NumbaNodeParser.get_ts_out_value_return_name(ts_idx)
+            ] = NumbaTSTypedFunctionResolver.get_value_returner_function(output.typ.typ)
 
         numba_scalar_types = []
         for i in self._signature.raw_inputs():