Add GodotType::ToFfi<'f> to keep by-ref also in FFI layer

Refactors some related parts:
* GodotNullableFfi now has a null() associated function.
* Correct usage of as_arg() and move_return_ptr().
* into_ffi() helper needs marshal_args! macro, since it has transient
  borrowed state.

Author: Bromeon

godot-core/src/builtin/collections/array.rs

@@ -12,7 +12,7 @@ use crate::builtin::*;
 use crate::meta::error::{ConvertError, FromGodotError, FromVariantError};
 use crate::meta::{
     ArrayElement, ArrayTypeInfo, FromGodot, GodotConvert, GodotFfiVariant, GodotType,
-    PropertyHintInfo, ToGodot,
+    PropertyHintInfo, RefArg, ToGodot,
 };
 use crate::registry::property::{Export, Var};
 use godot_ffi as sys;
@@ -1047,10 +1047,11 @@ impl<T: ArrayElement> Drop for Array<T> {
 impl<T: ArrayElement> GodotType for Array<T> {
     type Ffi = Self;
 
-    fn to_ffi(&self) -> Self::Ffi {
-        // SAFETY: we may pass type-transmuted arrays to FFI (e.g. Array<T> as Array<Variant>). This would fail the regular
-        // type-check in clone(), so we disable it. Type invariants are upheld by the "front-end" APIs.
-        unsafe { self.clone_unchecked() }
+    type ToFfi<'f> = RefArg<'f, Array<T>>
+    where Self: 'f;
+
+    fn to_ffi(&self) -> Self::ToFfi<'_> {
+        RefArg::new(self)
     }
 
     fn into_ffi(self) -> Self::Ffi {

godot-core/src/builtin/variant/impls.rs

@@ -9,7 +9,9 @@ use super::*;
 use crate::builtin::*;
 use crate::global;
 use crate::meta::error::{ConvertError, FromVariantError};
-use crate::meta::{ArrayElement, GodotFfiVariant, GodotType, PropertyHintInfo, PropertyInfo};
+use crate::meta::{
+    ArrayElement, GodotFfiVariant, GodotType, PropertyHintInfo, PropertyInfo, RefArg,
+};
 use godot_ffi as sys;
 // For godot-cpp, see https://github.com/godotengine/godot-cpp/blob/master/include/godot_cpp/core/type_info.hpp.
 
@@ -22,7 +24,15 @@ use godot_ffi as sys;
 //
 // Therefore, we can use `init` to indicate when it must be initialized in 4.0.
 macro_rules! impl_ffi_variant {
-    ($T:ty, $from_fn:ident, $to_fn:ident $(; $godot_type_name:ident)?) => {
+    (ref $T:ty, $from_fn:ident, $to_fn:ident $(; $GodotTy:ident)?) => {
+        impl_ffi_variant!(@impls by_ref; $T, $from_fn, $to_fn $(; $GodotTy)?);
+    };
+    ($T:ty, $from_fn:ident, $to_fn:ident $(; $GodotTy:ident)?) => {
+        impl_ffi_variant!(@impls by_val; $T, $from_fn, $to_fn $(; $GodotTy)?);
+    };
+
+    // Implementations
+    (@impls $by_ref_or_val:ident; $T:ty, $from_fn:ident, $to_fn:ident $(; $GodotTy:ident)?) => {
         impl GodotFfiVariant for $T {
             fn ffi_to_variant(&self) -> Variant {
                 let variant = unsafe {
@@ -58,10 +68,7 @@ macro_rules! impl_ffi_variant {
 
         impl GodotType for $T {
             type Ffi = Self;
-
-            fn to_ffi(&self) -> Self::Ffi {
-                self.clone()
-            }
+            impl_ffi_variant!(@assoc_to_ffi $by_ref_or_val);
 
             fn into_ffi(self) -> Self::Ffi {
                 self
@@ -71,7 +78,7 @@ macro_rules! impl_ffi_variant {
                 Ok(ffi)
             }
 
-            impl_ffi_variant!(@godot_type_name $T $(, $godot_type_name)?);
+            impl_ffi_variant!(@godot_type_name $T $(, $GodotTy)?);
         }
 
         impl ArrayElement for $T {}
@@ -88,6 +95,22 @@ macro_rules! impl_ffi_variant {
             stringify!($godot_type_name).into()
         }
     };
+
+    (@assoc_to_ffi by_ref) => {
+        type ToFfi<'a> =  RefArg<'a, Self>;
+
+        fn to_ffi(&self) -> Self::ToFfi<'_> {
+            RefArg::new(self)
+        }
+    };
+
+    (@assoc_to_ffi by_val) => {
+        type ToFfi<'a> = Self;
+
+        fn to_ffi(&self) -> Self::ToFfi<'_> {
+            self.clone()
+        }
+    };
 }
 
 // ----------------------------------------------------------------------------------------------------------------------------------------------
@@ -116,17 +139,17 @@ mod impls {
     impl_ffi_variant!(GString, string_to_variant, string_from_variant; String);
     impl_ffi_variant!(StringName, string_name_to_variant, string_name_from_variant);
     impl_ffi_variant!(NodePath, node_path_to_variant, node_path_from_variant);
-    impl_ffi_variant!(PackedByteArray, packed_byte_array_to_variant, packed_byte_array_from_variant);
-    impl_ffi_variant!(PackedInt32Array, packed_int32_array_to_variant, packed_int32_array_from_variant);
-    impl_ffi_variant!(PackedInt64Array, packed_int64_array_to_variant, packed_int64_array_from_variant);
-    impl_ffi_variant!(PackedFloat32Array, packed_float32_array_to_variant, packed_float32_array_from_variant);
-    impl_ffi_variant!(PackedFloat64Array, packed_float64_array_to_variant, packed_float64_array_from_variant);
-    impl_ffi_variant!(PackedStringArray, packed_string_array_to_variant, packed_string_array_from_variant);
-    impl_ffi_variant!(PackedVector2Array, packed_vector2_array_to_variant, packed_vector2_array_from_variant);
-    impl_ffi_variant!(PackedVector3Array, packed_vector3_array_to_variant, packed_vector3_array_from_variant);
+    impl_ffi_variant!(ref PackedByteArray, packed_byte_array_to_variant, packed_byte_array_from_variant);
+    impl_ffi_variant!(ref PackedInt32Array, packed_int32_array_to_variant, packed_int32_array_from_variant);
+    impl_ffi_variant!(ref PackedInt64Array, packed_int64_array_to_variant, packed_int64_array_from_variant);
+    impl_ffi_variant!(ref PackedFloat32Array, packed_float32_array_to_variant, packed_float32_array_from_variant);
+    impl_ffi_variant!(ref PackedFloat64Array, packed_float64_array_to_variant, packed_float64_array_from_variant);
+    impl_ffi_variant!(ref PackedStringArray, packed_string_array_to_variant, packed_string_array_from_variant);
+    impl_ffi_variant!(ref PackedVector2Array, packed_vector2_array_to_variant, packed_vector2_array_from_variant);
+    impl_ffi_variant!(ref PackedVector3Array, packed_vector3_array_to_variant, packed_vector3_array_from_variant);
     #[cfg(since_api = "4.3")]
-    impl_ffi_variant!(PackedVector4Array, packed_vector4_array_to_variant, packed_vector4_array_from_variant);
-    impl_ffi_variant!(PackedColorArray, packed_color_array_to_variant, packed_color_array_from_variant);
+    impl_ffi_variant!(ref PackedVector4Array, packed_vector4_array_to_variant, packed_vector4_array_from_variant);
+    impl_ffi_variant!(ref PackedColorArray, packed_color_array_to_variant, packed_color_array_from_variant);
     impl_ffi_variant!(Plane, plane_to_variant, plane_from_variant);
     impl_ffi_variant!(Projection, projection_to_variant, projection_from_variant);
     impl_ffi_variant!(Rid, rid_to_variant, rid_from_variant; RID);
@@ -135,7 +158,7 @@ mod impls {
     impl_ffi_variant!(Signal, signal_to_variant, signal_from_variant);
     impl_ffi_variant!(Transform2D, transform_2d_to_variant, transform_2d_from_variant);
     impl_ffi_variant!(Transform3D, transform_3d_to_variant, transform_3d_from_variant);
-    impl_ffi_variant!(Dictionary, dictionary_to_variant, dictionary_from_variant);
+    impl_ffi_variant!(ref Dictionary, dictionary_to_variant, dictionary_from_variant);
 
 }
 
@@ -162,9 +185,10 @@ impl GodotFfiVariant for () {
 }
 
 impl GodotType for () {
-    type Ffi = Self;
+    type Ffi = ();
+    type ToFfi<'a> = ();
 
-    fn to_ffi(&self) -> Self::Ffi {}
+    fn to_ffi(&self) -> Self::ToFfi<'_> {}
 
     fn into_ffi(self) -> Self::Ffi {}
 
@@ -189,9 +213,10 @@ impl GodotFfiVariant for Variant {
 
 impl GodotType for Variant {
     type Ffi = Variant;
+    type ToFfi<'a> = RefArg<'a, Variant>;
 
-    fn to_ffi(&self) -> Self::Ffi {
-        self.clone()
+    fn to_ffi(&self) -> Self::ToFfi<'_> {
+        RefArg::new(self)
     }
 
     fn into_ffi(self) -> Self::Ffi {

godot-core/src/meta/godot_convert/impls.rs

@@ -25,10 +25,13 @@ impl<T> GodotType for Option<T>
 where
     T: GodotType,
     T::Ffi: GodotNullableFfi,
+    for<'f> T::ToFfi<'f>: GodotNullableFfi,
 {
     type Ffi = T::Ffi;
 
-    fn to_ffi(&self) -> Self::Ffi {
+    type ToFfi<'f> = T::ToFfi<'f>;
+
+    fn to_ffi(&self) -> Self::ToFfi<'_> {
         GodotNullableFfi::flatten_option(self.as_ref().map(|t| t.to_ffi()))
     }
 
@@ -91,7 +94,11 @@ where
 impl<T: ToGodot> ToGodot for Option<T>
 where
     Option<T::Via>: GodotType,
-    for<'f> T::ToVia<'f>: GodotType<Ffi: GodotNullableFfi>,
+    for<'v, 'f> T::ToVia<'v>: GodotType<
+        // Associated types need to be nullable.
+        Ffi: GodotNullableFfi,
+        ToFfi<'f>: GodotNullableFfi,
+    >,
 {
     type ToVia<'v> = Option<T::ToVia<'v>>
     // type ToVia<'v> = Self::Via
@@ -155,8 +162,9 @@ macro_rules! impl_godot_scalar {
     ($T:ty as $Via:ty, $err:path, $param_metadata:expr) => {
         impl GodotType for $T {
             type Ffi = $Via;
+            type ToFfi<'f> = $Via;
 
-            fn to_ffi(&self) -> Self::Ffi {
+            fn to_ffi(&self) -> Self::ToFfi<'_> {
                 (*self).into()
             }
 
@@ -188,8 +196,9 @@ macro_rules! impl_godot_scalar {
     ($T:ty as $Via:ty, $param_metadata:expr; lossy) => {
         impl GodotType for $T {
             type Ffi = $Via;
+            type ToFfi<'f> = $Via;
 
-            fn to_ffi(&self) -> Self::Ffi {
+            fn to_ffi(&self) -> Self::ToFfi<'_> {
                 *self as $Via
             }
 
@@ -289,8 +298,9 @@ impl_godot_scalar!(
 
 impl GodotType for u64 {
     type Ffi = i64;
+    type ToFfi<'f> = i64;
 
-    fn to_ffi(&self) -> Self::Ffi {
+    fn to_ffi(&self) -> Self::ToFfi<'_> {
         *self as i64
     }
 

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

@@ -40,6 +40,7 @@ pub trait GodotConvert {
 ///
 /// Please read the [`godot::meta` module docs][crate::meta] for further information about conversions.
 pub trait ToGodot: Sized + GodotConvert {
+    /// Target type of [`to_godot()`](ToGodot::to_godot), which differs from [`Via`](GodotConvert::Via) for pass-by-reference types.
     type ToVia<'v>: GodotType
     where
         Self: 'v;
@@ -95,16 +96,10 @@ pub trait FromGodot: Sized + GodotConvert {
     }
 }
 
-// Note: removing the implicit lifetime (by taking value: T instead of &T) causes issues due to allegedly returning a lifetime
-// to a local variable, even though the result Ffi is 'static by definition.
-#[allow(clippy::needless_lifetimes)] // eliding causes error: missing generics for associated type `godot_convert::ToGodot::ToVia`
-pub(crate) fn into_ffi<'v, T: ToGodot>(value: &'v T) -> <T::ToVia<'v> as GodotType>::Ffi {
-    let by_ref = value.to_godot();
-    by_ref.to_ffi()
-}
-
 pub(crate) fn into_ffi_variant<T: ToGodot>(value: &T) -> Variant {
-    GodotFfiVariant::ffi_to_variant(&into_ffi(value))
+    let via = value.to_godot();
+    let ffi = via.to_ffi();
+    GodotFfiVariant::ffi_to_variant(&ffi)
 }
 
 pub(crate) fn try_from_ffi<T: FromGodot>(

godot-core/src/meta/ref_arg.rs

@@ -4,20 +4,35 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/.
  */
+use crate::builtin::Variant;
 use crate::meta::error::ConvertError;
-use crate::meta::{FromGodot, GodotConvert, ToGodot};
+use crate::meta::{FromGodot, GodotConvert, GodotFfiVariant, ToGodot};
+use crate::sys;
+use godot_ffi::{GodotFfi, GodotNullableFfi, PtrcallType};
 use std::fmt;
 
 pub struct RefArg<'r, T> {
-    pub(crate) shared_ref: &'r T,
+    /// Only `None` if `T: GodotNullableFfi` and `T::is_null()` is true.
+    shared_ref: Option<&'r T>,
 }
 
 impl<'r, T> RefArg<'r, T> {
     pub fn new(shared_ref: &'r T) -> Self {
-        RefArg { shared_ref }
+        RefArg {
+            shared_ref: Some(shared_ref),
+        }
     }
 }
 
+macro_rules! wrong_direction {
+    ($fn:ident) => {
+        unreachable!(concat!(
+            stringify!($fn),
+            ": RefArg should only be passed *to* Godot, not *from*."
+        ))
+    };
+}
+
 impl<'r, T> GodotConvert for RefArg<'r, T>
 where
     T: GodotConvert,
@@ -33,7 +48,11 @@ where
     where Self: 'v;
 
     fn to_godot(&self) -> Self::ToVia<'_> {
-        self.shared_ref.to_godot()
+        let shared_ref = self
+            .shared_ref
+            .expect("Objects are currently mapped through ObjectArg; RefArg shouldn't be null");
+
+        shared_ref.to_godot()
     }
 }
 
@@ -43,7 +62,7 @@ where
     T: FromGodot,
 {
     fn try_from_godot(_via: Self::Via) -> Result<Self, ConvertError> {
-        unreachable!("RefArg should only be passed *to* Godot, not *from*.")
+        wrong_direction!(try_from_godot)
     }
 }
 
@@ -55,3 +74,83 @@ where
         write!(f, "&{:?}", self.shared_ref)
     }
 }
+
+// SAFETY: delegated to T.
+unsafe impl<'r, T> GodotFfi for RefArg<'r, T>
+where
+    T: GodotFfi,
+{
+    fn variant_type() -> sys::VariantType {
+        T::variant_type()
+    }
+
+    unsafe fn new_from_sys(_ptr: sys::GDExtensionConstTypePtr) -> Self {
+        wrong_direction!(new_from_sys)
+    }
+
+    unsafe fn new_with_uninit(_init_fn: impl FnOnce(sys::GDExtensionUninitializedTypePtr)) -> Self {
+        wrong_direction!(new_with_uninit)
+    }
+
+    unsafe fn new_with_init(_init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
+        wrong_direction!(new_with_init)
+    }
+
+    fn sys(&self) -> sys::GDExtensionConstTypePtr {
+        match self.shared_ref {
+            Some(r) => r.sys(),
+            None => std::ptr::null(),
+        }
+    }
+
+    fn sys_mut(&mut self) -> sys::GDExtensionTypePtr {
+        unreachable!("RefArg::sys_mut() currently not used by FFI marshalling layer, but only by specific functions");
+    }
+
+    // This function must be overridden; the default delegating to sys() is wrong for e.g. RawGd<T>.
+    // See also other manual overrides of as_arg_ptr().
+    fn as_arg_ptr(&self) -> sys::GDExtensionConstTypePtr {
+        match self.shared_ref {
+            Some(r) => r.as_arg_ptr(),
+            None => std::ptr::null(),
+        }
+    }
+
+    unsafe fn from_arg_ptr(_ptr: sys::GDExtensionTypePtr, _call_type: PtrcallType) -> Self {
+        wrong_direction!(from_arg_ptr)
+    }
+
+    unsafe fn move_return_ptr(self, _dst: sys::GDExtensionTypePtr, _call_type: PtrcallType) {
+        // This one is implemented, because it's used for return types implementing ToGodot.
+        unreachable!("Calling RefArg::move_return_ptr is a mistake, as RefArg is intended only for arguments. Use the underlying value type.");
+    }
+}
+
+impl<'r, T> GodotFfiVariant for RefArg<'r, T>
+where
+    T: GodotFfiVariant,
+{
+    fn ffi_to_variant(&self) -> Variant {
+        match self.shared_ref {
+            Some(r) => r.ffi_to_variant(),
+            None => Variant::nil(),
+        }
+    }
+
+    fn ffi_from_variant(_variant: &Variant) -> Result<Self, ConvertError> {
+        wrong_direction!(ffi_from_variant)
+    }
+}
+
+impl<'r, T> GodotNullableFfi for RefArg<'r, T>
+where
+    T: GodotNullableFfi,
+{
+    fn null() -> Self {
+        RefArg { shared_ref: None }
+    }
+
+    fn is_null(&self) -> bool {
+        self.shared_ref.map(|r| r.is_null()).unwrap_or(true)
+    }
+}