Add OnceString for the "leak_string_sys" pattern

Bromeon · Bromeon · commit 8c91d8958e2d · 2022-11-20T16:26:24.000+01:00
diff --git a/godot-codegen/src/class_generator.rs b/godot-codegen/src/class_generator.rs
@@ -69,8 +69,8 @@ fn make_constructor(class: &Class, ctx: &Context, class_name_str: &Literal) -> T
         quote! {
             pub fn singleton() -> Gd<Self> {
                 unsafe {
-                    let class_name = StringName::from(#class_name_str);
-                    let object_ptr = sys::interface_fn!(global_get_singleton)(class_name.leak_string_sys());
+                    let class_name = OnceString::new(#class_name_str);
+                    let object_ptr = sys::interface_fn!(global_get_singleton)(class_name.leak_sys());
                     Gd::from_obj_sys(object_ptr)
                 }
             }
@@ -83,8 +83,8 @@ fn make_constructor(class: &Class, ctx: &Context, class_name_str: &Literal) -> T
         quote! {
             pub fn new() -> Gd<Self> {
                 unsafe {
-                    let class_name = StringName::from(#class_name_str);
-                    let object_ptr = sys::interface_fn!(classdb_construct_object)(class_name.leak_string_sys());
+                    let class_name = OnceString::new(#class_name_str);
+                    let object_ptr = sys::interface_fn!(classdb_construct_object)(class_name.leak_sys());
                     //let instance = Self { object_ptr };
                     Gd::from_obj_sys(object_ptr)
                 }
@@ -96,8 +96,8 @@ fn make_constructor(class: &Class, ctx: &Context, class_name_str: &Literal) -> T
             #[must_use]
             pub fn new_alloc() -> Gd<Self> {
                 unsafe {
-                    let class_name = StringName::from(#class_name_str);
-                    let object_ptr = sys::interface_fn!(classdb_construct_object)(class_name.leak_string_sys());
+                    let class_name = OnceString::new(#class_name_str);
+                    let object_ptr = sys::interface_fn!(classdb_construct_object)(class_name.leak_sys());
                     Gd::from_obj_sys(object_ptr)
                 }
             }
@@ -392,11 +392,11 @@ fn make_method_definition(method: &Method, class_name: &str, ctx: &mut Context)
         quote! {
             #vis fn #method_name( #receiver #(, #params )*, varargs: &[Variant]) #return_decl {
                 unsafe {
-                    let class_name = StringName::from(#class_name);
-                    let method_name = StringName::from(#method_name_str);
+                    let class_name = OnceString::new(#class_name);
+                    let method_name = OnceString::new(#method_name_str);
                     let method_bind = sys::interface_fn!(classdb_get_method_bind)(
-                        class_name.leak_string_sys(),
-                        method_name.leak_string_sys(),
+                        class_name.leak_sys(),
+                        method_name.leak_sys(),
                         #hash
                     );
                     let call_fn = sys::interface_fn!(object_method_bind_call);
@@ -419,11 +419,11 @@ fn make_method_definition(method: &Method, class_name: &str, ctx: &mut Context)
         quote! {
             #vis fn #method_name( #receiver, #( #params ),* ) #return_decl {
                 unsafe {
-                    let class_name = StringName::from(#class_name);
-                    let method_name = StringName::from(#method_name_str);
+                    let class_name = OnceString::new(#class_name);
+                    let method_name = OnceString::new(#method_name_str);
                     let method_bind = sys::interface_fn!(classdb_get_method_bind)(
-                        class_name.leak_string_sys(),
-                        method_name.leak_string_sys(),
+                        class_name.leak_sys(),
+                        method_name.leak_sys(),
                         #hash
                     );
                     let call_fn = sys::interface_fn!(object_method_bind_ptrcall);
@@ -461,8 +461,8 @@ pub(crate) fn make_function_definition(
     quote! {
         pub fn #function_name( #( #params ),* ) #return_decl {
             let result = unsafe {
-                let function_name = StringName::from(#function_name_str);
-                let call_fn = sys::interface_fn!(variant_get_ptr_utility_function)(function_name.leak_string_sys(), #hash);
+                let function_name = OnceString::new(#function_name_str);
+                let call_fn = sys::interface_fn!(variant_get_ptr_utility_function)(function_name.leak_sys(), #hash);
                 let call_fn = call_fn.unwrap_unchecked();
 
                 let args = [
diff --git a/godot-core/src/builtin/meta/class_name.rs b/godot-core/src/builtin/meta/class_name.rs
@@ -35,9 +35,8 @@ impl ClassName {
         self.backing.string_sys()
     }
 
-    #[must_use]
-    pub fn leak_string_name(self) -> sys::GDNativeStringNamePtr {
-        self.backing.leak_string_sys()
+    pub(crate) fn into_once(self) -> OnceString {
+        OnceString::from_owned(self.backing)
     }
 }
 
diff --git a/godot-core/src/builtin/meta/mod.rs b/godot-core/src/builtin/meta/mod.rs
@@ -5,9 +5,11 @@
  */
 
 mod class_name;
+mod once_string;
 mod signature;
 
 pub use class_name::*;
+pub use once_string::*;
 pub use signature::*;
 
 use crate::builtin::*;
@@ -66,7 +68,7 @@ impl PropertyInfo {
     }
 
     /// Converts to the FFI type. Keep this object allocated while using that!
-    pub fn sys(&self) -> sys::GDNativePropertyInfo {
+    pub fn property_sys(&self) -> sys::GDNativePropertyInfo {
         use crate::obj::EngineEnum as _;
 
         sys::GDNativePropertyInfo {
diff --git a/godot-core/src/builtin/meta/once_string.rs b/godot-core/src/builtin/meta/once_string.rs
@@ -0,0 +1,95 @@
+use crate::builtin::StringName;
+use std::cell::Cell;
+use std::mem::ManuallyDrop;
+
+use crate::builtin::meta::PropertyInfo;
+use godot_ffi as sys;
+
+/// Utility to safely pass strings to FFI without creating dangling pointers or memory leaks.
+///
+/// Models a recurring pattern when passing strings to FFI functions:
+/// ```no_run
+/// # fn some_ffi_function(_ptr: godot_ffi::GDNativeStringNamePtr) {}
+/// # use godot_core::builtin::StringName;
+/// let s: StringName = todo!();
+///
+/// // Pass pointer to Godot FFI -- the underlying object must remain valid, because the sys pointer
+/// // might point to this object. So we can't use a .leak_sys() function which consumes self.
+/// some_ffi_function(s.string_sys());
+///
+/// // We don't own the string anymore, so we must not destroy it
+/// std::mem::forget(s);
+///
+/// // However, if in fact string_sys() was never invoked (e.g. behind an if),
+/// // we now have a memory leak...
+/// ```
+#[doc(hidden)]
+pub struct OnceArg<T: OnceSys> {
+    obj: ManuallyDrop<T>,
+    has_ownership: Cell<bool>,
+}
+
+#[doc(hidden)]
+pub type OnceString = OnceArg<StringName>;
+
+impl OnceString {
+    pub fn new(rust_string: &str) -> Self {
+        Self::from_owned(StringName::from(rust_string))
+    }
+}
+
+impl<T: OnceSys> OnceArg<T> {
+    pub fn from_owned(value: T) -> Self {
+        Self {
+            obj: ManuallyDrop::new(value),
+            has_ownership: Cell::new(true),
+        }
+    }
+
+    /// Access sys pointer while retaining underlying referred-to object.
+    ///
+    /// Can only be called once. Leaks memory if unused.
+    #[must_use]
+    pub fn leak_sys(&self) -> T::SysPointer {
+        // Interior mutability because this semantically acts like a consuming `self` function, which doesn't require `mut` either.
+
+        let had_ownership = self.has_ownership.replace(false);
+        assert!(had_ownership, "cannot call leak_sys() more than once");
+
+        self.obj.once_sys()
+    }
+}
+
+impl<T: OnceSys> Drop for OnceArg<T> {
+    fn drop(&mut self) {
+        // If no one called leak, destroy normally to avoid memory leaks
+        if self.has_ownership.get() {
+            unsafe { ManuallyDrop::drop(&mut self.obj) };
+        }
+    }
+}
+
+// ----------------------------------------------------------------------------------------------------------------------------------------------
+
+#[doc(hidden)]
+pub trait OnceSys {
+    type SysPointer;
+
+    fn once_sys(&self) -> Self::SysPointer;
+}
+
+impl OnceSys for StringName {
+    type SysPointer = sys::GDNativeStringNamePtr;
+
+    fn once_sys(&self) -> Self::SysPointer {
+        self.string_sys()
+    }
+}
+
+impl OnceSys for PropertyInfo {
+    type SysPointer = sys::GDNativePropertyInfo;
+
+    fn once_sys(&self) -> Self::SysPointer {
+        self.property_sys()
+    }
+}
diff --git a/godot-core/src/builtin/mod.rs b/godot-core/src/builtin/mod.rs
@@ -29,3 +29,5 @@ pub use variant::*;
 pub use vector2::*;
 pub use vector3::*;
 pub use vector4::*;
+
+pub use meta::OnceString;
diff --git a/godot-core/src/builtin/string.rs b/godot-core/src/builtin/string.rs
@@ -32,13 +32,6 @@ impl GodotString {
         fn string_sys = sys;
         fn write_string_sys = write_sys;
     }
-
-    // #[doc(hidden)]
-    // pub fn leak_string_sys(self) -> sys::GDNativeStringPtr {
-    //     let ptr = self.string_sys();
-    //     std::mem::forget(self);
-    //     ptr
-    // }
 }
 
 impl GodotFfi for GodotString {
diff --git a/godot-core/src/builtin/string_name.rs b/godot-core/src/builtin/string_name.rs
@@ -30,24 +30,6 @@ impl StringName {
         fn string_sys = sys;
         fn write_string_sys = write_sys;
     }
-
-    /// Do not call on temporary objects!
-    // Important: do not abstract over this with a function taking &str and returning GDNativeStringNamePtr.
-    // This does not work, because it needs an intermediate StringName object which must stay valid, as long
-    // as the pointer is in use. The right way to do it is to keep a local StringName (not temporary) around.
-    #[doc(hidden)]
-    #[must_use]
-    pub fn leak_string_sys(&self) -> sys::GDNativeStringNamePtr {
-        let ptr = self.string_sys();
-        //std::mem::forget(self);
-
-        let boks = Box::new(self.clone());
-        let ptr = boks.string_sys();
-        println!("Cloned: '{}' -> '{}'", self, &*boks);
-        Box::leak(boks);
-
-        ptr
-    }
 }
 
 impl GodotFfi for StringName {
diff --git a/godot-core/src/log.rs b/godot-core/src/log.rs
@@ -69,14 +69,14 @@ macro_rules! godot_print {
 
 pub use crate::{godot_error, godot_print, godot_script_error, godot_warn};
 
-use crate::builtin::{StringName, Variant};
+use crate::builtin::{OnceString, Variant};
 use crate::sys::{self, GodotFfi};
 
 pub fn print(varargs: &[Variant]) {
     unsafe {
-        let method_name = StringName::from("print");
+        let method_name = OnceString::new("print");
         let call_fn = sys::interface_fn!(variant_get_ptr_utility_function)(
-            method_name.leak_string_sys(),
+            method_name.leak_sys(),
             2648703342i64,
         );
         let call_fn = call_fn.unwrap_unchecked();
diff --git a/godot-core/src/macros.rs b/godot-core/src/macros.rs
@@ -127,30 +127,36 @@ macro_rules! gdext_register_method_inner {
             // Return value meta-information
             let has_return_value: bool = $crate::gdext_is_not_unit!($($RetTy)+);
             let return_value_info = Sig::property_info(-1, "");
-            let mut return_value_info_sys = return_value_info.sys();
+            let mut return_value_info_sys = return_value_info.property_sys();
             let return_value_metadata = Sig::param_metadata(-1);
 
             // Arguments meta-information
             let argument_count = NUM_ARGS as u32;
             let mut arguments_info: [PropertyInfo; NUM_ARGS] = {
                 let mut i = -1i32;
                 [$(
-                    { i += 1; Sig::property_info(i, stringify!($param)) },
+                    {
+                        i += 1;
+                        let prop = Sig::property_info(i, stringify!($param));
+                        //OnceArg::new(prop)
+                        prop
+                    },
                 )*]
             };
             let mut arguments_info_sys: [sys::GDNativePropertyInfo; NUM_ARGS]
-                = std::array::from_fn(|i| arguments_info[i].sys());
+                = std::array::from_fn(|i| arguments_info[i].property_sys());
+//                = std::array::from_fn(|i| arguments_info[i].once_sys());
             let mut arguments_metadata: [sys::GDNativeExtensionClassMethodArgumentMetadata; NUM_ARGS]
                 = std::array::from_fn(|i| Sig::param_metadata(i as i32));
 
-            let class_name = StringName::from(stringify!($Class));
-            let method_name = StringName::from(stringify!($method_name));
+            let class_name = OnceString::new(stringify!($Class));
+            let method_name = OnceString::new(stringify!($method_name));
 
-            println!("REG {class_name}::{method_name}");
-            println!("  ret {return_value_info:?}");
+            // println!("REG {class_name}::{method_name}");
+            // println!("  ret {return_value_info:?}");
 
             let method_info = sys::GDNativeExtensionClassMethodInfo {
-                name: method_name.leak_string_sys(),
+                name: method_name.leak_sys(),
                 method_userdata: std::ptr::null_mut(),
                 call_func: Some(varcall_func),
                 ptrcall_func: Some(ptrcall_func),
@@ -168,7 +174,7 @@ macro_rules! gdext_register_method_inner {
             $crate::out!("   Register fn:   {}::{}", stringify!($Class), stringify!($method_name));
             sys::interface_fn!(classdb_register_extension_class_method)(
                 sys::get_library(),
-                class_name.leak_string_sys(),
+                class_name.leak_sys(),
                 std::ptr::addr_of!(method_info),
             );
 
diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs
@@ -331,8 +331,8 @@ impl<T: GodotClass> Gd<T> {
     where
         U: GodotClass,
     {
-        let class_name = ClassName::new::<U>();
-        let class_tag = interface_fn!(classdb_get_class_tag)(class_name.leak_string_name());
+        let class_name = ClassName::new::<U>().into_once();
+        let class_tag = interface_fn!(classdb_get_class_tag)(class_name.leak_sys());
         let cast_object_ptr = interface_fn!(object_cast_to)(self.obj_sys(), class_tag);
 
         if cast_object_ptr.is_null() {
diff --git a/godot-core/src/registry.rs b/godot-core/src/registry.rs
@@ -221,16 +221,17 @@ fn fill_into<T>(dst: &mut Option<T>, src: Option<T>) {
 fn register_class_raw(info: ClassRegistrationInfo) {
     // First register class...
 
-    let class_name = info.class_name;
+    let class_name = info.class_name.into_once();
     let parent_class_name = info
         .parent_class_name
-        .expect("class defined (parent_class_name)");
+        .expect("class defined (parent_class_name)")
+        .into_once();
 
     unsafe {
         interface_fn!(classdb_register_extension_class)(
             sys::get_library(),
-            class_name.leak_string_name(),
-            parent_class_name.leak_string_name(),
+            class_name.leak_sys(),
+            parent_class_name.leak_sys(),
             ptr::addr_of!(info.godot_params),
         );
     }
@@ -271,13 +272,13 @@ pub mod callbacks {
         T: GodotClass,
         F: FnOnce(Base<T::Base>) -> T,
     {
-        let class_name = ClassName::new::<T>();
-        let base_class_name = ClassName::new::<T::Base>();
+        let class_name = ClassName::new::<T>().into_once();
+        let base_class_name = ClassName::new::<T::Base>().into_once();
 
         //out!("create callback: {}", class_name.backing);
 
         let base_ptr =
-            unsafe { interface_fn!(classdb_construct_object)(base_class_name.leak_string_name()) };
+            unsafe { interface_fn!(classdb_construct_object)(base_class_name.leak_sys()) };
         let base = unsafe { Base::from_sys(base_ptr) };
 
         let user_instance = make_user_instance(base);
@@ -287,11 +288,7 @@ pub mod callbacks {
 
         let binding_data_callbacks = crate::storage::nop_instance_callbacks();
         unsafe {
-            interface_fn!(object_set_instance)(
-                base_ptr,
-                class_name.leak_string_name(),
-                instance_ptr,
-            );
+            interface_fn!(object_set_instance)(base_ptr, class_name.leak_sys(), instance_ptr);
             interface_fn!(object_set_instance_binding)(
                 base_ptr,
                 sys::get_library(),
diff --git a/godot-macros/src/godot_api.rs b/godot-macros/src/godot_api.rs

Original file line number	Diff line number	Diff line change
`@@ -35,9 +35,8 @@ impl ClassName {`
`35`	`35`	`self.backing.string_sys()`
`36`	`36`	`}`
`37`	`37`
`38`		`- #[must_use]`
`39`		`- pub fn leak_string_name(self) -> sys::GDNativeStringNamePtr {`
`40`		`- self.backing.leak_string_sys()`
	`38`	`+ pub(crate) fn into_once(self) -> OnceString {`
	`39`	`+ OnceString::from_owned(self.backing)`
`41`	`40`	`}`
`42`	`41`	`}`
`43`	`42`
Original file line number	Diff line number	Diff line change
`@@ -32,13 +32,6 @@ impl GodotString {`
`32`	`32`	`fn string_sys = sys;`
`33`	`33`	`fn write_string_sys = write_sys;`
`34`	`34`	`}`
`35`		`-`
`36`		`- // #[doc(hidden)]`
`37`		`- // pub fn leak_string_sys(self) -> sys::GDNativeStringPtr {`
`38`		`- // let ptr = self.string_sys();`
`39`		`- // std::mem::forget(self);`
`40`		`- // ptr`
`41`		`- // }`
`42`	`35`	`}`
`43`	`36`
`44`	`37`	`impl GodotFfi for GodotString {`