ToGodot for Option<T> is again pass-by-ref

Author: Bromeon

godot-core/src/meta/args/as_arg.rs

@@ -9,7 +9,7 @@ use std::ffi::CStr;
 
 use crate::builtin::{GString, NodePath, StringName, Variant};
 use crate::meta::sealed::Sealed;
-use crate::meta::traits::GodotFfiVariant;
+use crate::meta::traits::{GodotFfiVariant, GodotNullableFfi};
 use crate::meta::{CowArg, GodotType, ObjectArg, ToGodot};
 use crate::obj::{bounds, Bounds, DynGd, Gd, GodotClass, Inherits};
 
@@ -423,7 +423,9 @@ impl AsArg<NodePath> for &String {
 /// See [`ToGodot::Pass`].
 pub trait ArgPassing: Sealed {
     /// Return type: `T` or `&'r T`.
-    type Output<'r, T: 'r>;
+    type Output<'r, T: 'r>
+    where
+        Self: 'r;
 
     /// FFI argument type: `T::Ffi` or `T::ToFfi<'f>`.
     #[doc(hidden)]
@@ -521,6 +523,8 @@ impl ArgPassing for ByRef {
 
 /// Pass arguments to Godot by object pointer (for objects only).
 ///
+/// Currently distinct from [`ByRef`] to not interfere with the blanket impl for `&T` for all `ByRef` types. Semantics are largely the same.
+///
 /// See [`ToGodot::Pass`].
 pub enum ByObject {}
 impl Sealed for ByObject {}
@@ -551,5 +555,54 @@ impl ArgPassing for ByObject {
     }
 }
 
+/// Pass optional arguments by returning `Option<&T::Via>`, allowing delegation to underlying type's strategy.
+///
+/// This enables `Option<T>` to benefit from the underlying type's efficient passing without cloning. [`ByRef`] doesn't support this because it
+/// would transform `Option<T>` to `&Option<T>`; however, we need `Option<&T>` instead.
+///
+/// See [`ToGodot::Pass`].
+pub enum ByOption<Via> {
+    // Uses `Via` generic type to work around the near-impossibility of Output<'r, T> pointing to a metafunction that transforms Option<T> to
+    // Option<&'r T>. Such a metafunction cannot be implemented via trait (overlapping impls cause coherence issues), and we would need to
+    // pollute also the other `By*` types by anything. Using a generic parameter on the trait rather than the associated type avoids that.
+    _Phantom(std::marker::PhantomData<Via>),
+}
+impl<Via> Sealed for ByOption<Via> {}
+impl<Via> ArgPassing for ByOption<Via>
+where
+    Via: GodotType,
+    for<'f> Via::ToFfi<'f>: GodotNullableFfi,
+{
+    type Output<'r, T: 'r>
+        = Option<&'r Via>
+    where
+        Self: 'r;
+
+    type FfiOutput<'f, T>
+        = <Via as GodotType>::ToFfi<'f>
+    where
+        T: GodotType + 'f;
+
+    // value:  &Option<U>
+    // return: T::Via = Option<U::Via>
+    fn ref_to_owned_via<T>(value: &T) -> T::Via
+    where
+        T: ToGodot<Pass = Self>,
+        T::Via: Clone,
+    {
+        value.to_godot_owned()
+    }
+
+    fn ref_to_ffi<T>(value: &T) -> Self::FfiOutput<'_, T::Via>
+    where
+        T: ToGodot<Pass = Self>,
+        T::Via: GodotType,
+    {
+        // Reuse pattern from impl GodotType for Option<T>:
+        // Convert Option<&Via> to Option<Via::ToFfi> and then flatten to Via::ToFfi with null handling.
+        GodotNullableFfi::flatten_option(value.to_godot().map(|via_ref| via_ref.to_ffi()))
+    }
+}
+
 #[doc(hidden)] // Easier for internal use.
 pub type ToArg<'r, Via, Pass> = <Pass as ArgPassing>::Output<'r, Via>;

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

@@ -13,7 +13,9 @@ mod ref_arg;
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // Public APIs
 
-pub use as_arg::{owned_into_arg, ref_to_arg, ArgPassing, AsArg, ByObject, ByRef, ByValue, ToArg};
+pub use as_arg::{
+    owned_into_arg, ref_to_arg, ArgPassing, AsArg, ByObject, ByOption, ByRef, ByValue, ToArg,
+};
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // Internal APIs
 

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

@@ -8,6 +8,7 @@
 use godot_ffi as sys;
 
 use crate::builtin::{Array, Variant};
+use crate::meta;
 use crate::meta::error::{ConvertError, ErrorKind, FromFfiError, FromVariantError};
 use crate::meta::{
     ArrayElement, ClassName, FromGodot, GodotConvert, GodotNullableFfi, GodotType,
@@ -85,47 +86,47 @@ where
     }
 
     // Only relevant for object types T.
-    unsafe fn as_object_arg(&self) -> crate::meta::ObjectArg {
+    unsafe fn as_object_arg(&self) -> meta::ObjectArg {
         match self {
             Some(inner) => unsafe { inner.as_object_arg() },
-            None => crate::meta::ObjectArg::null(),
+            None => meta::ObjectArg::null(),
         }
     }
 }
 
-impl<T: GodotConvert> GodotConvert for Option<T>
+impl<T> GodotConvert for Option<T>
 where
+    T: GodotConvert,
     Option<T::Via>: GodotType,
 {
     type Via = Option<T::Via>;
 }
 
-impl<T: ToGodot> ToGodot for Option<T>
+impl<T> ToGodot for Option<T>
 where
-    Option<T::Via>: GodotType,
+    // Currently limited to holding objects -> needed to establish to_godot() relation T::to_godot() = Option<&T::Via>.
+    T: ToGodot<Pass = meta::ByObject>,
+    // Extra Clone bound for to_godot_owned(); might be extracted in the future.
+    T::Via: Clone,
+    // T::Via must be a Godot nullable type (to support the None case).
     for<'f> T::Via: GodotType<
         // Associated types need to be nullable.
         Ffi: GodotNullableFfi,
         ToFfi<'f>: GodotNullableFfi,
     >,
-    T::Via: Clone,
+    // Previously used bound, not needed right now but don't remove: Option<T::Via>: GodotType,
 {
-    // Potential optimization: use underlying T::Pass instead of ByValue.
-    // Problem is that ByRef requires to_godot() -> &Self::Via, which is &Option<T::Via>. We would however need Option<&T::Via>.
-    // Might need a third ArgPassing impl, or different design.
-    type Pass = crate::meta::ByValue;
+    // Basically ByRef, but allows Option<T> -> Option<&T::Via> conversion.
+    type Pass = meta::ByOption<T::Via>;
 
-    fn to_godot(&self) -> Option<T::Via> {
-        self.as_ref().map(T::to_godot_owned)
+    fn to_godot(&self) -> Option<&T::Via> {
+        self.as_ref().map(T::to_godot)
     }
 
-    fn to_godot_owned(&self) -> Self::Via
+    fn to_godot_owned(&self) -> Option<T::Via>
     where
         Self::Via: Clone,
     {
-        // Default implementation calls underlying T::to_godot().clone(), which may be wrong.
-        // Some to_godot_owned() calls are specialized/overridden, we need to honor that.
-
         self.as_ref().map(T::to_godot_owned)
     }
 
@@ -256,7 +257,7 @@ macro_rules! impl_godot_scalar {
         }
 
         impl ToGodot for $T {
-            type Pass = crate::meta::ByValue;
+            type Pass = meta::ByValue;
 
             fn to_godot(&self) -> Self::Via {
                *self
@@ -272,10 +273,10 @@ macro_rules! impl_godot_scalar {
 }
 
 // `GodotType` for these three is implemented in `godot-core/src/builtin/variant/impls.rs`.
-crate::meta::impl_godot_as_self!(bool: ByValue);
-crate::meta::impl_godot_as_self!(i64: ByValue);
-crate::meta::impl_godot_as_self!(f64: ByValue);
-crate::meta::impl_godot_as_self!((): ByValue);
+meta::impl_godot_as_self!(bool: ByValue);
+meta::impl_godot_as_self!(i64: ByValue);
+meta::impl_godot_as_self!(f64: ByValue);
+meta::impl_godot_as_self!((): ByValue);
 
 // Also implements ArrayElement.
 impl_godot_scalar!(
@@ -342,7 +343,7 @@ impl GodotConvert for u64 {
 }
 
 impl ToGodot for u64 {
-    type Pass = crate::meta::ByValue;
+    type Pass = meta::ByValue;
 
     fn to_godot(&self) -> Self::Via {
         *self
@@ -382,7 +383,7 @@ impl<T: ArrayElement> GodotConvert for Vec<T> {
 }
 
 impl<T: ArrayElement> ToGodot for Vec<T> {
-    type Pass = crate::meta::ByValue;
+    type Pass = meta::ByValue;
 
     fn to_godot(&self) -> Self::Via {
         Array::from(self.as_slice())
@@ -400,7 +401,7 @@ impl<T: ArrayElement, const LEN: usize> GodotConvert for [T; LEN] {
 }
 
 impl<T: ArrayElement, const LEN: usize> ToGodot for [T; LEN] {
-    type Pass = crate::meta::ByValue;
+    type Pass = meta::ByValue;
 
     fn to_godot(&self) -> Self::Via {
         Array::from(self)
@@ -440,7 +441,7 @@ impl<T: ArrayElement> GodotConvert for &[T] {
 }
 
 impl<T: ArrayElement> ToGodot for &[T] {
-    type Pass = crate::meta::ByValue;
+    type Pass = meta::ByValue;
 
     fn to_godot(&self) -> Self::Via {
         Array::from(*self)
@@ -461,7 +462,7 @@ macro_rules! impl_pointer_convert {
         }
 
         impl ToGodot for $Ptr {
-            type Pass = crate::meta::ByValue;
+            type Pass = meta::ByValue;
 
             fn to_godot(&self) -> Self::Via {
                 *self as i64

itest/rust/src/object_tests/object_test.rs

@@ -100,17 +100,20 @@ fn object_engine_roundtrip() {
 }
 
 #[itest]
-fn object_null_argument() {
-    // Objects currently use ObjectArg instead of RefArg, so this scenario shouldn't occur. Test can be updated if code is refactored.
+fn object_option_argument() {
+    // Tests following things:
+    // - to_godot() returns Option<&T>
+    // - None maps to None
+    // - Some(gd) maps to Some(&gd)
 
-    let null_obj = Option::<Gd<Node>>::None;
+    let null_obj = None::<Gd<Node>>;
+    let via: Option<&Gd<Node>> = null_obj.to_godot();
+    assert_eq!(via, None);
 
-    let via = null_obj.to_godot();
-    let ffi = via.to_ffi();
-
-    expect_panic("not yet implemented: pass objects through RefArg", || {
-        ffi.to_godot();
-    });
+    let refc = RefCounted::new_gd();
+    let some_obj = Some(refc.clone());
+    let via: Option<&Gd<RefCounted>> = some_obj.to_godot();
+    assert_eq!(via, Some(&refc));
 }
 
 #[itest]