diff --git a/godot-core/src/obj/base.rs b/godot-core/src/obj/base.rs index 09a08f4ba..d6c2740e0 100644 --- a/godot-core/src/obj/base.rs +++ b/godot-core/src/obj/base.rs @@ -5,12 +5,55 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ +#[cfg(debug_assertions)] +use std::cell::Cell; +use std::cell::RefCell; +use std::collections::hash_map::Entry; +use std::collections::HashMap; use std::fmt::{Debug, Display, Formatter, Result as FmtResult}; use std::mem::ManuallyDrop; +use std::rc::Rc; -use crate::obj::{Gd, GodotClass}; +use crate::builtin::{Callable, Variant}; +use crate::obj::{bounds, Gd, GodotClass, InstanceId}; use crate::{classes, sys}; +thread_local! { + /// Extra strong references for each instance ID, needed for [`Base::to_init_gd()`]. + /// + /// At the moment, all Godot objects must be accessed from the main thread, because their deferred destruction (`Drop`) runs on the + /// main thread, too. This may be relaxed in the future, and a `sys::Global` could be used instead of a `thread_local!`. + static PENDING_STRONG_REFS: RefCell>> = RefCell::new(HashMap::new()); +} + +/// Represents the initialization state of a `Base` object. +#[cfg(debug_assertions)] +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +enum InitState { + /// Object is being constructed (inside `I*::init()` or `Gd::from_init_fn()`). + ObjectConstructing, + /// Object construction is complete. + ObjectInitialized, + /// `ScriptInstance` context - always considered initialized (bypasses lifecycle checks). + Script, +} + +#[cfg(debug_assertions)] +macro_rules! base_from_obj { + ($obj:expr, $state:expr) => { + Base::from_obj($obj, $state) + }; +} + +#[cfg(not(debug_assertions))] +macro_rules! base_from_obj { + ($obj:expr, $state:expr) => { + Base::from_obj($obj) + }; +} + +// ---------------------------------------------------------------------------------------------------------------------------------------------- + /// Restricted version of `Gd`, to hold the base instance inside a user's `GodotClass`. /// /// Behaves similarly to [`Gd`][crate::obj::Gd], but is more constrained. Cannot be constructed by the user. @@ -35,6 +78,12 @@ pub struct Base { // 1. Gd -- triggers InstanceStorage destruction // 2. obj: ManuallyDrop>, + + /// Tracks the initialization state of this `Base` in Debug mode. + /// + /// Rc allows to "copy-construct" the base from an existing one, while still affecting the user-instance through the original `Base`. + #[cfg(debug_assertions)] + init_state: Rc>, } impl Base { @@ -47,7 +96,14 @@ impl Base { /// If `base` is destroyed while the returned `Base` is in use, that constitutes a logic error, not a safety issue. pub(crate) unsafe fn from_base(base: &Base) -> Base { debug_assert!(base.obj.is_instance_valid()); - Base::from_obj(Gd::from_obj_sys_weak(base.obj.obj_sys())) + + let obj = Gd::from_obj_sys_weak(base.obj.obj_sys()); + + Self { + obj: ManuallyDrop::new(obj), + #[cfg(debug_assertions)] + init_state: Rc::clone(&base.init_state), + } } /// Create base from existing object (used in script instances). @@ -57,9 +113,11 @@ impl Base { /// # Safety /// `gd` must be alive at the time of invocation. If it is destroyed while the returned `Base` is in use, that constitutes a logic /// error, not a safety issue. - pub(crate) unsafe fn from_gd(gd: &Gd) -> Self { + pub(crate) unsafe fn from_script_gd(gd: &Gd) -> Self { debug_assert!(gd.is_instance_valid()); - Base::from_obj(Gd::from_obj_sys_weak(gd.obj_sys())) + + let obj = Gd::from_obj_sys_weak(gd.obj_sys()); + base_from_obj!(obj, InitState::Script) } /// Create new base from raw Godot object. @@ -72,7 +130,7 @@ impl Base { pub(crate) unsafe fn from_sys(base_ptr: sys::GDExtensionObjectPtr) -> Self { assert!(!base_ptr.is_null(), "instance base is null pointer"); - // Initialize only as weak pointer (don't increment reference count) + // Initialize only as weak pointer (don't increment reference count). let obj = Gd::from_obj_sys_weak(base_ptr); // This obj does not contribute to the strong count, otherwise we create a reference cycle: @@ -80,9 +138,18 @@ impl Base { // 2. holds user T (via extension instance and storage) // 3. holds Base RefCounted (last ref, dropped in T destructor, but T is never destroyed because this ref keeps storage alive) // Note that if late-init never happened on self, we have the same behavior (still a raw pointer instead of weak Gd) - Base::from_obj(obj) + base_from_obj!(obj, InitState::ObjectConstructing) } + #[cfg(debug_assertions)] + fn from_obj(obj: Gd, init_state: InitState) -> Self { + Self { + obj: ManuallyDrop::new(obj), + init_state: Rc::new(Cell::new(init_state)), + } + } + + #[cfg(not(debug_assertions))] fn from_obj(obj: Gd) -> Self { Self { obj: ManuallyDrop::new(obj), @@ -94,10 +161,132 @@ impl Base { /// Using this method to call methods on the base field of a Rust object is discouraged, instead use the /// methods from [`WithBaseField`](super::WithBaseField) when possible. #[doc(hidden)] + #[deprecated = "Private API. Use `Base::to_init_gd()` or `WithBaseField::to_gd()` instead."] // TODO(v0.4): remove. pub fn to_gd(&self) -> Gd { (*self.obj).clone() } + /// Returns a [`Gd`] referencing the base object, for exclusive use during object initialization. + /// + /// Can be used during an initialization function [`I*::init()`][crate::classes::IObject::init] or [`Gd::from_init_fn()`]. + /// + /// The base pointer is only pointing to a base object; you cannot yet downcast it to the object being constructed. + /// The instance ID is the same as the one the in-construction object will have. + /// + /// # Lifecycle for ref-counted classes + /// If `T: Inherits`, then the ref-counted object is not yet fully-initialized at the time of the `init` function running. + /// Accessing the base object without further measures would be dangerous. Here, godot-rust employs a workaround: the `Base` object (which + /// holds a weak pointer to the actual instance) is temporarily upgraded to a strong pointer, preventing use-after-free. + /// + /// This additional reference is automatically dropped at an implementation-defined point in time (which may change, and technically delay + /// destruction of your object as soon as you use `Base::to_init_gd()`). Right now, this refcount-decrement is deferred to the next frame. + /// + /// For now, ref-counted bases can only use `to_init_gd()` on the main thread. + /// + /// # Panics (Debug) + /// If called outside an initialization function, or for ref-counted objects on a non-main thread. + #[cfg(since_api = "4.2")] + pub fn to_init_gd(&self) -> Gd { + #[cfg(debug_assertions)] // debug_assert! still checks existence of symbols. + assert!( + self.is_initializing(), + "Base::to_init_gd() can only be called during object initialization, inside I*::init() or Gd::from_init_fn()" + ); + + // For manually-managed objects, regular clone is fine. + // Only static type matters, because this happens immediately after initialization, so T is both static and dynamic type. + if !::IS_REF_COUNTED { + return Gd::clone(&self.obj); + } + + debug_assert!( + sys::is_main_thread(), + "Base::to_init_gd() can only be called on the main thread for ref-counted objects (for now)" + ); + + // First time handing out a Gd, we need to take measures to temporarily upgrade the Base's weak pointer to a strong one. + // During the initialization phase (derived object being constructed), increment refcount by 1. + let instance_id = self.obj.instance_id(); + PENDING_STRONG_REFS.with(|refs| { + let mut pending_refs = refs.borrow_mut(); + if let Entry::Vacant(e) = pending_refs.entry(instance_id) { + let strong_ref: Gd = unsafe { Gd::from_obj_sys(self.obj.obj_sys()) }; + + // We know that T: Inherits due to check above, but don't it as a static bound for Gd::upcast(). + // Thus fall back to low-level FFI cast on RawGd. + let strong_ref_raw = strong_ref.raw; + let raw = strong_ref_raw + .ffi_cast::() + .expect("Base must be RefCounted") + .into_dest(strong_ref_raw); + + e.insert(Gd { raw }); + } + }); + + let name = format!("Base<{}> deferred unref", T::class_name()); + let callable = Callable::from_once_fn(&name, move |_args| { + Self::drop_strong_ref(instance_id); + Ok(Variant::nil()) + }); + + // Use Callable::call_deferred() instead of Gd::apply_deferred(). The latter implicitly borrows &mut self, + // causing a "destroyed while bind was active" panic. + callable.call_deferred(&[]); + + (*self.obj).clone() + } + + /// Drops any extra strong references, possibly causing object destruction. + fn drop_strong_ref(instance_id: InstanceId) { + PENDING_STRONG_REFS.with(|refs| { + let mut pending_refs = refs.borrow_mut(); + let strong_ref = pending_refs.remove(&instance_id); + assert!( + strong_ref.is_some(), + "Base unexpectedly had its strong ref rug-pulled" + ); + + // Triggers RawGd::drop() -> dec-ref -> possibly object destruction. + }); + } + + /// Finalizes the initialization of this `Base` and returns whether + pub(crate) fn mark_initialized(&mut self) { + #[cfg(debug_assertions)] + { + assert_eq!( + self.init_state.get(), + InitState::ObjectConstructing, + "Base is already initialized, or holds a script instance" + ); + + self.init_state.set(InitState::ObjectInitialized); + } + + // May return whether there is a "surplus" strong ref in the future, as self.extra_strong_ref.borrow().is_some(). + } + + /// Returns a [`Gd`] referencing the base object, assuming the derived object is fully constructed. + #[doc(hidden)] + pub fn __fully_constructed_gd(&self) -> Gd { + #[cfg(debug_assertions)] // debug_assert! still checks existence of symbols. + assert!( + !self.is_initializing(), + "WithBaseField::to_gd(), base(), base_mut() can only be called on fully-constructed objects, after I*::init() or Gd::from_init_fn()" + ); + + (*self.obj).clone() + } + + /// Returns a [`Gd`] referencing the base object, for use in script contexts only. + #[doc(hidden)] + pub fn __script_gd(&self) -> Gd { + // Used internally by `SiMut::base()` and `SiMut::base_mut()` for script re-entrancy. + // Could maybe add debug validation to ensure script context in the future. + (*self.obj).clone() + } + // Currently only used in outbound virtual calls (for scripts); search for: base_field(self).obj_sys(). #[doc(hidden)] pub fn obj_sys(&self) -> sys::GDExtensionObjectPtr { @@ -109,6 +298,36 @@ impl Base { pub(crate) fn debug_instance_id(&self) -> crate::obj::InstanceId { self.obj.instance_id() } + + /// Returns a [`Gd`] referencing the base object, for use in script contexts only. + pub(crate) fn to_script_gd(&self) -> Gd { + #[cfg(debug_assertions)] + assert_eq!( + self.init_state.get(), + InitState::Script, + "to_script_gd() can only be called on script-context Base objects" + ); + + (*self.obj).clone() + } + + /// Returns `true` if this `Base` is currently in the initializing state. + #[cfg(debug_assertions)] + fn is_initializing(&self) -> bool { + self.init_state.get() == InitState::ObjectConstructing + } + + /// Returns a [`Gd`] referencing the base object, assuming the derived object is fully constructed. + #[doc(hidden)] + pub fn __constructed_gd(&self) -> Gd { + #[cfg(debug_assertions)] // debug_assert! still checks existence of symbols. + assert!( + !self.is_initializing(), + "WithBaseField::to_gd(), base(), base_mut() can only be called on fully-constructed objects, after I*::init() or Gd::from_init_fn()" + ); + + (*self.obj).clone() + } } impl Debug for Base { diff --git a/godot-core/src/obj/call_deferred.rs b/godot-core/src/obj/call_deferred.rs index 32098699c..07900a028 100644 --- a/godot-core/src/obj/call_deferred.rs +++ b/godot-core/src/obj/call_deferred.rs @@ -20,6 +20,8 @@ use crate::registry::signal::ToSignalObj; #[cfg(before_api = "4.2")] pub trait WithDeferredCall {} +// TODO(v0.4): seal this and similar traits. + /// Enables `Gd::apply_deferred()` for type-safe deferred calls. /// /// The trait is automatically available for all engine-defined Godot classes and user classes containing a `Base` field. diff --git a/godot-core/src/obj/casts.rs b/godot-core/src/obj/casts.rs index 270f54405..b0504c68b 100644 --- a/godot-core/src/obj/casts.rs +++ b/godot-core/src/obj/casts.rs @@ -48,6 +48,7 @@ impl CastSuccess { } /// Access shared reference to destination, without consuming object. + #[cfg(debug_assertions)] pub fn as_dest_ref(&self) -> &RawGd { self.check_validity(); &self.dest diff --git a/godot-core/src/obj/script.rs b/godot-core/src/obj/script.rs index 4215abf97..fd97e1818 100644 --- a/godot-core/src/obj/script.rs +++ b/godot-core/src/obj/script.rs @@ -319,7 +319,7 @@ pub unsafe fn create_script_instance( method_lists: BoundedPtrList::new(), // SAFETY: The script instance is always freed before the base object is destroyed. The weak reference should therefore never be // accessed after it has been freed. - base: unsafe { Base::from_gd(&for_object) }, + base: unsafe { Base::from_script_gd(&for_object) }, }; let data_ptr = Box::into_raw(Box::new(data)); @@ -454,7 +454,7 @@ impl<'a, T: ScriptInstance> SiMut<'a, T> { /// } /// ``` pub fn base(&self) -> ScriptBaseRef<'_, T> { - ScriptBaseRef::new(self.base_ref.to_gd(), self.mut_ref) + ScriptBaseRef::new(self.base_ref.to_script_gd(), self.mut_ref) } /// Returns a mutable reference suitable for calling engine methods on this object. @@ -518,7 +518,7 @@ impl<'a, T: ScriptInstance> SiMut<'a, T> { pub fn base_mut(&mut self) -> ScriptBaseMut<'_, T> { let guard = self.cell.make_inaccessible(self.mut_ref).unwrap(); - ScriptBaseMut::new(self.base_ref.to_gd(), guard) + ScriptBaseMut::new(self.base_ref.to_script_gd(), guard) } } diff --git a/godot-core/src/obj/traits.rs b/godot-core/src/obj/traits.rs index 0ee625c8d..69a9bdb77 100644 --- a/godot-core/src/obj/traits.rs +++ b/godot-core/src/obj/traits.rs @@ -355,9 +355,13 @@ pub trait WithBaseField: GodotClass + Bounds { /// /// This is intended to be stored or passed to engine methods. You cannot call `bind()` or `bind_mut()` on it, while the method /// calling `to_gd()` is still running; that would lead to a double borrow panic. + /// + /// # Panics + /// If called during initialization (the `init()` function or `Gd::from_init_fn()`). Use [`Base::to_init_gd()`] instead. fn to_gd(&self) -> Gd; /// Returns a reference to the `Base` stored by this object. + #[doc(hidden)] fn base_field(&self) -> &Base; /// Returns a shared reference suitable for calling engine methods on this object. @@ -419,7 +423,7 @@ pub trait WithBaseField: GodotClass + Bounds { /// /// For this, use [`base_mut()`](WithBaseField::base_mut()) instead. fn base(&self) -> BaseRef<'_, Self> { - let gd = self.base_field().to_gd(); + let gd = self.base_field().__constructed_gd(); BaseRef::new(gd, self) } @@ -490,7 +494,7 @@ pub trait WithBaseField: GodotClass + Bounds { /// ``` #[allow(clippy::let_unit_value)] fn base_mut(&mut self) -> BaseMut<'_, Self> { - let base_gd = self.base_field().to_gd(); + let base_gd = self.base_field().__constructed_gd(); let gd = self.to_gd(); // SAFETY: diff --git a/godot-core/src/registry/callbacks.rs b/godot-core/src/registry/callbacks.rs index 8dda6aa62..d8e704535 100644 --- a/godot-core/src/registry/callbacks.rs +++ b/godot-core/src/registry/callbacks.rs @@ -124,7 +124,6 @@ where F: FnOnce(Base) -> T, { let class_name = T::class_name(); - //out!("create callback: {}", class_name.backing); let base = unsafe { Base::from_sys(base_ptr) }; @@ -133,12 +132,15 @@ where let context = || format!("panic during {class_name}::init() constructor"); let code = || make_user_instance(unsafe { Base::from_base(&base) }); let user_instance = handle_panic(context, std::panic::AssertUnwindSafe(code))?; + // Print shouldn't be necessary as panic itself is printed. If this changes, re-enable in error case: // godot_error!("failed to create instance of {class_name}; Rust init() panicked"); + let mut base_copy = unsafe { Base::from_base(&base) }; + let instance = InstanceStorage::::construct(user_instance, base); - let instance_ptr = instance.into_raw(); - let instance_ptr = instance_ptr as sys::GDExtensionClassInstancePtr; + let instance_rust_ptr = instance.into_raw(); + let instance_ptr = instance_rust_ptr as sys::GDExtensionClassInstancePtr; let binding_data_callbacks = crate::storage::nop_instance_callbacks(); unsafe { @@ -151,7 +153,10 @@ where ); } - // std::mem::forget(class_name); + // Mark initialization as complete, now that user constructor has finished. + base_copy.mark_initialized(); + + // No std::mem::forget(base_copy) here, since Base may stores other fields that need deallocation. Ok(instance_ptr) } diff --git a/godot-core/src/storage/instance_storage.rs b/godot-core/src/storage/instance_storage.rs index e3bb6c029..79d925f57 100644 --- a/godot-core/src/storage/instance_storage.rs +++ b/godot-core/src/storage/instance_storage.rs @@ -119,7 +119,7 @@ pub unsafe trait Storage { where Self::Instance: Inherits<::Base>, { - self.base().to_gd().cast() + self.base().__constructed_gd().cast() } /// Puts self onto the heap and returns a pointer to this new heap-allocation. diff --git a/godot-core/src/storage/single_threaded.rs b/godot-core/src/storage/single_threaded.rs index e5931788d..9caf022a5 100644 --- a/godot-core/src/storage/single_threaded.rs +++ b/godot-core/src/storage/single_threaded.rs @@ -19,7 +19,7 @@ pub struct InstanceStorage { user_instance: GdCell, pub(super) base: Base, - // Declared after `user_instance`, is dropped last + // Declared after `user_instance`, is dropped last. pub(super) lifecycle: cell::Cell, // No-op in Release mode. @@ -99,10 +99,18 @@ unsafe impl Storage for InstanceStorage { impl StorageRefCounted for InstanceStorage { fn on_inc_ref(&self) { + // Note: on_inc_ref() and on_dec_ref() do not track extra strong references from Base::to_init_gd(). + // See https://github.com/godot-rust/gdext/pull/1273 for code that had it. + super::log_inc_ref(self); } fn on_dec_ref(&self) { + // IMPORTANT: it is too late here to perform dec-ref operations on the Base (for "surplus" strong references). + // This callback is only invoked in the C++ condition `if (rc_val <= 1 /* higher is not relevant */)` -- see Godot ref_counted.cpp. + // The T <-> RefCounted hierarchical relation is usually already broken up at this point, and further dec-ref may bring the count + // down to 0. + super::log_dec_ref(self); } } diff --git a/godot-macros/src/class/derive_godot_class.rs b/godot-macros/src/class/derive_godot_class.rs index df770b0d6..59720c619 100644 --- a/godot-macros/src/class/derive_godot_class.rs +++ b/godot-macros/src/class/derive_godot_class.rs @@ -90,7 +90,8 @@ pub fn derive_godot_class(item: venial::Item) -> ParseResult { // By not referencing the base field directly here we ensure that the user only gets one error when the base // field's type is wrong. let base = <#class_name as ::godot::obj::WithBaseField>::base_field(self); - base.to_gd().cast() + + base.__constructed_gd().cast() } fn base_field(&self) -> &::godot::obj::Base<<#class_name as ::godot::obj::GodotClass>::Base> { diff --git a/itest/godot/SpecialTests.gd b/itest/godot/SpecialTests.gd index 3cd44c75b..e787a45b0 100644 --- a/itest/godot/SpecialTests.gd +++ b/itest/godot/SpecialTests.gd @@ -49,7 +49,7 @@ func test_collision_object_2d_input_event(): # Needed push_unhandled_input() in Godot 4.0; no longer supported. window.push_input(event) - # Ensure we run a full physics frame + # Ensure we run a full physics frame. await root.get_tree().physics_frame assert_that(collision_object.input_event_called(), "Input event should be propagated") diff --git a/itest/rust/src/framework/mod.rs b/itest/rust/src/framework/mod.rs index ddc76e11a..b2c9b8d99 100644 --- a/itest/rust/src/framework/mod.rs +++ b/itest/rust/src/framework/mod.rs @@ -254,7 +254,9 @@ pub fn expect_debug_panic_or_release_ok(_context: &str, code: impl FnOnce()) { code() } -/// Run code asynchronously, immediately before the next _process_ frame. +/// Run code asynchronously, at the very start of the next _process_ frame (before `INode::process()`). +/// +/// If there is a custom `MainLoop` present, it will be run _before_ this. /// /// Useful for assertions that run expect a `call_deferred()` or similar operation, and still want to check the result. #[cfg(since_api = "4.2")] diff --git a/itest/rust/src/object_tests/base_init_test.rs b/itest/rust/src/object_tests/base_init_test.rs new file mode 100644 index 000000000..19b76ead3 --- /dev/null +++ b/itest/rust/src/object_tests/base_init_test.rs @@ -0,0 +1,168 @@ +/* + * Copyright (c) godot-rust; Bromeon and contributors. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at https://mozilla.org/MPL/2.0/. + */ +use std::f32::consts::FRAC_PI_3; + +use godot::classes::ClassDb; +use godot::prelude::*; +use godot::task::TaskHandle; + +use crate::framework::{expect_panic, itest, next_frame}; +use crate::object_tests::base_test::{Based, RefcBased}; + +#[itest] +fn base_init_propagation() { + let obj = Gd::::from_init_fn(|base| { + // Test both temporary + local-variable syntax. + base.to_init_gd().set_rotation(FRAC_PI_3); + + let mut gd = base.to_init_gd(); + gd.set_position(Vector2::new(100.0, 200.0)); + + Based { base, i: 456 } + }); + + // Check that values are propagated to derived object. + let guard = obj.bind(); + assert_eq!(guard.i, 456); + assert_eq!(guard.base().get_rotation(), FRAC_PI_3); + assert_eq!(guard.base().get_position(), Vector2::new(100.0, 200.0)); + drop(guard); + + obj.free(); +} + +// This isn't recommended, but test what happens if someone clones and stores the Gd. +#[itest] +fn base_init_extracted_gd() { + let mut extractor = None; + + let obj = Gd::::from_init_fn(|base| { + extractor = Some(base.to_init_gd()); + + Based { base, i: 456 } + }); + + let extracted = extractor.expect("extraction failed"); + assert_eq!(extracted.instance_id(), obj.instance_id()); + assert_eq!(extracted, obj.clone().upcast()); + + // Destroy through the extracted Gd. + extracted.free(); + assert!( + !obj.is_instance_valid(), + "object should be invalid after base ptr is freed" + ); +} + +// Checks bad practice of rug-pulling the base pointer. +#[itest] +fn base_init_freed_gd() { + let mut free_executed = false; + + expect_panic("base object is destroyed", || { + let _obj = Gd::::from_init_fn(|base| { + let obj = base.to_init_gd(); + obj.free(); // Causes the problem, but doesn't panic yet. + free_executed = true; + + Based { base, i: 456 } + }); + }); + + assert!( + free_executed, + "free() itself doesn't panic, but following construction does" + ); +} + +// Verifies that there are no panics, memory leaks or UB in basic case. +#[itest] +fn base_init_refcounted_simple() { + let _obj = Gd::from_init_fn(|base| { + drop(base.to_init_gd()); + + RefcBased { base } + }); +} + +// Tests that the auto-decrement of surplus references also works when instantiated through the engine. +#[itest(async)] +fn base_init_refcounted_from_engine() -> TaskHandle { + let db = ClassDb::singleton(); + let obj = db.instantiate("RefcBased").to::>(); + + assert_eq!(obj.get_reference_count(), 2); + next_frame(move || assert_eq!(obj.get_reference_count(), 1, "eventual dec-ref happens")) +} + +#[itest(async)] +fn base_init_refcounted_from_rust() -> TaskHandle { + let obj = RefcBased::new_gd(); + + assert_eq!(obj.get_reference_count(), 2); + next_frame(move || assert_eq!(obj.get_reference_count(), 1, "eventual dec-ref happens")) +} + +#[itest(async)] +fn base_init_refcounted_complex() -> TaskHandle { + // Instantiate with multiple Gd references. + let id_simple = verify_complex_init(RefcBased::split_simple()); + let id_intermixed = verify_complex_init(RefcBased::split_intermixed()); + + next_frame(move || { + assert!(!id_simple.lookup_validity(), "object destroyed eventually"); + assert!( + !id_intermixed.lookup_validity(), + "object destroyed eventually" + ); + }) +} + +fn verify_complex_init((obj, base): (Gd, Gd)) -> InstanceId { + let id = obj.instance_id(); + + assert_eq!(obj.instance_id(), base.instance_id()); + assert_eq!(base.get_reference_count(), 3); + assert_eq!(obj.get_reference_count(), 3); + + drop(base); + assert_eq!(obj.get_reference_count(), 2); + assert_eq!(obj.get_reference_count(), 2); + drop(obj); + + // Not dead yet. + assert!(id.lookup_validity(), "object retained (dec-ref deferred)"); + id +} + +#[cfg(debug_assertions)] +#[itest] +fn base_init_outside_init() { + let mut obj = Based::new_alloc(); + + expect_panic("to_init_gd() outside init() function", || { + let guard = obj.bind_mut(); + let _gd = guard.base.to_init_gd(); // Panics in Debug builds. + }); + + obj.free(); +} + +#[cfg(debug_assertions)] +#[itest] +fn base_init_to_gd() { + expect_panic("WithBaseField::to_gd() inside init() function", || { + let _obj = Gd::::from_init_fn(|base| { + let temp_obj = Based { base, i: 999 }; + + // Call to self.to_gd() during initialization should panic in Debug builds. + let _gd = godot::obj::WithBaseField::to_gd(&temp_obj); + + temp_obj + }); + }); +} diff --git a/itest/rust/src/object_tests/base_test.rs b/itest/rust/src/object_tests/base_test.rs index b06dc8454..79725a465 100644 --- a/itest/rust/src/object_tests/base_test.rs +++ b/itest/rust/src/object_tests/base_test.rs @@ -15,16 +15,6 @@ fn base_test_is_weak() { // This might not be needed, as we have leak detection, but it could highlight regressions faster } -#[itest] -fn base_instance_id() { - let obj = Based::new_alloc(); - let obj_id = obj.instance_id(); - let base_id = obj.bind().base().instance_id(); - - assert_eq!(obj_id, base_id); - obj.free(); -} - #[itest] fn base_access_unbound() { let mut obj = Based::new_alloc(); @@ -80,9 +70,11 @@ fn base_debug() { obj.free(); } +// Compatibility check until v0.4 Base::to_gd() is removed. #[itest] fn base_with_init() { let obj = Gd::::from_init_fn(|base| { + #[allow(deprecated)] base.to_gd().set_rotation(11.0); Based { base, i: 732 } }); @@ -112,17 +104,15 @@ fn base_smuggling() { let (mut obj, extracted_base) = create_object_with_extracted_base(); // This works because Gd additionally stores the instance ID (through cached_rtti). - assert_eq!(extracted_base.to_gd().instance_id(), obj.instance_id()); + let extracted_base_obj = extracted_base.__constructed_gd(); + assert_eq!(extracted_base_obj.instance_id(), obj.instance_id()); // This _also_ works because Gd has the direct object pointer to the Godot object. obj.set_position(Vector2::new(1.0, 2.0)); - assert_eq!( - extracted_base.to_gd().get_position(), - Vector2::new(1.0, 2.0) - ); + assert_eq!(extracted_base_obj.get_position(), Vector2::new(1.0, 2.0)); // Destroy base externally. - extracted_base.to_gd().free(); + extracted_base_obj.free(); // Access to object should now fail. expect_panic("object with dead base: calling base methods", || { @@ -146,7 +136,7 @@ fn base_smuggling() { obj.free(); expect_panic("accessing extracted base of dead object", || { - extracted_base.to_gd().get_position(); + extracted_base.__constructed_gd().get_position(); }); } @@ -172,7 +162,10 @@ fn base_swapping() { // Base side, since that only has direct access to the object pointer, while Gd has access to the object pointer _and_ the base field). // Not sure if this is worth the effort + complexity though, given that it almost requires malice to get into such a situation. assert_eq!(one.instance_id(), two.bind().base().instance_id()); - assert_eq!(two.instance_id(), one_ext_base.to_gd().instance_id()); + assert_eq!( + two.instance_id(), + one_ext_base.__constructed_gd().instance_id() + ); one.free(); two.free(); @@ -188,8 +181,25 @@ fn create_object_with_extracted_base() -> (Gd, Base) { // ---------------------------------------------------------------------------------------------------------------------------------------------- -use renamed_bases::Based; +#[derive(GodotClass)] +pub struct RefBase { + pub base: Base, +} + +#[godot_api] +impl IRefCounted for RefBase { + fn init(base: Base) -> Self { + Self { base } + } +} + +// Also used in base_init_test.rs. +pub(super) use renamed_bases::Based; + mod renamed_bases { + use godot::classes::INode2D; + use godot::prelude::godot_api; + use super::{GodotClass, Node2D}; // Test #[hint]. @@ -197,7 +207,7 @@ mod renamed_bases { type Base = T; #[derive(GodotClass)] - #[class(init, base = Node2D)] + #[class(base = Node2D)] pub struct Based { #[hint(base)] pub base: Super, // de-facto: Base. @@ -206,6 +216,13 @@ mod renamed_bases { #[hint(no_base)] pub i: Base, // de-facto: i32 } + + #[godot_api] + impl INode2D for Based { + fn init(base: godot::obj::Base) -> Self { + Based { base, i: 0 } + } + } } impl Based { @@ -230,3 +247,57 @@ impl Baseless { }) } } + +#[derive(GodotClass)] +#[class] // <- also test this syntax. +pub(super) struct RefcBased { + // pub(super): also used in base_init_test.rs. + pub base: Base, +} + +// Only needed in base_init_test.rs. +#[godot_api] +impl IRefCounted for RefcBased { + fn init(base: Base) -> Self { + #[cfg(since_api = "4.2")] + base.to_init_gd(); // Immediately dropped. + Self { base } + } +} + +// Only needed in base_init_test.rs. +#[cfg(since_api = "4.2")] +#[godot_api(no_typed_signals)] +impl RefcBased { + /// Used in `base_init_test.rs` to test that a base pointer can be extracted during initialization. + pub fn split_simple() -> (Gd, Gd) { + let mut moved_out = None; + + let self_gd = Gd::from_init_fn(|base| { + moved_out = Some(base.to_init_gd()); // Moved out. + Self { base } + }); + + (self_gd, moved_out.unwrap()) + } + + /// Used in `base_init_test.rs`, testing extraction + several drops happening at different times. + pub fn split_intermixed() -> (Gd, Gd) { + let mut moved_out = None; + + let self_gd = Gd::from_init_fn(|base| { + let gd = base.to_init_gd(); // Explicitly dropped below. + + drop(base.to_init_gd()); // Immediately dropped. + + let _local_copy = base.to_init_gd(); // Dropped at end of scope. + moved_out = Some(base.to_init_gd()); // Moved out. + + drop(gd); + + Self { base } + }); + + (self_gd, moved_out.unwrap()) + } +} diff --git a/itest/rust/src/object_tests/mod.rs b/itest/rust/src/object_tests/mod.rs index 6381f54d1..1eb4386bc 100644 --- a/itest/rust/src/object_tests/mod.rs +++ b/itest/rust/src/object_tests/mod.rs @@ -30,6 +30,8 @@ mod reentrant_test; mod singleton_test; // `validate_property` is only supported in Godot 4.2+. #[cfg(since_api = "4.2")] +mod base_init_test; +#[cfg(since_api = "4.2")] mod validate_property_test; mod virtual_methods_niche_test; mod virtual_methods_test;