diff --git a/godot-core/src/builtin/callable.rs b/godot-core/src/builtin/callable.rs index bd6a028a3..45a361ccf 100644 --- a/godot-core/src/builtin/callable.rs +++ b/godot-core/src/builtin/callable.rs @@ -46,7 +46,8 @@ impl Callable { unsafe { sys::from_sys_init_or_init_default::(|self_ptr| { let ctor = sys::builtin_fn!(callable_from_object_method); - let args = [object.to_ffi().as_arg_ptr(), method.sys_const()]; + let raw = object.to_ffi(); + let args = [raw.as_arg_ptr(), method.sys_const()]; ctor(self_ptr, args.as_ptr()); }) } diff --git a/godot-core/src/builtin/meta/godot_compat/impls.rs b/godot-core/src/builtin/meta/godot_compat/impls.rs index 23b98819a..21152e105 100644 --- a/godot-core/src/builtin/meta/godot_compat/impls.rs +++ b/godot-core/src/builtin/meta/godot_compat/impls.rs @@ -45,16 +45,16 @@ where } impl GodotCompatible for sys::VariantType { - type Via = i64; + type Via = i32; } impl ToGodot for sys::VariantType { fn to_godot(&self) -> Self::Via { - *self as i64 + *self as i32 } fn into_godot(self) -> Self::Via { - self as i64 + self as i32 } } @@ -65,16 +65,16 @@ impl FromGodot for sys::VariantType { } impl GodotCompatible for sys::VariantOperator { - type Via = i64; + type Via = i32; } impl ToGodot for sys::VariantOperator { fn to_godot(&self) -> Self::Via { - *self as i64 + *self as i32 } fn into_godot(self) -> Self::Via { - self as i64 + self as i32 } } diff --git a/godot-core/src/builtin/meta/mod.rs b/godot-core/src/builtin/meta/mod.rs index cce48934a..78f2f06b5 100644 --- a/godot-core/src/builtin/meta/mod.rs +++ b/godot-core/src/builtin/meta/mod.rs @@ -183,11 +183,6 @@ where } } -/// Stores meta-information about registered types or properties. -/// -/// Filling this information properly is important so that Godot can use ptrcalls instead of varcalls -/// (requires typed GDScript + sufficient information from the extension side) - // ---------------------------------------------------------------------------------------------------------------------------------------------- /// Rusty abstraction of `sys::GDExtensionPropertyInfo`. diff --git a/godot-core/src/builtin/meta/signature.rs b/godot-core/src/builtin/meta/signature.rs index cd2e5c325..1ccc06b2e 100644 --- a/godot-core/src/builtin/meta/signature.rs +++ b/godot-core/src/builtin/meta/signature.rs @@ -112,8 +112,6 @@ macro_rules! impl_varcall_signature_for_tuple { $R:ident $(, ($pn:ident, $n:tt) : $Pn:ident)* // $n cannot be literal if substituted as tuple index .0 ) => { - // R: FromVariantIndirect, Pn: ToVariant -> when calling engine APIs - // R: ToVariant, Pn: #[allow(unused_variables)] impl<$R, $($Pn,)*> VarcallSignatureTuple for ($R, $($Pn,)*) where @@ -400,7 +398,7 @@ unsafe fn varcall_return( /// # Safety /// See [`varcall_return`]. #[cfg(since_api = "4.2")] // unused before -pub(crate) unsafe fn varcall_return_checked( +pub(crate) unsafe fn varcall_return_checked( ret_val: Result, // TODO Err should be custom CallError enum ret: sys::GDExtensionVariantPtr, err: *mut sys::GDExtensionCallError, diff --git a/godot-core/src/builtin/string/string_name.rs b/godot-core/src/builtin/string/string_name.rs index a4d02af32..eb16a9fba 100644 --- a/godot-core/src/builtin/string/string_name.rs +++ b/godot-core/src/builtin/string/string_name.rs @@ -4,6 +4,8 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ +use std::fmt; + use godot_ffi as sys; use sys::{ffi_methods, GodotFfi}; @@ -11,8 +13,6 @@ use crate::builtin::inner; use crate::builtin::meta::impl_godot_as_self; use crate::builtin::{GodotString, NodePath}; -use std::fmt; - /// A string optimized for unique names. /// /// StringNames are immutable strings designed for representing unique names. StringName ensures that only diff --git a/godot-core/src/builtin/vectors/vector_axis.rs b/godot-core/src/builtin/vectors/vector_axis.rs index 64a99dca9..50b5c16a9 100644 --- a/godot-core/src/builtin/vectors/vector_axis.rs +++ b/godot-core/src/builtin/vectors/vector_axis.rs @@ -98,18 +98,18 @@ impl EngineEnum for Vector2Axis { } impl GodotCompatible for Vector2Axis { - type Via = i64; + type Via = i32; } impl ToGodot for Vector2Axis { fn to_godot(&self) -> Self::Via { - self.ord() as i64 + self.ord() } } impl FromGodot for Vector2Axis { fn try_from_godot(via: Self::Via) -> Option { - Self::try_from_ord(via as i32) + Self::try_from_ord(via) } } @@ -146,18 +146,18 @@ impl EngineEnum for Vector3Axis { } impl GodotCompatible for Vector3Axis { - type Via = i64; + type Via = i32; } impl ToGodot for Vector3Axis { fn to_godot(&self) -> Self::Via { - self.ord() as i64 + self.ord() } } impl FromGodot for Vector3Axis { fn try_from_godot(via: Self::Via) -> Option { - Self::try_from_ord(via as i32) + Self::try_from_ord(via) } } @@ -197,18 +197,18 @@ impl EngineEnum for Vector4Axis { } impl GodotCompatible for Vector4Axis { - type Via = i64; + type Via = i32; } impl ToGodot for Vector4Axis { fn to_godot(&self) -> Self::Via { - self.ord() as i64 + self.ord() } } impl FromGodot for Vector4Axis { fn try_from_godot(via: Self::Via) -> Option { - Self::try_from_ord(via as i32) + Self::try_from_ord(via) } } diff --git a/godot-core/src/engine.rs b/godot-core/src/engine.rs index 142999aca..4b2fec23f 100644 --- a/godot-core/src/engine.rs +++ b/godot-core/src/engine.rs @@ -6,8 +6,6 @@ //! Godot engine classes and methods. -use godot_ffi::GodotNullableFfi; - // Re-exports of generated symbols use crate::builtin::{GodotString, NodePath}; use crate::obj::dom::EngineDomain; @@ -223,7 +221,7 @@ pub(crate) fn ensure_object_alive( method_name: &'static str, ) { let Some(instance_id) = instance_id else { - panic!("{method_name}: instance id is null") + panic!("{method_name}: cannot call method on null object") }; let new_object_ptr = object_ptr_from_id(instance_id); diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index 155ac15e3..ffedc88f1 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -10,7 +10,7 @@ use std::ptr; use godot_ffi as sys; use godot_ffi::VariantType; -use sys::{interface_fn, static_assert_eq_size, GodotNullableFfi}; +use sys::static_assert_eq_size; use crate::builtin::meta::{FromGodot, GodotCompatible, GodotType, ToGodot}; use crate::builtin::{Callable, StringName}; @@ -149,28 +149,6 @@ where pub fn bind_mut(&mut self) -> GdMut { self.raw.bind_mut() } - - /// Storage object associated with the extension instance. - // pub(crate) fn storage_mut(&mut self) -> &mut InstanceStorage { - // // SAFETY: - // unsafe { - // let binding = self.resolve_instance_ptr(); - // crate::private::as_storage_mut::(binding) - // } - // } - - unsafe fn resolve_instance_ptr(&self) -> sys::GDExtensionClassInstancePtr { - let callbacks = crate::storage::nop_instance_callbacks(); - let token = sys::get_library() as *mut std::ffi::c_void; - let binding = interface_fn!(object_get_instance_binding)(self.obj_sys(), token, &callbacks); - - debug_assert!( - !binding.is_null(), - "Class {} -- null instance; does the class have a Godot creator function?", - std::any::type_name::() - ); - binding as sys::GDExtensionClassInstancePtr - } } /// _The methods in this impl block are available for any `T`._

