Merge #204

204: Fix various issues with pointer calls r=Bromeon a=sayaks

Combines #165 and #200 with more complete fixes.

# Overview

Properly clones strings and shares arrays/dictionaries in pointer calls to ensure they're not prematurely freed.
Frees the value currently in the `ret` pointer in pointer calls, to avoid memory leak.
Moves `ret_val` into `ret`, to avoid premature destruction.
Changes integer conversions in pointer calls to use `as`, since that seems like the right way of converting the values we get from godot. `try_from` lead to wrong conversions sometimes.
Objects inheriting from `RefCounted` now use `ref_get_object` and `ref_set_object` in virtual method calls. 

# Details

Add `from_arg_ptr` and `move_return_ptr` to `GodotFfi` that will be used when converting argument pointers in pointer calls, and moving things into a pointer.
Add `CallType` so `from_arg_ptr` and `move_return_ptr` can have different behavior depending on whether it's a virtual method call or not.
Remove `write_sys` in `GodotFfi`.
Override `from_arg_ptr` in `GodotString`, `NodePath`, `StringName`, `Array`, `Dictionary`, `Gd`.
Override `move_return_ptr` in `Gd`.
Rename `try_write_sys` to `try_return`, and have it use `move_return_ptr` instead of `write_sys`.
Rename `try_from_sys` to `try_from_arg`, and have it use `from_arg_ptr` instead of `from_sys`.
Add `u8`, `u16`, `u32` to the ptrcall tests.
Add a couple of test of virtual methods.
Fix a test for virtual method with return values to actually call the virtual method as a virtual method.

closes #195 
closes #189 



Co-authored-by: Jan Haller <[email protected]>
Co-authored-by: thebigG <[email protected]>
Co-authored-by: Lili Zoey <[email protected]>

Author: 4 people

godot-codegen/src/central_generator.rs

@@ -176,7 +176,9 @@ fn make_sys_code(central_items: &CentralItems) -> String {
             }
         }
 
-        impl GodotFfi for VariantType {
+        // SAFETY:
+        // This type is represented as `Self` in Godot, so `*mut Self` is sound.
+        unsafe impl GodotFfi for VariantType {
             ffi_methods! { type GDExtensionTypePtr = *mut Self; .. }
         }
 
@@ -205,7 +207,9 @@ fn make_sys_code(central_items: &CentralItems) -> String {
             }
         }
 
-        impl GodotFfi for VariantOperator {
+        // SAFETY:
+        // This type is represented as `Self` in Godot, so `*mut Self` is sound.
+        unsafe impl GodotFfi for VariantOperator {
             ffi_methods! { type GDExtensionTypePtr = *mut Self; .. }
         }
     };

godot-codegen/src/lib.rs

@@ -258,6 +258,7 @@ const SELECTED_CLASSES: &[&str] = &[
     "AudioStreamPlayer",
     "BaseButton",
     "Button",
+    "BoxMesh",
     "Camera2D",
     "Camera3D",
     "CanvasItem",
@@ -271,9 +272,12 @@ const SELECTED_CLASSES: &[&str] = &[
     "Image",
     "ImageTextureLayered",
     "Input",
+    "InputEvent",
+    "InputEventAction",
     "Label",
     "MainLoop",
     "Marker2D",
+    "Mesh",
     "Node",
     "Node2D",
     "Node3D",
@@ -283,9 +287,11 @@ const SELECTED_CLASSES: &[&str] = &[
     "PackedScene",
     "PathFollow2D",
     "PhysicsBody2D",
+    "PrimitiveMesh",
     "RefCounted",
     "RenderingServer",
     "Resource",
+    "ResourceFormatLoader",
     "ResourceLoader",
     "RigidBody2D",
     "SceneTree",
@@ -296,4 +302,6 @@ const SELECTED_CLASSES: &[&str] = &[
     "TextureLayered",
     "Time",
     "Timer",
+    "Window",
+    "Viewport",
 ];

godot-codegen/src/util.rs

@@ -99,7 +99,9 @@ pub fn make_enum_definition(enum_: &Enum) -> TokenStream {
                 self.ord
             }
         }
-        impl sys::GodotFfi for #enum_name {
+        // SAFETY:
+        // The enums are transparently represented as an `i32`, so `*mut Self` is sound.
+        unsafe impl sys::GodotFfi for #enum_name {
             sys::ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. }
         }
         #bitfield_ops

