From bd76361a29a895ec522b78b052ae4b206fb45c8d Mon Sep 17 00:00:00 2001 From: Lili Zoey Zyli Date: Sat, 9 Mar 2024 19:13:23 +0100 Subject: [PATCH 1/9] Rename `GodotFfi` methods Update `ffi_methods` to account for that Replace `AsUninit` with `SysPtr` Add `borrow_string_sys` to `StringName` --- godot-codegen/src/generator/builtins.rs | 4 +- godot-core/src/builtin/array.rs | 41 ++---- godot-core/src/builtin/callable.rs | 22 ++- godot-core/src/builtin/dictionary.rs | 36 ++--- godot-core/src/builtin/macros.rs | 10 +- godot-core/src/builtin/meta/class_name.rs | 2 +- godot-core/src/builtin/meta/mod.rs | 10 +- .../src/builtin/meta/registration/method.rs | 9 +- godot-core/src/builtin/meta/return_marshal.rs | 8 +- godot-core/src/builtin/meta/signature.rs | 10 +- godot-core/src/builtin/packed_array.rs | 30 +--- godot-core/src/builtin/plane.rs | 15 +- godot-core/src/builtin/rid.rs | 15 +- godot-core/src/builtin/signal.rs | 19 ++- godot-core/src/builtin/string/gstring.rs | 32 ++--- godot-core/src/builtin/string/node_path.rs | 21 +-- godot-core/src/builtin/string/string_name.rs | 36 ++--- godot-core/src/builtin/variant/impls.rs | 6 +- godot-core/src/builtin/variant/mod.rs | 53 +++---- godot-core/src/engine/script_instance.rs | 31 +--- godot-core/src/init/mod.rs | 4 +- godot-core/src/log.rs | 12 +- godot-core/src/obj/gd.rs | 2 +- godot-core/src/obj/raw.rs | 28 +++- godot-core/src/registry/callbacks.rs | 17 +-- godot-ffi/src/extras.rs | 69 ++++++--- godot-ffi/src/godot_ffi.rs | 135 ++++++++++-------- .../builtin_tests/containers/variant_test.rs | 4 +- itest/rust/src/object_tests/object_test.rs | 8 +- .../src/register_tests/option_ffi_test.rs | 4 +- 30 files changed, 317 insertions(+), 376 deletions(-) diff --git a/godot-codegen/src/generator/builtins.rs b/godot-codegen/src/generator/builtins.rs index 9fc67c6a6..6aa1eb0e9 100644 --- a/godot-codegen/src/generator/builtins.rs +++ b/godot-codegen/src/generator/builtins.rs @@ -117,7 +117,7 @@ fn make_builtin_class(class: &BuiltinClass, ctx: &mut Context) -> GeneratedBuilt pub fn from_outer(outer: &#outer_class) -> Self { Self { _outer_lifetime: std::marker::PhantomData, - sys_ptr: outer.sys(), + sys_ptr: sys::SysPtr::force_mut(outer.sys()), } } #special_constructors @@ -154,7 +154,7 @@ fn make_special_builtin_methods(class_name: &TyName, _ctx: &Context) -> TokenStr { Self { _outer_lifetime: std::marker::PhantomData, - sys_ptr: outer.sys(), + sys_ptr: sys::SysPtr::force_mut(outer.sys()), } } } diff --git a/godot-core/src/builtin/array.rs b/godot-core/src/builtin/array.rs index 269a084ba..7a1488275 100644 --- a/godot-core/src/builtin/array.rs +++ b/godot-core/src/builtin/array.rs @@ -253,7 +253,7 @@ impl Array { /// # Panics /// /// If `index` is out of bounds. - fn ptr_mut(&self, index: usize) -> *mut Variant { + fn ptr_mut(&mut self, index: usize) -> *mut Variant { let ptr = self.ptr_mut_or_null(index); assert!( !ptr.is_null(), @@ -264,11 +264,11 @@ impl Array { } /// Returns a pointer to the element at the given index, or null if out of bounds. - fn ptr_mut_or_null(&self, index: usize) -> *mut Variant { + fn ptr_mut_or_null(&mut self, index: usize) -> *mut Variant { // SAFETY: array_operator_index returns null for invalid indexes. let variant_ptr = unsafe { let index = to_i64(index); - interface_fn!(array_operator_index)(self.sys(), index) + interface_fn!(array_operator_index)(self.sys_mut(), index) }; Variant::ptr_from_sys_mut(variant_ptr) @@ -456,7 +456,7 @@ impl Array { // SAFETY: The array is a newly created empty untyped array. unsafe { interface_fn!(array_set_typed)( - self.sys(), + self.sys_mut(), type_info.variant_type.sys(), type_info.class_name.string_sys(), script.var_sys(), @@ -744,24 +744,7 @@ unsafe impl GodotFfi for Array { sys::VariantType::Array } - ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; - fn from_sys; - fn sys; - fn from_sys_init; - fn move_return_ptr; - } - - unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self { - let mut result = Self::default(); - init_fn(result.sys_mut()); - result - } - - 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 - } + ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. } } impl GodotConvert for Array { @@ -825,9 +808,9 @@ impl Clone for Array { fn clone(&self) -> Self { // SAFETY: `self` is a valid array, since we have a reference that keeps it alive. let array = unsafe { - Self::from_sys_init(|self_ptr| { + Self::new_with_uninit(|self_ptr| { let ctor = ::godot_ffi::builtin_fn!(array_construct_copy); - let args = [self.sys_const()]; + let args = [self.sys()]; ctor(self_ptr, args.as_ptr()); }) }; @@ -891,7 +874,7 @@ impl Default for Array { #[inline] fn default() -> Self { let mut array = unsafe { - Self::from_sys_init(|self_ptr| { + Self::new_with_uninit(|self_ptr| { let ctor = sys::builtin_fn!(array_construct_default); ctor(self_ptr, std::ptr::null_mut()) }) @@ -937,9 +920,9 @@ impl GodotType for Array { impl GodotFfiVariant for Array { fn ffi_to_variant(&self) -> Variant { unsafe { - Variant::from_var_sys_init(|variant_ptr| { + Variant::new_with_var_uninit(|variant_ptr| { let array_to_variant = sys::builtin_fn!(array_to_variant); - array_to_variant(variant_ptr, self.sys()); + array_to_variant(variant_ptr, sys::SysPtr::force_mut(self.sys())); }) } } @@ -956,7 +939,7 @@ impl GodotFfiVariant for Array { let array = unsafe { sys::from_sys_init_or_init_default::(|self_ptr| { let array_from_variant = sys::builtin_fn!(array_from_variant); - array_from_variant(self_ptr, variant.var_sys()); + array_from_variant(self_ptr, sys::SysPtr::force_mut(variant.var_sys())); }) }; @@ -977,7 +960,7 @@ impl From<&[T; N]> for Array { /// Creates a `Array` from the given slice. impl From<&[T]> for Array { fn from(slice: &[T]) -> Self { - let array = Self::new(); + let mut array = Self::new(); let len = slice.len(); if len == 0 { return array; diff --git a/godot-core/src/builtin/callable.rs b/godot-core/src/builtin/callable.rs index 6be70c97a..b8a33e272 100644 --- a/godot-core/src/builtin/callable.rs +++ b/godot-core/src/builtin/callable.rs @@ -51,7 +51,7 @@ impl Callable { sys::from_sys_init_or_init_default::(|self_ptr| { let ctor = sys::builtin_fn!(callable_from_object_method); let raw = object.to_ffi(); - let args = [raw.as_arg_ptr(), method.sys_const()]; + let args = [raw.as_arg_ptr(), method.sys()]; ctor(self_ptr, args.as_ptr()); }) } @@ -140,7 +140,7 @@ impl Callable { fn from_custom_info(mut info: sys::GDExtensionCallableCustomInfo) -> Callable { // SAFETY: callable_custom_create() is a valid way of creating callables. unsafe { - Callable::from_sys_init(|type_ptr| { + Callable::new_with_uninit(|type_ptr| { sys::interface_fn!(callable_custom_create)(type_ptr, ptr::addr_of_mut!(info)) }) } @@ -151,7 +151,7 @@ impl Callable { /// _Godot equivalent: `Callable()`_ pub fn invalid() -> Self { unsafe { - Self::from_sys_init(|self_ptr| { + Self::new_with_uninit(|self_ptr| { let ctor = sys::builtin_fn!(callable_construct_default); ctor(self_ptr, ptr::null_mut()) }) @@ -300,21 +300,17 @@ unsafe impl GodotFfi for Callable { } ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; - fn from_sys; + fn new_from_sys; fn sys; - fn from_sys_init; + fn sys_mut; + fn new_with_uninit; fn move_return_ptr; + fn from_arg_ptr; } - unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::PtrcallType) -> Self { - let callable = Self::from_sys(ptr); - callable.inc_ref(); - callable - } - - unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self { + unsafe fn new_with_init(init_fn: impl FnOnce(&mut Self)) -> Self { let mut result = Self::invalid(); - init_fn(result.sys_mut()); + init_fn(&mut result); result } } diff --git a/godot-core/src/builtin/dictionary.rs b/godot-core/src/builtin/dictionary.rs index a5b54b821..2af4c73d8 100644 --- a/godot-core/src/builtin/dictionary.rs +++ b/godot-core/src/builtin/dictionary.rs @@ -13,7 +13,7 @@ use crate::property::{Export, PropertyHintInfo, TypeStringHint, Var}; use std::marker::PhantomData; use std::{fmt, ptr}; use sys::types::OpaqueDictionary; -use sys::{ffi_methods, interface_fn, AsUninit, GodotFfi}; +use sys::{ffi_methods, interface_fn, GodotFfi}; use super::meta::impl_godot_as_self; @@ -244,9 +244,8 @@ impl Dictionary { let key = key.to_variant(); // SAFETY: accessing an unknown key _mutably_ creates that entry in the dictionary, with value `NIL`. - let ptr = unsafe { - interface_fn!(dictionary_operator_index)(self.sys_mut(), key.var_sys_const()) - }; + let ptr = + unsafe { interface_fn!(dictionary_operator_index)(self.sys_mut(), key.var_sys()) }; // Never a null pointer, since entry either existed already or was inserted above. Variant::ptr_from_sys_mut(ptr) @@ -270,24 +269,7 @@ unsafe impl GodotFfi for Dictionary { sys::VariantType::Dictionary } - ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; - fn from_sys; - fn from_sys_init; - fn sys; - fn move_return_ptr; - } - - unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self { - let mut result = Self::default(); - init_fn(result.sys_mut()); - result - } - - unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::PtrcallType) -> Self { - let dictionary = Self::from_sys(ptr); - std::mem::forget(dictionary.clone()); - dictionary - } + ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. } } impl_godot_as_self!(Dictionary); @@ -331,9 +313,9 @@ impl Clone for Dictionary { fn clone(&self) -> Self { // SAFETY: `self` is a valid dictionary, since we have a reference that keeps it alive. unsafe { - Self::from_sys_init(|self_ptr| { + Self::new_with_uninit(|self_ptr| { let ctor = sys::builtin_fn!(dictionary_construct_copy); - let args = [self.sys_const()]; + let args = [self.sys()]; ctor(self_ptr, args.as_ptr()); }) } @@ -459,7 +441,7 @@ impl<'a> DictionaryIter<'a> { fn call_init(dictionary: &Dictionary) -> Option { let variant: Variant = Variant::nil(); let iter_fn = |dictionary, next_value: sys::GDExtensionVariantPtr, valid| unsafe { - interface_fn!(variant_iter_init)(dictionary, next_value.as_uninit(), valid) + interface_fn!(variant_iter_init)(dictionary, sys::SysPtr::as_uninit(next_value), valid) }; Self::ffi_iterate(iter_fn, dictionary, variant) @@ -484,7 +466,7 @@ impl<'a> DictionaryIter<'a> { *mut sys::GDExtensionBool, ) -> sys::GDExtensionBool, dictionary: &Dictionary, - next_value: Variant, + mut next_value: Variant, ) -> Option { let dictionary = dictionary.to_variant(); let mut valid_u8: u8 = 0; @@ -496,7 +478,7 @@ impl<'a> DictionaryIter<'a> { let has_next = unsafe { iter_fn( dictionary.var_sys(), - next_value.var_sys(), + next_value.var_sys_mut(), ptr::addr_of_mut!(valid_u8), ) }; diff --git a/godot-core/src/builtin/macros.rs b/godot-core/src/builtin/macros.rs index 432936667..a55e7d9ff 100644 --- a/godot-core/src/builtin/macros.rs +++ b/godot-core/src/builtin/macros.rs @@ -13,7 +13,7 @@ macro_rules! impl_builtin_traits_inner { #[inline] fn default() -> Self { unsafe { - Self::from_sys_init(|self_ptr| { + Self::new_with_uninit(|self_ptr| { let ctor = ::godot_ffi::builtin_fn!($gd_method); ctor(self_ptr, std::ptr::null_mut()) }) @@ -27,9 +27,9 @@ macro_rules! impl_builtin_traits_inner { #[inline] fn clone(&self) -> Self { unsafe { - Self::from_sys_init(|self_ptr| { + Self::new_with_uninit(|self_ptr| { let ctor = ::godot_ffi::builtin_fn!($gd_method); - let args = [self.sys_const()]; + let args = [self.sys()]; ctor(self_ptr, args.as_ptr()); }) } @@ -129,8 +129,8 @@ macro_rules! impl_builtin_froms { fn from(other: &$From) -> Self { unsafe { // TODO should this be from_sys_init_default()? - Self::from_sys_init(|ptr| { - let args = [other.sys_const()]; + Self::new_with_uninit(|ptr| { + let args = [other.sys()]; ::godot_ffi::builtin_call! { $from_fn(ptr, args.as_ptr()) } diff --git a/godot-core/src/builtin/meta/class_name.rs b/godot-core/src/builtin/meta/class_name.rs index cd71ff669..dcf1da534 100644 --- a/godot-core/src/builtin/meta/class_name.rs +++ b/godot-core/src/builtin/meta/class_name.rs @@ -82,7 +82,7 @@ impl ClassName { /// The returned pointer is valid indefinitely, as entries are never deleted from the cache. /// Since we use `Box`, `HashMap` reallocations don't affect the validity of the StringName. #[doc(hidden)] - pub fn string_sys(&self) -> sys::GDExtensionStringNamePtr { + pub fn string_sys(&self) -> sys::GDExtensionConstStringNamePtr { self.with_string_name(|s| s.string_sys()) } diff --git a/godot-core/src/builtin/meta/mod.rs b/godot-core/src/builtin/meta/mod.rs index ff5a5715b..185989817 100644 --- a/godot-core/src/builtin/meta/mod.rs +++ b/godot-core/src/builtin/meta/mod.rs @@ -265,10 +265,10 @@ impl PropertyInfo { sys::GDExtensionPropertyInfo { type_: self.variant_type.sys(), - name: self.property_name.string_sys(), - class_name: self.class_name.string_sys(), + name: sys::SysPtr::force_mut(self.property_name.string_sys()), + class_name: sys::SysPtr::force_mut(self.class_name.string_sys()), hint: u32::try_from(self.hint.ord()).expect("hint.ord()"), - hint_string: self.hint_string.string_sys(), + hint_string: sys::SysPtr::force_mut(self.hint_string.string_sys()), usage: u32::try_from(self.usage.ord()).expect("usage.ord()"), } } @@ -328,7 +328,7 @@ impl MethodInfo { let default_argument_vec = self .default_arguments .iter() - .map(|arg| arg.var_sys()) + .map(|arg| sys::SysPtr::force_mut(arg.var_sys())) .collect::>() .into_boxed_slice(); @@ -337,7 +337,7 @@ impl MethodInfo { sys::GDExtensionMethodInfo { id: self.id, - name: self.method_name.string_sys(), + name: sys::SysPtr::force_mut(self.method_name.string_sys()), return_value: self.return_type.property_sys(), argument_count, arguments, diff --git a/godot-core/src/builtin/meta/registration/method.rs b/godot-core/src/builtin/meta/registration/method.rs index b728b84bf..a0eae0ace 100644 --- a/godot-core/src/builtin/meta/registration/method.rs +++ b/godot-core/src/builtin/meta/registration/method.rs @@ -118,11 +118,14 @@ impl ClassMethodInfo { let mut arguments_metadata: Vec = self.arguments.iter().map(|info| info.metadata).collect(); - let mut default_arguments_sys: Vec = - self.default_arguments.iter().map(|v| v.var_sys()).collect(); + let mut default_arguments_sys: Vec = self + .default_arguments + .iter() + .map(|v| sys::SysPtr::force_mut(v.var_sys())) + .collect(); let method_info_sys = sys::GDExtensionClassMethodInfo { - name: self.method_name.string_sys(), + name: sys::SysPtr::force_mut(self.method_name.string_sys()), method_userdata: std::ptr::null_mut(), call_func: self.call_func, ptrcall_func: self.ptrcall_func, diff --git a/godot-core/src/builtin/meta/return_marshal.rs b/godot-core/src/builtin/meta/return_marshal.rs index 53f21f3d4..213dd5d43 100644 --- a/godot-core/src/builtin/meta/return_marshal.rs +++ b/godot-core/src/builtin/meta/return_marshal.rs @@ -50,10 +50,10 @@ impl PtrcallReturn for PtrcallReturnT { unsafe fn call( mut process_return_ptr: impl FnMut(sys::GDExtensionTypePtr), ) -> Result { - let ffi = - <::Ffi as sys::GodotFfi>::from_sys_init_default(|return_ptr| { - process_return_ptr(return_ptr) - }); + let ffi = <::Ffi as sys::GodotFfi>::new_with_init(|return_val| { + let return_ptr = sys::GodotFfi::sys_mut(return_val); + process_return_ptr(return_ptr) + }); T::Via::try_from_ffi(ffi).and_then(T::try_from_godot) } diff --git a/godot-core/src/builtin/meta/signature.rs b/godot-core/src/builtin/meta/signature.rs index 1c00a2a7d..00c3e9392 100644 --- a/godot-core/src/builtin/meta/signature.rs +++ b/godot-core/src/builtin/meta/signature.rs @@ -222,10 +222,10 @@ macro_rules! impl_varcall_signature_for_tuple { ]; let mut variant_ptrs = Vec::with_capacity(explicit_args.len() + varargs.len()); - variant_ptrs.extend(explicit_args.iter().map(Variant::var_sys_const)); - variant_ptrs.extend(varargs.iter().map(Variant::var_sys_const)); + variant_ptrs.extend(explicit_args.iter().map(Variant::var_sys)); + variant_ptrs.extend(varargs.iter().map(Variant::var_sys)); - let variant: Result = Variant::from_var_sys_init_result(|return_ptr| { + let variant: Result = Variant::new_with_var_uninit_result(|return_ptr| { let mut err = sys::default_call_error(); class_fn( method_bind.0, @@ -302,8 +302,8 @@ macro_rules! impl_varcall_signature_for_tuple { ]; let mut type_ptrs = Vec::with_capacity(explicit_args.len() + varargs.len()); - type_ptrs.extend(explicit_args.iter().map(sys::GodotFfi::sys_const)); - type_ptrs.extend(varargs.iter().map(sys::GodotFfi::sys_const)); + type_ptrs.extend(explicit_args.iter().map(sys::GodotFfi::sys)); + type_ptrs.extend(varargs.iter().map(sys::GodotFfi::sys)); // Important: this calls from_sys_init_default(). let result = PtrcallReturnT::<$R>::call(|return_ptr| { diff --git a/godot-core/src/builtin/packed_array.rs b/godot-core/src/builtin/packed_array.rs index f8d78594f..2807337e1 100644 --- a/godot-core/src/builtin/packed_array.rs +++ b/godot-core/src/builtin/packed_array.rs @@ -362,13 +362,13 @@ macro_rules! impl_packed_array { /// # Panics /// /// If `index` is out of bounds. - fn ptr_mut(&self, index: usize) -> *mut $Element { + fn ptr_mut(&mut self, index: usize) -> *mut $Element { self.check_bounds(index); // SAFETY: We just checked that the index is not out of bounds. let ptr = unsafe { let item_ptr: *mut $IndexRetType = - (interface_fn!($operator_index))(self.sys(), to_i64(index)); + (interface_fn!($operator_index))(self.sys_mut(), to_i64(index)); item_ptr as *mut $Element }; assert!(!ptr.is_null()); @@ -475,31 +475,7 @@ macro_rules! impl_packed_array { sys::VariantType::$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(); - init_fn(result.sys_mut()); - result - } + ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. } } $crate::builtin::meta::impl_godot_as_self!($PackedArray); diff --git a/godot-core/src/builtin/plane.rs b/godot-core/src/builtin/plane.rs index 3630cdf6d..77864d1d3 100644 --- a/godot-core/src/builtin/plane.rs +++ b/godot-core/src/builtin/plane.rs @@ -267,7 +267,20 @@ unsafe impl GodotFfi for Plane { sys::VariantType::Plane } - ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } + ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; + fn new_from_sys; + fn sys; + fn sys_mut; + fn new_with_uninit; + fn from_arg_ptr; + fn move_return_ptr; + } + + unsafe fn new_with_init(init: impl FnOnce(&mut Self)) -> Self { + let mut plane = Plane::new(Vector3::UP, 0.0); + init(&mut plane); + plane + } } impl_godot_as_self!(Plane); diff --git a/godot-core/src/builtin/rid.rs b/godot-core/src/builtin/rid.rs index cc3da542f..933fa66d5 100644 --- a/godot-core/src/builtin/rid.rs +++ b/godot-core/src/builtin/rid.rs @@ -123,7 +123,20 @@ unsafe impl GodotFfi for Rid { sys::VariantType::Rid } - ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } + ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; + fn new_from_sys; + fn sys; + fn sys_mut; + fn new_with_uninit; + fn from_arg_ptr; + fn move_return_ptr; + } + + unsafe fn new_with_init(init: impl FnOnce(&mut Self)) -> Self { + let mut rid = Self::Invalid; + init(&mut rid); + rid + } } impl_godot_as_self!(Rid); diff --git a/godot-core/src/builtin/signal.rs b/godot-core/src/builtin/signal.rs index 906e18301..3ae76bc15 100644 --- a/godot-core/src/builtin/signal.rs +++ b/godot-core/src/builtin/signal.rs @@ -44,7 +44,7 @@ impl Signal { sys::from_sys_init_or_init_default::(|self_ptr| { let ctor = sys::builtin_fn!(signal_from_object_signal); let raw = object.to_ffi(); - let args = [raw.as_arg_ptr(), signal_name.sys_const()]; + let args = [raw.as_arg_ptr(), signal_name.sys()]; ctor(self_ptr, args.as_ptr()); }) } @@ -55,7 +55,7 @@ impl Signal { /// _Godot equivalent: `Signal()`_ pub fn invalid() -> Self { unsafe { - Self::from_sys_init(|self_ptr| { + Self::new_with_uninit(|self_ptr| { let ctor = sys::builtin_fn!(signal_construct_default); ctor(self_ptr, ptr::null_mut()) }) @@ -163,20 +163,17 @@ unsafe impl GodotFfi for Signal { } ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; - fn from_sys; + fn new_from_sys; fn sys; - fn from_sys_init; + fn sys_mut; + fn new_with_uninit; fn move_return_ptr; + fn from_arg_ptr; } - unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::PtrcallType) -> Self { - Self::from_sys(ptr) - } - - #[cfg(before_api = "4.1")] - unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self { + unsafe fn new_with_init(init_fn: impl FnOnce(&mut Self)) -> Self { let mut result = Self::invalid(); - init_fn(result.sys_mut()); + init_fn(&mut result); result } } diff --git a/godot-core/src/builtin/string/gstring.rs b/godot-core/src/builtin/string/gstring.rs index 435a76560..485397daa 100644 --- a/godot-core/src/builtin/string/gstring.rs +++ b/godot-core/src/builtin/string/gstring.rs @@ -123,11 +123,12 @@ impl GString { } ffi_methods! { - type sys::GDExtensionStringPtr = *mut Opaque; + type sys::GDExtensionStringPtr = *mut Self; - fn from_string_sys = from_sys; - fn from_string_sys_init = from_sys_init; + fn new_from_string_sys = new_from_sys; + fn new_with_string_uninit = new_with_uninit; fn string_sys = sys; + fn string_sys_mut = sys_mut; } /// Move `self` into a system pointer. This transfers ownership and thus does not call the destructor. @@ -158,24 +159,7 @@ unsafe impl GodotFfi for GString { sys::VariantType::String } - 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 string = Self::from_sys(ptr); - std::mem::forget(string.clone()); - string - } - - unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self { - let mut result = Self::default(); - init_fn(result.sys_mut()); - result - } + ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; ..} } impl_godot_as_self!(GString); @@ -217,7 +201,7 @@ where let bytes = s.as_ref().as_bytes(); unsafe { - Self::from_string_sys_init(|string_ptr| { + Self::new_with_string_uninit(|string_ptr| { let ctor = interface_fn!(string_new_with_utf8_chars_and_len); ctor( string_ptr, @@ -275,7 +259,7 @@ impl From<&StringName> for GString { unsafe { sys::from_sys_init_or_init_default::(|self_ptr| { let ctor = sys::builtin_fn!(string_from_string_name); - let args = [string.sys_const()]; + let args = [string.sys()]; ctor(self_ptr, args.as_ptr()); }) } @@ -296,7 +280,7 @@ impl From<&NodePath> for GString { unsafe { sys::from_sys_init_or_init_default::(|self_ptr| { let ctor = sys::builtin_fn!(string_from_node_path); - let args = [path.sys_const()]; + let args = [path.sys()]; ctor(self_ptr, args.as_ptr()); }) } diff --git a/godot-core/src/builtin/string/node_path.rs b/godot-core/src/builtin/string/node_path.rs index da3ef2068..7d27c7cc8 100644 --- a/godot-core/src/builtin/string/node_path.rs +++ b/godot-core/src/builtin/string/node_path.rs @@ -58,24 +58,7 @@ unsafe impl GodotFfi for NodePath { sys::VariantType::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(); - init_fn(result.sys_mut()); - result - } + ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. } } impl_godot_as_self!(NodePath); @@ -126,7 +109,7 @@ impl From<&GString> for NodePath { unsafe { sys::from_sys_init_or_init_default::(|self_ptr| { let ctor = sys::builtin_fn!(node_path_from_string); - let args = [string.sys_const()]; + let args = [string.sys()]; ctor(self_ptr, args.as_ptr()); }) } diff --git a/godot-core/src/builtin/string/string_name.rs b/godot-core/src/builtin/string/string_name.rs index 4ce8508b4..4a7fe1e92 100644 --- a/godot-core/src/builtin/string/string_name.rs +++ b/godot-core/src/builtin/string/string_name.rs @@ -59,7 +59,7 @@ impl StringName { // SAFETY: latin1_c_str is nul-terminated and remains valid for entire program duration. let result = unsafe { - Self::from_string_sys_init(|ptr| { + Self::new_with_string_uninit(|ptr| { sys::interface_fn!(string_name_new_with_latin1_chars)( ptr, c_str.as_ptr(), @@ -115,14 +115,15 @@ impl StringName { type sys::GDExtensionStringNamePtr = *mut Opaque; // Note: unlike from_sys, from_string_sys does not default-construct instance first. Typical usage in C++ is placement new. - fn from_string_sys = from_sys; - fn from_string_sys_init = from_sys_init; + fn new_from_string_sys = new_from_sys; + fn new_with_string_uninit = new_with_uninit; fn string_sys = sys; + fn string_sys_mut = sys_mut; } - #[doc(hidden)] - pub fn string_sys_const(&self) -> sys::GDExtensionConstStringNamePtr { - sys::to_const_ptr(self.string_sys()) + pub unsafe fn borrow_string_sys<'a>(ptr: sys::GDExtensionConstStringNamePtr) -> &'a StringName { + sys::static_assert_eq_size!(StringName, sys::types::OpaqueStringName); + &*(ptr.cast::()) } #[doc(hidden)] @@ -150,24 +151,7 @@ unsafe impl GodotFfi for StringName { sys::VariantType::StringName } - 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 string_name = Self::from_sys(ptr); - string_name.inc_ref(); - string_name - } - - unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self { - let mut result = Self::default(); - init_fn(result.sys_mut()); - result - } + ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. } } impl_godot_as_self!(StringName); @@ -227,7 +211,7 @@ where // SAFETY: Rust guarantees validity and range of string. unsafe { - Self::from_string_sys_init(|ptr| { + Self::new_with_string_uninit(|ptr| { sys::interface_fn!(string_name_new_with_utf8_chars_and_len)( ptr, utf8.as_ptr() as *const std::ffi::c_char, @@ -243,7 +227,7 @@ impl From<&GString> for StringName { unsafe { sys::from_sys_init_or_init_default::(|self_ptr| { let ctor = sys::builtin_fn!(string_name_from_string); - let args = [string.sys_const()]; + let args = [string.sys()]; ctor(self_ptr, args.as_ptr()); }) } diff --git a/godot-core/src/builtin/variant/impls.rs b/godot-core/src/builtin/variant/impls.rs index ac963a247..c8d4c3019 100644 --- a/godot-core/src/builtin/variant/impls.rs +++ b/godot-core/src/builtin/variant/impls.rs @@ -26,9 +26,9 @@ macro_rules! impl_ffi_variant { impl GodotFfiVariant for $T { fn ffi_to_variant(&self) -> Variant { let variant = unsafe { - Variant::from_var_sys_init(|variant_ptr| { + Variant::new_with_var_uninit(|variant_ptr| { let converter = sys::builtin_fn!($from_fn); - converter(variant_ptr, self.sys()); + converter(variant_ptr, sys::SysPtr::force_mut(self.sys())); }) }; @@ -56,7 +56,7 @@ macro_rules! impl_ffi_variant { let result = unsafe { sys::from_sys_init_or_init_default(|self_ptr| { let converter = sys::builtin_fn!($to_fn); - converter(self_ptr, variant.var_sys()); + converter(self_ptr, sys::SysPtr::force_mut(variant.var_sys())); }) }; diff --git a/godot-core/src/builtin/variant/mod.rs b/godot-core/src/builtin/variant/mod.rs index d85012ed8..ee7a2f660 100644 --- a/godot-core/src/builtin/variant/mod.rs +++ b/godot-core/src/builtin/variant/mod.rs @@ -24,7 +24,7 @@ pub use sys::{VariantOperator, VariantType}; /// value. /// /// See also [Godot documentation for `Variant`](https://docs.godotengine.org/en/stable/classes/class_variant.html). -#[repr(C, align(8))] +#[repr(transparent)] pub struct Variant { opaque: OpaqueVariant, } @@ -81,7 +81,7 @@ impl Variant { let object_ptr = unsafe { crate::obj::raw_object_init(|type_ptr| { let converter = sys::builtin_fn!(object_from_variant); - converter(type_ptr, self.var_sys()); + converter(type_ptr, sys::SysPtr::force_mut(self.var_sys())); }) }; @@ -111,13 +111,13 @@ impl Variant { } fn call_inner(&self, method: StringName, args: &[Variant]) -> Variant { - let args_sys: Vec<_> = args.iter().map(|v| v.var_sys_const()).collect(); + let args_sys: Vec<_> = args.iter().map(|v| v.var_sys()).collect(); let mut error = sys::default_call_error(); let result = unsafe { Variant::from_var_sys_init_or_init_default(|variant_ptr| { interface_fn!(variant_call)( - self.var_sys(), + sys::SysPtr::force_mut(self.var_sys()), method.string_sys(), args_sys.as_ptr(), args_sys.len() as i64, @@ -177,7 +177,7 @@ impl Variant { pub fn stringify(&self) -> GString { let mut result = GString::new(); unsafe { - interface_fn!(variant_stringify)(self.var_sys(), result.string_sys()); + interface_fn!(variant_stringify)(self.var_sys(), result.string_sys_mut()); } result } @@ -212,19 +212,11 @@ impl Variant { ffi_methods! { type sys::GDExtensionVariantPtr = *mut Opaque; - fn from_var_sys = from_sys; - fn from_var_sys_init = from_sys_init; + fn new_from_var_sys = new_from_sys; + fn new_with_var_uninit = new_with_uninit; + fn new_with_var_init = new_with_init; fn var_sys = sys; - } - - #[doc(hidden)] - pub unsafe fn from_var_sys_init_default( - init_fn: impl FnOnce(sys::GDExtensionVariantPtr), - ) -> Self { - #[allow(unused_mut)] - let mut variant = Variant::nil(); - init_fn(variant.var_sys()); - variant + fn var_sys_mut = sys_mut; } /// # Safety @@ -233,7 +225,7 @@ impl Variant { pub unsafe fn from_var_sys_init_or_init_default( init_fn: impl FnOnce(sys::GDExtensionVariantPtr), ) -> Self { - Self::from_var_sys_init_default(init_fn) + Self::new_with_var_init(|value| init_fn(value)) } /// # Safety @@ -243,13 +235,13 @@ impl Variant { pub unsafe fn from_var_sys_init_or_init_default( init_fn: impl FnOnce(sys::GDExtensionUninitializedVariantPtr), ) -> Self { - Self::from_var_sys_init(init_fn) + Self::new_with_var_uninit(init_fn) } /// # Safety /// See [`GodotFfi::from_sys_init`]. #[doc(hidden)] - pub unsafe fn from_var_sys_init_result( + pub unsafe fn new_with_var_uninit_result( init_fn: impl FnOnce(sys::GDExtensionUninitializedVariantPtr) -> Result<(), E>, ) -> Result { // Relies on current macro expansion of from_var_sys_init() having a certain implementation. @@ -257,16 +249,11 @@ impl Variant { let mut raw = std::mem::MaybeUninit::::uninit(); let var_uninit_ptr = - raw.as_mut_ptr() as ::Ptr; + raw.as_mut_ptr() as ::Uninit; init_fn(var_uninit_ptr).map(|_err| Self::from_opaque(raw.assume_init())) } - #[doc(hidden)] - pub fn var_sys_const(&self) -> sys::GDExtensionConstVariantPtr { - sys::to_const_ptr(self.var_sys()) - } - /// Converts to variant pointer; can be a null pointer. pub(crate) fn ptr_from_sys(variant_ptr: sys::GDExtensionConstVariantPtr) -> *const Variant { variant_ptr as *const Variant @@ -318,13 +305,7 @@ unsafe impl GodotFfi for Variant { sys::VariantType::Nil } - ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. } - - unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self { - let mut result = Self::default(); - init_fn(result.sys_mut()); - result - } + ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } impl_godot_as_self!(Variant); @@ -332,7 +313,7 @@ impl_godot_as_self!(Variant); impl Default for Variant { fn default() -> Self { unsafe { - Self::from_var_sys_init(|variant_ptr| { + Self::new_with_var_uninit(|variant_ptr| { interface_fn!(variant_new_nil)(variant_ptr); }) } @@ -342,7 +323,7 @@ impl Default for Variant { impl Clone for Variant { fn clone(&self) -> Self { unsafe { - Self::from_var_sys_init(|variant_ptr| { + Self::new_with_var_uninit(|variant_ptr| { interface_fn!(variant_new_copy)(variant_ptr, self.var_sys()); }) } @@ -352,7 +333,7 @@ impl Clone for Variant { impl Drop for Variant { fn drop(&mut self) { unsafe { - interface_fn!(variant_destroy)(self.var_sys()); + interface_fn!(variant_destroy)(self.var_sys_mut()); } } } diff --git a/godot-core/src/engine/script_instance.rs b/godot-core/src/engine/script_instance.rs index 009bbae8d..da819bc07 100644 --- a/godot-core/src/engine/script_instance.rs +++ b/godot-core/src/engine/script_instance.rs @@ -351,23 +351,6 @@ mod script_instance_info { &*(p_instance as *mut ScriptInstanceData) } - /// # Safety - /// - /// - We expect the engine to provide a valid const string name pointer. - unsafe fn transfer_string_name_from_godot( - p_name: sys::GDExtensionConstStringNamePtr, - ) -> StringName { - // This `StringName` is not ours and the engine will decrement the reference count later. To own our own `StringName` reference we - // cannot call the destructor on the "original" `StringName`, but have to clone it to increase the reference count. This new instance can - // then be passed around and eventually droped which will decrement the reference count again. - // - // By wrapping the initial `StringName` in a `ManuallyDrop` the destructor is prevented from being executed, which would decrement the - // reference count. - ManuallyDrop::new(StringName::from_string_sys(sys::force_mut_ptr(p_name))) - .deref() - .clone() - } - fn transfer_bool_to_godot(value: bool) -> sys::GDExtensionBool { value as sys::GDExtensionBool } @@ -452,7 +435,7 @@ mod script_instance_info { p_name: sys::GDExtensionConstStringNamePtr, p_value: sys::GDExtensionConstVariantPtr, ) -> sys::GDExtensionBool { - let name = transfer_string_name_from_godot(p_name); + let name = StringName::new_from_string_sys(p_name); let value = &*Variant::ptr_from_sys(p_value); let ctx = || format!("error when calling {}::set", type_name::()); @@ -476,7 +459,7 @@ mod script_instance_info { p_name: sys::GDExtensionConstStringNamePtr, r_ret: sys::GDExtensionVariantPtr, ) -> sys::GDExtensionBool { - let name = transfer_string_name_from_godot(p_name); + let name = StringName::new_from_string_sys(p_name); let ctx = || format!("error when calling {}::get", type_name::()); let return_value = handle_panic(ctx, || { @@ -596,7 +579,7 @@ mod script_instance_info { r_return: sys::GDExtensionVariantPtr, r_error: *mut sys::GDExtensionCallError, ) { - let method = transfer_string_name_from_godot(p_method); + let method = StringName::new_from_string_sys(p_method); let args = Variant::unbounded_refs_from_sys(p_args, p_argument_count as usize); let ctx = || format!("error when calling {}::call", type_name::()); @@ -668,7 +651,7 @@ mod script_instance_info { p_instance: sys::GDExtensionScriptInstanceDataPtr, p_method: sys::GDExtensionConstStringNamePtr, ) -> sys::GDExtensionBool { - let method = transfer_string_name_from_godot(p_method); + let method = StringName::new_from_string_sys(p_method); let ctx = || format!("error when calling {}::has_method", type_name::()); let has_method = handle_panic(ctx, || { @@ -730,7 +713,7 @@ mod script_instance_info { type_name::() ) }; - let name = transfer_string_name_from_godot(p_name); + let name = StringName::new_from_string_sys(p_name); let result = handle_panic(ctx, || { let instance = instance_data_as_script_instance::(p_instance); @@ -902,7 +885,7 @@ mod script_instance_info { p_name: sys::GDExtensionConstStringNamePtr, r_ret: sys::GDExtensionVariantPtr, ) -> sys::GDExtensionBool { - let name = transfer_string_name_from_godot(p_name); + let name = StringName::new_from_string_sys(p_name); let ctx = || { format!( @@ -936,7 +919,7 @@ mod script_instance_info { p_name: sys::GDExtensionConstStringNamePtr, p_value: sys::GDExtensionConstVariantPtr, ) -> sys::GDExtensionBool { - let name = transfer_string_name_from_godot(p_name); + let name = StringName::new_from_string_sys(p_name); let value = &*Variant::ptr_from_sys(p_value); let ctx = || { diff --git a/godot-core/src/init/mod.rs b/godot-core/src/init/mod.rs index e5ad8fe87..82b6a5f54 100644 --- a/godot-core/src/init/mod.rs +++ b/godot-core/src/init/mod.rs @@ -289,8 +289,8 @@ fn ensure_godot_features_compatible() { // SAFETY: main thread, after initialize(), valid string pointers. let gdext_is_double = cfg!(feature = "double-precision"); let godot_is_double = unsafe { - let is_single = sys::godot_has_feature(os_class.string_sys(), single.sys_const()); - let is_double = sys::godot_has_feature(os_class.string_sys(), double.sys_const()); + let is_single = sys::godot_has_feature(os_class.string_sys(), single.sys()); + let is_double = sys::godot_has_feature(os_class.string_sys(), double.sys()); assert_ne!( is_single, is_double, diff --git a/godot-core/src/log.rs b/godot-core/src/log.rs index d72b14493..03cd9e8dc 100644 --- a/godot-core/src/log.rs +++ b/godot-core/src/log.rs @@ -123,11 +123,11 @@ pub fn print(varargs: &[Variant]) { let call_fn = call_fn.unwrap_unchecked(); let mut args = Vec::new(); - args.extend(varargs.iter().map(Variant::sys_const)); + args.extend(varargs.iter().map(Variant::sys)); let args_ptr = args.as_ptr(); - let _variant = Variant::from_sys_init_default(|return_ptr| { - call_fn(return_ptr, args_ptr, args.len() as i32); + let _variant = Variant::new_with_init(|return_var| { + call_fn(return_var.sys_mut(), args_ptr, args.len() as i32); }); } @@ -147,11 +147,11 @@ pub fn print_rich(varargs: &[Variant]) { let call_fn = call_fn.unwrap_unchecked(); let mut args = Vec::new(); - args.extend(varargs.iter().map(Variant::sys_const)); + args.extend(varargs.iter().map(Variant::sys)); let args_ptr = args.as_ptr(); - let _variant = Variant::from_sys_init_default(|return_ptr| { - call_fn(return_ptr, args_ptr, args.len() as i32); + let _variant = Variant::new_with_init(|return_var| { + call_fn(return_var.sys_mut(), args_ptr, args.len() as i32); }); } diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index f9f43f63e..23f4db345 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -445,7 +445,7 @@ impl Gd { pub unsafe fn from_sys_init_opt(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Option { // TODO(uninit) - should we use GDExtensionUninitializedTypePtr instead? Then update all the builtin codegen... let init_fn = |ptr| { - init_fn(sys::AsUninit::force_init(ptr)); + init_fn(sys::SysPtr::force_init(ptr)); }; // Note: see _call_native_mb_ret_obj() in godot-cpp, which does things quite different (e.g. querying the instance binding). diff --git a/godot-core/src/obj/raw.rs b/godot-core/src/obj/raw.rs index 43ffb0cc5..eebe71fc3 100644 --- a/godot-core/src/obj/raw.rs +++ b/godot-core/src/obj/raw.rs @@ -444,17 +444,31 @@ where sys::VariantType::Object } - unsafe fn from_sys(ptr: sys::GDExtensionTypePtr) -> Self { + unsafe fn new_from_sys(ptr: sys::GDExtensionConstTypePtr) -> Self { Self::from_obj_sys_weak(ptr as sys::GDExtensionObjectPtr) } - unsafe fn from_sys_init(init: impl FnOnce(sys::GDExtensionUninitializedTypePtr)) -> Self { + unsafe fn new_with_uninit(init: impl FnOnce(sys::GDExtensionUninitializedTypePtr)) -> Self { let obj = raw_object_init(init); Self::from_obj_sys_weak(obj) } - fn sys(&self) -> sys::GDExtensionTypePtr { - self.obj as sys::GDExtensionTypePtr + unsafe fn new_with_init(init: impl FnOnce(&mut Self)) -> Self { + let mut obj = Self { + obj: std::ptr::null_mut(), + cached_rtti: None, + }; + + init(&mut obj); + obj + } + + fn sys(&self) -> sys::GDExtensionConstTypePtr { + self.obj.cast() + } + + fn sys_mut(&mut self) -> sys::GDExtensionTypePtr { + self.obj.cast() } // For more context around `ref_get_object` and `ref_set_object`, see: @@ -647,7 +661,7 @@ impl GodotFfiVariant for RawGd { // (This behaves differently in the opposite direction `variant_to_object`.) unsafe { - Variant::from_var_sys_init(|variant_ptr| { + Variant::new_with_var_uninit(|variant_ptr| { let converter = sys::builtin_fn!(object_to_variant); // Note: this is a special case because of an inconsistency in Godot, where sometimes the equivalency is @@ -670,9 +684,9 @@ impl GodotFfiVariant for RawGd { // TODO(uninit) - see if we can use from_sys_init() // raw_object_init? - RawGd::::from_sys_init(|self_ptr| { + RawGd::::new_with_uninit(|self_ptr| { let converter = sys::builtin_fn!(object_from_variant); - converter(self_ptr, variant.var_sys()); + converter(self_ptr, sys::SysPtr::force_mut(variant.var_sys())); }) }; diff --git a/godot-core/src/registry/callbacks.rs b/godot-core/src/registry/callbacks.rs index 5ed7a2333..640eb96d4 100644 --- a/godot-core/src/registry/callbacks.rs +++ b/godot-core/src/registry/callbacks.rs @@ -103,9 +103,8 @@ pub unsafe extern "C" fn get_virtual( name: sys::GDExtensionConstStringNamePtr, ) -> sys::GDExtensionClassCallVirtual { // This string is not ours, so we cannot call the destructor on it. - let borrowed_string = StringName::from_string_sys(sys::force_mut_ptr(name)); + let borrowed_string = StringName::borrow_string_sys(name); let method_name = borrowed_string.to_string(); - std::mem::forget(borrowed_string); T::__virtual_call(method_name.as_str()) } @@ -115,9 +114,8 @@ pub unsafe extern "C" fn default_get_virtual( name: sys::GDExtensionConstStringNamePtr, ) -> sys::GDExtensionClassCallVirtual { // This string is not ours, so we cannot call the destructor on it. - let borrowed_string = StringName::from_string_sys(sys::force_mut_ptr(name)); + let borrowed_string = StringName::borrow_string_sys(name); let method_name = borrowed_string.to_string(); - std::mem::forget(borrowed_string); T::__default_virtual_call(method_name.as_str()) } @@ -168,9 +166,7 @@ pub unsafe extern "C" fn get_property( ) -> sys::GDExtensionBool { let storage = as_storage::(instance); let instance = storage.get(); - let property = StringName::from_string_sys(sys::force_mut_ptr(name)); - - std::mem::forget(property.clone()); + let property = StringName::new_from_string_sys(name); match T::__godot_get_property(&*instance, property) { Some(value) => { @@ -189,11 +185,8 @@ pub unsafe extern "C" fn set_property( let storage = as_storage::(instance); let mut instance = storage.get_mut(); - let property = StringName::from_string_sys(sys::force_mut_ptr(name)); - let value = Variant::from_var_sys(sys::force_mut_ptr(value)); - - std::mem::forget(property.clone()); - std::mem::forget(value.clone()); + let property = StringName::new_from_string_sys(name); + let value = Variant::new_from_var_sys(value); T::__godot_set_property(&mut *instance, property, value) as sys::GDExtensionBool } diff --git a/godot-ffi/src/extras.rs b/godot-ffi/src/extras.rs index d82b06247..188babd5d 100644 --- a/godot-ffi/src/extras.rs +++ b/godot-ffi/src/extras.rs @@ -23,37 +23,70 @@ impl Distinct for GDExtensionConstTypePtr {} // Extension traits for conversion /// Convert a GDExtension pointer type to its uninitialized version. -pub trait AsUninit { - type Ptr; +pub trait SysPtr { + type Const; + type Uninit; #[allow(clippy::wrong_self_convention)] - fn as_uninit(self) -> Self::Ptr; + fn as_const(self) -> Self::Const; + #[allow(clippy::wrong_self_convention)] + fn as_uninit(self) -> Self::Uninit; - fn force_init(uninit: Self::Ptr) -> Self; + fn force_mut(const_ptr: Self::Const) -> Self; + fn force_init(uninit_ptr: Self::Uninit) -> Self; } -macro_rules! impl_as_uninit { - ($Ptr:ty, $Uninit:ty) => { - impl AsUninit for $Ptr { - type Ptr = $Uninit; +macro_rules! impl_sys_ptr { + ($Ptr:ty, $Const:ty, $Uninit:ty) => { + impl SysPtr for $Ptr { + type Const = $Const; + type Uninit = $Uninit; + + fn as_const(self) -> Self::Const { + self as Self::Const + } + + #[allow(clippy::wrong_self_convention)] + fn as_uninit(self) -> Self::Uninit { + self as Self::Uninit + } - fn as_uninit(self) -> $Uninit { - self as $Uninit + fn force_mut(const_ptr: Self::Const) -> Self { + const_ptr as Self } - fn force_init(uninit: Self::Ptr) -> Self { - uninit as Self + fn force_init(uninit_ptr: Self::Uninit) -> Self { + uninit_ptr as Self } } }; } -#[rustfmt::skip] -impl_as_uninit!(GDExtensionStringNamePtr, GDExtensionUninitializedStringNamePtr); -impl_as_uninit!(GDExtensionVariantPtr, GDExtensionUninitializedVariantPtr); -impl_as_uninit!(GDExtensionStringPtr, GDExtensionUninitializedStringPtr); -impl_as_uninit!(GDExtensionObjectPtr, GDExtensionUninitializedObjectPtr); -impl_as_uninit!(GDExtensionTypePtr, GDExtensionUninitializedTypePtr); +impl_sys_ptr!( + GDExtensionStringNamePtr, + GDExtensionConstStringNamePtr, + GDExtensionUninitializedStringNamePtr +); +impl_sys_ptr!( + GDExtensionVariantPtr, + GDExtensionConstVariantPtr, + GDExtensionUninitializedVariantPtr +); +impl_sys_ptr!( + GDExtensionStringPtr, + GDExtensionConstStringPtr, + GDExtensionUninitializedStringPtr +); +impl_sys_ptr!( + GDExtensionObjectPtr, + GDExtensionConstObjectPtr, + GDExtensionUninitializedObjectPtr +); +impl_sys_ptr!( + GDExtensionTypePtr, + GDExtensionConstTypePtr, + GDExtensionUninitializedTypePtr +); // ---------------------------------------------------------------------------------------------------------------------------------------------- // Helper functions diff --git a/godot-ffi/src/godot_ffi.rs b/godot-ffi/src/godot_ffi.rs index 73d124f32..a87053552 100644 --- a/godot-ffi/src/godot_ffi.rs +++ b/godot-ffi/src/godot_ffi.rs @@ -31,13 +31,13 @@ pub unsafe trait GodotFfi { /// which is different depending on the type. /// The type in `ptr` must not require any special consideration upon referencing. Such as /// incrementing a refcount. - unsafe fn from_sys(ptr: sys::GDExtensionTypePtr) -> Self; + unsafe fn new_from_sys(ptr: sys::GDExtensionConstTypePtr) -> Self; /// Construct uninitialized opaque data, then initialize it with `init_fn` function. /// /// # Safety /// `init_fn` must be a function that correctly handles a (possibly-uninitialized) _type ptr_. - unsafe fn from_sys_init(init_fn: impl FnOnce(sys::GDExtensionUninitializedTypePtr)) -> Self; + unsafe fn new_with_uninit(init_fn: impl FnOnce(sys::GDExtensionUninitializedTypePtr)) -> Self; /// Like [`Self::from_sys_init`], but pre-initializes the sys pointer to a `Default::default()` instance /// before calling `init_fn`. @@ -49,44 +49,23 @@ pub unsafe trait GodotFfi { /// /// # Safety /// `init_fn` must be a function that correctly handles a (possibly-uninitialized) _type ptr_. - unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self - where - Self: Sized, // + Default - { - // SAFETY: this default implementation is potentially incorrect. - // By implementing the GodotFfi trait, you acknowledge that these may need to be overridden. - Self::from_sys_init(|ptr| init_fn(sys::AsUninit::force_init(ptr))) - - // TODO consider using this, if all the implementors support it - // let mut result = Self::default(); - // init_fn(result.sys_mut().as_uninit()); - // result - } + unsafe fn new_with_init(init_fn: impl FnOnce(&mut Self)) -> Self; /// Return Godot opaque pointer, for an immutable operation. /// /// Note that this is a `*mut` pointer despite taking `&self` by shared-ref. /// This is because most of Godot's Rust API is not const-correct. This can still /// enhance user code (calling `sys_mut` ensures no aliasing at the time of the call). - fn sys(&self) -> sys::GDExtensionTypePtr; + fn sys(&self) -> sys::GDExtensionConstTypePtr; /// Return Godot opaque pointer, for a mutable operation. /// /// Should usually not be overridden; behaves like `sys()` but ensures no aliasing /// at the time of the call (not necessarily during any subsequent modifications though). - fn sys_mut(&mut self) -> sys::GDExtensionTypePtr { - self.sys() - } - - // TODO check if sys() can take over this - // also, from_sys() might take *const T - // possibly separate 2 pointer types - fn sys_const(&self) -> sys::GDExtensionConstTypePtr { - self.sys() - } + fn sys_mut(&mut self) -> sys::GDExtensionTypePtr; fn as_arg_ptr(&self) -> sys::GDExtensionConstTypePtr { - self.sys_const() + self.sys() } /// Construct from a pointer to an argument in a call. @@ -120,7 +99,7 @@ pub unsafe trait GodotFfi { pub unsafe fn from_sys_init_or_init_default( init_fn: impl FnOnce(sys::GDExtensionTypePtr), ) -> T { - T::from_sys_init_default(init_fn) + T::new_with_init(|value| init_fn(value.sys_mut())) } /// # Safety @@ -130,7 +109,7 @@ pub unsafe fn from_sys_init_or_init_default( pub unsafe fn from_sys_init_or_init_default( init_fn: impl FnOnce(sys::GDExtensionUninitializedTypePtr), ) -> T { - T::from_sys_init(init_fn) + T::new_with_uninit(init_fn) } // ---------------------------------------------------------------------------------------------------------------------------------------------- @@ -183,73 +162,104 @@ pub enum PtrcallType { #[doc(hidden)] macro_rules! ffi_methods_one { // type $Ptr = *mut Opaque - (OpaquePtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $from_sys:ident = from_sys) => { + (OpaquePtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $new_from_sys:ident = new_from_sys) => { $( #[$attr] )? $vis - unsafe fn $from_sys(ptr: $Ptr) -> Self { - let opaque = std::ptr::read(ptr as *mut _); - Self::from_opaque(opaque) + unsafe fn $new_from_sys(ptr: <$Ptr as $crate::SysPtr>::Const) -> Self { + let opaque = std::ptr::read(ptr.cast()); + let new = Self::from_opaque(opaque); + std::mem::forget(new.clone()); + new } }; - (OpaquePtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $from_sys_init:ident = from_sys_init) => { + (OpaquePtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $new_with_uninit:ident = new_with_uninit) => { $( #[$attr] )? $vis - unsafe fn $from_sys_init(init: impl FnOnce(<$Ptr as $crate::AsUninit>::Ptr)) -> Self { + unsafe fn $new_with_uninit(init: impl FnOnce(<$Ptr as $crate::SysPtr>::Uninit)) -> Self { let mut raw = std::mem::MaybeUninit::uninit(); - init(raw.as_mut_ptr() as <$Ptr as $crate::AsUninit>::Ptr); + init(raw.as_mut_ptr() as *mut _); Self::from_opaque(raw.assume_init()) } }; + (OpaquePtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $new_with_init:ident = new_with_init) => { + $( #[$attr] )? $vis + unsafe fn $new_with_init(init: impl FnOnce(&mut Self)) -> Self { + let mut default = Default::default(); + init(&mut default); + default + } + }; (OpaquePtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $sys:ident = sys) => { $( #[$attr] )? $vis - fn $sys(&self) -> $Ptr { - &self.opaque as *const _ as $Ptr + fn $sys(&self) -> <$Ptr as $crate::SysPtr>::Const { + std::ptr::from_ref(&self.opaque).cast() + } + }; + (OpaquePtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $sys_mut:ident = sys_mut) => { + $( #[$attr] )? $vis + fn $sys_mut(&mut self) -> $Ptr { + std::ptr::from_mut(&mut self.opaque).cast() } }; (OpaquePtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $from_arg_ptr:ident = from_arg_ptr) => { $( #[$attr] )? $vis unsafe fn $from_arg_ptr(ptr: $Ptr, _call_type: $crate::PtrcallType) -> Self { - Self::from_sys(ptr as *mut _) + Self::new_from_sys(ptr.cast()) } }; (OpaquePtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $move_return_ptr:ident = move_return_ptr) => { $( #[$attr] )? $vis unsafe fn $move_return_ptr(mut self, dst: $Ptr, _call_type: $crate::PtrcallType) { - std::ptr::swap(dst as *mut _, std::ptr::addr_of_mut!(self.opaque)) + std::ptr::swap(dst.cast(), std::ptr::addr_of_mut!(self.opaque)) } }; // type $Ptr = *mut Self - (SelfPtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $from_sys:ident = from_sys) => { + (SelfPtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $new_from_sys:ident = new_from_sys) => { $( #[$attr] )? $vis - unsafe fn $from_sys(ptr: $Ptr) -> Self { - *(ptr as *mut Self) + unsafe fn $new_from_sys(ptr: <$Ptr as $crate::SysPtr>::Const) -> Self { + let borrowed = &*ptr.cast::(); + borrowed.clone() } }; - (SelfPtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $from_sys_init:ident = from_sys_init) => { + (SelfPtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $new_with_uninit:ident = new_with_uninit) => { $( #[$attr] )? $vis - unsafe fn $from_sys_init(init: impl FnOnce(<$Ptr as $crate::AsUninit>::Ptr)) -> Self { + unsafe fn $new_with_uninit(init: impl FnOnce(<$Ptr as $crate::SysPtr>::Uninit)) -> Self { let mut raw = std::mem::MaybeUninit::::uninit(); - init(raw.as_mut_ptr() as <$Ptr as $crate::AsUninit>::Ptr); + init(raw.as_mut_ptr().cast()); raw.assume_init() } }; + (SelfPtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $new_with_init:ident = new_with_init) => { + $( #[$attr] )? $vis + unsafe fn $new_with_init(init: impl FnOnce(&mut Self)) -> Self { + let mut default = Default::default(); + init(&mut default); + default + } + }; (SelfPtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $sys:ident = sys) => { $( #[$attr] )? $vis - fn $sys(&self) -> $Ptr { - self as *const Self as $Ptr + fn $sys(&self) -> <$Ptr as $crate::SysPtr>::Const { + std::ptr::from_ref(self).cast() + } + }; + (SelfPtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $sys_mut:ident = sys_mut) => { + $( #[$attr] )? $vis + fn $sys_mut(&mut self) -> $Ptr { + std::ptr::from_mut(self).cast() } }; (SelfPtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $from_arg_ptr:ident = from_arg_ptr) => { $( #[$attr] )? $vis unsafe fn $from_arg_ptr(ptr: $Ptr, _call_type: $crate::PtrcallType) -> Self { - *(ptr as *mut Self) + Self::new_from_sys(ptr.cast()) } }; (SelfPtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $move_return_ptr:ident = move_return_ptr) => { $( #[$attr] )? $vis unsafe fn $move_return_ptr(self, dst: $Ptr, _call_type: $crate::PtrcallType) { - *(dst as *mut Self) = self + *(dst.cast::()) = self } }; } @@ -272,9 +282,11 @@ macro_rules! ffi_methods_rest { ( // impl GodotFfi for T (default all 5) $Impl:ident $Ptr:ty; .. ) => { - $crate::ffi_methods_one!($Impl $Ptr; from_sys = from_sys); - $crate::ffi_methods_one!($Impl $Ptr; from_sys_init = from_sys_init); + $crate::ffi_methods_one!($Impl $Ptr; new_from_sys = new_from_sys); + $crate::ffi_methods_one!($Impl $Ptr; new_with_uninit = new_with_uninit); + $crate::ffi_methods_one!($Impl $Ptr; new_with_init = new_with_init); $crate::ffi_methods_one!($Impl $Ptr; sys = sys); + $crate::ffi_methods_one!($Impl $Ptr; sys_mut = sys_mut); $crate::ffi_methods_one!($Impl $Ptr; from_arg_ptr = from_arg_ptr); $crate::ffi_methods_one!($Impl $Ptr; move_return_ptr = move_return_ptr); }; @@ -474,17 +486,28 @@ mod scalars { sys::VariantType::Nil } - unsafe fn from_sys(_ptr: sys::GDExtensionTypePtr) -> Self { + unsafe fn new_from_sys(_ptr: sys::GDExtensionConstTypePtr) -> Self { // Do nothing } - unsafe fn from_sys_init(_init: impl FnOnce(sys::GDExtensionUninitializedTypePtr)) -> Self { + unsafe fn new_with_uninit( + _init: impl FnOnce(sys::GDExtensionUninitializedTypePtr), + ) -> Self { // Do nothing } - fn sys(&self) -> sys::GDExtensionTypePtr { + unsafe fn new_with_init(_init: impl FnOnce(&mut ())) -> Self { + // Do nothing + } + + fn sys(&self) -> sys::GDExtensionConstTypePtr { + // ZST dummy pointer + std::ptr::from_ref(self).cast() + } + + fn sys_mut(&mut self) -> sys::GDExtensionTypePtr { // ZST dummy pointer - self as *const _ as sys::GDExtensionTypePtr + std::ptr::from_mut(self).cast() } // SAFETY: diff --git a/itest/rust/src/builtin_tests/containers/variant_test.rs b/itest/rust/src/builtin_tests/containers/variant_test.rs index a1c65d7a2..9f3dbec42 100644 --- a/itest/rust/src/builtin_tests/containers/variant_test.rs +++ b/itest/rust/src/builtin_tests/containers/variant_test.rs @@ -261,7 +261,7 @@ fn variant_sys_conversion() { let v = Variant::from(7); let ptr = v.sys(); - let v2 = unsafe { Variant::from_sys(ptr) }; + let v2 = unsafe { Variant::new_from_sys(ptr) }; assert_eq!(v2, v); } @@ -281,7 +281,7 @@ fn variant_sys_conversion2() { }; let v2 = unsafe { - Variant::from_sys_init(|ptr| { + Variant::new_with_uninit(|ptr| { std::ptr::copy( buffer.as_ptr(), ptr as *mut u8, diff --git a/itest/rust/src/object_tests/object_test.rs b/itest/rust/src/object_tests/object_test.rs index bf7b7dac1..55534182a 100644 --- a/itest/rust/src/object_tests/object_test.rs +++ b/itest/rust/src/object_tests/object_test.rs @@ -55,7 +55,7 @@ fn object_user_roundtrip_return() { let ptr = raw.sys(); std::mem::forget(obj); - let raw2 = unsafe { RawGd::::from_sys(ptr) }; + let raw2 = unsafe { RawGd::::new_from_sys(ptr) }; let obj2 = Gd::from_ffi(raw2); assert_eq!(obj2.bind().value, value); } // drop @@ -70,8 +70,8 @@ fn object_user_roundtrip_write() { let raw = obj.to_ffi(); let raw2 = unsafe { - RawGd::::from_sys_init(|ptr| { - raw.move_return_ptr(sys::AsUninit::force_init(ptr), sys::PtrcallType::Standard) + RawGd::::new_with_uninit(|ptr| { + raw.move_return_ptr(sys::SysPtr::force_init(ptr), sys::PtrcallType::Standard) }) }; let obj2 = Gd::from_ffi(raw2); @@ -89,7 +89,7 @@ fn object_engine_roundtrip() { let raw = obj.to_ffi(); let ptr = raw.sys(); - let raw2 = unsafe { RawGd::::from_sys(ptr) }; + let raw2 = unsafe { RawGd::::new_from_sys(ptr) }; let obj2 = Gd::from_ffi(raw2); assert_eq!(obj2.get_position(), pos); obj.free(); diff --git a/itest/rust/src/register_tests/option_ffi_test.rs b/itest/rust/src/register_tests/option_ffi_test.rs index 32d61cdcb..f7f168532 100644 --- a/itest/rust/src/register_tests/option_ffi_test.rs +++ b/itest/rust/src/register_tests/option_ffi_test.rs @@ -19,7 +19,7 @@ fn option_some_sys_conversion() { let v_raw = v.to_ffi(); let ptr = v_raw.sys(); - let v2_raw = unsafe { RawGd::::from_sys(ptr) }; + let v2_raw = unsafe { RawGd::::new_from_sys(ptr) }; let v2 = Option::>::from_ffi(v2_raw); assert_eq!(v2, v); @@ -34,7 +34,7 @@ fn option_none_sys_conversion() { let v_raw = v.to_ffi(); let ptr = v_raw.sys(); - let v2_raw = unsafe { RawGd::::from_sys(ptr) }; + let v2_raw = unsafe { RawGd::::new_from_sys(ptr) }; let v2 = Option::>::from_ffi(v2_raw); assert_eq!(v2, v); } From 112ccbb43ddccdbdb85cb1a19963538ef332b480 Mon Sep 17 00:00:00 2001 From: Lili Zoey Zyli Date: Sat, 9 Mar 2024 19:45:24 +0100 Subject: [PATCH 2/9] Fix some api-version related issues --- godot-core/src/builtin/meta/signature.rs | 4 ++-- godot-core/src/builtin/variant/mod.rs | 2 +- godot-core/src/obj/raw.rs | 2 +- godot-macros/src/class/godot_api.rs | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/godot-core/src/builtin/meta/signature.rs b/godot-core/src/builtin/meta/signature.rs index 00c3e9392..f9ea73761 100644 --- a/godot-core/src/builtin/meta/signature.rs +++ b/godot-core/src/builtin/meta/signature.rs @@ -266,9 +266,9 @@ macro_rules! impl_varcall_signature_for_tuple { )* ]; - let variant_ptrs = explicit_args.iter().map(Variant::var_sys_const).collect::>(); + let variant_ptrs = explicit_args.iter().map(Variant::var_sys).collect::>(); - let variant = Variant::from_var_sys_init(|return_ptr| { + let variant = Variant::new_with_var_uninit(|return_ptr| { let mut err = sys::default_call_error(); object_call_script_method( object_ptr, diff --git a/godot-core/src/builtin/variant/mod.rs b/godot-core/src/builtin/variant/mod.rs index ee7a2f660..0d4d35a66 100644 --- a/godot-core/src/builtin/variant/mod.rs +++ b/godot-core/src/builtin/variant/mod.rs @@ -225,7 +225,7 @@ impl Variant { pub unsafe fn from_var_sys_init_or_init_default( init_fn: impl FnOnce(sys::GDExtensionVariantPtr), ) -> Self { - Self::new_with_var_init(|value| init_fn(value)) + Self::new_with_var_init(|value| init_fn(value.var_sys_mut())) } /// # Safety diff --git a/godot-core/src/obj/raw.rs b/godot-core/src/obj/raw.rs index eebe71fc3..2898bf77b 100644 --- a/godot-core/src/obj/raw.rs +++ b/godot-core/src/obj/raw.rs @@ -518,7 +518,7 @@ where // In 4.1, argument pointers were standardized to always be `T**`. #[cfg(before_api = "4.1")] { - self.sys_const() + self.sys() } #[cfg(since_api = "4.1")] diff --git a/godot-macros/src/class/godot_api.rs b/godot-macros/src/class/godot_api.rs index a594bf587..710193582 100644 --- a/godot-macros/src/class/godot_api.rs +++ b/godot-macros/src/class/godot_api.rs @@ -427,7 +427,7 @@ fn add_virtual_script_call( let code = quote! { let object_ptr = #object_ptr; let method_sname = ::godot::builtin::StringName::from(#method_name_str); - let method_sname_ptr = method_sname.string_sys_const(); + let method_sname_ptr = method_sname.string_sys(); let has_virtual_method = unsafe { ::godot::private::has_virtual_script_method(object_ptr, method_sname_ptr) }; if has_virtual_method { From bab1a2981986d44dd4076b58db099ebb8d3f1137 Mon Sep 17 00:00:00 2001 From: Lili Zoey Zyli Date: Sun, 10 Mar 2024 00:19:43 +0100 Subject: [PATCH 3/9] Some cleanup --- godot-core/src/builtin/array.rs | 2 +- godot-core/src/builtin/callable.rs | 12 +++--- godot-core/src/builtin/signal.rs | 2 +- godot-core/src/builtin/string/gstring.rs | 21 +++------ godot-core/src/builtin/string/node_path.rs | 5 +-- godot-core/src/builtin/string/string_name.rs | 5 ++- godot-core/src/builtin/variant/impls.rs | 2 +- godot-core/src/builtin/variant/mod.rs | 45 ++++++++++---------- godot-core/src/engine/script_instance.rs | 6 +-- godot-core/src/registry/callbacks.rs | 4 +- godot-ffi/src/godot_ffi.rs | 4 +- godot-ffi/src/lib.rs | 3 +- 12 files changed, 51 insertions(+), 60 deletions(-) diff --git a/godot-core/src/builtin/array.rs b/godot-core/src/builtin/array.rs index 7a1488275..15844a43f 100644 --- a/godot-core/src/builtin/array.rs +++ b/godot-core/src/builtin/array.rs @@ -937,7 +937,7 @@ impl GodotFfiVariant for Array { } let array = unsafe { - sys::from_sys_init_or_init_default::(|self_ptr| { + sys::new_with_uninit_or_init::(|self_ptr| { let array_from_variant = sys::builtin_fn!(array_from_variant); array_from_variant(self_ptr, sys::SysPtr::force_mut(variant.var_sys())); }) diff --git a/godot-core/src/builtin/callable.rs b/godot-core/src/builtin/callable.rs index b8a33e272..35eb2e0cc 100644 --- a/godot-core/src/builtin/callable.rs +++ b/godot-core/src/builtin/callable.rs @@ -48,7 +48,7 @@ impl Callable { // upcast not needed let method = method_name.into(); unsafe { - sys::from_sys_init_or_init_default::(|self_ptr| { + sys::new_with_uninit_or_init::(|self_ptr| { let ctor = sys::builtin_fn!(callable_from_object_method); let raw = object.to_ffi(); let args = [raw.as_arg_ptr(), method.sys()]; @@ -392,7 +392,7 @@ mod custom_callable { r_error: *mut sys::GDExtensionCallError, ) { let arg_refs: &[&Variant] = - Variant::unbounded_refs_from_sys(p_args, p_argument_count as usize); + Variant::borrow_var_ref_slice(p_args, p_argument_count as usize); let c: &mut C = CallableUserdata::inner_from_raw(callable_userdata); @@ -410,7 +410,7 @@ mod custom_callable { F: FnMut(&[&Variant]) -> Result, { let arg_refs: &[&Variant] = - Variant::unbounded_refs_from_sys(p_args, p_argument_count as usize); + Variant::borrow_var_ref_slice(p_args, p_argument_count as usize); let w: &mut FnWrapper = CallableUserdata::inner_from_raw(callable_userdata); @@ -450,7 +450,7 @@ mod custom_callable { let c: &T = CallableUserdata::inner_from_raw(callable_userdata); let s = crate::builtin::GString::from(c.to_string()); - s.move_string_ptr(r_out); + s.move_return_string_ptr(r_out, sys::PtrcallType::Standard); *r_is_valid = true as sys::GDExtensionBool; } @@ -461,7 +461,9 @@ mod custom_callable { ) { let w: &mut FnWrapper = CallableUserdata::inner_from_raw(callable_userdata); - w.name.clone().move_string_ptr(r_out); + w.name + .clone() + .move_return_string_ptr(r_out, sys::PtrcallType::Standard); *r_is_valid = true as sys::GDExtensionBool; } } diff --git a/godot-core/src/builtin/signal.rs b/godot-core/src/builtin/signal.rs index 3ae76bc15..cf6f0477f 100644 --- a/godot-core/src/builtin/signal.rs +++ b/godot-core/src/builtin/signal.rs @@ -41,7 +41,7 @@ impl Signal { { let signal_name = signal_name.into(); unsafe { - sys::from_sys_init_or_init_default::(|self_ptr| { + sys::new_with_uninit_or_init::(|self_ptr| { let ctor = sys::builtin_fn!(signal_from_object_signal); let raw = object.to_ffi(); let args = [raw.as_arg_ptr(), signal_name.sys()]; diff --git a/godot-core/src/builtin/string/gstring.rs b/godot-core/src/builtin/string/gstring.rs index 485397daa..fbf7b4a5d 100644 --- a/godot-core/src/builtin/string/gstring.rs +++ b/godot-core/src/builtin/string/gstring.rs @@ -54,7 +54,7 @@ use super::{NodePath, StringName}; // #[repr] is needed on GString itself rather than the opaque field, because PackedStringArray::as_slice() relies on a packed representation. #[repr(transparent)] pub struct GString { - opaque: OpaqueString, + _opaque: OpaqueString, } impl GString { @@ -63,10 +63,6 @@ impl GString { Self::default() } - fn from_opaque(opaque: OpaqueString) -> Self { - Self { opaque } - } - pub fn len(&self) -> usize { self.as_inner().length().try_into().unwrap() } @@ -129,14 +125,7 @@ impl GString { fn new_with_string_uninit = new_with_uninit; fn string_sys = sys; fn string_sys_mut = sys_mut; - } - - /// Move `self` into a system pointer. This transfers ownership and thus does not call the destructor. - /// - /// # Safety - /// `dst` must be a pointer to a `GString` which is suitable for ffi with Godot. - pub(crate) unsafe fn move_string_ptr(self, dst: sys::GDExtensionStringPtr) { - self.move_return_ptr(dst as *mut _, sys::PtrcallType::Standard); + fn move_return_string_ptr = move_return_ptr; } #[doc(hidden)] @@ -159,7 +148,7 @@ unsafe impl GodotFfi for GString { sys::VariantType::String } - ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; ..} + ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } impl_godot_as_self!(GString); @@ -257,7 +246,7 @@ impl FromStr for GString { impl From<&StringName> for GString { fn from(string: &StringName) -> Self { unsafe { - sys::from_sys_init_or_init_default::(|self_ptr| { + sys::new_with_uninit_or_init::(|self_ptr| { let ctor = sys::builtin_fn!(string_from_string_name); let args = [string.sys()]; ctor(self_ptr, args.as_ptr()); @@ -278,7 +267,7 @@ impl From for GString { impl From<&NodePath> for GString { fn from(path: &NodePath) -> Self { unsafe { - sys::from_sys_init_or_init_default::(|self_ptr| { + sys::new_with_uninit_or_init::(|self_ptr| { let ctor = sys::builtin_fn!(string_from_node_path); let args = [path.sys()]; ctor(self_ptr, args.as_ptr()); diff --git a/godot-core/src/builtin/string/node_path.rs b/godot-core/src/builtin/string/node_path.rs index 7d27c7cc8..8197c5d29 100644 --- a/godot-core/src/builtin/string/node_path.rs +++ b/godot-core/src/builtin/string/node_path.rs @@ -8,7 +8,7 @@ use std::fmt; use godot_ffi as sys; -use godot_ffi::{ffi_methods, GDExtensionTypePtr, GodotFfi}; +use godot_ffi::{ffi_methods, GodotFfi}; use crate::builtin::inner; use crate::builtin::meta::impl_godot_as_self; @@ -16,7 +16,6 @@ use crate::builtin::meta::impl_godot_as_self; use super::{GString, StringName}; /// A pre-parsed scene tree path. -#[repr(C)] pub struct NodePath { opaque: sys::types::OpaqueNodePath, } @@ -107,7 +106,7 @@ where impl From<&GString> for NodePath { fn from(string: &GString) -> Self { unsafe { - sys::from_sys_init_or_init_default::(|self_ptr| { + sys::new_with_uninit_or_init::(|self_ptr| { let ctor = sys::builtin_fn!(node_path_from_string); let args = [string.sys()]; ctor(self_ptr, args.as_ptr()); diff --git a/godot-core/src/builtin/string/string_name.rs b/godot-core/src/builtin/string/string_name.rs index 4a7fe1e92..f9e2c2c6a 100644 --- a/godot-core/src/builtin/string/string_name.rs +++ b/godot-core/src/builtin/string/string_name.rs @@ -26,7 +26,8 @@ use crate::builtin::{GString, NodePath}; /// relying on lexicographical ordering. /// /// Instead, we provide [`transient_ord()`][Self::transient_ord] for ordering relations. -#[repr(C)] +// Currently we rely on `transparent` for `borrow_string_sys`. +#[repr(transparent)] pub struct StringName { opaque: sys::types::OpaqueStringName, } @@ -225,7 +226,7 @@ where impl From<&GString> for StringName { fn from(string: &GString) -> Self { unsafe { - sys::from_sys_init_or_init_default::(|self_ptr| { + sys::new_with_uninit_or_init::(|self_ptr| { let ctor = sys::builtin_fn!(string_name_from_string); let args = [string.sys()]; ctor(self_ptr, args.as_ptr()); diff --git a/godot-core/src/builtin/variant/impls.rs b/godot-core/src/builtin/variant/impls.rs index c8d4c3019..c328fb0be 100644 --- a/godot-core/src/builtin/variant/impls.rs +++ b/godot-core/src/builtin/variant/impls.rs @@ -54,7 +54,7 @@ macro_rules! impl_ffi_variant { // // This was changed in 4.1. let result = unsafe { - sys::from_sys_init_or_init_default(|self_ptr| { + sys::new_with_uninit_or_init(|self_ptr| { let converter = sys::builtin_fn!($to_fn); converter(self_ptr, sys::SysPtr::force_mut(variant.var_sys())); }) diff --git a/godot-core/src/builtin/variant/mod.rs b/godot-core/src/builtin/variant/mod.rs index 0d4d35a66..bfcd5c524 100644 --- a/godot-core/src/builtin/variant/mod.rs +++ b/godot-core/src/builtin/variant/mod.rs @@ -115,7 +115,7 @@ impl Variant { let mut error = sys::default_call_error(); let result = unsafe { - Variant::from_var_sys_init_or_init_default(|variant_ptr| { + Variant::new_with_var_uninit_or_init(|variant_ptr| { interface_fn!(variant_call)( sys::SysPtr::force_mut(self.var_sys()), method.string_sys(), @@ -145,7 +145,7 @@ impl Variant { let mut is_valid = false as u8; let result = unsafe { - Self::from_var_sys_init_or_init_default(|variant_ptr| { + Self::new_with_var_uninit_or_init(|variant_ptr| { interface_fn!(variant_evaluate)( op_sys, self.var_sys(), @@ -217,12 +217,13 @@ impl Variant { fn new_with_var_init = new_with_init; fn var_sys = sys; fn var_sys_mut = sys_mut; + fn move_return_var_ptr = move_return_ptr; } /// # Safety /// See [`GodotFfi::from_sys_init`] and [`GodotFfi::from_sys_init_default`]. #[cfg(before_api = "4.1")] - pub unsafe fn from_var_sys_init_or_init_default( + pub unsafe fn new_with_var_uninit_or_init( init_fn: impl FnOnce(sys::GDExtensionVariantPtr), ) -> Self { Self::new_with_var_init(|value| init_fn(value.var_sys_mut())) @@ -232,7 +233,7 @@ impl Variant { /// See [`GodotFfi::from_sys_init`] and [`GodotFfi::from_sys_init_default`]. #[cfg(since_api = "4.1")] #[doc(hidden)] - pub unsafe fn from_var_sys_init_or_init_default( + pub unsafe fn new_with_var_uninit_or_init( init_fn: impl FnOnce(sys::GDExtensionUninitializedVariantPtr), ) -> Self { Self::new_with_var_uninit(init_fn) @@ -256,16 +257,30 @@ impl Variant { /// Converts to variant pointer; can be a null pointer. pub(crate) fn ptr_from_sys(variant_ptr: sys::GDExtensionConstVariantPtr) -> *const Variant { + sys::static_assert_eq_size!(Variant, sys::types::OpaqueVariant); variant_ptr as *const Variant } + /// Converts to variant mut pointer; can be a null pointer. + pub(crate) fn ptr_from_sys_mut(variant_ptr: sys::GDExtensionVariantPtr) -> *mut Variant { + sys::static_assert_eq_size!(Variant, sys::types::OpaqueVariant); + variant_ptr as *mut Variant + } + + pub unsafe fn borrow_var_sys<'a>(ptr: sys::GDExtensionConstVariantPtr) -> &'a Variant { + sys::static_assert_eq_size!(Variant, sys::types::OpaqueVariant); + &*(ptr.cast::()) + } + /// # Safety /// `variant_ptr_array` must be a valid pointer to an array of `length` variant pointers. /// The caller is responsible of keeping the backing storage alive while the unbounded references exist. - pub(crate) unsafe fn unbounded_refs_from_sys<'a>( + pub(crate) unsafe fn borrow_var_ref_slice<'a>( variant_ptr_array: *const sys::GDExtensionConstVariantPtr, length: usize, ) -> &'a [&'a Variant] { + sys::static_assert_eq_size!(Variant, sys::types::OpaqueVariant); + // Godot may pass null to signal "no arguments" (e.g. in custom callables). if variant_ptr_array.is_null() { debug_assert_eq!( @@ -275,25 +290,11 @@ impl Variant { return &[]; } - let variant_ptr_array: &'a [sys::GDExtensionConstVariantPtr] = - std::slice::from_raw_parts(variant_ptr_array, length); - - // SAFETY: raw pointers and references have the same memory layout. + // raw pointers and references have the same memory layout. // See https://doc.rust-lang.org/reference/type-layout.html#pointers-and-references-layout. - unsafe { std::mem::transmute(variant_ptr_array) } - } - - /// Converts to variant mut pointer; can be a null pointer. - pub(crate) fn ptr_from_sys_mut(variant_ptr: sys::GDExtensionVariantPtr) -> *mut Variant { - variant_ptr as *mut Variant - } + let variant_ptr_array = variant_ptr_array.cast::<&Variant>(); - /// Move `self` into a system pointer. This transfers ownership and thus does not call the destructor. - /// - /// # Safety - /// `dst` must be a pointer to a [`Variant`] which is suitable for ffi with Godot. - pub(crate) unsafe fn move_var_ptr(self, dst: sys::GDExtensionVariantPtr) { - self.move_return_ptr(dst as *mut _, sys::PtrcallType::Standard); + std::slice::from_raw_parts(variant_ptr_array, length) } } diff --git a/godot-core/src/engine/script_instance.rs b/godot-core/src/engine/script_instance.rs index da819bc07..850a931da 100644 --- a/godot-core/src/engine/script_instance.rs +++ b/godot-core/src/engine/script_instance.rs @@ -359,7 +359,7 @@ mod script_instance_info { /// /// - We expect the engine to provide a valid variant pointer the return value can be moved into. unsafe fn transfer_variant_to_godot(variant: Variant, return_ptr: sys::GDExtensionVariantPtr) { - variant.move_var_ptr(return_ptr) + variant.move_return_var_ptr(return_ptr, sys::PtrcallType::Standard) } /// # Safety @@ -391,7 +391,7 @@ mod script_instance_info { /// /// - The engine has to provide a valid string return pointer. unsafe fn transfer_string_to_godot(string: GString, return_ptr: sys::GDExtensionStringPtr) { - string.move_string_ptr(return_ptr); + string.move_return_string_ptr(return_ptr, sys::PtrcallType::Standard); } /// # Safety @@ -580,7 +580,7 @@ mod script_instance_info { r_error: *mut sys::GDExtensionCallError, ) { let method = StringName::new_from_string_sys(p_method); - let args = Variant::unbounded_refs_from_sys(p_args, p_argument_count as usize); + let args = Variant::borrow_var_ref_slice(p_args, p_argument_count as usize); let ctx = || format!("error when calling {}::call", type_name::()); let result = handle_panic(ctx, || { diff --git a/godot-core/src/registry/callbacks.rs b/godot-core/src/registry/callbacks.rs index 640eb96d4..3abf6f510 100644 --- a/godot-core/src/registry/callbacks.rs +++ b/godot-core/src/registry/callbacks.rs @@ -133,7 +133,7 @@ pub unsafe extern "C" fn to_string( let string = T::__godot_to_string(&*instance); // Transfer ownership to Godot - string.move_string_ptr(out_string); + string.move_return_string_ptr(out_string, sys::PtrcallType::Standard); } #[cfg(before_api = "4.2")] @@ -170,7 +170,7 @@ pub unsafe extern "C" fn get_property( match T::__godot_get_property(&*instance, property) { Some(value) => { - value.move_var_ptr(ret); + value.move_return_var_ptr(ret, sys::PtrcallType::Standard); true as sys::GDExtensionBool } None => false as sys::GDExtensionBool, diff --git a/godot-ffi/src/godot_ffi.rs b/godot-ffi/src/godot_ffi.rs index a87053552..92dd127c1 100644 --- a/godot-ffi/src/godot_ffi.rs +++ b/godot-ffi/src/godot_ffi.rs @@ -96,7 +96,7 @@ pub unsafe trait GodotFfi { /// /// See [`GodotFfi::from_sys_init`] and [`GodotFfi::from_sys_init_default`]. #[cfg(before_api = "4.1")] -pub unsafe fn from_sys_init_or_init_default( +pub unsafe fn new_with_uninit_or_init( init_fn: impl FnOnce(sys::GDExtensionTypePtr), ) -> T { T::new_with_init(|value| init_fn(value.sys_mut())) @@ -106,7 +106,7 @@ pub unsafe fn from_sys_init_or_init_default( /// /// See [`GodotFfi::from_sys_init`] and [`GodotFfi::from_sys_init_default`]. #[cfg(since_api = "4.1")] -pub unsafe fn from_sys_init_or_init_default( +pub unsafe fn new_with_uninit_or_init( init_fn: impl FnOnce(sys::GDExtensionUninitializedTypePtr), ) -> T { T::new_with_uninit(init_fn) diff --git a/godot-ffi/src/lib.rs b/godot-ffi/src/lib.rs index d5523f800..f9e0b6725 100644 --- a/godot-ffi/src/lib.rs +++ b/godot-ffi/src/lib.rs @@ -52,8 +52,7 @@ pub use paste; pub use gensym::gensym; pub use crate::godot_ffi::{ - from_sys_init_or_init_default, GodotFfi, GodotNullableFfi, PrimitiveConversionError, - PtrcallType, + new_with_uninit_or_init, GodotFfi, GodotNullableFfi, PrimitiveConversionError, PtrcallType, }; // Method tables From 3c4a89233a1edea1ad616d816320ac935ea7d469 Mon Sep 17 00:00:00 2001 From: Lili Zoey Zyli Date: Sun, 10 Mar 2024 12:57:11 +0100 Subject: [PATCH 4/9] More cleanup --- godot-core/src/builtin/callable.rs | 4 ---- godot-core/src/builtin/meta/signature.rs | 2 +- godot-core/src/builtin/string/string_name.rs | 4 +++- godot-core/src/builtin/variant/impls.rs | 1 - godot-core/src/builtin/variant/mod.rs | 9 +++++---- godot-core/src/engine/script_instance.rs | 5 ++--- godot-ffi/src/godot_ffi.rs | 18 +++++++++++++----- 7 files changed, 24 insertions(+), 19 deletions(-) diff --git a/godot-core/src/builtin/callable.rs b/godot-core/src/builtin/callable.rs index 35eb2e0cc..6b0a648d2 100644 --- a/godot-core/src/builtin/callable.rs +++ b/godot-core/src/builtin/callable.rs @@ -270,10 +270,6 @@ impl Callable { pub fn as_inner(&self) -> inner::InnerCallable { inner::InnerCallable::from_outer(self) } - - fn inc_ref(&self) { - std::mem::forget(self.clone()) - } } impl_builtin_traits! { diff --git a/godot-core/src/builtin/meta/signature.rs b/godot-core/src/builtin/meta/signature.rs index f9ea73761..369463d48 100644 --- a/godot-core/src/builtin/meta/signature.rs +++ b/godot-core/src/builtin/meta/signature.rs @@ -467,7 +467,7 @@ unsafe fn varcall_arg( args_ptr: *const sys::GDExtensionConstVariantPtr, call_ctx: &CallContext, ) -> Result { - let variant_ref = &*Variant::ptr_from_sys(*args_ptr.offset(N)); + let variant_ref = Variant::borrow_var_sys(*args_ptr.offset(N)); P::try_from_variant(variant_ref) .map_err(|err| CallError::failed_param_conversion::

(call_ctx, N, err)) diff --git a/godot-core/src/builtin/string/string_name.rs b/godot-core/src/builtin/string/string_name.rs index f9e2c2c6a..d3451931e 100644 --- a/godot-core/src/builtin/string/string_name.rs +++ b/godot-core/src/builtin/string/string_name.rs @@ -122,7 +122,9 @@ impl StringName { fn string_sys_mut = sys_mut; } - pub unsafe fn borrow_string_sys<'a>(ptr: sys::GDExtensionConstStringNamePtr) -> &'a StringName { + pub(crate) unsafe fn borrow_string_sys<'a>( + ptr: sys::GDExtensionConstStringNamePtr, + ) -> &'a StringName { sys::static_assert_eq_size!(StringName, sys::types::OpaqueStringName); &*(ptr.cast::()) } diff --git a/godot-core/src/builtin/variant/impls.rs b/godot-core/src/builtin/variant/impls.rs index c328fb0be..c6fb2c67b 100644 --- a/godot-core/src/builtin/variant/impls.rs +++ b/godot-core/src/builtin/variant/impls.rs @@ -10,7 +10,6 @@ use crate::builtin::meta::{FromVariantError, GodotFfiVariant, GodotType, Propert use crate::builtin::*; use crate::engine::global; use godot_ffi as sys; -use sys::GodotFfi; // ---------------------------------------------------------------------------------------------------------------------------------------------- // Macro definitions diff --git a/godot-core/src/builtin/variant/mod.rs b/godot-core/src/builtin/variant/mod.rs index bfcd5c524..cd7e0d570 100644 --- a/godot-core/src/builtin/variant/mod.rs +++ b/godot-core/src/builtin/variant/mod.rs @@ -221,8 +221,9 @@ impl Variant { } /// # Safety - /// See [`GodotFfi::from_sys_init`] and [`GodotFfi::from_sys_init_default`]. + /// See [`GodotFfi::new_with_uninit`] and [`GodotFfi::new_with_init`]. #[cfg(before_api = "4.1")] + #[doc(hidden)] pub unsafe fn new_with_var_uninit_or_init( init_fn: impl FnOnce(sys::GDExtensionVariantPtr), ) -> Self { @@ -230,7 +231,7 @@ impl Variant { } /// # Safety - /// See [`GodotFfi::from_sys_init`] and [`GodotFfi::from_sys_init_default`]. + /// See [`GodotFfi::new_with_uninit`] and [`GodotFfi::new_with_init`]. #[cfg(since_api = "4.1")] #[doc(hidden)] pub unsafe fn new_with_var_uninit_or_init( @@ -240,7 +241,7 @@ impl Variant { } /// # Safety - /// See [`GodotFfi::from_sys_init`]. + /// See [`GodotFfi::new_with_uninit`]. #[doc(hidden)] pub unsafe fn new_with_var_uninit_result( init_fn: impl FnOnce(sys::GDExtensionUninitializedVariantPtr) -> Result<(), E>, @@ -267,7 +268,7 @@ impl Variant { variant_ptr as *mut Variant } - pub unsafe fn borrow_var_sys<'a>(ptr: sys::GDExtensionConstVariantPtr) -> &'a Variant { + pub(crate) unsafe fn borrow_var_sys<'a>(ptr: sys::GDExtensionConstVariantPtr) -> &'a Variant { sys::static_assert_eq_size!(Variant, sys::types::OpaqueVariant); &*(ptr.cast::()) } diff --git a/godot-core/src/engine/script_instance.rs b/godot-core/src/engine/script_instance.rs index 850a931da..3033a462c 100644 --- a/godot-core/src/engine/script_instance.rs +++ b/godot-core/src/engine/script_instance.rs @@ -317,7 +317,6 @@ mod script_instance_info { use std::cell::{BorrowError, Ref, RefMut}; use std::ffi::c_void; use std::mem::ManuallyDrop; - use std::ops::Deref; use crate::builtin::{GString, StringName, Variant}; use crate::engine::ScriptLanguage; @@ -436,7 +435,7 @@ mod script_instance_info { p_value: sys::GDExtensionConstVariantPtr, ) -> sys::GDExtensionBool { let name = StringName::new_from_string_sys(p_name); - let value = &*Variant::ptr_from_sys(p_value); + let value = Variant::borrow_var_sys(p_value); let ctx = || format!("error when calling {}::set", type_name::()); let result = handle_panic(ctx, || { @@ -920,7 +919,7 @@ mod script_instance_info { p_value: sys::GDExtensionConstVariantPtr, ) -> sys::GDExtensionBool { let name = StringName::new_from_string_sys(p_name); - let value = &*Variant::ptr_from_sys(p_value); + let value = Variant::borrow_var_sys(p_value); let ctx = || { format!( diff --git a/godot-ffi/src/godot_ffi.rs b/godot-ffi/src/godot_ffi.rs index 92dd127c1..c8cebc3e0 100644 --- a/godot-ffi/src/godot_ffi.rs +++ b/godot-ffi/src/godot_ffi.rs @@ -19,7 +19,10 @@ use std::marker::PhantomData; /// must properly initialize and clean up values given the [`PtrcallType`] provided by the caller. #[doc(hidden)] // shows up in implementors otherwise pub unsafe trait GodotFfi { + #[doc(hidden)] fn variant_type() -> sys::VariantType; + + #[doc(hidden)] fn default_param_metadata() -> sys::GDExtensionClassMethodArgumentMetadata { sys::GDEXTENSION_METHOD_ARGUMENT_METADATA_NONE } @@ -29,14 +32,14 @@ pub unsafe trait GodotFfi { /// # Safety /// `ptr` must be a valid _type ptr_: it must follow Godot's convention to encode `Self`, /// which is different depending on the type. - /// The type in `ptr` must not require any special consideration upon referencing. Such as - /// incrementing a refcount. + #[doc(hidden)] unsafe fn new_from_sys(ptr: sys::GDExtensionConstTypePtr) -> Self; /// Construct uninitialized opaque data, then initialize it with `init_fn` function. /// /// # Safety /// `init_fn` must be a function that correctly handles a (possibly-uninitialized) _type ptr_. + #[doc(hidden)] unsafe fn new_with_uninit(init_fn: impl FnOnce(sys::GDExtensionUninitializedTypePtr)) -> Self; /// Like [`Self::from_sys_init`], but pre-initializes the sys pointer to a `Default::default()` instance @@ -45,10 +48,10 @@ pub unsafe trait GodotFfi { /// Some FFI functions in Godot expect a pre-existing instance at the destination pointer, e.g. CoW/ref-counted /// builtin types like `Array`, `Dictionary`, `String`, `StringName`. /// - /// If not overridden, this just calls [`Self::from_sys_init`]. - /// /// # Safety - /// `init_fn` must be a function that correctly handles a (possibly-uninitialized) _type ptr_. + /// While this does call `init_fn` with a `&mut Self` reference, that value may have a broken safety invariant. + /// So `init_fn` must ensure that the reference passed to it is fully initialized when the function returns. + #[doc(hidden)] unsafe fn new_with_init(init_fn: impl FnOnce(&mut Self)) -> Self; /// Return Godot opaque pointer, for an immutable operation. @@ -56,14 +59,17 @@ pub unsafe trait GodotFfi { /// Note that this is a `*mut` pointer despite taking `&self` by shared-ref. /// This is because most of Godot's Rust API is not const-correct. This can still /// enhance user code (calling `sys_mut` ensures no aliasing at the time of the call). + #[doc(hidden)] fn sys(&self) -> sys::GDExtensionConstTypePtr; /// Return Godot opaque pointer, for a mutable operation. /// /// Should usually not be overridden; behaves like `sys()` but ensures no aliasing /// at the time of the call (not necessarily during any subsequent modifications though). + #[doc(hidden)] fn sys_mut(&mut self) -> sys::GDExtensionTypePtr; + #[doc(hidden)] fn as_arg_ptr(&self) -> sys::GDExtensionConstTypePtr { self.sys() } @@ -75,6 +81,7 @@ pub unsafe trait GodotFfi { /// which is different depending on the type. /// /// * `ptr` must encode `Self` according to the given `call_type`'s encoding of argument values. + #[doc(hidden)] unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, call_type: PtrcallType) -> Self; /// Move self into the pointer in pointer `dst`, dropping what is already in `dst. @@ -85,6 +92,7 @@ pub unsafe trait GodotFfi { /// /// * `dst` must be able to accept a value of type `Self` encoded according to the given /// `call_type`'s encoding of return values. + #[doc(hidden)] unsafe fn move_return_ptr(self, dst: sys::GDExtensionTypePtr, call_type: PtrcallType); } From 5bd069840459b4dad24b13306aae213461e442cf Mon Sep 17 00:00:00 2001 From: Lili Zoey Zyli Date: Sun, 10 Mar 2024 14:09:04 +0100 Subject: [PATCH 5/9] Remove the `ptr_from_sys` methods on `Variant` so people have to go through explicit methods that do what they want --- godot-core/src/builtin/array.rs | 57 +++++----- godot-core/src/builtin/dictionary.rs | 10 +- godot-core/src/builtin/string/string_name.rs | 5 + godot-core/src/builtin/variant/mod.rs | 107 ++++++++++++++----- godot-ffi/src/godot_ffi.rs | 12 +-- 5 files changed, 124 insertions(+), 67 deletions(-) diff --git a/godot-core/src/builtin/array.rs b/godot-core/src/builtin/array.rs index 15844a43f..3a87af09f 100644 --- a/godot-core/src/builtin/array.rs +++ b/godot-core/src/builtin/array.rs @@ -225,7 +225,7 @@ impl Array { /// # Panics /// /// If `index` is out of bounds. - fn ptr(&self, index: usize) -> *const Variant { + fn ptr(&self, index: usize) -> sys::GDExtensionConstVariantPtr { let ptr = self.ptr_or_null(index); assert!( !ptr.is_null(), @@ -236,7 +236,7 @@ impl Array { } /// Returns a pointer to the element at the given index, or null if out of bounds. - fn ptr_or_null(&self, index: usize) -> *const Variant { + fn ptr_or_null(&self, index: usize) -> sys::GDExtensionConstVariantPtr { // SAFETY: array_operator_index_const returns null for invalid indexes. let variant_ptr = unsafe { let index = to_i64(index); @@ -244,8 +244,7 @@ impl Array { }; // Signature is wrong in GDExtension, semantically this is a const ptr - let variant_ptr = sys::to_const_ptr(variant_ptr); - Variant::ptr_from_sys(variant_ptr) + sys::SysPtr::as_const(variant_ptr) } /// Returns a mutable pointer to the element at the given index. @@ -253,7 +252,7 @@ impl Array { /// # Panics /// /// If `index` is out of bounds. - fn ptr_mut(&mut self, index: usize) -> *mut Variant { + fn ptr_mut(&mut self, index: usize) -> sys::GDExtensionVariantPtr { let ptr = self.ptr_mut_or_null(index); assert!( !ptr.is_null(), @@ -264,14 +263,14 @@ impl Array { } /// Returns a pointer to the element at the given index, or null if out of bounds. - fn ptr_mut_or_null(&mut self, index: usize) -> *mut Variant { + fn ptr_mut_or_null(&mut self, index: usize) -> sys::GDExtensionVariantPtr { // SAFETY: array_operator_index returns null for invalid indexes. let variant_ptr = unsafe { let index = to_i64(index); interface_fn!(array_operator_index)(self.sys_mut(), index) }; - Variant::ptr_from_sys_mut(variant_ptr) + variant_ptr } /// # Safety @@ -490,8 +489,8 @@ impl Array { // Panics on out-of-bounds let ptr = self.ptr(index); - // SAFETY: `ptr()` just verified that the index is not out of bounds. - let variant = unsafe { &*ptr }; + // SAFETY: `ptr` is a live pointer to a variant since `ptr.is_null()` just verified that the index is not out of bounds. + let variant = unsafe { Variant::borrow_var_sys(ptr) }; T::from_variant(variant) } @@ -501,8 +500,8 @@ impl Array { if ptr.is_null() { None } else { - // SAFETY: `ptr.is_null()` just verified that the index is not out of bounds. - let variant = unsafe { &*ptr }; + // SAFETY: `ptr` is a live pointer to a variant since `ptr.is_null()` just verified that the index is not out of bounds. + let variant = unsafe { Variant::borrow_var_sys(ptr) }; Some(T::from_variant(variant)) } } @@ -663,7 +662,9 @@ impl Array { // SAFETY: `ptr_mut` just checked that the index is not out of bounds. unsafe { - *ptr_mut = value.to_variant(); + value + .to_variant() + .move_return_var_ptr(ptr_mut, sys::PtrcallType::Standard); } } @@ -970,14 +971,14 @@ impl From<&[T]> for Array { // the nulls with values of type `T`. unsafe { array.as_inner_mut() }.resize(to_i64(len)); - let ptr = array.ptr_mut_or_null(0); - for (i, element) in slice.iter().enumerate() { - // SAFETY: The array contains exactly `len` elements, stored contiguously in memory. - // Also, the pointer is non-null, as we checked for emptiness above. - unsafe { - *ptr.offset(to_isize(i)) = element.to_variant(); - } + // SAFETY: `array` has `len` elements since we just resized it, and they are all valid `Variant`s. Additionally, since + // the array was created in this function, and we do not access the array while this slice exists, the slice has unique + // access to the elements. + let elements = unsafe { Variant::borrow_var_slice_mut(array.ptr_mut(0), len) }; + for (element, array_slot) in slice.iter().zip(elements.iter_mut()) { + *array_slot = element.to_variant(); } + array } } @@ -1010,14 +1011,13 @@ impl From<&Array> for Vec { fn from(array: &Array) -> Vec { let len = array.len(); let mut vec = Vec::with_capacity(len); - let ptr = array.ptr(0); - for offset in 0..to_isize(len) { - // SAFETY: Arrays are stored contiguously in memory, so we can use pointer arithmetic - // instead of going through `array_operator_index_const` for every index. - let variant = unsafe { &*ptr.offset(offset) }; - let element = T::from_variant(variant); - vec.push(element); - } + + // SAFETY: Unless `experimental-threads` is enabled, then we cannot have concurrent access to this array. + // And since we dont concurrently access the array in this function, we can create a slice to its contents. + let elements = unsafe { Variant::borrow_var_slice(array.ptr(0), len) }; + + vec.extend(elements.iter().map(T::from_variant)); + vec } } @@ -1040,7 +1040,8 @@ impl<'a, T: GodotType + FromGodot> Iterator for Iter<'a, T> { let element_ptr = self.array.ptr_or_null(idx); // SAFETY: We just checked that the index is not out of bounds, so the pointer won't be null. - let variant = unsafe { &*element_ptr }; + // We immediately convert this to the right element, so barring `experimental-threads` the pointer wont be invalidated in time. + let variant = unsafe { Variant::borrow_var_sys(element_ptr) }; let element = T::from_variant(variant); Some(element) } else { diff --git a/godot-core/src/builtin/dictionary.rs b/godot-core/src/builtin/dictionary.rs index 2af4c73d8..9838d6ee9 100644 --- a/godot-core/src/builtin/dictionary.rs +++ b/godot-core/src/builtin/dictionary.rs @@ -204,9 +204,11 @@ impl Dictionary { pub fn set(&mut self, key: K, value: V) { let key = key.to_variant(); - // SAFETY: always returns a valid pointer to a value in the dictionary; either pre-existing or newly inserted. + // SAFETY: `self.get_ptr_mut(key)` always returns a valid pointer to a value in the dictionary; either pre-existing or newly inserted. unsafe { - *self.get_ptr_mut(key) = value.to_variant(); + value + .to_variant() + .move_return_var_ptr(self.get_ptr_mut(key), sys::PtrcallType::Standard); } } @@ -240,7 +242,7 @@ impl Dictionary { /// Get the pointer corresponding to the given key in the dictionary. /// /// If there exists no value at the given key, a `NIL` variant will be inserted for that key. - fn get_ptr_mut(&mut self, key: K) -> *mut Variant { + fn get_ptr_mut(&mut self, key: K) -> sys::GDExtensionVariantPtr { let key = key.to_variant(); // SAFETY: accessing an unknown key _mutably_ creates that entry in the dictionary, with value `NIL`. @@ -248,7 +250,7 @@ impl Dictionary { unsafe { interface_fn!(dictionary_operator_index)(self.sys_mut(), key.var_sys()) }; // Never a null pointer, since entry either existed already or was inserted above. - Variant::ptr_from_sys_mut(ptr) + ptr } } diff --git a/godot-core/src/builtin/string/string_name.rs b/godot-core/src/builtin/string/string_name.rs index d3451931e..6197ed127 100644 --- a/godot-core/src/builtin/string/string_name.rs +++ b/godot-core/src/builtin/string/string_name.rs @@ -122,6 +122,11 @@ impl StringName { fn string_sys_mut = sys_mut; } + /// Convert a `StringName` sys pointer to a reference to a `StringName`. + /// + /// # Safety + /// + /// `ptr` must point to a live `StringName` for the duration of `'a`. pub(crate) unsafe fn borrow_string_sys<'a>( ptr: sys::GDExtensionConstStringNamePtr, ) -> &'a StringName { diff --git a/godot-core/src/builtin/variant/mod.rs b/godot-core/src/builtin/variant/mod.rs index cd7e0d570..ab25dfd1f 100644 --- a/godot-core/src/builtin/variant/mod.rs +++ b/godot-core/src/builtin/variant/mod.rs @@ -204,13 +204,9 @@ impl Variant { unsafe { interface_fn!(variant_booleanize)(self.var_sys()) != 0 } } - fn from_opaque(opaque: OpaqueVariant) -> Self { - Self { opaque } - } - // Conversions from/to Godot C++ `Variant*` pointers ffi_methods! { - type sys::GDExtensionVariantPtr = *mut Opaque; + type sys::GDExtensionVariantPtr = *mut Self; fn new_from_var_sys = new_from_sys; fn new_with_var_uninit = new_with_uninit; @@ -240,48 +236,48 @@ impl Variant { Self::new_with_var_uninit(init_fn) } + /// Fallible construction of a `Variant` using a function. + /// /// # Safety - /// See [`GodotFfi::new_with_uninit`]. + /// + /// If `init_fn` returns `Ok(())`, then it must have initialized the pointer passed to it in accordance with [`GodotFfi::new_with_uninit`]. #[doc(hidden)] pub unsafe fn new_with_var_uninit_result( init_fn: impl FnOnce(sys::GDExtensionUninitializedVariantPtr) -> Result<(), E>, ) -> Result { // Relies on current macro expansion of from_var_sys_init() having a certain implementation. - let mut raw = std::mem::MaybeUninit::::uninit(); + let mut raw = std::mem::MaybeUninit::::uninit(); let var_uninit_ptr = raw.as_mut_ptr() as ::Uninit; - init_fn(var_uninit_ptr).map(|_err| Self::from_opaque(raw.assume_init())) - } - - /// Converts to variant pointer; can be a null pointer. - pub(crate) fn ptr_from_sys(variant_ptr: sys::GDExtensionConstVariantPtr) -> *const Variant { - sys::static_assert_eq_size!(Variant, sys::types::OpaqueVariant); - variant_ptr as *const Variant - } - - /// Converts to variant mut pointer; can be a null pointer. - pub(crate) fn ptr_from_sys_mut(variant_ptr: sys::GDExtensionVariantPtr) -> *mut Variant { - sys::static_assert_eq_size!(Variant, sys::types::OpaqueVariant); - variant_ptr as *mut Variant + // SAFETY: `map` only runs the provided closure for the `Ok(())` variant, in which case `raw` has definitely been initialized. + init_fn(var_uninit_ptr).map(|_succ| unsafe { raw.assume_init() }) } + /// Convert a `Variant` sys pointer to a reference to a `Variant`. + /// + /// # Safety + /// + /// `ptr` must point to a live `Variant` for the duration of `'a`. pub(crate) unsafe fn borrow_var_sys<'a>(ptr: sys::GDExtensionConstVariantPtr) -> &'a Variant { sys::static_assert_eq_size!(Variant, sys::types::OpaqueVariant); - &*(ptr.cast::()) + + // SAFETY: `ptr` is a pointer to a live `Variant` for the duration of `'a`. + unsafe { &*(ptr.cast::()) } } + /// Convert an array of `Variant` sys pointers to a slice of `Variant` references. + /// /// # Safety - /// `variant_ptr_array` must be a valid pointer to an array of `length` variant pointers. - /// The caller is responsible of keeping the backing storage alive while the unbounded references exist. + /// + /// Either `variant_ptr_array` is null, or it must be safe to call [`std::slice::from_raw_parts`] with + /// `variant_ptr_array` cast to `*const &'a Variant` and `length`. pub(crate) unsafe fn borrow_var_ref_slice<'a>( variant_ptr_array: *const sys::GDExtensionConstVariantPtr, length: usize, ) -> &'a [&'a Variant] { - sys::static_assert_eq_size!(Variant, sys::types::OpaqueVariant); - // Godot may pass null to signal "no arguments" (e.g. in custom callables). if variant_ptr_array.is_null() { debug_assert_eq!( @@ -295,7 +291,66 @@ impl Variant { // See https://doc.rust-lang.org/reference/type-layout.html#pointers-and-references-layout. let variant_ptr_array = variant_ptr_array.cast::<&Variant>(); - std::slice::from_raw_parts(variant_ptr_array, length) + // SAFETY: `variant_ptr_array` isn't null so it is safe to call `from_raw_parts` on the pointer cast to `*const &Variant`. + unsafe { std::slice::from_raw_parts(variant_ptr_array, length) } + } + + /// Convert an array of `Variant` sys pointers to a slice of mutable `Variant` references. + /// + /// # Safety + /// + /// Either `variant_array` is null, or it must be safe to call [`std::slice::from_raw_parts`] with + /// `variant_array` cast to `*const Variant` and `length`. + pub(crate) unsafe fn borrow_var_slice<'a>( + variant_array: sys::GDExtensionConstVariantPtr, + length: usize, + ) -> &'a [Variant] { + sys::static_assert_eq_size!(Variant, sys::types::OpaqueVariant); + + // Godot may pass null to signal "no arguments" (e.g. in custom callables). + if variant_array.is_null() { + debug_assert_eq!( + length, 0, + "Variant::unbounded_refs_from_sys(): pointer is null but length is not 0" + ); + return &[]; + } + + // raw pointers and references have the same memory layout. + // See https://doc.rust-lang.org/reference/type-layout.html#pointers-and-references-layout. + let variant_array = variant_array.cast::(); + + // SAFETY: `variant_array` isn't null so it is safe to call `from_raw_parts` on the pointer cast to `*const &Variant`. + unsafe { std::slice::from_raw_parts(variant_array, length) } + } + + /// Convert an array of `Variant` sys pointers to a slice of mutable `Variant` references. + /// + /// # Safety + /// + /// Either `variant_array` is null, or it must be safe to call [`std::slice::from_raw_parts_mut`] with + /// `variant_array` cast to `*mut Variant` and `length`. + pub(crate) unsafe fn borrow_var_slice_mut<'a>( + variant_array: sys::GDExtensionVariantPtr, + length: usize, + ) -> &'a mut [Variant] { + sys::static_assert_eq_size!(Variant, sys::types::OpaqueVariant); + + // Godot may pass null to signal "no arguments" (e.g. in custom callables). + if variant_array.is_null() { + debug_assert_eq!( + length, 0, + "Variant::unbounded_refs_from_sys(): pointer is null but length is not 0" + ); + return &mut []; + } + + // raw pointers and references have the same memory layout. + // See https://doc.rust-lang.org/reference/type-layout.html#pointers-and-references-layout. + let variant_array = variant_array.cast::(); + + // SAFETY: `variant_array` isn't null so it is safe to call `from_raw_parts_mut` on the pointer cast to `*const &Variant`. + unsafe { std::slice::from_raw_parts_mut(variant_array, length) } } } diff --git a/godot-ffi/src/godot_ffi.rs b/godot-ffi/src/godot_ffi.rs index c8cebc3e0..3e2370d80 100644 --- a/godot-ffi/src/godot_ffi.rs +++ b/godot-ffi/src/godot_ffi.rs @@ -55,17 +55,10 @@ pub unsafe trait GodotFfi { unsafe fn new_with_init(init_fn: impl FnOnce(&mut Self)) -> Self; /// Return Godot opaque pointer, for an immutable operation. - /// - /// Note that this is a `*mut` pointer despite taking `&self` by shared-ref. - /// This is because most of Godot's Rust API is not const-correct. This can still - /// enhance user code (calling `sys_mut` ensures no aliasing at the time of the call). #[doc(hidden)] fn sys(&self) -> sys::GDExtensionConstTypePtr; /// Return Godot opaque pointer, for a mutable operation. - /// - /// Should usually not be overridden; behaves like `sys()` but ensures no aliasing - /// at the time of the call (not necessarily during any subsequent modifications though). #[doc(hidden)] fn sys_mut(&mut self) -> sys::GDExtensionTypePtr; @@ -102,7 +95,7 @@ pub unsafe trait GodotFfi { /// # Safety /// -/// See [`GodotFfi::from_sys_init`] and [`GodotFfi::from_sys_init_default`]. +/// See [`GodotFfi::new_with_uninit`] and [`GodotFfi::new_with_init`]. #[cfg(before_api = "4.1")] pub unsafe fn new_with_uninit_or_init( init_fn: impl FnOnce(sys::GDExtensionTypePtr), @@ -112,7 +105,7 @@ pub unsafe fn new_with_uninit_or_init( /// # Safety /// -/// See [`GodotFfi::from_sys_init`] and [`GodotFfi::from_sys_init_default`]. +/// See [`GodotFfi::new_with_uninit`] and [`GodotFfi::new_with_init`]. #[cfg(since_api = "4.1")] pub unsafe fn new_with_uninit_or_init( init_fn: impl FnOnce(sys::GDExtensionUninitializedTypePtr), @@ -173,6 +166,7 @@ macro_rules! ffi_methods_one { (OpaquePtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $new_from_sys:ident = new_from_sys) => { $( #[$attr] )? $vis unsafe fn $new_from_sys(ptr: <$Ptr as $crate::SysPtr>::Const) -> Self { + // TODO: Directly use copy constructors here? let opaque = std::ptr::read(ptr.cast()); let new = Self::from_opaque(opaque); std::mem::forget(new.clone()); From c8b7ff04e9cb5aa531a76510846bd90413b429ce Mon Sep 17 00:00:00 2001 From: Lili Zoey Zyli Date: Sun, 10 Mar 2024 15:41:50 +0100 Subject: [PATCH 6/9] Fix clippy stuff remove reprs --- godot-core/src/builtin/array.rs | 7 ++----- godot-core/src/builtin/callable.rs | 1 - godot-core/src/builtin/dictionary.rs | 8 ++------ godot-core/src/builtin/packed_array.rs | 1 - godot-core/src/builtin/signal.rs | 1 - godot-core/src/builtin/variant/mod.rs | 15 ++++++++------- godot-ffi/src/toolbox.rs | 18 ++++++++++++++++++ 7 files changed, 30 insertions(+), 21 deletions(-) diff --git a/godot-core/src/builtin/array.rs b/godot-core/src/builtin/array.rs index 3a87af09f..5d1886416 100644 --- a/godot-core/src/builtin/array.rs +++ b/godot-core/src/builtin/array.rs @@ -54,7 +54,6 @@ use super::meta::{ // for `T: GodotType` because `drop()` requires `sys_mut()`, which is on the `GodotFfi` // trait, whose `from_sys_init()` requires `Default`, which is only implemented for `T: // GodotType`. Whew. This could be fixed by splitting up `GodotFfi` if desired. -#[repr(C)] pub struct Array { // Safety Invariant: The type of all values in `opaque` matches the type `T`. opaque: sys::types::OpaqueArray, @@ -265,12 +264,10 @@ impl Array { /// Returns a pointer to the element at the given index, or null if out of bounds. fn ptr_mut_or_null(&mut self, index: usize) -> sys::GDExtensionVariantPtr { // SAFETY: array_operator_index returns null for invalid indexes. - let variant_ptr = unsafe { + unsafe { let index = to_i64(index); interface_fn!(array_operator_index)(self.sys_mut(), index) - }; - - variant_ptr + } } /// # Safety diff --git a/godot-core/src/builtin/callable.rs b/godot-core/src/builtin/callable.rs index 6b0a648d2..c8374f3c1 100644 --- a/godot-core/src/builtin/callable.rs +++ b/godot-core/src/builtin/callable.rs @@ -25,7 +25,6 @@ use sys::{ffi_methods, GodotFfi}; /// Currently it is impossible to use `bind` and `unbind` in GDExtension, see [godot-cpp#802]. /// /// [godot-cpp#802]: https://github.com/godotengine/godot-cpp/issues/802 -#[repr(C, align(8))] pub struct Callable { opaque: sys::types::OpaqueCallable, } diff --git a/godot-core/src/builtin/dictionary.rs b/godot-core/src/builtin/dictionary.rs index 9838d6ee9..4fdd93dc9 100644 --- a/godot-core/src/builtin/dictionary.rs +++ b/godot-core/src/builtin/dictionary.rs @@ -25,7 +25,6 @@ use super::meta::impl_godot_as_self; /// # Thread safety /// /// The same principles apply as for [`VariantArray`]. Consult its documentation for details. -#[repr(C)] pub struct Dictionary { opaque: OpaqueDictionary, } @@ -245,12 +244,9 @@ impl Dictionary { fn get_ptr_mut(&mut self, key: K) -> sys::GDExtensionVariantPtr { let key = key.to_variant(); - // SAFETY: accessing an unknown key _mutably_ creates that entry in the dictionary, with value `NIL`. - let ptr = - unsafe { interface_fn!(dictionary_operator_index)(self.sys_mut(), key.var_sys()) }; - // Never a null pointer, since entry either existed already or was inserted above. - ptr + // SAFETY: accessing an unknown key _mutably_ creates that entry in the dictionary, with value `NIL`. + unsafe { interface_fn!(dictionary_operator_index)(self.sys_mut(), key.var_sys()) } } } diff --git a/godot-core/src/builtin/packed_array.rs b/godot-core/src/builtin/packed_array.rs index 2807337e1..aa44da970 100644 --- a/godot-core/src/builtin/packed_array.rs +++ b/godot-core/src/builtin/packed_array.rs @@ -71,7 +71,6 @@ macro_rules! impl_packed_array { /// but any writes must be externally synchronized. The Rust compiler will enforce this as /// long as you use only Rust threads, but it cannot protect against concurrent modification /// on other threads (e.g. created through GDScript). - #[repr(C)] pub struct $PackedArray { opaque: $Opaque, } diff --git a/godot-core/src/builtin/signal.rs b/godot-core/src/builtin/signal.rs index cf6f0477f..c3eb6f8e4 100644 --- a/godot-core/src/builtin/signal.rs +++ b/godot-core/src/builtin/signal.rs @@ -21,7 +21,6 @@ use sys::{ffi_methods, GodotFfi}; /// A `Signal` represents a signal of an Object instance in Godot. /// /// Signals are composed of a reference to an `Object` and the name of the signal on this object. -#[repr(C, align(8))] pub struct Signal { opaque: sys::types::OpaqueSignal, } diff --git a/godot-core/src/builtin/variant/mod.rs b/godot-core/src/builtin/variant/mod.rs index ab25dfd1f..ceac24d16 100644 --- a/godot-core/src/builtin/variant/mod.rs +++ b/godot-core/src/builtin/variant/mod.rs @@ -26,7 +26,7 @@ pub use sys::{VariantOperator, VariantType}; /// See also [Godot documentation for `Variant`](https://docs.godotengine.org/en/stable/classes/class_variant.html). #[repr(transparent)] pub struct Variant { - opaque: OpaqueVariant, + _opaque: OpaqueVariant, } impl Variant { @@ -236,7 +236,7 @@ impl Variant { Self::new_with_var_uninit(init_fn) } - /// Fallible construction of a `Variant` using a function. + /// Fallible construction of a `Variant` using a fallible initialization function. /// /// # Safety /// @@ -262,7 +262,7 @@ impl Variant { /// /// `ptr` must point to a live `Variant` for the duration of `'a`. pub(crate) unsafe fn borrow_var_sys<'a>(ptr: sys::GDExtensionConstVariantPtr) -> &'a Variant { - sys::static_assert_eq_size!(Variant, sys::types::OpaqueVariant); + sys::static_assert_eq_size_align!(Variant, sys::types::OpaqueVariant); // SAFETY: `ptr` is a pointer to a live `Variant` for the duration of `'a`. unsafe { &*(ptr.cast::()) } @@ -278,6 +278,7 @@ impl Variant { variant_ptr_array: *const sys::GDExtensionConstVariantPtr, length: usize, ) -> &'a [&'a Variant] { + sys::static_assert_eq_size_align!(Variant, sys::types::OpaqueVariant); // Godot may pass null to signal "no arguments" (e.g. in custom callables). if variant_ptr_array.is_null() { debug_assert_eq!( @@ -305,7 +306,7 @@ impl Variant { variant_array: sys::GDExtensionConstVariantPtr, length: usize, ) -> &'a [Variant] { - sys::static_assert_eq_size!(Variant, sys::types::OpaqueVariant); + sys::static_assert_eq_size_align!(Variant, sys::types::OpaqueVariant); // Godot may pass null to signal "no arguments" (e.g. in custom callables). if variant_array.is_null() { @@ -320,7 +321,7 @@ impl Variant { // See https://doc.rust-lang.org/reference/type-layout.html#pointers-and-references-layout. let variant_array = variant_array.cast::(); - // SAFETY: `variant_array` isn't null so it is safe to call `from_raw_parts` on the pointer cast to `*const &Variant`. + // SAFETY: `variant_array` isn't null so it is safe to call `from_raw_parts` on the pointer cast to `*const Variant`. unsafe { std::slice::from_raw_parts(variant_array, length) } } @@ -334,7 +335,7 @@ impl Variant { variant_array: sys::GDExtensionVariantPtr, length: usize, ) -> &'a mut [Variant] { - sys::static_assert_eq_size!(Variant, sys::types::OpaqueVariant); + sys::static_assert_eq_size_align!(Variant, sys::types::OpaqueVariant); // Godot may pass null to signal "no arguments" (e.g. in custom callables). if variant_array.is_null() { @@ -349,7 +350,7 @@ impl Variant { // See https://doc.rust-lang.org/reference/type-layout.html#pointers-and-references-layout. let variant_array = variant_array.cast::(); - // SAFETY: `variant_array` isn't null so it is safe to call `from_raw_parts_mut` on the pointer cast to `*const &Variant`. + // SAFETY: `variant_array` isn't null so it is safe to call `from_raw_parts_mut` on the pointer cast to `*mut Variant`. unsafe { std::slice::from_raw_parts_mut(variant_array, length) } } } diff --git a/godot-ffi/src/toolbox.rs b/godot-ffi/src/toolbox.rs index 5bd39f790..c597c336c 100644 --- a/godot-ffi/src/toolbox.rs +++ b/godot-ffi/src/toolbox.rs @@ -35,6 +35,24 @@ macro_rules! static_assert_eq_size { }; } +/// Verifies at compile time that two types `T` and `U` have the same size and alignment. +#[macro_export] +macro_rules! static_assert_eq_size_align { + ($T:ty, $U:ty) => { + godot_ffi::static_assert!( + std::mem::size_of::<$T>() == std::mem::size_of::<$U>() + && std::mem::align_of::<$T>() == std::mem::align_of::<$U>() + ); + }; + ($T:ty, $U:ty, $msg:literal) => { + godot_ffi::static_assert!( + std::mem::size_of::<$T>() == std::mem::size_of::<$U>() + && std::mem::align_of::<$T>() == std::mem::align_of::<$U>(), + $msg + ); + }; +} + /// Trace output. #[cfg(feature = "trace")] #[macro_export] From 4c58c1aae2ebbaf822a5cba0b01cec82c4649748 Mon Sep 17 00:00:00 2001 From: Lili Zoey Zyli Date: Wed, 13 Mar 2024 17:59:59 +0100 Subject: [PATCH 7/9] Tweaks --- godot-core/src/builtin/array.rs | 4 +--- godot-core/src/builtin/callable.rs | 6 ++---- godot-core/src/builtin/dictionary.rs | 4 +--- godot-core/src/builtin/string/gstring.rs | 15 ++++++++++++++- godot-core/src/builtin/variant/mod.rs | 15 ++++++++++++++- godot-core/src/engine/script_instance.rs | 4 ++-- godot-core/src/registry/callbacks.rs | 4 ++-- 7 files changed, 36 insertions(+), 16 deletions(-) diff --git a/godot-core/src/builtin/array.rs b/godot-core/src/builtin/array.rs index 5d1886416..5fcefaf9f 100644 --- a/godot-core/src/builtin/array.rs +++ b/godot-core/src/builtin/array.rs @@ -659,9 +659,7 @@ impl Array { // SAFETY: `ptr_mut` just checked that the index is not out of bounds. unsafe { - value - .to_variant() - .move_return_var_ptr(ptr_mut, sys::PtrcallType::Standard); + value.to_variant().move_into_var_ptr(ptr_mut); } } diff --git a/godot-core/src/builtin/callable.rs b/godot-core/src/builtin/callable.rs index c8374f3c1..6cc8cac81 100644 --- a/godot-core/src/builtin/callable.rs +++ b/godot-core/src/builtin/callable.rs @@ -445,7 +445,7 @@ mod custom_callable { let c: &T = CallableUserdata::inner_from_raw(callable_userdata); let s = crate::builtin::GString::from(c.to_string()); - s.move_return_string_ptr(r_out, sys::PtrcallType::Standard); + s.move_into_string_ptr(r_out); *r_is_valid = true as sys::GDExtensionBool; } @@ -456,9 +456,7 @@ mod custom_callable { ) { let w: &mut FnWrapper = CallableUserdata::inner_from_raw(callable_userdata); - w.name - .clone() - .move_return_string_ptr(r_out, sys::PtrcallType::Standard); + w.name.clone().move_into_string_ptr(r_out); *r_is_valid = true as sys::GDExtensionBool; } } diff --git a/godot-core/src/builtin/dictionary.rs b/godot-core/src/builtin/dictionary.rs index 4fdd93dc9..242fca528 100644 --- a/godot-core/src/builtin/dictionary.rs +++ b/godot-core/src/builtin/dictionary.rs @@ -205,9 +205,7 @@ impl Dictionary { // SAFETY: `self.get_ptr_mut(key)` always returns a valid pointer to a value in the dictionary; either pre-existing or newly inserted. unsafe { - value - .to_variant() - .move_return_var_ptr(self.get_ptr_mut(key), sys::PtrcallType::Standard); + value.to_variant().move_into_var_ptr(self.get_ptr_mut(key)); } } diff --git a/godot-core/src/builtin/string/gstring.rs b/godot-core/src/builtin/string/gstring.rs index fbf7b4a5d..8f52bcf2b 100644 --- a/godot-core/src/builtin/string/gstring.rs +++ b/godot-core/src/builtin/string/gstring.rs @@ -125,7 +125,20 @@ impl GString { fn new_with_string_uninit = new_with_uninit; fn string_sys = sys; fn string_sys_mut = sys_mut; - fn move_return_string_ptr = move_return_ptr; + } + + /// Moves this string into a string sys pointer. This is the same as using [`GodotFfi::move_return_ptr`]. + /// + /// # Safety + /// + /// `dst` must be a valid string pointer. + pub(crate) unsafe fn move_into_string_ptr(self, dst: sys::GDExtensionStringPtr) { + let dst: sys::GDExtensionTypePtr = dst.cast(); + // SAFETY: `dst` is a valid string pointer. Additionally `String` doesn't behave differently for `Standard` and `Virtual` + // pointer calls. + unsafe { + self.move_return_ptr(dst, sys::PtrcallType::Standard); + } } #[doc(hidden)] diff --git a/godot-core/src/builtin/variant/mod.rs b/godot-core/src/builtin/variant/mod.rs index ceac24d16..3a3bd3d62 100644 --- a/godot-core/src/builtin/variant/mod.rs +++ b/godot-core/src/builtin/variant/mod.rs @@ -213,7 +213,20 @@ impl Variant { fn new_with_var_init = new_with_init; fn var_sys = sys; fn var_sys_mut = sys_mut; - fn move_return_var_ptr = move_return_ptr; + } + + /// Moves this variant into a variant sys pointer. This is the same as using [`GodotFfi::move_return_ptr`]. + /// + /// # Safety + /// + /// `dst` must be a valid variant pointer. + pub(crate) unsafe fn move_into_var_ptr(self, dst: sys::GDExtensionVariantPtr) { + let dst: sys::GDExtensionTypePtr = dst.cast(); + // SAFETY: `dst` is a valid string pointer. Additionally `Variant` doesn't behave differently for `Standard` and `Virtual` + // pointer calls. + unsafe { + self.move_return_ptr(dst, sys::PtrcallType::Standard); + } } /// # Safety diff --git a/godot-core/src/engine/script_instance.rs b/godot-core/src/engine/script_instance.rs index 3033a462c..e767a44ce 100644 --- a/godot-core/src/engine/script_instance.rs +++ b/godot-core/src/engine/script_instance.rs @@ -358,7 +358,7 @@ mod script_instance_info { /// /// - We expect the engine to provide a valid variant pointer the return value can be moved into. unsafe fn transfer_variant_to_godot(variant: Variant, return_ptr: sys::GDExtensionVariantPtr) { - variant.move_return_var_ptr(return_ptr, sys::PtrcallType::Standard) + variant.move_into_var_ptr(return_ptr) } /// # Safety @@ -390,7 +390,7 @@ mod script_instance_info { /// /// - The engine has to provide a valid string return pointer. unsafe fn transfer_string_to_godot(string: GString, return_ptr: sys::GDExtensionStringPtr) { - string.move_return_string_ptr(return_ptr, sys::PtrcallType::Standard); + string.move_into_string_ptr(return_ptr); } /// # Safety diff --git a/godot-core/src/registry/callbacks.rs b/godot-core/src/registry/callbacks.rs index 3abf6f510..f27cdd6cd 100644 --- a/godot-core/src/registry/callbacks.rs +++ b/godot-core/src/registry/callbacks.rs @@ -133,7 +133,7 @@ pub unsafe extern "C" fn to_string( let string = T::__godot_to_string(&*instance); // Transfer ownership to Godot - string.move_return_string_ptr(out_string, sys::PtrcallType::Standard); + string.move_into_string_ptr(out_string); } #[cfg(before_api = "4.2")] @@ -170,7 +170,7 @@ pub unsafe extern "C" fn get_property( match T::__godot_get_property(&*instance, property) { Some(value) => { - value.move_return_var_ptr(ret, sys::PtrcallType::Standard); + value.move_into_var_ptr(ret); true as sys::GDExtensionBool } None => false as sys::GDExtensionBool, From 57cc60d8fb5a2fe1a44ab5515fdcf7cf7c9c531a Mon Sep 17 00:00:00 2001 From: Lili Zoey Zyli Date: Sat, 16 Mar 2024 03:02:38 +0100 Subject: [PATCH 8/9] Final touchups. --- godot-ffi/src/godot_ffi.rs | 26 ++++++-------------------- 1 file changed, 6 insertions(+), 20 deletions(-) diff --git a/godot-ffi/src/godot_ffi.rs b/godot-ffi/src/godot_ffi.rs index 3e2370d80..d24f8295c 100644 --- a/godot-ffi/src/godot_ffi.rs +++ b/godot-ffi/src/godot_ffi.rs @@ -29,6 +29,10 @@ pub unsafe trait GodotFfi { /// Construct from Godot opaque pointer. /// + /// This will increment reference counts if the type is reference counted. If you need to avoid this then a `borrow_sys` method should + /// usually be used. Which is a method that takes a sys-pointer and returns it as a `&Self` reference. This must be manually implemented for + /// each relevant type as not all types can be borrowed like this. + /// /// # Safety /// `ptr` must be a valid _type ptr_: it must follow Godot's convention to encode `Self`, /// which is different depending on the type. @@ -42,8 +46,8 @@ pub unsafe trait GodotFfi { #[doc(hidden)] unsafe fn new_with_uninit(init_fn: impl FnOnce(sys::GDExtensionUninitializedTypePtr)) -> Self; - /// Like [`Self::from_sys_init`], but pre-initializes the sys pointer to a `Default::default()` instance - /// before calling `init_fn`. + /// Like [`new_with_uninit`](GodotFfi::new_with_uninit), but pre-initializes the sys pointer to a default instance (usually + /// [`Default::default()`]) before calling `init_fn`. /// /// Some FFI functions in Godot expect a pre-existing instance at the destination pointer, e.g. CoW/ref-counted /// builtin types like `Array`, `Dictionary`, `String`, `StringName`. @@ -302,11 +306,6 @@ macro_rules! ffi_methods_rest { /// The **address of** the `Opaque` field is used as the sys pointer. /// Expects a `from_opaque()` constructor and a `opaque` field. /// -/// * `Opaque`
-/// Implements FFI methods for a type with `Opaque` data. -/// The sys pointer is directly reinterpreted from/to the `Opaque` and **not** its address. -/// Expects a `from_opaque()` constructor and a `opaque` field. -/// /// * `*mut Self`
/// Implements FFI methods for a type implemented with standard Rust fields (not opaque). /// The address of `Self` is directly reinterpreted as the sys pointer. @@ -321,12 +320,6 @@ macro_rules! ffi_methods_rest { /// dereferenced argument pointer. /// Returning a value from a pointer call is simply calling [`std::ptr::swap`] on the return pointer /// and the address to the `opaque` field. -/// -/// ## Using `Opaque` -/// -/// Turning pointer call arguments into a value is simply calling `from_opaque` on the argument pointer. -/// Returning a value from a pointer call is simply calling [`std::ptr::swap`] on the return pointer -/// and the `opaque` field transmuted into a pointer. /// /// ## Using `*mut Self` /// @@ -341,13 +334,6 @@ macro_rules! ffi_methods { $crate::ffi_methods_rest!(OpaquePtr $Ptr; $($rest)*); }; - ( // Sys pointer = value of opaque - type $Ptr:ty = Opaque; - $( $rest:tt )* - ) => { - $crate::ffi_methods_rest!(OpaqueValue $Ptr; $($rest)*); - }; - ( // Sys pointer = address of self type $Ptr:ty = *mut Self; $( $rest:tt )* From 600d3e90adaa465d38abc70fcbaf5261cafcad43 Mon Sep 17 00:00:00 2001 From: Lili Zoey Zyli Date: Mon, 25 Mar 2024 18:01:41 +0100 Subject: [PATCH 9/9] Fixes from review --- godot-core/src/builtin/array.rs | 4 +- godot-core/src/builtin/callable.rs | 12 +++--- godot-core/src/builtin/meta/return_marshal.rs | 3 +- godot-core/src/builtin/plane.rs | 6 +-- godot-core/src/builtin/rid.rs | 10 ++--- godot-core/src/builtin/signal.rs | 6 +-- godot-core/src/builtin/string/gstring.rs | 7 +--- godot-core/src/builtin/string/string_name.rs | 4 +- godot-core/src/builtin/variant/mod.rs | 41 +++++++++++-------- godot-core/src/engine/script_instance.rs | 2 +- godot-core/src/obj/gd.rs | 12 +++++- godot-core/src/obj/raw.rs | 9 +++- godot-ffi/src/godot_ffi.rs | 29 ++++++++----- godot-ffi/src/toolbox.rs | 11 ----- 14 files changed, 86 insertions(+), 70 deletions(-) diff --git a/godot-core/src/builtin/array.rs b/godot-core/src/builtin/array.rs index 5fcefaf9f..83e104c23 100644 --- a/godot-core/src/builtin/array.rs +++ b/godot-core/src/builtin/array.rs @@ -969,7 +969,7 @@ impl From<&[T]> for Array { // SAFETY: `array` has `len` elements since we just resized it, and they are all valid `Variant`s. Additionally, since // the array was created in this function, and we do not access the array while this slice exists, the slice has unique // access to the elements. - let elements = unsafe { Variant::borrow_var_slice_mut(array.ptr_mut(0), len) }; + let elements = unsafe { Variant::borrow_slice_mut(array.ptr_mut(0), len) }; for (element, array_slot) in slice.iter().zip(elements.iter_mut()) { *array_slot = element.to_variant(); } @@ -1009,7 +1009,7 @@ impl From<&Array> for Vec { // SAFETY: Unless `experimental-threads` is enabled, then we cannot have concurrent access to this array. // And since we dont concurrently access the array in this function, we can create a slice to its contents. - let elements = unsafe { Variant::borrow_var_slice(array.ptr(0), len) }; + let elements = unsafe { Variant::borrow_slice(array.ptr(0), len) }; vec.extend(elements.iter().map(T::from_variant)); diff --git a/godot-core/src/builtin/callable.rs b/godot-core/src/builtin/callable.rs index 6cc8cac81..eebeb27be 100644 --- a/godot-core/src/builtin/callable.rs +++ b/godot-core/src/builtin/callable.rs @@ -296,14 +296,14 @@ unsafe impl GodotFfi for Callable { ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; fn new_from_sys; + fn new_with_uninit; + fn from_arg_ptr; fn sys; fn sys_mut; - fn new_with_uninit; fn move_return_ptr; - fn from_arg_ptr; } - unsafe fn new_with_init(init_fn: impl FnOnce(&mut Self)) -> Self { + fn new_with_init(init_fn: impl FnOnce(&mut Self)) -> Self { let mut result = Self::invalid(); init_fn(&mut result); result @@ -386,8 +386,7 @@ mod custom_callable { r_return: sys::GDExtensionVariantPtr, r_error: *mut sys::GDExtensionCallError, ) { - let arg_refs: &[&Variant] = - Variant::borrow_var_ref_slice(p_args, p_argument_count as usize); + let arg_refs: &[&Variant] = Variant::borrow_ref_slice(p_args, p_argument_count as usize); let c: &mut C = CallableUserdata::inner_from_raw(callable_userdata); @@ -404,8 +403,7 @@ mod custom_callable { ) where F: FnMut(&[&Variant]) -> Result, { - let arg_refs: &[&Variant] = - Variant::borrow_var_ref_slice(p_args, p_argument_count as usize); + let arg_refs: &[&Variant] = Variant::borrow_ref_slice(p_args, p_argument_count as usize); let w: &mut FnWrapper = CallableUserdata::inner_from_raw(callable_userdata); diff --git a/godot-core/src/builtin/meta/return_marshal.rs b/godot-core/src/builtin/meta/return_marshal.rs index 213dd5d43..23eaa62dd 100644 --- a/godot-core/src/builtin/meta/return_marshal.rs +++ b/godot-core/src/builtin/meta/return_marshal.rs @@ -7,6 +7,7 @@ use crate::obj::{Gd, GodotClass}; use crate::sys; +use sys::GodotFfi; use super::{ConvertError, FromGodot, GodotType}; @@ -51,7 +52,7 @@ impl PtrcallReturn for PtrcallReturnT { mut process_return_ptr: impl FnMut(sys::GDExtensionTypePtr), ) -> Result { let ffi = <::Ffi as sys::GodotFfi>::new_with_init(|return_val| { - let return_ptr = sys::GodotFfi::sys_mut(return_val); + let return_ptr = return_val.sys_mut(); process_return_ptr(return_ptr) }); diff --git a/godot-core/src/builtin/plane.rs b/godot-core/src/builtin/plane.rs index 77864d1d3..ef069f7dd 100644 --- a/godot-core/src/builtin/plane.rs +++ b/godot-core/src/builtin/plane.rs @@ -269,14 +269,14 @@ unsafe impl GodotFfi for Plane { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; fn new_from_sys; - fn sys; - fn sys_mut; fn new_with_uninit; fn from_arg_ptr; + fn sys; + fn sys_mut; fn move_return_ptr; } - unsafe fn new_with_init(init: impl FnOnce(&mut Self)) -> Self { + fn new_with_init(init: impl FnOnce(&mut Self)) -> Self { let mut plane = Plane::new(Vector3::UP, 0.0); init(&mut plane); plane diff --git a/godot-core/src/builtin/rid.rs b/godot-core/src/builtin/rid.rs index 933fa66d5..08837ed10 100644 --- a/godot-core/src/builtin/rid.rs +++ b/godot-core/src/builtin/rid.rs @@ -8,7 +8,7 @@ use std::num::NonZeroU64; use godot_ffi as sys; -use sys::{ffi_methods, static_assert, static_assert_eq_size, GodotFfi}; +use sys::{ffi_methods, static_assert, static_assert_eq_size_align, GodotFfi}; use super::meta::impl_godot_as_self; @@ -45,7 +45,7 @@ pub enum Rid { // Ensure that `Rid`s actually have the layout we expect. Since `Rid` has the same size as `u64`, it cannot // have any padding. As the `Valid` variant must take up all but one of the niches (since it contains a // `NonZerou64`), and the `Invalid` variant must take up the final niche. -static_assert_eq_size!(Rid, u64); +static_assert_eq_size_align!(Rid, u64); // SAFETY: // As Rid and u64 have the same size, and `Rid::Invalid` is initialized, it must be represented by some `u64` @@ -125,14 +125,14 @@ unsafe impl GodotFfi for Rid { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; fn new_from_sys; - fn sys; - fn sys_mut; fn new_with_uninit; fn from_arg_ptr; + fn sys; + fn sys_mut; fn move_return_ptr; } - unsafe fn new_with_init(init: impl FnOnce(&mut Self)) -> Self { + fn new_with_init(init: impl FnOnce(&mut Self)) -> Self { let mut rid = Self::Invalid; init(&mut rid); rid diff --git a/godot-core/src/builtin/signal.rs b/godot-core/src/builtin/signal.rs index c3eb6f8e4..cd3a9a9df 100644 --- a/godot-core/src/builtin/signal.rs +++ b/godot-core/src/builtin/signal.rs @@ -163,14 +163,14 @@ unsafe impl GodotFfi for Signal { ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; fn new_from_sys; + fn new_with_uninit; + fn from_arg_ptr; fn sys; fn sys_mut; - fn new_with_uninit; fn move_return_ptr; - fn from_arg_ptr; } - unsafe fn new_with_init(init_fn: impl FnOnce(&mut Self)) -> Self { + fn new_with_init(init_fn: impl FnOnce(&mut Self)) -> Self { let mut result = Self::invalid(); init_fn(&mut result); result diff --git a/godot-core/src/builtin/string/gstring.rs b/godot-core/src/builtin/string/gstring.rs index 8f52bcf2b..9b0c6eff5 100644 --- a/godot-core/src/builtin/string/gstring.rs +++ b/godot-core/src/builtin/string/gstring.rs @@ -134,11 +134,8 @@ impl GString { /// `dst` must be a valid string pointer. pub(crate) unsafe fn move_into_string_ptr(self, dst: sys::GDExtensionStringPtr) { let dst: sys::GDExtensionTypePtr = dst.cast(); - // SAFETY: `dst` is a valid string pointer. Additionally `String` doesn't behave differently for `Standard` and `Virtual` - // pointer calls. - unsafe { - self.move_return_ptr(dst, sys::PtrcallType::Standard); - } + + self.move_return_ptr(dst, sys::PtrcallType::Standard); } #[doc(hidden)] diff --git a/godot-core/src/builtin/string/string_name.rs b/godot-core/src/builtin/string/string_name.rs index 6197ed127..d9ff97b60 100644 --- a/godot-core/src/builtin/string/string_name.rs +++ b/godot-core/src/builtin/string/string_name.rs @@ -122,7 +122,7 @@ impl StringName { fn string_sys_mut = sys_mut; } - /// Convert a `StringName` sys pointer to a reference to a `StringName`. + /// Convert a `StringName` sys pointer to a reference with unbounded lifetime. /// /// # Safety /// @@ -130,7 +130,7 @@ impl StringName { pub(crate) unsafe fn borrow_string_sys<'a>( ptr: sys::GDExtensionConstStringNamePtr, ) -> &'a StringName { - sys::static_assert_eq_size!(StringName, sys::types::OpaqueStringName); + sys::static_assert_eq_size_align!(StringName, sys::types::OpaqueStringName); &*(ptr.cast::()) } diff --git a/godot-core/src/builtin/variant/mod.rs b/godot-core/src/builtin/variant/mod.rs index 3a3bd3d62..47be9bc7d 100644 --- a/godot-core/src/builtin/variant/mod.rs +++ b/godot-core/src/builtin/variant/mod.rs @@ -24,6 +24,7 @@ pub use sys::{VariantOperator, VariantType}; /// value. /// /// See also [Godot documentation for `Variant`](https://docs.godotengine.org/en/stable/classes/class_variant.html). +// We rely on the layout of `Variant` being the same as Godot's layout in `borrow_slice` and `borrow_slice_mut`. #[repr(transparent)] pub struct Variant { _opaque: OpaqueVariant, @@ -214,7 +215,13 @@ impl Variant { fn var_sys = sys; fn var_sys_mut = sys_mut; } +} +// All manually implemented unsafe functions on `Variant`. +// Deny `unsafe_op_in_unsafe_fn` so we don't forget to check safety invariants. +#[doc(hidden)] +#[deny(unsafe_op_in_unsafe_fn)] +impl Variant { /// Moves this variant into a variant sys pointer. This is the same as using [`GodotFfi::move_return_ptr`]. /// /// # Safety @@ -222,7 +229,7 @@ impl Variant { /// `dst` must be a valid variant pointer. pub(crate) unsafe fn move_into_var_ptr(self, dst: sys::GDExtensionVariantPtr) { let dst: sys::GDExtensionTypePtr = dst.cast(); - // SAFETY: `dst` is a valid string pointer. Additionally `Variant` doesn't behave differently for `Standard` and `Virtual` + // SAFETY: `dst` is a valid Variant pointer. Additionally `Variant` doesn't behave differently for `Standard` and `Virtual` // pointer calls. unsafe { self.move_return_ptr(dst, sys::PtrcallType::Standard); @@ -230,23 +237,29 @@ impl Variant { } /// # Safety - /// See [`GodotFfi::new_with_uninit`] and [`GodotFfi::new_with_init`]. + /// + /// For Godot 4.0, see [`GodotFfi::new_with_init`]. + /// For all other versions, see [`GodotFfi::new_with_uninit`]. #[cfg(before_api = "4.1")] #[doc(hidden)] pub unsafe fn new_with_var_uninit_or_init( init_fn: impl FnOnce(sys::GDExtensionVariantPtr), ) -> Self { + // SAFETY: We're in Godot 4.0, and so the caller must ensure this is safe. Self::new_with_var_init(|value| init_fn(value.var_sys_mut())) } /// # Safety - /// See [`GodotFfi::new_with_uninit`] and [`GodotFfi::new_with_init`]. + /// + /// For Godot 4.0, see [`GodotFfi::new_with_init`]. + /// For all other versions, see [`GodotFfi::new_with_uninit`]. #[cfg(since_api = "4.1")] #[doc(hidden)] pub unsafe fn new_with_var_uninit_or_init( init_fn: impl FnOnce(sys::GDExtensionUninitializedVariantPtr), ) -> Self { - Self::new_with_var_uninit(init_fn) + // SAFETY: We're not in Godot 4.0, and so the caller must ensure this is safe. + unsafe { Self::new_with_var_uninit(init_fn) } } /// Fallible construction of a `Variant` using a fallible initialization function. @@ -266,7 +279,7 @@ impl Variant { raw.as_mut_ptr() as ::Uninit; // SAFETY: `map` only runs the provided closure for the `Ok(())` variant, in which case `raw` has definitely been initialized. - init_fn(var_uninit_ptr).map(|_succ| unsafe { raw.assume_init() }) + init_fn(var_uninit_ptr).map(|_success| unsafe { raw.assume_init() }) } /// Convert a `Variant` sys pointer to a reference to a `Variant`. @@ -281,13 +294,13 @@ impl Variant { unsafe { &*(ptr.cast::()) } } - /// Convert an array of `Variant` sys pointers to a slice of `Variant` references. + /// Convert an array of `Variant` sys pointers to a slice of `Variant` references all with unbounded lifetimes. /// /// # Safety /// /// Either `variant_ptr_array` is null, or it must be safe to call [`std::slice::from_raw_parts`] with /// `variant_ptr_array` cast to `*const &'a Variant` and `length`. - pub(crate) unsafe fn borrow_var_ref_slice<'a>( + pub(crate) unsafe fn borrow_ref_slice<'a>( variant_ptr_array: *const sys::GDExtensionConstVariantPtr, length: usize, ) -> &'a [&'a Variant] { @@ -301,7 +314,7 @@ impl Variant { return &[]; } - // raw pointers and references have the same memory layout. + // Note: Raw pointers and references have the same memory layout. // See https://doc.rust-lang.org/reference/type-layout.html#pointers-and-references-layout. let variant_ptr_array = variant_ptr_array.cast::<&Variant>(); @@ -309,13 +322,13 @@ impl Variant { unsafe { std::slice::from_raw_parts(variant_ptr_array, length) } } - /// Convert an array of `Variant` sys pointers to a slice of mutable `Variant` references. + /// Convert an array of `Variant` sys pointers to a slice with unbounded lifetime. /// /// # Safety /// /// Either `variant_array` is null, or it must be safe to call [`std::slice::from_raw_parts`] with /// `variant_array` cast to `*const Variant` and `length`. - pub(crate) unsafe fn borrow_var_slice<'a>( + pub(crate) unsafe fn borrow_slice<'a>( variant_array: sys::GDExtensionConstVariantPtr, length: usize, ) -> &'a [Variant] { @@ -330,21 +343,19 @@ impl Variant { return &[]; } - // raw pointers and references have the same memory layout. - // See https://doc.rust-lang.org/reference/type-layout.html#pointers-and-references-layout. let variant_array = variant_array.cast::(); // SAFETY: `variant_array` isn't null so it is safe to call `from_raw_parts` on the pointer cast to `*const Variant`. unsafe { std::slice::from_raw_parts(variant_array, length) } } - /// Convert an array of `Variant` sys pointers to a slice of mutable `Variant` references. + /// Convert an array of `Variant` sys pointers to a mutable slice with unbounded lifetime. /// /// # Safety /// /// Either `variant_array` is null, or it must be safe to call [`std::slice::from_raw_parts_mut`] with /// `variant_array` cast to `*mut Variant` and `length`. - pub(crate) unsafe fn borrow_var_slice_mut<'a>( + pub(crate) unsafe fn borrow_slice_mut<'a>( variant_array: sys::GDExtensionVariantPtr, length: usize, ) -> &'a mut [Variant] { @@ -359,8 +370,6 @@ impl Variant { return &mut []; } - // raw pointers and references have the same memory layout. - // See https://doc.rust-lang.org/reference/type-layout.html#pointers-and-references-layout. let variant_array = variant_array.cast::(); // SAFETY: `variant_array` isn't null so it is safe to call `from_raw_parts_mut` on the pointer cast to `*mut Variant`. diff --git a/godot-core/src/engine/script_instance.rs b/godot-core/src/engine/script_instance.rs index e767a44ce..6af78402c 100644 --- a/godot-core/src/engine/script_instance.rs +++ b/godot-core/src/engine/script_instance.rs @@ -579,7 +579,7 @@ mod script_instance_info { r_error: *mut sys::GDExtensionCallError, ) { let method = StringName::new_from_string_sys(p_method); - let args = Variant::borrow_var_ref_slice(p_args, p_argument_count as usize); + let args = Variant::borrow_ref_slice(p_args, p_argument_count as usize); let ctx = || format!("error when calling {}::call", type_name::()); let result = handle_panic(ctx, || { diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index 23f4db345..7b4f687c9 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -10,7 +10,7 @@ use std::ops::{Deref, DerefMut}; use godot_ffi as sys; -use sys::{static_assert_eq_size, VariantType}; +use sys::{static_assert_eq_size_align, VariantType}; use crate::builtin::meta::{ CallContext, ConvertError, FromFfiError, FromGodot, GodotConvert, GodotType, ToGodot, @@ -94,7 +94,7 @@ pub struct Gd { } // Size equality check (should additionally be covered by mem::transmute()) -static_assert_eq_size!( +static_assert_eq_size_align!( sys::GDExtensionObjectPtr, sys::types::OpaqueObject, "Godot FFI: pointer type `Object*` should have size advertised in JSON extension file" @@ -407,6 +407,14 @@ impl Gd { self.raw.obj_sys() } + #[doc(hidden)] + pub fn script_sys(&self) -> sys::GDExtensionScriptLanguagePtr + where + T: Inherits, + { + self.raw.script_sys() + } + /// Returns a callable referencing a method from this object named `method_name`. /// /// This is shorter syntax for [`Callable::from_object_method(self, method_name)`][Callable::from_object_method]. diff --git a/godot-core/src/obj/raw.rs b/godot-core/src/obj/raw.rs index 2898bf77b..c044e8224 100644 --- a/godot-core/src/obj/raw.rs +++ b/godot-core/src/obj/raw.rs @@ -349,6 +349,13 @@ impl RawGd { pub(super) fn obj_sys(&self) -> sys::GDExtensionObjectPtr { self.obj as sys::GDExtensionObjectPtr } + + pub(super) fn script_sys(&self) -> sys::GDExtensionScriptLanguagePtr + where + T: super::Inherits, + { + self.obj.cast() + } } impl RawGd @@ -453,7 +460,7 @@ where Self::from_obj_sys_weak(obj) } - unsafe fn new_with_init(init: impl FnOnce(&mut Self)) -> Self { + fn new_with_init(init: impl FnOnce(&mut Self)) -> Self { let mut obj = Self { obj: std::ptr::null_mut(), cached_rtti: None, diff --git a/godot-ffi/src/godot_ffi.rs b/godot-ffi/src/godot_ffi.rs index d24f8295c..251468a61 100644 --- a/godot-ffi/src/godot_ffi.rs +++ b/godot-ffi/src/godot_ffi.rs @@ -15,8 +15,13 @@ use std::marker::PhantomData; /// /// # Safety /// -/// [`from_arg_ptr`](GodotFfi::from_arg_ptr) and [`move_return_ptr`](GodotFfi::move_return_ptr) +/// - [`from_arg_ptr`](GodotFfi::from_arg_ptr) and [`move_return_ptr`](GodotFfi::move_return_ptr) /// must properly initialize and clean up values given the [`PtrcallType`] provided by the caller. +/// +/// - [`new_with_uninit`](GodotFfi::new_with_uninit) must call `init_fn` with a pointer to a *new* +/// [allocated object](https://doc.rust-lang.org/std/ptr/index.html#safety). +/// +/// - [`new_with_init`](GodotFfi::new_with_init) must call `init_fn` with a reference to a *new* value. #[doc(hidden)] // shows up in implementors otherwise pub unsafe trait GodotFfi { #[doc(hidden)] @@ -29,9 +34,9 @@ pub unsafe trait GodotFfi { /// Construct from Godot opaque pointer. /// - /// This will increment reference counts if the type is reference counted. If you need to avoid this then a `borrow_sys` method should - /// usually be used. Which is a method that takes a sys-pointer and returns it as a `&Self` reference. This must be manually implemented for - /// each relevant type as not all types can be borrowed like this. + /// This will increment reference counts if the type is reference counted. If you need to avoid this, then a `borrow_sys` associated + /// function should usually be used. That function that takes a sys-pointer and returns it as a `&Self` reference. This must be manually + /// implemented for each relevant type, as not all types can be borrowed like this. /// /// # Safety /// `ptr` must be a valid _type ptr_: it must follow Godot's convention to encode `Self`, @@ -52,11 +57,13 @@ pub unsafe trait GodotFfi { /// Some FFI functions in Godot expect a pre-existing instance at the destination pointer, e.g. CoW/ref-counted /// builtin types like `Array`, `Dictionary`, `String`, `StringName`. /// - /// # Safety - /// While this does call `init_fn` with a `&mut Self` reference, that value may have a broken safety invariant. - /// So `init_fn` must ensure that the reference passed to it is fully initialized when the function returns. + /// # Note + /// + /// This does call `init_fn` with a `&mut Self` reference, but in some cases initializing the reference to a more appropriate + /// value may involve violating the value's safety invariant. In those cases it is important to ensure that this violation isn't + /// leaked to user-code. #[doc(hidden)] - unsafe fn new_with_init(init_fn: impl FnOnce(&mut Self)) -> Self; + fn new_with_init(init_fn: impl FnOnce(&mut Self)) -> Self; /// Return Godot opaque pointer, for an immutable operation. #[doc(hidden)] @@ -188,7 +195,7 @@ macro_rules! ffi_methods_one { }; (OpaquePtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $new_with_init:ident = new_with_init) => { $( #[$attr] )? $vis - unsafe fn $new_with_init(init: impl FnOnce(&mut Self)) -> Self { + fn $new_with_init(init: impl FnOnce(&mut Self)) -> Self { let mut default = Default::default(); init(&mut default); default @@ -238,7 +245,7 @@ macro_rules! ffi_methods_one { }; (SelfPtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $new_with_init:ident = new_with_init) => { $( #[$attr] )? $vis - unsafe fn $new_with_init(init: impl FnOnce(&mut Self)) -> Self { + fn $new_with_init(init: impl FnOnce(&mut Self)) -> Self { let mut default = Default::default(); init(&mut default); default @@ -484,7 +491,7 @@ mod scalars { // Do nothing } - unsafe fn new_with_init(_init: impl FnOnce(&mut ())) -> Self { + fn new_with_init(_init: impl FnOnce(&mut ())) -> Self { // Do nothing } diff --git a/godot-ffi/src/toolbox.rs b/godot-ffi/src/toolbox.rs index c597c336c..4b82c7c8b 100644 --- a/godot-ffi/src/toolbox.rs +++ b/godot-ffi/src/toolbox.rs @@ -24,17 +24,6 @@ macro_rules! static_assert { }; } -/// Verifies at compile time that two types `T` and `U` have the same size. -#[macro_export] -macro_rules! static_assert_eq_size { - ($T:ty, $U:ty) => { - godot_ffi::static_assert!(std::mem::size_of::<$T>() == std::mem::size_of::<$U>()); - }; - ($T:ty, $U:ty, $msg:literal) => { - godot_ffi::static_assert!(std::mem::size_of::<$T>() == std::mem::size_of::<$U>(), $msg); - }; -} - /// Verifies at compile time that two types `T` and `U` have the same size and alignment. #[macro_export] macro_rules! static_assert_eq_size_align {