Add GodotFfi::from_sys_init_default(), which can call Default::default() before running the init fn

Bromeon · Bromeon · commit 585076ef922f · 2023-02-26T23:13:33.000+01:00
Impacts:
* Simplifies ffi_methods! usage for Variant, GodotString, StringName, TypedArray, PackedArray, Dictionary
* impl Default does not need to re-implement from_sys_init()
* Functions which do *not* expect a pre-initialized instance can directly call from_sys_init() in the future, avoiding 2 inits (memory leak)
diff --git a/godot-codegen/src/class_generator.rs b/godot-codegen/src/class_generator.rs
@@ -686,7 +686,7 @@ impl VariantFfi {
     fn type_ptr() -> Self {
         Self {
             sys_method: ident("sys_const"),
-            from_sys_init_method: ident("from_sys_init"),
+            from_sys_init_method: ident("from_sys_init_default"),
         }
     }
 }
@@ -876,7 +876,7 @@ fn make_return(
         }
         (None, Some(return_ty)) => {
             quote! {
-                <#return_ty as sys::GodotFfi>::from_sys_init(|return_ptr| {
+                <#return_ty as sys::GodotFfi>::from_sys_init_default(|return_ptr| {
                     #ptrcall_invocation
                 })
             }
diff --git a/godot-core/src/builtin/array.rs b/godot-core/src/builtin/array.rs
@@ -70,12 +70,11 @@ impl_builtin_froms!(Array;
 
 impl<T: VariantMetadata> TypedArray<T> {
     fn from_opaque(opaque: sys::types::OpaqueArray) -> Self {
-        let array = Self {
+        // Note: type is not yet checked at this point, because array has not yet been initialized!
+        Self {
             opaque,
             _phantom: PhantomData,
-        };
-        array.check_type();
-        array
+        }
     }
 
     /// Returns the number of elements in the array. Equivalent of `size()` in Godot.
@@ -320,8 +319,9 @@ impl<T: VariantMetadata> TypedArray<T> {
     /// # Panics
     ///
     /// If the inner type doesn't match `T` or no type is set at all.
-    fn check_type(&self) {
+    fn with_checked_type(self) -> Self {
         assert_eq!(self.type_info(), TypeInfo::new::<T>());
+        self
     }
 
     /// Sets the type of the inner array. Can only be called once, directly after creation.
@@ -567,17 +567,9 @@ impl<T: VariantMetadata + ToVariant> TypedArray<T> {
 // }
 
 impl<T: VariantMetadata> GodotFfi for TypedArray<T> {
-    ffi_methods! {
-        type sys::GDExtensionTypePtr = *mut Opaque;
-        fn from_sys;
-        fn sys;
-        fn write_sys;
-    }
-
-    unsafe fn from_sys_init(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
-        // Can't use uninitialized pointer -- Array CoW implementation in C++ expects that on
-        // assignment, the target CoW pointer is either initialized or nullptr
+    ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
 
+    unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
         let mut result = Self::default();
         init_fn(result.sys_mut());
         result
@@ -598,28 +590,25 @@ impl<T: VariantMetadata> fmt::Debug for TypedArray<T> {
 /// [`Array::duplicate_deep()`].
 impl<T: VariantMetadata> Share for TypedArray<T> {
     fn share(&self) -> Self {
-        unsafe {
-            Self::from_sys_init(|self_ptr| {
+        let array = unsafe {
+            Self::from_sys_init_default(|self_ptr| {
                 let ctor = ::godot_ffi::builtin_fn!(array_construct_copy);
                 let args = [self.sys_const()];
                 ctor(self_ptr, args.as_ptr());
             })
-        }
+        };
+        array.with_checked_type()
     }
 }
 
 impl<T: VariantMetadata> Default for TypedArray<T> {
     #[inline]
     fn default() -> Self {
-        // Note: can't use from_sys_init(), as that calls the default constructor
-        // (because most assignments expect initialized target type)
-        let mut uninit = std::mem::MaybeUninit::<TypedArray<T>>::uninit();
         let mut array = unsafe {
-            let self_ptr = (*uninit.as_mut_ptr()).sys_mut();
-            sys::builtin_call! {
-                array_construct_default(self_ptr, std::ptr::null_mut())
-            };
-            uninit.assume_init()
+            Self::from_sys_init(|self_ptr| {
+                let ctor = ::godot_ffi::builtin_fn!(array_construct_default);
+                ctor(self_ptr, std::ptr::null_mut())
+            })
         };
         array.init_inner_type();
         array
@@ -661,14 +650,14 @@ impl<T: VariantMetadata> FromVariant for TypedArray<T> {
         if variant.get_type() != Self::variant_type() {
             return Err(VariantConversionError);
         }
-        let result = unsafe {
-            Self::from_sys_init(|self_ptr| {
+        let array = unsafe {
+            Self::from_sys_init_default(|self_ptr| {
                 let array_from_variant = sys::builtin_fn!(array_from_variant);
                 array_from_variant(self_ptr, variant.var_sys());
             })
         };
 
-        Ok(result)
+        Ok(array.with_checked_type())
     }
 }
 
@@ -773,7 +762,7 @@ impl<T: VariantMetadata> PartialEq for TypedArray<T> {
             let mut result = false;
             sys::builtin_call! {
                 array_operator_equal(self.sys(), other.sys(), result.sys_mut())
-            };
+            }
             result
         }
     }
diff --git a/godot-core/src/builtin/dictionary.rs b/godot-core/src/builtin/dictionary.rs
@@ -240,17 +240,9 @@ impl Dictionary {
 // Traits
 
 impl GodotFfi for Dictionary {
-    ffi_methods! {
-        type sys::GDExtensionTypePtr = *mut Opaque;
-        fn from_sys;
-        fn sys;
-        fn write_sys;
-    }
-
-    unsafe fn from_sys_init(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
-        // Can't use uninitialized pointer -- Dictionary CoW implementation in C++ expects that on
-        // assignment, the target CoW pointer is either initialized or nullptr
+    ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
 
+    unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
         let mut result = Self::default();
         init_fn(result.sys_mut());
         result
@@ -279,7 +271,7 @@ impl fmt::Debug for Dictionary {
 impl Share for Dictionary {
     fn share(&self) -> Self {
         unsafe {
-            Self::from_sys_init(|self_ptr| {
+            Self::from_sys_init_default(|self_ptr| {
                 let ctor = sys::builtin_fn!(dictionary_construct_copy);
                 let args = [self.sys_const()];
                 ctor(self_ptr, args.as_ptr());
diff --git a/godot-core/src/builtin/macros.rs b/godot-core/src/builtin/macros.rs
@@ -33,7 +33,7 @@ macro_rules! impl_builtin_traits_inner {
             #[inline]
             fn clone(&self) -> Self {
                 unsafe {
-                    Self::from_sys_init(|self_ptr| {
+                    Self::from_sys_init_default(|self_ptr| {
                         let ctor = ::godot_ffi::builtin_fn!($gd_method);
                         let args = [self.sys_const()];
                         ctor(self_ptr, args.as_ptr());
@@ -116,7 +116,7 @@ macro_rules! impl_builtin_traits_inner {
                     return Err($crate::builtin::variant::VariantConversionError)
                 }
                 let result = unsafe {
-                    Self::from_sys_init(|self_ptr| {
+                    Self::from_sys_init_default(|self_ptr| {
                         let converter = sys::builtin_fn!($gd_method);
                         converter(self_ptr, variant.var_sys());
                     })
@@ -168,7 +168,7 @@ macro_rules! impl_builtin_froms {
         $(impl From<&$From> for $To {
             fn from(other: &$From) -> Self {
                 unsafe {
-                    Self::from_sys_init(|ptr| {
+                    Self::from_sys_init_default(|ptr| {
                         let args = [other.sys_const()];
                         ::godot_ffi::builtin_call! {
                             $from_fn(ptr, args.as_ptr())
diff --git a/godot-core/src/builtin/node_path.rs b/godot-core/src/builtin/node_path.rs
@@ -6,7 +6,7 @@
 
 use crate::builtin::GodotString;
 use godot_ffi as sys;
-use godot_ffi::{ffi_methods, GodotFfi};
+use godot_ffi::{ffi_methods, GDExtensionTypePtr, GodotFfi};
 use std::fmt::{Display, Formatter, Result as FmtResult};
 
 pub struct NodePath {
@@ -21,12 +21,18 @@ impl NodePath {
 
 impl GodotFfi for NodePath {
     ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
+
+    unsafe fn from_sys_init_default(init_fn: impl FnOnce(GDExtensionTypePtr)) -> Self {
+        let mut result = Self::default();
+        init_fn(result.sys_mut());
+        result
+    }
 }
 
 impl From<&GodotString> for NodePath {
     fn from(path: &GodotString) -> Self {
         unsafe {
-            Self::from_sys_init(|self_ptr| {
+            Self::from_sys_init_default(|self_ptr| {
                 let ctor = sys::builtin_fn!(node_path_from_string);
                 let args = [path.sys_const()];
                 ctor(self_ptr, args.as_ptr());
@@ -38,7 +44,7 @@ impl From<&GodotString> for NodePath {
 impl From<&NodePath> for GodotString {
     fn from(path: &NodePath) -> Self {
         unsafe {
-            Self::from_sys_init(|self_ptr| {
+            Self::from_sys_init_default(|self_ptr| {
                 let ctor = sys::builtin_fn!(string_from_node_path);
                 let args = [path.sys_const()];
                 ctor(self_ptr, args.as_ptr());
diff --git a/godot-core/src/builtin/others.rs b/godot-core/src/builtin/others.rs
@@ -50,7 +50,7 @@ impl Callable {
         // upcast not needed
         let method = method.into();
         unsafe {
-            Self::from_sys_init(|self_ptr| {
+            Self::from_sys_init_default(|self_ptr| {
                 let ctor = sys::builtin_fn!(callable_from_object_method);
                 let args = [object.sys_const(), method.sys_const()];
                 ctor(self_ptr, args.as_ptr());
@@ -66,6 +66,7 @@ impl Callable {
 
 impl_builtin_traits! {
     for Callable {
+        // Default => callable_construct_default;
         FromVariant => callable_from_variant;
     }
 }
diff --git a/godot-core/src/builtin/packed_array.rs b/godot-core/src/builtin/packed_array.rs
@@ -386,17 +386,9 @@ macro_rules! impl_packed_array {
         }
 
         impl GodotFfi for $PackedArray {
-            ffi_methods! {
-                type sys::GDExtensionTypePtr = *mut Opaque;
-                fn from_sys;
-                fn sys;
-                fn write_sys;
-            }
-
-            unsafe fn from_sys_init(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
-                // Can't use uninitialized pointer -- Vector CoW implementation in C++ expects that
-                // on assignment, the target CoW pointer is either initialized or nullptr
+            ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
 
+            unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
                 let mut result = Self::default();
                 init_fn(result.sys_mut());
                 result
diff --git a/godot-core/src/builtin/string.rs b/godot-core/src/builtin/string.rs
@@ -37,43 +37,18 @@ impl GodotString {
 }
 
 impl GodotFfi for GodotString {
-    ffi_methods! {
-        type sys::GDExtensionTypePtr = *mut Opaque;
-        fn from_sys;
-        fn sys;
-        fn write_sys;
-    }
-
-    unsafe fn from_sys_init(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
-        // Can't use uninitialized pointer -- String CoW implementation in C++ expects that on assignment,
-        // the target CoW pointer is either initialized or nullptr
+    ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
 
+    unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
         let mut result = Self::default();
         init_fn(result.sys_mut());
         result
     }
 }
 
-impl Default for GodotString {
-    fn default() -> Self {
-        // Note: can't use from_sys_init(), as that calls the default constructor
-        // (because most assignments expect initialized target type)
-
-        let mut uninit = std::mem::MaybeUninit::<GodotString>::uninit();
-
-        unsafe {
-            let self_ptr = (*uninit.as_mut_ptr()).sys_mut();
-            sys::builtin_call! {
-                string_construct_default(self_ptr, std::ptr::null_mut())
-            };
-
-            uninit.assume_init()
-        }
-    }
-}
-
 impl_builtin_traits! {
     for GodotString {
+        Default => string_construct_default;
         Clone => string_construct_copy;
         Drop => string_destroy;
         Eq => string_operator_equal;
diff --git a/godot-core/src/builtin/string_name.rs b/godot-core/src/builtin/string_name.rs
@@ -33,17 +33,9 @@ impl StringName {
 }
 
 impl GodotFfi for StringName {
-    ffi_methods! {
-        type sys::GDExtensionTypePtr = *mut Opaque;
-        fn from_sys;
-        fn sys;
-        fn write_sys;
-    }
-
-    unsafe fn from_sys_init(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
-        // Can't use uninitialized pointer -- StringName implementation in C++ expects that on assignment,
-        // the target type is a valid string (possibly empty)
+    ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
 
+    unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
         let mut result = Self::default();
         init_fn(result.sys_mut());
         result
@@ -106,7 +98,7 @@ impl Hash for StringName {
 impl From<&GodotString> for StringName {
     fn from(s: &GodotString) -> Self {
         unsafe {
-            Self::from_sys_init(|self_ptr| {
+            Self::from_sys_init_default(|self_ptr| {
                 let ctor = sys::builtin_fn!(string_name_from_string);
                 let args = [s.sys_const()];
                 ctor(self_ptr, args.as_ptr());
@@ -128,7 +120,7 @@ where
 impl From<&StringName> for GodotString {
     fn from(s: &StringName) -> Self {
         unsafe {
-            Self::from_sys_init(|self_ptr| {
+            Self::from_sys_init_default(|self_ptr| {
                 let ctor = sys::builtin_fn!(string_from_string_name);
                 let args = [s.sys_const()];
                 ctor(self_ptr, args.as_ptr());
diff --git a/godot-core/src/builtin/variant/impls.rs b/godot-core/src/builtin/variant/impls.rs
@@ -55,19 +55,21 @@ macro_rules! impl_variant_traits {
 
         impl FromVariant for $T {
             fn try_from_variant(variant: &Variant) -> Result<Self, VariantConversionError> {
+                // Type check -- at the moment, a strict match is required.
+                if variant.get_type() != Self::variant_type() {
+                    return Err(VariantConversionError)
+                }
+
                 // In contrast to T -> Variant, the conversion Variant -> T assumes
                 // that the destination is initialized (at least for some T). For example:
                 // void String::operator=(const String &p_str) { _cowdata._ref(p_str._cowdata); }
                 // does a copy-on-write and explodes if this->_cowdata is not initialized.
                 // We can thus NOT use Self::from_sys_init().
-                if variant.get_type() != Self::variant_type() {
-                    return Err(VariantConversionError)
-                }
-                let mut value = <$T>::default();
                 let result = unsafe {
-                    let converter = sys::builtin_fn!($to_fn);
-                    converter(value.sys_mut(), variant.var_sys());
-                    value
+                    Self::from_sys_init_default(|self_ptr| {
+                        let converter = sys::builtin_fn!($to_fn);
+                        converter(self_ptr, variant.var_sys());
+                    })
                 };
 
                 Ok(result)
diff --git a/godot-core/src/builtin/variant/mod.rs b/godot-core/src/builtin/variant/mod.rs
diff --git a/godot-core/src/log.rs b/godot-core/src/log.rs
diff --git a/godot-ffi/src/godot_ffi.rs b/godot-ffi/src/godot_ffi.rs
diff --git a/itest/rust/src/string_test.rs b/itest/rust/src/string_test.rs

Original file line number	Diff line number	Diff line change
`@@ -686,7 +686,7 @@ impl VariantFfi {`
`686`	`686`	`fn type_ptr() -> Self {`
`687`	`687`	`Self {`
`688`	`688`	`sys_method: ident("sys_const"),`
`689`		`- from_sys_init_method: ident("from_sys_init"),`
	`689`	`+ from_sys_init_method: ident("from_sys_init_default"),`
`690`	`690`	`}`
`691`	`691`	`}`
`692`	`692`	`}`
`@@ -876,7 +876,7 @@ fn make_return(`
`876`	`876`	`}`
`877`	`877`	`(None, Some(return_ty)) => {`
`878`	`878`	`quote! {`
`879`		`- <#return_ty as sys::GodotFfi>::from_sys_init(\|return_ptr\| {`
	`879`	`+ <#return_ty as sys::GodotFfi>::from_sys_init_default(\|return_ptr\| {`
`880`	`880`	`#ptrcall_invocation`
`881`	`881`	`})`
`882`	`882`	`}`