godot-core/src/builtin/aabb.rs

@@ -82,6 +82,8 @@ impl Aabb {
     */
 }
 
-impl GodotFfi for Aabb {
+// SAFETY:
+// This type is represented as `Self` in Godot, so `*mut Self` is sound.
+unsafe impl GodotFfi for Aabb {
     ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. }
 }

godot-core/src/builtin/array.rs

@@ -566,8 +566,27 @@ impl<T: VariantMetadata + ToVariant> Array<T> {
 //     ...
 // }
 
-impl<T: VariantMetadata> GodotFfi for Array<T> {
-    ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
+// SAFETY:
+// - `move_return_ptr`
+//   Nothing special needs to be done beyond a `std::mem::swap` when returning an Array.
+//   So we can just use `ffi_methods`.
+//
+// - `from_arg_ptr`
+//   Arrays are properly initialized through a `from_sys` call, but the ref-count should be incremented
+//   as that is the callee's responsibility. Which we do by calling `std::mem::forget(array.share())`.
+unsafe impl<T: VariantMetadata> GodotFfi for Array<T> {
+    ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque;
+        fn from_sys;
+        fn sys;
+        fn from_sys_init;
+        fn move_return_ptr;
+    }
+
+    unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::PtrcallType) -> Self {
+        let array = Self::from_sys(ptr);
+        std::mem::forget(array.share());
+        array
+    }
 
     unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
         let mut result = Self::default();
@@ -855,7 +874,7 @@ macro_rules! varray {
 /// [`set_typed`](https://docs.godotengine.org/en/latest/classes/class_array.html#class-array-method-set-typed).
 ///
 /// We ignore the `script` parameter because it has no impact on typing in Godot.
-#[derive(Debug, PartialEq, Eq)]
+#[derive(PartialEq, Eq)]
 struct TypeInfo {
     variant_type: VariantType,
     class_name: StringName,
@@ -880,3 +899,16 @@ impl TypeInfo {
         self.variant_type != VariantType::Nil
     }
 }
+
+impl fmt::Debug for TypeInfo {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let class = self.class_name.to_string();
+        let class_str = if class.is_empty() {
+            String::new()
+        } else {
+            format!(" (class={class})")
+        };
+
+        write!(f, "{:?}{}", self.variant_type, class_str)
+    }
+}

godot-core/src/builtin/basis.rs

@@ -570,7 +570,9 @@ impl Mul<Vector3> for Basis {
     }
 }
 
-impl GodotFfi for Basis {
+// SAFETY:
+// This type is represented as `Self` in Godot, so `*mut Self` is sound.
+unsafe impl GodotFfi for Basis {
     ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. }
 }
 

godot-core/src/builtin/color.rs

@@ -311,7 +311,9 @@ impl Color {
     }
 }
 
-impl GodotFfi for Color {
+// SAFETY:
+// This type is represented as `Self` in Godot, so `*mut Self` is sound.
+unsafe impl GodotFfi for Color {
     ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. }
 }
 

godot-core/src/builtin/dictionary.rs