@@ -182,13 +160,9 @@ impl Gd { pub fn try_from_instance_id(instance_id: InstanceId) -> Option { let ptr = engine::object_ptr_from_id(instance_id); - if ptr.is_null() { - None - } else { - // SAFETY: assumes that the returned GDExtensionObjectPtr is convertible to Object* (i.e. C++ upcast doesn't modify the pointer) - let untyped = unsafe { Gd::::from_obj_sys(ptr) }; - untyped.owned_cast::().ok() - } + // SAFETY: assumes that the returned GDExtensionObjectPtr is convertible to Object* (i.e. C++ upcast doesn't modify the pointer) + let untyped = unsafe { Gd::::from_obj_sys_or_none(ptr)? }; + untyped.owned_cast::().ok() } /// ⚠️ Looks up the given instance ID and returns the associated object. @@ -227,9 +201,13 @@ impl Gd { /// ⚠️ Returns the last known, possibly invalid instance ID of this object. /// /// This function does not check that the returned instance ID points to a valid instance! - /// Unless performance is a problem, use [`instance_id()`][Self::instance_id] or [`instance_id_or_none()`][Self::instance_id_or_none] instead. + /// Unless performance is a problem, use [`instance_id()`][Self::instance_id] or + /// [`instance_id_or_none()`][Self::instance_id_or_none] instead. pub fn instance_id_unchecked(&self) -> InstanceId { - self.raw.cached_instance_id.get().unwrap() + // SAFETY: + // A `Gd` can only be created from a non-null `RawGd`. Meaning `raw.instance_id_unchecked()` will + // always return `Some`. + unsafe { self.raw.instance_id_unchecked().unwrap_unchecked() } } /// Checks if this smart pointer points to a live object (read description!). @@ -308,23 +286,33 @@ impl Gd { .map_err(Self::from_ffi) } + #[doc(hidden)] + pub(crate) unsafe fn from_obj_sys_or_none(ptr: sys::GDExtensionObjectPtr) -> Option { + Self::try_from_ffi(RawGd::from_obj_sys(ptr)) + } + /// Initializes this `Gd` from the object pointer as a **strong ref**, meaning /// it initializes/increments the reference counter and keeps the object alive. /// /// This is the default for most initializations from FFI. In cases where reference counter /// should explicitly **not** be updated, [`Self::from_obj_sys_weak`] is available. #[doc(hidden)] - pub unsafe fn from_obj_sys(ptr: sys::GDExtensionObjectPtr) -> Self { - Self::from_ffi(RawGd::from_obj_sys(ptr)) + pub(crate) unsafe fn from_obj_sys(ptr: sys::GDExtensionObjectPtr) -> Self { + Self::from_obj_sys_or_none(ptr).unwrap() } #[doc(hidden)] - pub unsafe fn from_obj_sys_weak(ptr: sys::GDExtensionObjectPtr) -> Self { - Self::from_ffi(RawGd::from_obj_sys_weak(ptr)) + pub(crate) unsafe fn from_obj_sys_weak_or_none(ptr: sys::GDExtensionObjectPtr) -> Option { + Self::try_from_ffi(RawGd::from_obj_sys_weak(ptr)) } #[doc(hidden)] - pub fn obj_sys(&self) -> sys::GDExtensionObjectPtr { + pub(crate) unsafe fn from_obj_sys_weak(ptr: sys::GDExtensionObjectPtr) -> Self { + Self::from_obj_sys_weak_or_none(ptr).unwrap() + } + + #[doc(hidden)] + pub(crate) fn obj_sys(&self) -> sys::GDExtensionObjectPtr { self.raw.obj_sys() } @@ -341,34 +329,13 @@ impl Deref for Gd { type Target = <::Declarer as dom::Domain>::DerefTarget; fn deref(&self) -> &Self::Target { - // SAFETY: - // - // This relies on `Gd` having the layout as `Node3D` (as an example), - // which also needs #[repr(transparent)]: - // - // struct Gd { - // opaque: OpaqueObject, // <- size of GDExtensionObjectPtr - // cached_instance_id, // <- Cell is #[repr(transparent)] to its inner T - // _marker: PhantomData, // <- ZST - // } - // struct Node3D { - // object_ptr: sys::GDExtensionObjectPtr, - // } - self.raw.as_target() + self.raw.as_target().expect("`Gd` is never null") } } impl DerefMut for Gd { fn deref_mut(&mut self) -> &mut Self::Target { - // SAFETY: see also Deref - // - // The resulting `&mut T` is transmuted from `&mut OpaqueObject`, i.e. a *pointer* to the `opaque` field. - // `opaque` itself has a different *address* for each Gd instance, meaning that two simultaneous - // DerefMut borrows on two Gd instances will not alias, *even if* the underlying Godot object is the - // same (i.e. `opaque` has the same value, but not address). - // - // The `&mut self` guarantees that no other base access can take place for *the same Gd instance* (access to other Gds is OK). - self.raw.as_target_mut() + self.raw.as_target_mut().expect("`Gd` is never null") } } diff --git a/godot-core/src/obj/instance_id.rs b/godot-core/src/obj/instance_id.rs index b6e5cac30..644e8990d 100644 --- a/godot-core/src/obj/instance_id.rs +++ b/godot-core/src/obj/instance_id.rs @@ -89,28 +89,3 @@ impl FromGodot for InstanceId { Self::try_from_u64(via) } } - -/* -// Note: Option impl is only possible as long as `FromVariant` and `InstanceId` are in same crate. -// (Rust rationale: if upstream crate later adds blanket `impl FromVariant for Option`, this would collide) -impl FromVariant for Option { - fn try_from_variant(variant: &Variant) -> Result { - if variant.is_nil() { - Ok(None) - } else { - // Should 0 in variant be mapped to None, or cause an error like now? - i64::try_from_variant(variant).map(|i| InstanceId::try_from_i64(i)) - } - } -} - -impl ToVariant for Option { - fn to_variant(&self) -> Variant { - if let Some(id) = self { - id.to_variant() - } else { - 0i64.to_variant() - } - } -} -*/ diff --git a/godot-core/src/obj/raw.rs b/godot-core/src/obj/raw.rs index bd42a167d..646b102ee 100644 --- a/godot-core/src/obj/raw.rs +++ b/godot-core/src/obj/raw.rs @@ -20,20 +20,33 @@ use crate::obj::{GdMut, GdRef, InstanceId}; use crate::storage::InstanceStorage; use crate::{engine, out}; +/// Low-level bindings for object pointers in Godot. +/// +/// This should not be used directly, you should either use [`Gd`](super::Gd) or [`Option>`] +/// depending on whether you need a nullable object pointer or not. +#[repr(C)] pub struct RawGd { pub(super) obj: *mut T, - pub(super) cached_instance_id: std::cell::Cell>, + // Must not be changed after initialization. + cached_instance_id: Option, } impl RawGd { + /// Create a new object representing a null in Godot. pub(super) fn new_null() -> Self { Self { obj: ptr::null_mut(), - cached_instance_id: std::cell::Cell::new(None), + cached_instance_id: None, } } - pub(super) fn from_obj_sys_weak(obj: sys::GDExtensionObjectPtr) -> Self { + /// Initializes this `RawGd` from the object pointer as a **weak ref**, meaning it does not + /// initialize/increment the reference counter. + /// + /// # Safety + /// + /// `obj` must be a valid object pointer or a null pointer. + pub(super) unsafe fn from_obj_sys_weak(obj: sys::GDExtensionObjectPtr) -> Self { let mut instance_id = None; if !obj.is_null() { let id = @@ -43,53 +56,65 @@ impl RawGd { Self { obj: obj as *mut T, - cached_instance_id: std::cell::Cell::new(instance_id), + cached_instance_id: instance_id, } } - pub(super) fn from_obj_sys(obj: sys::GDExtensionObjectPtr) -> Self { + /// Initializes this `RawGd` from the object pointer as a **strong ref**, meaning it initializes + /// /increments the reference counter and keeps the object alive. + /// + /// This is the default for most initializations from FFI. In cases where reference counter + /// should explicitly **not** be updated, [`from_obj_sys_weak()`](Self::from_obj_sys_weak) is available. + /// + /// # Safety + /// + /// `obj` must be a valid object pointer or a null pointer. + pub(super) unsafe fn from_obj_sys(obj: sys::GDExtensionObjectPtr) -> Self { Self::from_obj_sys_weak(obj).with_inc_refcount() } - pub(crate) fn instance_id_or_none(&self) -> Option { - let known_id = match self.cached_instance_id.get() { - // Already dead - None => return None, + /// Returns `self` but with initialized ref-count. + fn with_inc_refcount(self) -> Self { + // Note: use init_ref and not inc_ref, since this might be the first reference increment. + // Godot expects RefCounted::init_ref to be called instead of RefCounted::reference in that case. + // init_ref also doesn't hurt (except 1 possibly unnecessary check). + if self.is_instance_valid() { + T::Mem::maybe_init_ref(&self); + } + self + } - // Possibly alive - Some(id) => id, - }; + /// Returns `true` if the object is null. + /// + /// This does not check if the object is dead, for that use + /// [`instance_id_or_none()`](Self::instance_id_or_none). + pub(crate) fn is_null(&self) -> bool { + self.obj.is_null() || self.cached_instance_id.is_none() + } + + /// Returns the instance ID of this object, or `None` if the object is dead or null. + pub(crate) fn instance_id_or_none(&self) -> Option { + let known_id = self.cached_instance_id?; // Refreshes the internal cached ID on every call, as we cannot be sure that the object has not been // destroyed since last time. The only reliable way to find out is to call is_instance_id_valid(). if engine::utilities::is_instance_id_valid(known_id.to_i64()) { Some(known_id) } else { - self.cached_instance_id.set(None); None } } - pub(super) fn is_instance_valid(&self) -> bool { - // This call refreshes the instance ID, and recognizes dead objects. - self.instance_id_or_none().is_some() + /// Returns the instance ID of this object, or `None` if the object is null. + /// + /// Does not check if the object is live, and will always return the same value for the same `RawGd`. + pub(super) fn instance_id_unchecked(&self) -> Option { + self.cached_instance_id } - // Note: does not transfer ownership and is thus unsafe. Also operates on shared ref. - // Either the parameter or the return value *must* be forgotten (since reference counts are not updated). - pub(super) unsafe fn ffi_cast(&self) -> Option> - where - U: GodotClass, - { - if self.is_null() { - return Some(RawGd::new_null()); - } - - let class_tag = interface_fn!(classdb_get_class_tag)(U::class_name().string_sys()); - let cast_object_ptr = interface_fn!(object_cast_to)(self.obj_sys(), class_tag); - - // Create weak object, as ownership will be moved and reference-counter stays the same - sys::ptr_then(cast_object_ptr, |ptr| RawGd::from_obj_sys_weak(ptr)) + /// Returns `true` if this object is neither dead nor null. + pub(super) fn is_instance_valid(&self) -> bool { + self.instance_id_or_none().is_some() } // See use-site for explanation. @@ -98,6 +123,7 @@ impl RawGd { U: GodotClass, { if self.is_null() { + // Null can be cast to anything. return true; } @@ -105,6 +131,7 @@ impl RawGd { unsafe { self.ffi_cast::() }.expect("Everything inherits object"); let cast_is_valid = as_obj .as_target() + .expect("object is not null") .is_class(U::class_name().to_godot_string()); std::mem::forget(as_obj); cast_is_valid @@ -139,15 +166,24 @@ impl RawGd { } } - /// Returns `self` but with initialized ref-count. - fn with_inc_refcount(self) -> Self { - // Note: use init_ref and not inc_ref, since this might be the first reference increment. - // Godot expects RefCounted::init_ref to be called instead of RefCounted::reference in that case. - // init_ref also doesn't hurt (except 1 possibly unnecessary check). - if self.is_instance_valid() { - T::Mem::maybe_init_ref(&self); + /// # Safety + /// Does not transfer ownership and is thus unsafe. Also operates on shared ref. Either the parameter or + /// the return value *must* be forgotten (since reference counts are not updated). + pub(super) unsafe fn ffi_cast(&self) -> Option> + where + U: GodotClass, + { + if self.is_null() { + // Null can be cast to anything. + // Forgetting a null doesn't do anything, since dropping a null also does nothing. + return Some(RawGd::new_null()); } - self + + let class_tag = interface_fn!(classdb_get_class_tag)(U::class_name().string_sys()); + let cast_object_ptr = interface_fn!(object_cast_to)(self.obj_sys(), class_tag); + + // Create weak object, as ownership will be moved and reference-counter stays the same + sys::ptr_then(cast_object_ptr, |ptr| RawGd::from_obj_sys_weak(ptr)) } pub(crate) fn as_ref_counted(&self, apply: impl Fn(&mut engine::RefCounted) -> R) -> R { @@ -178,46 +214,67 @@ impl RawGd { return_val } + // Target is always an engine class: + // * if T is an engine class => T + // * if T is a user class => T::Base pub(super) fn as_target( &self, - ) -> &<::Declarer as dom::Domain>::DerefTarget { + ) -> Option<&<::Declarer as dom::Domain>::DerefTarget> { + if self.is_null() { + return None; + } + // SAFETY: + // Every engine object is a struct like // - // This relies on `Gd.opaque` having the layout as `Node3D` (as an example), - // which also needs #[repr(transparent)]: - // - // struct Gd { - // opaque: OpaqueObject, // <- size of GDExtensionObjectPtr - // _marker: PhantomData, // <- ZST - // } + // #[repr(C)] // struct Node3D { - // object_ptr: sys::GDExtensionObjectPtr, + // object_ptr: sys::GDExtensionObjectPtr, // <- pointer + // instance_id: InstanceId, // <- non-zero u64 // } - unsafe { + // + // and `RawGd` looks like + // + // #[repr(C)] + // pub struct RawGd { + // pub(super) obj: *mut T, // <- pointer + // cached_instance_id: Option, // <- u64 + // } + // + // So since self isn't null, that means `cached_instance_id` is not 0, and the two layouts are + // compatible. + let target = unsafe { std::mem::transmute::< - &*mut T, + &Self, &<::Declarer as dom::Domain>::DerefTarget, - >(&self.obj) - } + >(self) + }; + + Some(target) } + // Target is always an engine class: + // * if T is an engine class => T + // * if T is a user class => T::Base pub(super) fn as_target_mut( &mut self, - ) -> &mut <::Declarer as dom::Domain>::DerefTarget { - // SAFETY: see also Deref - // - // The resulting `&mut T` is transmuted from `&mut OpaqueObject`, i.e. a *pointer* to the `opaque` field. - // `opaque` itself has a different *address* for each Gd instance, meaning that two simultaneous - // DerefMut borrows on two Gd instances will not alias, *even if* the underlying Godot object is the - // same (i.e. `opaque` has the same value, but not address). + ) -> Option<&mut <::Declarer as dom::Domain>::DerefTarget> { + if self.is_null() { + return None; + } + + // SAFETY: see also [`as_target()`](Self::as_target) // - // The `&mut self` guarantees that no other base access can take place for *the same Gd instance* (access to other Gds is OK). - unsafe { + // We have a mutable reference to self, and thus it's entirely safe to transmute that into a + // mutable reference to a compatible type. + let target = unsafe { std::mem::transmute::< - &mut *mut T, + &mut Self, &mut <::Declarer as dom::Domain>::DerefTarget, - >(&mut self.obj) - } + >(self) + }; + + Some(target) } pub(super) fn obj_sys(&self) -> sys::GDExtensionObjectPtr { @@ -231,58 +288,39 @@ where { /// Hands out a guard for a shared borrow, through which the user instance can be read. /// - /// The pattern is very similar to interior mutability with standard [`RefCell`][std::cell::RefCell]. - /// You can either have multiple `GdRef` shared guards, or a single `GdMut` exclusive guard to a Rust - /// `GodotClass` instance, independently of how many `Gd` smart pointers point to it. There are runtime - /// checks to ensure that Rust safety rules (e.g. no `&` and `&mut` coexistence) are upheld. - /// - /// # Panics - /// * If another `Gd` smart pointer pointing to the same Rust instance has a live `GdMut` guard bound. - /// * If there is an ongoing function call from GDScript to Rust, which currently holds a `&mut T` - /// reference to the user instance. This can happen through re-entrancy (Rust -> GDScript -> Rust call). + /// See [`crate::obj::Gd::bind()`] for a more in depth explanation. // Note: possible names: write/read, hold/hold_mut, r/w, r/rw, ... pub fn bind(&self) -> GdRef { - engine::ensure_object_alive(self.cached_instance_id.get(), self.obj_sys(), "bind"); - GdRef::from_cell(self.storage().get()) + engine::ensure_object_alive(self.cached_instance_id, self.obj_sys(), "bind"); + GdRef::from_cell(self.storage().unwrap().get()) } /// Hands out a guard for an exclusive borrow, through which the user instance can be read and written. /// - /// The pattern is very similar to interior mutability with standard [`RefCell`][std::cell::RefCell]. - /// You can either have multiple `GdRef` shared guards, or a single `GdMut` exclusive guard to a Rust - /// `GodotClass` instance, independently of how many `Gd` smart pointers point to it. There are runtime - /// checks to ensure that Rust safety rules (e.g. no `&mut` aliasing) are upheld. - /// - /// # Panics - /// * If another `Gd` smart pointer pointing to the same Rust instance has a live `GdRef` or `GdMut` guard bound. - /// * If there is an ongoing function call from GDScript to Rust, which currently holds a `&T` or `&mut T` - /// reference to the user instance. This can happen through re-entrancy (Rust -> GDScript -> Rust call). + /// See [`crate::obj::Gd::bind_mut()`] for a more in depth explanation. pub fn bind_mut(&mut self) -> GdMut { - engine::ensure_object_alive(self.cached_instance_id.get(), self.obj_sys(), "bind_mut"); - GdMut::from_cell(self.storage().get_mut()) + engine::ensure_object_alive(self.cached_instance_id, self.obj_sys(), "bind_mut"); + GdMut::from_cell(self.storage().unwrap().get_mut()) } /// Storage object associated with the extension instance. - pub(crate) fn storage(&self) -> &InstanceStorage { + /// + /// Returns `None` if self is null. + pub(crate) fn storage(&self) -> Option<&InstanceStorage> { // SAFETY: instance pointer belongs to this instance. We only get a shared reference, no exclusive access, so even // calling this from multiple Gd pointers is safe. // Potential issue is a concurrent free() in multi-threaded access; but that would need to be guarded against inside free(). unsafe { let binding = self.resolve_instance_ptr(); - crate::private::as_storage::(binding) + sys::ptr_then(binding, |binding| crate::private::as_storage::(binding)) } } - /// Storage object associated with the extension instance. - // pub(crate) fn storage_mut(&mut self) -> &mut InstanceStorage { - // // SAFETY: - // unsafe { - // let binding = self.resolve_instance_ptr(); - // crate::private::as_storage_mut::(binding) - // } - // } - unsafe fn resolve_instance_ptr(&self) -> sys::GDExtensionClassInstancePtr { + if self.is_null() { + return ptr::null_mut(); + } + let callbacks = crate::storage::nop_instance_callbacks(); let token = sys::get_library() as *mut std::ffi::c_void; let binding = interface_fn!(object_get_instance_binding)(self.obj_sys(), token, &callbacks); @@ -317,10 +355,8 @@ where } unsafe fn from_sys_init(init: impl FnOnce(sys::GDExtensionUninitializedTypePtr)) -> Self { - let mut raw: std::mem::MaybeUninit = - std::mem::MaybeUninit::uninit(); - init(raw.as_mut_ptr() as sys::GDExtensionUninitializedTypePtr); - Self::from_obj_sys_weak(raw.assume_init()) + let obj = raw_object_init(init); + Self::from_obj_sys_weak(obj) } fn sys(&self) -> sys::GDExtensionTypePtr { @@ -413,18 +449,8 @@ where { /// Destroy the manually-managed Godot object. /// - /// Consumes this smart pointer and renders all other `Gd` smart pointers (as well as any GDScript references) to the same object - /// immediately invalid. Using those `Gd` instances will lead to panics, but not undefined behavior. - /// - /// This operation is **safe** and effectively prevents double-free. - /// - /// Not calling `free()` on manually-managed instances causes memory leaks, unless their ownership is delegated, for - /// example to the node tree in case of nodes. - /// - /// # Panics - /// * When the referred-to object has already been destroyed. - /// * When this is invoked on an upcast `Gd` that dynamically points to a reference-counted type (i.e. operation not supported). - pub fn free(self) { + /// See [`Gd::free()`] for more information. + pub(crate) fn free(self) { // TODO disallow for singletons, either only at runtime or both at compile time (new memory policy) and runtime // Runtime check in case of T=Object, no-op otherwise @@ -434,6 +460,8 @@ where "called free() on Gd which points to a RefCounted dynamic type; free() only supported for manually managed types." ); + assert!(!self.is_null(), "called free() on null object"); + // If ref_counted returned None, that means the instance was destroyed assert!( ref_counted == Some(false) && self.is_instance_valid(), @@ -458,7 +486,7 @@ impl GodotNullableFfi for RawGd { } fn is_null(&self) -> bool { - self.obj.is_null() + Self::is_null(self) } } @@ -483,11 +511,11 @@ pub unsafe fn raw_object_init( /// Destructor with semantics depending on memory strategy. /// -/// * If this `Gd` smart pointer holds a reference-counted type, this will decrement the reference counter. +/// * If this `RawGd` smart pointer holds a reference-counted type, this will decrement the reference counter. /// If this was the last remaining reference, dropping it will invoke `T`'s destructor. /// /// * If the held object is manually-managed, **nothing happens**. -/// To destroy manually-managed `Gd` pointers, you need to call [`Self::free()`]. +/// To destroy manually-managed `RawGd` pointers, you need to call [`crate::obj::Gd::free()`]. impl Drop for RawGd { fn drop(&mut self) { // No-op for manually managed objects @@ -521,7 +549,15 @@ impl Drop for RawGd { impl Clone for RawGd { fn clone(&self) -> Self { out!("RawGd::clone"); - Self::from_obj_sys(self.obj as sys::GDExtensionObjectPtr) + if self.is_instance_valid() { + unsafe { Self::from_obj_sys(self.obj as sys::GDExtensionObjectPtr) } + } else { + // Object is either null or dead, so just copy the values over and do nothing. + Self { + obj: self.obj, + cached_instance_id: self.cached_instance_id, + } + } } } diff --git a/godot-core/src/obj/traits.rs b/godot-core/src/obj/traits.rs index 393570d91..6a931166d 100644 --- a/godot-core/src/obj/traits.rs +++ b/godot-core/src/obj/traits.rs @@ -288,7 +288,10 @@ pub mod dom { T: GodotClass, F: FnOnce(&mut T) -> R, { - closure(obj.as_target_mut()) + closure( + obj.as_target_mut() + .expect("scoped mut should not be called on a null object"), + ) } } @@ -312,7 +315,7 @@ pub mod dom { // ---------------------------------------------------------------------------------------------------------------------------------------------- pub mod mem { - use godot_ffi::{GodotNullableFfi, PtrcallType}; + use godot_ffi::PtrcallType; use super::private::Sealed; use crate::obj::{GodotClass, RawGd}; diff --git a/itest/rust/src/object_tests/object_test.rs b/itest/rust/src/object_tests/object_test.rs index 3ee73cc13..2f91c713b 100644 --- a/itest/rust/src/object_tests/object_test.rs +++ b/itest/rust/src/object_tests/object_test.rs @@ -80,7 +80,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::::from_sys(ptr) }; let obj2 = Gd::from_ffi(raw2); assert_eq!(obj2.bind().value, value); } // drop @@ -95,7 +95,7 @@ fn object_user_roundtrip_write() { let raw = obj.to_ffi(); let raw2 = unsafe { - RawGd::::from_sys_init(|ptr| { + RawGd::::from_sys_init(|ptr| { raw.move_return_ptr(sys::AsUninit::force_init(ptr), sys::PtrcallType::Standard) }) };