Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 36 additions & 5 deletions godot-core/src/obj/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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! {
Expand Down Expand Up @@ -305,16 +305,16 @@ impl<T: GodotClass> Base<T> {
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<T> {
/// 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<T>` is currently in the initializing state.
Expand All @@ -334,6 +334,37 @@ impl<T: GodotClass> Base<T> {

(*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<T: GodotClass> Debug for Base<T> {
Expand Down
9 changes: 9 additions & 0 deletions godot-core/src/obj/gd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,15 @@ impl<T: GodotClass> Gd<T> {
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.
Expand Down
23 changes: 11 additions & 12 deletions godot-core/src/obj/guards.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand All @@ -45,8 +46,6 @@ impl<T: GodotClass> Drop for GdRef<'_, T> {
}
}

// TODO Clone or Share

// ----------------------------------------------------------------------------------------------------------------------------------------------

/// Mutably/exclusively bound reference guard for a [`Gd`][crate::obj::Gd] smart pointer.
Expand Down Expand Up @@ -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<T::Base>,
passive_gd: PassiveGd<'a, T::Base>,
_instance: &'a T,
}

impl<'a, T: $bound> $ident<'a, T> {
pub(crate) fn new(gd: Gd<T::Base>, instance: &'a T) -> Self {
pub(crate) fn new(passive_gd: PassiveGd<'a, T::Base>, instance: &'a T) -> Self {
Self {
gd,
passive_gd,
_instance: instance,
}
}
Expand All @@ -216,7 +215,7 @@ macro_rules! make_base_ref {
type Target = Gd<T::Base>;

fn deref(&self) -> &Gd<T::Base> {
&self.gd
&self.passive_gd
}
}
};
Expand All @@ -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<T::Base>,
passive_gd: PassiveGd<'a, T::Base>,
_inaccessible_guard: InaccessibleGuard<'a, T>,
}

impl<'a, T: $bound> $ident<'a, T> {
pub(crate) fn new(
gd: Gd<T::Base>,
passive_gd: PassiveGd<'a, T::Base>,
inaccessible_guard: InaccessibleGuard<'a, T>,
) -> Self {
Self {
gd,
passive_gd,
_inaccessible_guard: inaccessible_guard,
}
}
Expand All @@ -252,13 +251,13 @@ macro_rules! make_base_mut {
type Target = Gd<T::Base>;

fn deref(&self) -> &Gd<T::Base> {
&self.gd
&self.passive_gd
}
}

impl<T: $bound> DerefMut for $ident<'_, T> {
fn deref_mut(&mut self) -> &mut Gd<T::Base> {
&mut self.gd
&mut self.passive_gd
}
}
};
Expand Down
2 changes: 2 additions & 0 deletions godot-core/src/obj/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ mod guards;
mod instance_id;
mod on_editor;
mod on_ready;
mod passive_gd;
mod raw_gd;
mod traits;

Expand All @@ -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::*;

Expand Down
80 changes: 80 additions & 0 deletions godot-core/src/obj/passive_gd.rs
Original file line number Diff line number Diff line change
@@ -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<T>`, 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<T>` 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<Gd<T>>,

// 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<T>) -> 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<T>`.
///
/// 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<T>) -> Self {
Self {
weak_gd: ManuallyDrop::new(weak_gd),
_phantom: PhantomData,
}
}
}

impl<T: GodotClass> 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<T: GodotClass> Deref for PassiveGd<'_, T> {
type Target = Gd<T>;

fn deref(&self) -> &Self::Target {
&self.weak_gd
}
}

impl<T: GodotClass> 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.
6 changes: 4 additions & 2 deletions godot-core/src/obj/script.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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)
}
}

Expand Down
15 changes: 9 additions & 6 deletions godot-core/src/obj/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -422,9 +422,8 @@ pub trait WithBaseField: GodotClass + Bounds<Declarer = bounds::DeclUser> {
///
/// 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.
Expand Down Expand Up @@ -493,9 +492,12 @@ pub trait WithBaseField: GodotClass + Bounds<Declarer = bounds::DeclUser> {
/// ```
#[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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.to_gd still increments ref count.


// SAFETY:
// - We have a `Gd<Self>` so, provided that `storage_unbounded` succeeds, the associated instance
// storage has been created.
Expand All @@ -509,12 +511,13 @@ pub trait WithBaseField: GodotClass + Bounds<Declarer = bounds::DeclUser> {
let storage = unsafe {
gd.raw
.storage_unbounded()
.expect("we have a `Gd<Self>` so the raw should not be null")
.expect("we have Gd<Self>; 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)
}
}

Expand Down
35 changes: 35 additions & 0 deletions itest/rust/src/object_tests/base_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Baseless>, Base<Node2D>) {
let mut extracted_base = None;
let obj = Baseless::smuggle_out(&mut extracted_base);
Expand Down Expand Up @@ -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<Self> {
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<Self>, Gd<RefCounted>) {
let mut moved_out = None;
Expand Down
Loading