@@ -239,8 +239,28 @@ impl Dictionary {
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // Traits
 
-impl GodotFfi for Dictionary {
-    ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
+// SAFETY:
+// - `move_return_ptr`
+//   Nothing special needs to be done beyond a `std::mem::swap` when returning an Dictionary.
+//   So we can just use `ffi_methods`.
+//
+// - `from_arg_ptr`
+//   Dictionaries are properly initialized through a `from_sys` call, but the ref-count should be
+//   incremented as that is the callee's responsibility. Which we do by calling
+//   `std::mem::forget(dictionary.share())`.
+unsafe impl GodotFfi for Dictionary {
+    ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque;
+        fn from_sys;
+        fn from_sys_init;
+        fn sys;
+        fn move_return_ptr;
+    }
+
+    unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::PtrcallType) -> Self {
+        let dictionary = Self::from_sys(ptr);
+        std::mem::forget(dictionary.share());
+        dictionary
+    }
 
     unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
         let mut result = Self::default();

godot-core/src/builtin/macros.rs

@@ -157,7 +157,10 @@ macro_rules! impl_builtin_stub {
             }
         }
 
-        impl GodotFfi for $Class {
+        // SAFETY:
+        // This is simply a wrapper around an `Opaque` value representing a value of the type.
+        // So this is safe.
+        unsafe impl GodotFfi for $Class {
             ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
         }
     };

godot-core/src/builtin/meta/mod.rs

