From 28e158c79ab83b9d04192a1a06ef195b23f5fc3c Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Wed, 18 Sep 2024 23:03:27 +0200 Subject: [PATCH 1/3] 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. --- godot-core/src/builtin/collections/array.rs | 11 +- godot-core/src/builtin/variant/impls.rs | 69 +++++++++---- godot-core/src/meta/godot_convert/impls.rs | 20 +++- godot-core/src/meta/godot_convert/mod.rs | 13 +-- godot-core/src/meta/ref_arg.rs | 109 +++++++++++++++++++- godot-core/src/meta/signature.rs | 77 +++++++------- godot-core/src/meta/traits.rs | 21 +++- godot-core/src/obj/gd.rs | 9 +- godot-core/src/obj/object_arg.rs | 7 +- godot-core/src/obj/raw_gd.rs | 27 +++-- godot-ffi/src/godot_ffi.rs | 7 +- itest/rust/src/object_tests/object_test.rs | 18 +++- 12 files changed, 279 insertions(+), 109 deletions(-) diff --git a/godot-core/src/builtin/collections/array.rs b/godot-core/src/builtin/collections/array.rs index fc69a7d40..cd2ffe2cf 100644 --- a/godot-core/src/builtin/collections/array.rs +++ b/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 Drop for Array { impl GodotType for Array { type Ffi = Self; - fn to_ffi(&self) -> Self::Ffi { - // SAFETY: we may pass type-transmuted arrays to FFI (e.g. Array as Array). 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> + where Self: 'f; + + fn to_ffi(&self) -> Self::ToFfi<'_> { + RefArg::new(self) } fn into_ffi(self) -> Self::Ffi { diff --git a/godot-core/src/builtin/variant/impls.rs b/godot-core/src/builtin/variant/impls.rs index a5adf059e..4df041630 100644 --- a/godot-core/src/builtin/variant/impls.rs +++ b/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 { diff --git a/godot-core/src/meta/godot_convert/impls.rs b/godot-core/src/meta/godot_convert/impls.rs index 84de15e67..163fdaf58 100644 --- a/godot-core/src/meta/godot_convert/impls.rs +++ b/godot-core/src/meta/godot_convert/impls.rs @@ -25,10 +25,13 @@ impl GodotType for Option 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 ToGodot for Option where Option: GodotType, - for<'f> T::ToVia<'f>: GodotType, + for<'v, 'f> T::ToVia<'v>: GodotType< + // Associated types need to be nullable. + Ffi: GodotNullableFfi, + ToFfi<'f>: GodotNullableFfi, + >, { type ToVia<'v> = Option> // 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 } diff --git a/godot-core/src/meta/godot_convert/mod.rs b/godot-core/src/meta/godot_convert/mod.rs index e9ee39108..be9cc247a 100644 --- a/godot-core/src/meta/godot_convert/mod.rs +++ b/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) -> as GodotType>::Ffi { - let by_ref = value.to_godot(); - by_ref.to_ffi() -} - pub(crate) fn into_ffi_variant(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( diff --git a/godot-core/src/meta/ref_arg.rs b/godot-core/src/meta/ref_arg.rs index eadc7e3c3..024fb4acb 100644 --- a/godot-core/src/meta/ref_arg.rs +++ b/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 { - 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. + // 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 { + 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) + } +} diff --git a/godot-core/src/meta/signature.rs b/godot-core/src/meta/signature.rs index 33223dc86..fdb701f04 100644 --- a/godot-core/src/meta/signature.rs +++ b/godot-core/src/meta/signature.rs @@ -14,7 +14,7 @@ use sys::{BuiltinMethodBind, ClassMethodBind, GodotFfi, UtilityFunctionBind}; use crate::builtin::Variant; use crate::meta::error::{CallError, ConvertError}; -use crate::meta::godot_convert::{into_ffi, into_ffi_variant, try_from_ffi}; +use crate::meta::godot_convert::{into_ffi_variant, try_from_ffi}; use crate::meta::*; use crate::obj::{GodotClass, InstanceId}; @@ -119,23 +119,6 @@ pub trait PtrcallSignatureTuple { ) -> Self::Ret; } -// impl Sig for [P; N] -// impl Sig for (T0) -// where P: VariantMetadata { -// fn variant_type(index: usize) -> sys::GDExtensionVariantType { -// Self[index]:: -// } -// -// fn param_metadata(index: usize) -> sys::GDExtensionClassMethodArgumentMetadata { -// todo!() -// } -// -// fn property_info(index: usize, param_name: &str) -> sys::GDExtensionPropertyInfo { -// todo!() -// } -// } -// - macro_rules! impl_varcall_signature_for_tuple { ( $PARAM_COUNT:literal; @@ -333,6 +316,31 @@ macro_rules! impl_varcall_signature_for_tuple { }; } +macro_rules! marshal_args { + ( + let $out:ident = $( $pn:ident: $n:tt )* ; + ) => { + // Note: this used to be done in a single `into_ffi()` function, however with reference semantics, this causes issues with lifetimes: + // The to_godot() call creates a temporary value local to the function, and even when using the same 'v lifetime throughout, rustc + // assumes that the temporary value may store other state that would go out of scope after the function returns. + // Thus, this is now done in 2 steps, and abstracted with a macro. + + #[allow(clippy::let_unit_value)] + let vias = ( + $( + ToGodot::to_godot(&$pn), + )* + ); + + #[allow(clippy::let_unit_value)] + let $out = ( + $( + GodotType::to_ffi(&vias.$n), + )* + ); + } +} + macro_rules! impl_ptrcall_signature_for_tuple { ( $R:ident @@ -389,12 +397,9 @@ macro_rules! impl_ptrcall_signature_for_tuple { let class_fn = sys::interface_fn!(object_method_bind_ptrcall); - #[allow(clippy::let_unit_value)] - let marshalled_args = ( - $( - into_ffi(&$pn), - )* - ); + marshal_args! { + let marshalled_args = $($pn: $n)*; + } let type_ptrs = [ $( @@ -420,12 +425,9 @@ macro_rules! impl_ptrcall_signature_for_tuple { let call_ctx = CallContext::outbound(class_name, method_name); // $crate::out!("out_builtin_ptrcall: {call_ctx}"); - #[allow(clippy::let_unit_value)] - let marshalled_args = ( - $( - into_ffi(&$pn), - )* - ); + marshal_args! { + let marshalled_args = $($pn: $n)*; + } let type_ptrs = [ $( @@ -448,12 +450,9 @@ macro_rules! impl_ptrcall_signature_for_tuple { let call_ctx = CallContext::outbound("", function_name); // $crate::out!("out_utility_ptrcall: {call_ctx}"); - #[allow(clippy::let_unit_value)] - let marshalled_args = ( - $( - into_ffi(&$pn), - )* - ); + marshal_args! { + let marshalled_args = $($pn: $n)*; + } let arg_ptrs = [ $( @@ -548,10 +547,10 @@ unsafe fn ptrcall_return( _call_ctx: &CallContext, call_type: sys::PtrcallType, ) { - //let val = into_ffi(ret_val); - let val = ret_val.to_godot().to_ffi(); + let val = ret_val.to_godot(); + let ffi = val.into_ffi(); - val.move_return_ptr(ret, call_type); + ffi.move_return_ptr(ret, call_type); } fn param_error

(call_ctx: &CallContext, index: i32, err: ConvertError) -> ! { diff --git a/godot-core/src/meta/traits.rs b/godot-core/src/meta/traits.rs index b536b38cb..8261003a9 100644 --- a/godot-core/src/meta/traits.rs +++ b/godot-core/src/meta/traits.rs @@ -41,21 +41,38 @@ pub trait GodotFfiVariant: Sized + GodotFfi { pub trait GodotType: GodotConvert + sealed::Sealed + Sized + 'static // 'static is not technically required, but it simplifies a few things (limits e.g. ObjectArg). { + // Value type for this type's FFI representation. #[doc(hidden)] type Ffi: GodotFfiVariant + 'static; + // Value or reference type when passing this type *to* Godot FFI. #[doc(hidden)] - fn to_ffi(&self) -> Self::Ffi; + type ToFfi<'f>: GodotFfiVariant + where + Self: 'f; + /// Returns the FFI representation of this type, used for argument passing. + /// + /// Often returns a reference to the value, which can then be used to interact with Godot without cloning/inc-ref-ing the value. + /// For scalars and `Copy` types, this usually returns a copy of the value. + #[doc(hidden)] + fn to_ffi(&self) -> Self::ToFfi<'_>; + + /// Consumes value and converts into FFI representation, used for return types. + /// + /// Unlike [`to_ffi()`][Self:to_ffi], this method consumes the value and is used for return types rather than argument passing. + /// Using `to_ffi()` for return types can be incorrect, since the associated types `Ffi` and `ToFfi<'f>` may differ and the latter + /// may not implement return type conversions such as [`GodotFfi::move_return_ptr()`]. #[doc(hidden)] fn into_ffi(self) -> Self::Ffi; + /// Converts from FFI representation to Rust type. #[doc(hidden)] fn try_from_ffi(ffi: Self::Ffi) -> Result; #[doc(hidden)] fn from_ffi(ffi: Self::Ffi) -> Self { - Self::try_from_ffi(ffi).unwrap() + Self::try_from_ffi(ffi).expect("Failed conversion from FFI representation to Rust type") } #[doc(hidden)] diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index f85cf8d0c..e7f44aacd 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -17,7 +17,7 @@ use crate::global::PropertyHint; use crate::meta::error::{ConvertError, FromFfiError}; use crate::meta::{ ArrayElement, CallContext, ClassName, FromGodot, GodotConvert, GodotType, PropertyHintInfo, - ToGodot, + RefArg, ToGodot, }; use crate::obj::{ bounds, cap, Bounds, EngineEnum, GdDerefTarget, GdMut, GdRef, GodotClass, Inherits, InstanceId, @@ -696,8 +696,11 @@ impl FromGodot for Gd { impl GodotType for Gd { type Ffi = RawGd; - fn to_ffi(&self) -> Self::Ffi { - self.raw.clone() + type ToFfi<'f> = RefArg<'f, RawGd> + where Self: 'f; + + fn to_ffi(&self) -> Self::ToFfi<'_> { + RefArg::new(&self.raw) } fn into_ffi(self) -> Self::Ffi { diff --git a/godot-core/src/obj/object_arg.rs b/godot-core/src/obj/object_arg.rs index 1736d973a..a59e6f342 100644 --- a/godot-core/src/obj/object_arg.rs +++ b/godot-core/src/obj/object_arg.rs @@ -270,8 +270,9 @@ impl FromGodot for ObjectArg { impl GodotType for ObjectArg { type Ffi = Self; + type ToFfi<'f> = Self; // TODO: maybe ObjectArg becomes redundant with RefArg? - fn to_ffi(&self) -> Self::Ffi { + fn to_ffi(&self) -> Self::ToFfi<'_> { (*self).clone() } @@ -305,8 +306,8 @@ impl GodotFfiVariant for ObjectArg { } impl GodotNullableFfi for ObjectArg { - fn flatten_option(opt: Option) -> Self { - opt.unwrap_or_else(Self::null) + fn null() -> Self { + Self::null() } fn is_null(&self) -> bool { diff --git a/godot-core/src/obj/raw_gd.rs b/godot-core/src/obj/raw_gd.rs index d7714a4ab..c9b2079b8 100644 --- a/godot-core/src/obj/raw_gd.rs +++ b/godot-core/src/obj/raw_gd.rs @@ -13,7 +13,7 @@ use sys::{interface_fn, GodotFfi, GodotNullableFfi, PtrcallType}; use crate::builtin::Variant; use crate::meta::error::{ConvertError, FromVariantError}; use crate::meta::{ - CallContext, ClassName, FromGodot, GodotConvert, GodotFfiVariant, GodotType, ToGodot, + CallContext, ClassName, FromGodot, GodotConvert, GodotFfiVariant, GodotType, RefArg, ToGodot, }; use crate::obj::bounds::{Declarer, DynMemory as _}; use crate::obj::rtti::ObjectRtti; @@ -39,15 +39,6 @@ pub struct RawGd { } impl RawGd { - /// Create a new object representing a null in Godot. - pub(super) fn null() -> Self { - Self { - obj: ptr::null_mut(), - cached_rtti: None, - cached_storage_ptr: InstanceCache::null(), - } - } - /// Initializes this `RawGd` from the object pointer as a **weak ref**, meaning it does not /// initialize/increment the reference counter. /// @@ -552,8 +543,11 @@ impl FromGodot for RawGd { impl GodotType for RawGd { type Ffi = Self; - fn to_ffi(&self) -> Self::Ffi { - self.clone() + type ToFfi<'f> = RefArg<'f, RawGd> + where Self: 'f; + + fn to_ffi(&self) -> Self::ToFfi<'_> { + RefArg::new(self) } fn into_ffi(self) -> Self::Ffi { @@ -602,8 +596,13 @@ impl GodotFfiVariant for RawGd { } impl GodotNullableFfi for RawGd { - fn flatten_option(opt: Option) -> Self { - opt.unwrap_or_else(Self::null) + /// Create a new object representing a null in Godot. + fn null() -> Self { + Self { + obj: ptr::null_mut(), + cached_rtti: None, + cached_storage_ptr: InstanceCache::null(), + } } fn is_null(&self) -> bool { diff --git a/godot-ffi/src/godot_ffi.rs b/godot-ffi/src/godot_ffi.rs index 99a5477f1..e613344f1 100644 --- a/godot-ffi/src/godot_ffi.rs +++ b/godot-ffi/src/godot_ffi.rs @@ -109,8 +109,13 @@ pub unsafe trait GodotFfi { /// This is currently only implemented for `RawGd`. // TODO: Consider implementing for `Variant`. pub trait GodotNullableFfi: Sized + GodotFfi { - fn flatten_option(opt: Option) -> Self; + fn null() -> Self; + fn is_null(&self) -> bool; + + fn flatten_option(opt: Option) -> Self { + opt.unwrap_or_else(Self::null) + } } // ---------------------------------------------------------------------------------------------------------------------------------------------- diff --git a/itest/rust/src/object_tests/object_test.rs b/itest/rust/src/object_tests/object_test.rs index fc6383312..4e4f488fb 100644 --- a/itest/rust/src/object_tests/object_test.rs +++ b/itest/rust/src/object_tests/object_test.rs @@ -68,7 +68,9 @@ fn object_user_roundtrip_write() { let obj: Gd = Gd::from_object(user); assert_eq!(obj.bind().value, value); - let raw = obj.to_ffi(); + + // Use into_ffi() instead of to_ffi(), as the latter returns a reference and isn't used for returns anymore. + let raw = obj.into_ffi(); let raw2 = unsafe { RawGd::::new_with_uninit(|ptr| { @@ -96,6 +98,20 @@ fn object_engine_roundtrip() { obj.free(); } +#[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. + + let null_obj = Option::>::None; + + let via = null_obj.to_godot(); + let ffi = via.to_ffi(); + + expect_panic("not yet implemented: pass objects through RefArg", || { + ffi.to_godot(); + }); +} + #[itest] fn object_user_display() { let obj = Gd::from_object(RefcPayload { value: 774 }); From eeec4d11d852996288fb0ffd5b478008a621baeb Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Mon, 23 Sep 2024 19:35:06 +0200 Subject: [PATCH 2/3] Fix incorrect link in deprecated utilities --- godot-core/src/global/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/godot-core/src/global/mod.rs b/godot-core/src/global/mod.rs index b8525a17c..ce8d72699 100644 --- a/godot-core/src/global/mod.rs +++ b/godot-core/src/global/mod.rs @@ -42,19 +42,19 @@ use crate::obj::Gd; // Reminder: remove #![allow(deprecated)] in utilities.test along with the below functions. #[deprecated = "Instance utilities in `godot::global` will be removed. Use methods on `Gd` and `InstanceId` instead.\n\ - For detailed reasons, see https://github.com/godot-rust/gdext/pull/892."] + For detailed reasons, see https://github.com/godot-rust/gdext/pull/901."] pub fn instance_from_id(instance_id: i64) -> Option> { crate::gen::utilities::instance_from_id(instance_id) } #[deprecated = "Instance utilities in `godot::global` will be removed. Use methods on `Gd` and `InstanceId` instead.\n\ - For detailed reasons, see https://github.com/godot-rust/gdext/pull/892."] + For detailed reasons, see https://github.com/godot-rust/gdext/pull/901."] pub fn is_instance_valid(instance: Variant) -> bool { crate::gen::utilities::is_instance_valid(&instance) } #[deprecated = "Instance utilities in `godot::global` will be removed. Use methods on `Gd` and `InstanceId` instead.\n\ - For detailed reasons, see https://github.com/godot-rust/gdext/pull/892."] + For detailed reasons, see https://github.com/godot-rust/gdext/pull/901."] pub fn is_instance_id_valid(instance_id: i64) -> bool { crate::gen::utilities::is_instance_id_valid(instance_id) } From d8264bd72c5c25a2f8432e1bc0a604d209dc1d93 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Mon, 23 Sep 2024 22:47:06 +0200 Subject: [PATCH 3/3] Update markdown crate and use = version specifier Also clean up some formatting in conversion code. --- godot-macros/Cargo.toml | 2 +- godot-macros/src/docs/markdown_converter.rs | 47 ++++++++++++++------- 2 files changed, 33 insertions(+), 16 deletions(-) diff --git a/godot-macros/Cargo.toml b/godot-macros/Cargo.toml index bc0cd28d5..b5df22634 100644 --- a/godot-macros/Cargo.toml +++ b/godot-macros/Cargo.toml @@ -24,7 +24,7 @@ proc-macro2 = "1.0.80" # Literal::c_string() added in 1.0.80. quote = "1.0.29" # Enabled by `docs` -markdown = { version = "1.0.0-alpha.19", optional = true } +markdown = { version = "=1.0.0-alpha.21", optional = true } venial = "0.6" [build-dependencies] diff --git a/godot-macros/src/docs/markdown_converter.rs b/godot-macros/src/docs/markdown_converter.rs index 0cca3ff17..7b93a2236 100644 --- a/godot-macros/src/docs/markdown_converter.rs +++ b/godot-macros/src/docs/markdown_converter.rs @@ -7,19 +7,20 @@ //! Converts [Markdown](https://en.wikipedia.org/wiki/Markdown) to [BBCode](https://en.wikipedia.org/wiki/BBCode). -use markdown::mdast::Node; +use markdown::mdast as md; use markdown::{to_mdast, ParseOptions}; use std::collections::HashMap; pub fn to_bbcode(md: &str) -> String { - // to_mdast() never errors with normal arkdown, so unwrap is safe. + // to_mdast() never errors with normal Markdown, so unwrap is safe. let n = to_mdast(md, &ParseOptions::gfm()).unwrap(); + let definitions = n .children() .unwrap() // root node always has children .iter() .filter_map(|n| match n { - Node::Definition(definition) => Some((&*definition.identifier, &*definition.url)), + md::Node::Definition(def) => Some((&*def.identifier, &*def.url)), _ => None, }) .collect::>(); @@ -27,24 +28,32 @@ pub fn to_bbcode(md: &str) -> String { walk_node(&n, &definitions).unwrap_or_default() } -fn walk_node(node: &Node, definitions: &HashMap<&str, &str>) -> Option { - use Node::*; +fn walk_node(node: &md::Node, definitions: &HashMap<&str, &str>) -> Option { + use md::Node::*; + let bbcode = match node { Root(root) => walk_nodes(&root.children, definitions, "[br][br]"), - InlineCode(markdown::mdast::InlineCode { value, .. }) => format!("[code]{value}[/code]"), + + InlineCode(md::InlineCode { value, .. }) => format!("[code]{value}[/code]"), + Delete(delete) => format!("[s]{}[/s]", walk_nodes(&delete.children, definitions, "")), + Emphasis(emphasis) => format!("[i]{}[/i]", walk_nodes(&emphasis.children, definitions, "")), - Image(markdown::mdast::Image { url, .. }) => format!("[img]{url}[/img]",), + + Image(md::Image { url, .. }) => format!("[img]{url}[/img]",), + ImageReference(image) => { format!( "[img]{}[/img]", definitions.get(&&*image.identifier).unwrap() ) } - Link(markdown::mdast::Link { url, children, .. }) => { + + Link(md::Link { url, children, .. }) => { format!("[url={url}]{}[/url]", walk_nodes(children, definitions, "")) } - LinkReference(markdown::mdast::LinkReference { + + LinkReference(md::LinkReference { identifier, children, .. @@ -53,23 +62,31 @@ fn walk_node(node: &Node, definitions: &HashMap<&str, &str>) -> Option { definitions.get(&&**identifier).unwrap(), walk_nodes(children, definitions, "") ), + Strong(strong) => format!("[b]{}[/b]", walk_nodes(&strong.children, definitions, "")), + Text(text) => text.value.clone(), + // TODO: more langs? - Code(markdown::mdast::Code { value, .. }) => format!("[codeblock]{value}[/codeblock]"), + Code(md::Code { value, .. }) => format!("[codeblock]{value}[/codeblock]"), + Paragraph(paragraph) => walk_nodes(¶graph.children, definitions, ""), - // bbcode supports lists but docs dont - List(_) | BlockQuote(_) | FootnoteReference(_) | FootnoteDefinition(_) | Table(_) => { - "".into() + + // BBCode supports lists, but docs don't. + List(_) | Blockquote(_) | FootnoteReference(_) | FootnoteDefinition(_) | Table(_) => { + String::new() } + Html(html) => html.value.clone(), + _ => walk_nodes(&node.children()?, definitions, ""), }; + Some(bbcode) } -/// Calls [`walk_node`] over every node its given, joining them with the supplied separator. -fn walk_nodes(nodes: &[Node], definitions: &HashMap<&str, &str>, separator: &str) -> String { +/// Calls [`walk_node`] over every node it receives, joining them with the supplied separator. +fn walk_nodes(nodes: &[md::Node], definitions: &HashMap<&str, &str>, separator: &str) -> String { nodes .iter() .filter_map(|n| walk_node(n, definitions))