diff --git a/godot-core/src/obj/base.rs b/godot-core/src/obj/base.rs index 462e8d858..3b5f6432a 100644 --- a/godot-core/src/obj/base.rs +++ b/godot-core/src/obj/base.rs @@ -15,7 +15,7 @@ use std::mem::ManuallyDrop; use std::rc::Rc; use crate::builtin::{Callable, Variant}; -use crate::obj::{bounds, Gd, GodotClass, InstanceId}; +use crate::obj::{bounds, Gd, GodotClass, InstanceId, PassiveGd}; use crate::{classes, sys}; thread_local! { @@ -305,16 +305,16 @@ impl Base { 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 { + /// Returns a passive reference to the base object, for use in script contexts only. + pub(crate) fn to_script_passive(&self) -> PassiveGd<'_, T> { #[cfg(debug_assertions)] assert_eq!( self.init_state.get(), InitState::Script, - "to_script_gd() can only be called on script-context Base objects" + "to_script_passive() can only be called on script-context Base objects" ); - (*self.obj).clone() + PassiveGd::from_strong_ref(&self.obj) } /// Returns `true` if this `Base` is currently in the initializing state. @@ -334,6 +334,37 @@ impl Base { (*self.obj).clone() } + + /// Returns a [`PassiveGd`] referencing the base object, assuming the derived object is fully constructed. + /// + /// This method directly creates a PassiveGd from a weak reference, providing clean lifetime management + /// without the need for manual `drop_weak()` calls. + pub(crate) fn constructed_passive(&self) -> PassiveGd<'_, T> { + // SAFETY: returned lifetime here is re-bound to self. Covariant lifetime conversion 'static -> 'self. + unsafe { self.constructed_passive_unbounded() } + } + + /// Returns a weak [`Gd`] referencing the base object, assuming the derived object is fully constructed. + /// + /// Unlike [`Self::__constructed_gd()`], this does not increment the reference count for ref-counted `T`s. + /// The returned weak reference is safe to use only as long as the associated instance remains alive. + /// + /// # Safety + /// This method disconnects the lifetime, as opposed to [`Self::constructed_passive()]. Caller is responsible of re-binding the + /// lifetime to the instance. + pub(crate) unsafe fn constructed_passive_unbounded(&self) -> PassiveGd<'static, T> { + #[cfg(debug_assertions)] // debug_assert! still checks existence of symbols. + assert!( + !self.is_initializing(), + "WithBaseField::base(), base_mut() can only be called on fully-constructed objects, after I*::init() or Gd::from_init_fn()" + ); + + // Create weak reference from the same object pointer without cloning (incrementing refcount). + let weak_gd = unsafe { Gd::from_obj_sys_weak(self.obj.obj_sys()) }; + + // SAFETY: weak_gd is a weakly created Gd, and remains valid as long as self is alive (per safety precondition of this fn). + unsafe { PassiveGd::from_weak_owned(weak_gd) } + } } impl Debug for Base { diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index 18bf5fbee..23264c145 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -335,6 +335,15 @@ impl Gd { Some(rc.map(|i| i as usize)) } + /// Create a non-owning pointer from this. + /// + /// # Safety + /// Must be destroyed with [`drop_weak()`][Self::drop_weak]; regular `Drop` will cause use-after-free. + pub(crate) unsafe fn clone_weak(&self) -> Self { + // SAFETY: delegated to caller. + unsafe { Gd::from_obj_sys_weak(self.obj_sys()) } + } + /// Drop without decrementing ref-counter. /// /// Needed in situations where the instance should effectively be forgotten, but without leaking other associated data. diff --git a/godot-core/src/obj/guards.rs b/godot-core/src/obj/guards.rs index e065b4b9d..33b3f36e1 100644 --- a/godot-core/src/obj/guards.rs +++ b/godot-core/src/obj/guards.rs @@ -15,11 +15,12 @@ use godot_cell::panicking::{InaccessibleGuard, MutGuard, RefGuard}; use godot_ffi::out; use crate::obj::script::ScriptInstance; -use crate::obj::{AsDyn, Gd, GodotClass}; +use crate::obj::{AsDyn, Gd, GodotClass, PassiveGd}; /// Immutably/shared bound reference guard for a [`Gd`][crate::obj::Gd] smart pointer. /// /// See [`Gd::bind`][crate::obj::Gd::bind] for usage. +// GdRef could technically implement Clone, but it wasn't needed so far. #[derive(Debug)] pub struct GdRef<'a, T: GodotClass> { guard: RefGuard<'a, T>, @@ -45,8 +46,6 @@ impl Drop for GdRef<'_, T> { } } -// TODO Clone or Share - // ---------------------------------------------------------------------------------------------------------------------------------------------- /// Mutably/exclusively bound reference guard for a [`Gd`][crate::obj::Gd] smart pointer. @@ -199,14 +198,14 @@ macro_rules! make_base_ref { #[doc = concat!("This can be used to call methods on the base object of a ", $object_name, " that takes `&self` as the receiver.\n\n")] #[doc = concat!("See [`", stringify!($doc_type), "::base()`](", stringify!($doc_path), "::base()) for usage.")] pub struct $ident<'a, T: $bound> { - gd: Gd, + passive_gd: PassiveGd<'a, T::Base>, _instance: &'a T, } impl<'a, T: $bound> $ident<'a, T> { - pub(crate) fn new(gd: Gd, instance: &'a T) -> Self { + pub(crate) fn new(passive_gd: PassiveGd<'a, T::Base>, instance: &'a T) -> Self { Self { - gd, + passive_gd, _instance: instance, } } @@ -216,7 +215,7 @@ macro_rules! make_base_ref { type Target = Gd; fn deref(&self) -> &Gd { - &self.gd + &self.passive_gd } } }; @@ -232,17 +231,17 @@ macro_rules! make_base_mut { /// #[doc = concat!("See [`", stringify!($doc_type), "::base_mut()`](", stringify!($doc_path), "::base_mut()) for usage.\n")] pub struct $ident<'a, T: $bound> { - gd: Gd, + passive_gd: PassiveGd<'a, T::Base>, _inaccessible_guard: InaccessibleGuard<'a, T>, } impl<'a, T: $bound> $ident<'a, T> { pub(crate) fn new( - gd: Gd, + passive_gd: PassiveGd<'a, T::Base>, inaccessible_guard: InaccessibleGuard<'a, T>, ) -> Self { Self { - gd, + passive_gd, _inaccessible_guard: inaccessible_guard, } } @@ -252,13 +251,13 @@ macro_rules! make_base_mut { type Target = Gd; fn deref(&self) -> &Gd { - &self.gd + &self.passive_gd } } impl DerefMut for $ident<'_, T> { fn deref_mut(&mut self) -> &mut Gd { - &mut self.gd + &mut self.passive_gd } } }; diff --git a/godot-core/src/obj/mod.rs b/godot-core/src/obj/mod.rs index ae3633c32..e11c33aba 100644 --- a/godot-core/src/obj/mod.rs +++ b/godot-core/src/obj/mod.rs @@ -20,6 +20,7 @@ mod guards; mod instance_id; mod on_editor; mod on_ready; +mod passive_gd; mod raw_gd; mod traits; @@ -33,6 +34,7 @@ pub use guards::{BaseMut, BaseRef, DynGdMut, DynGdRef, GdMut, GdRef}; pub use instance_id::*; pub use on_editor::*; pub use on_ready::*; +pub(crate) use passive_gd::PassiveGd; pub use raw_gd::*; pub use traits::*; diff --git a/godot-core/src/obj/passive_gd.rs b/godot-core/src/obj/passive_gd.rs new file mode 100644 index 000000000..deef78357 --- /dev/null +++ b/godot-core/src/obj/passive_gd.rs @@ -0,0 +1,80 @@ +/* + * 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::marker::PhantomData; +use std::mem::ManuallyDrop; +use std::ops::{Deref, DerefMut}; + +use crate::obj::{Gd, GodotClass}; + +/// Passive (non-owning) reference to a Godot object. +/// +/// `PassiveGd<'gd, T>` provides a safe abstraction for weak references to Godot objects. Unlike `Gd`, it does not increment/decrement +/// the reference count for `RefCounted` objects, and its `Drop` impl only cleans up metadata, not the Godot object. +/// +/// The lifetime `'gd` can be used to tie it to a _strong_ `Gd` reference, however it can also be `'static` if more flexibility is needed. +/// +/// This type is primarily used internally for base object access in guards and traits, providing a clean alternative to manual +/// [`Gd::clone_weak()`] and [`Gd::drop_weak()`] patterns. +pub(crate) struct PassiveGd<'gd, T: GodotClass> { + weak_gd: ManuallyDrop>, + + // Covariant lifetime: PassiveGd<'a, T> can be used wherever PassiveGd<'b, T> is needed, if 'a: 'b. + _phantom: PhantomData<&'gd ()>, +} + +impl<'gd, T: GodotClass> PassiveGd<'gd, T> { + pub fn from_strong_ref(gd: &Gd) -> Self { + // SAFETY: + // - `clone_weak()` creates a pointer conforming to `from_weak_gd()` requirements. + // - PassiveGd will destroy the pointer with `drop_weak()`. + unsafe { + let weak_gd = gd.clone_weak(); + Self::from_weak_owned(weak_gd) + } + } + + /// Creates a passive reference directly from a weak `Gd`. + /// + /// Will invoke `Gd::drop_weak()` when dropped. Since the parameter has no lifetime, you need to provide the lifetime `'gd` explicitly. + /// + /// # Safety + /// - `weak_gd` must be a weakly created `Gd`, e.g. from [`Gd::clone_weak()`] or [`Gd::from_obj_sys_weak()`]. + /// - The caller must ensure that the `weak_gd` remains valid for the lifetime `'gd`. + pub unsafe fn from_weak_owned(weak_gd: Gd) -> Self { + Self { + weak_gd: ManuallyDrop::new(weak_gd), + _phantom: PhantomData, + } + } +} + +impl Drop for PassiveGd<'_, T> { + fn drop(&mut self) { + // SAFETY: Only extracted once, in Drop. + let weak = unsafe { ManuallyDrop::take(&mut self.weak_gd) }; + + weak.drop_weak(); + } +} + +impl Deref for PassiveGd<'_, T> { + type Target = Gd; + + fn deref(&self) -> &Self::Target { + &self.weak_gd + } +} + +impl DerefMut for PassiveGd<'_, T> { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.weak_gd + } +} + +// Note: We intentionally do NOT implement Clone for PassiveGd, as cloning weak references requires careful lifetime management that +// should be explicit. diff --git a/godot-core/src/obj/script.rs b/godot-core/src/obj/script.rs index 908858e3c..82b05be83 100644 --- a/godot-core/src/obj/script.rs +++ b/godot-core/src/obj/script.rs @@ -442,7 +442,8 @@ impl<'a, T: ScriptInstance> SiMut<'a, T> { /// } /// ``` pub fn base(&self) -> ScriptBaseRef<'_, T> { - ScriptBaseRef::new(self.base_ref.to_script_gd(), self.mut_ref) + let passive_gd = self.base_ref.to_script_passive(); + ScriptBaseRef::new(passive_gd, self.mut_ref) } /// Returns a mutable reference suitable for calling engine methods on this object. @@ -505,8 +506,9 @@ 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(); + let passive_gd = self.base_ref.to_script_passive(); - ScriptBaseMut::new(self.base_ref.to_script_gd(), guard) + ScriptBaseMut::new(passive_gd, guard) } } diff --git a/godot-core/src/obj/traits.rs b/godot-core/src/obj/traits.rs index cb9029624..7706a960b 100644 --- a/godot-core/src/obj/traits.rs +++ b/godot-core/src/obj/traits.rs @@ -422,9 +422,8 @@ pub trait WithBaseField: GodotClass + Bounds { /// /// For this, use [`base_mut()`](WithBaseField::base_mut()) instead. fn base(&self) -> BaseRef<'_, Self> { - let gd = self.base_field().__constructed_gd(); - - BaseRef::new(gd, self) + let passive_gd = self.base_field().constructed_passive(); + BaseRef::new(passive_gd, self) } /// Returns a mutable reference suitable for calling engine methods on this object. @@ -493,9 +492,12 @@ pub trait WithBaseField: GodotClass + Bounds { /// ``` #[allow(clippy::let_unit_value)] fn base_mut(&mut self) -> BaseMut<'_, Self> { - let base_gd = self.base_field().__constructed_gd(); + // We need to construct this first, as the mut-borrow below will block all other access. + // SAFETY: lifetime is re-established at the bottom BaseMut construction, since return type of this fn has lifetime bound to instance. + let passive_gd = unsafe { self.base_field().constructed_passive_unbounded() }; let gd = self.to_gd(); + // SAFETY: // - We have a `Gd` so, provided that `storage_unbounded` succeeds, the associated instance // storage has been created. @@ -509,12 +511,13 @@ pub trait WithBaseField: GodotClass + Bounds { let storage = unsafe { gd.raw .storage_unbounded() - .expect("we have a `Gd` so the raw should not be null") + .expect("we have Gd; its RawGd should not be null") }; let guard = storage.get_inaccessible(self); - BaseMut::new(base_gd, guard) + // Narrows lifetime again from 'static to 'self. + BaseMut::new(passive_gd, guard) } } diff --git a/itest/rust/src/object_tests/base_test.rs b/itest/rust/src/object_tests/base_test.rs index ffa127a09..acb91b118 100644 --- a/itest/rust/src/object_tests/base_test.rs +++ b/itest/rust/src/object_tests/base_test.rs @@ -171,6 +171,36 @@ fn base_swapping() { two.free(); } +#[itest] +fn base_refcounted_weak_reference() { + // Not new_gd(), to not interfere with to_init_gd() ref-count handling. + let obj = RefcBased::create_one(); + + let initial_refcount = obj.get_reference_count(); + assert_eq!(initial_refcount, 1); + + { + let bind_guard = obj.bind(); + let base_guard = bind_guard.base(); + + let intermediate_refcount = obj.get_reference_count(); + assert_eq!( + intermediate_refcount, 1, + "base() should not increment refcount" + ); + + // Call an API to ensure Base is functional. + let class_name = base_guard.get_class(); + assert_eq!(class_name, "RefcBased".into()); + } + + let final_refcount = obj.get_reference_count(); + assert_eq!( + final_refcount, 1, + "refcount should remain unchanged after dropping base guard" + ); +} + fn create_object_with_extracted_base() -> (Gd, Base) { let mut extracted_base = None; let obj = Baseless::smuggle_out(&mut extracted_base); @@ -267,6 +297,11 @@ impl IRefCounted for RefcBased { // Only needed in base_init_test.rs. #[godot_api(no_typed_signals)] impl RefcBased { + /// No `to_init_gd()` call, so the reference count is 1 after initialization. + pub fn create_one() -> Gd { + Gd::from_init_fn(|base| Self { base }) + } + /// 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;