@@ -27,13 +27,14 @@ pub trait VariantMetadata {
     }
 
     fn property_info(property_name: &str) -> PropertyInfo {
-        PropertyInfo::new(
-            Self::variant_type(),
-            Self::class_name(),
-            StringName::from(property_name),
-            global::PropertyHint::PROPERTY_HINT_NONE,
-            GodotString::new(),
-        )
+        PropertyInfo {
+            variant_type: Self::variant_type(),
+            class_name: Self::class_name(),
+            property_name: StringName::from(property_name),
+            hint: global::PropertyHint::PROPERTY_HINT_NONE,
+            hint_string: GodotString::new(),
+            usage: global::PropertyUsageFlags::PROPERTY_USAGE_DEFAULT,
+        }
     }
 
     fn param_metadata() -> sys::GDExtensionClassMethodArgumentMetadata {
@@ -52,33 +53,17 @@ impl<T: VariantMetadata> VariantMetadata for Option<T> {
 /// Rusty abstraction of sys::GDExtensionPropertyInfo
 /// Keeps the actual allocated values (the sys equivalent only keeps pointers, which fall out of scope)
 #[derive(Debug)]
+// Note: is not #[non_exhaustive], so adding fields is a breaking change. Mostly used internally at the moment though.
 pub struct PropertyInfo {
-    variant_type: VariantType,
-    class_name: ClassName,
-    property_name: StringName,
-    hint: global::PropertyHint,
-    hint_string: GodotString,
-    usage: global::PropertyUsageFlags,
+    pub variant_type: VariantType,
+    pub class_name: ClassName,
+    pub property_name: StringName,
+    pub hint: global::PropertyHint,
+    pub hint_string: GodotString,
+    pub usage: global::PropertyUsageFlags,
 }
 
 impl PropertyInfo {
-    pub fn new(
-        variant_type: VariantType,
-        class_name: ClassName,
-        property_name: StringName,
-        hint: global::PropertyHint,
-        hint_string: GodotString,
-    ) -> Self {
-        Self {
-            variant_type,
-            class_name,
-            property_name,
-            hint,
-            hint_string,
-            usage: global::PropertyUsageFlags::PROPERTY_USAGE_DEFAULT,
-        }
-    }
-
     /// Converts to the FFI type. Keep this object allocated while using that!
     pub fn property_sys(&self) -> sys::GDExtensionPropertyInfo {
         use crate::obj::EngineEnum as _;

godot-core/src/builtin/meta/signature.rs

@@ -34,6 +34,7 @@ pub trait SignatureTuple {
         ret: sys::GDExtensionTypePtr,
         func: fn(&mut C, Self::Params) -> Self::Ret,
         method_name: &str,
+        call_type: sys::PtrcallType,
     );
 }
 
@@ -143,6 +144,7 @@ macro_rules! impl_signature_for_tuple {
                 ret: sys::GDExtensionTypePtr,
                 func: fn(&mut C, Self::Params) -> Self::Ret,
                 method_name: &str,
+                call_type: sys::PtrcallType,
             ) {
                 $crate::out!("ptrcall: {}", method_name);
 
@@ -151,19 +153,20 @@ macro_rules! impl_signature_for_tuple {
 
 				let args = ( $(
                     unsafe {
-                        <$Pn as sys::GodotFuncMarshal>::try_from_sys(
-                            sys::force_mut_ptr(*args_ptr.offset($n))
+                        <$Pn as sys::GodotFuncMarshal>::try_from_arg(
+                            sys::force_mut_ptr(*args_ptr.offset($n)),
+                            call_type
                         )
                     }
                         .unwrap_or_else(|e| param_error::<$Pn>(method_name, $n, &e)),
                 )* );
 
                 let ret_val = func(&mut *instance, args);
-				unsafe { <$R as sys::GodotFuncMarshal>::try_write_sys(&ret_val, ret) }
-                    .unwrap_or_else(|e| return_error::<$R>(method_name, &e));
-
-                // FIXME is inc_ref needed here?
-				// std::mem::forget(ret_val);
+                // SAFETY:
+                // `ret` is always a pointer to an initialized value of type $R
+                // TODO: double-check the above
+				<$R as sys::GodotFuncMarshal>::try_return(ret_val, ret, call_type)
+                    .unwrap_or_else(|ret_val| return_error::<$R>(method_name, &ret_val));
             }
         }
     };

godot-core/src/builtin/node_path.rs

@@ -22,8 +22,28 @@ impl NodePath {
     }
 }
 
-impl GodotFfi for NodePath {
-    ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
+// SAFETY:
+// - `move_return_ptr`
+//   Nothing special needs to be done beyond a `std::mem::swap` when returning a NodePath.
+//   So we can just use `ffi_methods`.
+//
+// - `from_arg_ptr`
+//   NodePaths are properly initialized through a `from_sys` call, but the ref-count should be
+//   incremented as that is the callee's responsibility. Which we do by calling
+//   `std::mem::forget(node_path.share())`.
+unsafe impl GodotFfi for NodePath {
+    ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque;
+        fn from_sys;
+        fn sys;
+        fn from_sys_init;
+        fn move_return_ptr;
+    }
+
+    unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::PtrcallType) -> Self {
+        let node_path = Self::from_sys(ptr);
+        std::mem::forget(node_path.clone());
+        node_path
+    }
 
     unsafe fn from_sys_init_default(init_fn: impl FnOnce(GDExtensionTypePtr)) -> Self {
         let mut result = Self::default();
@@ -90,5 +110,7 @@ impl_builtin_traits! {
         Default => node_path_construct_default;
         Clone => node_path_construct_copy;
         Drop => node_path_destroy;
+        Eq => node_path_operator_equal;
+        // Ord => node_path_operator_less;
     }
 }

godot-core/src/builtin/packed_array.rs

@@ -390,8 +390,26 @@ macro_rules! impl_packed_array {
             }
         }
 
-        impl GodotFfi for $PackedArray {
-            ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
+        unsafe impl GodotFfi for $PackedArray {
+            ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque;
+                fn from_sys;
+                fn sys;
+                fn from_sys_init;
+                // SAFETY:
+                // Nothing special needs to be done beyond a `std::mem::swap` when returning a packed array.
+                fn move_return_ptr;
+            }
+
+            // SAFETY:
+            // Packed arrays are properly initialized through a `from_sys` call, but the ref-count should be
+            // incremented as that is the callee's responsibility.
+            //
+            // Using `std::mem::forget(array.clone())` increments the ref count.
+            unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::PtrcallType) -> Self {
+                let array = Self::from_sys(ptr);
+                std::mem::forget(array.clone());
+                array
+            }
 
             unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
                 let mut result = Self::default();

godot-core/src/builtin/plane.rs

@@ -134,7 +134,9 @@ impl Neg for Plane {
     }
 }
 
-impl GodotFfi for Plane {
+// SAFETY:
+// This type is represented as `Self` in Godot, so `*mut Self` is sound.
+unsafe impl GodotFfi for Plane {
     ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. }
 }