Merge pull request #1302 from godot-rust/perf/base-no-clone

`base()` + `base_mut()` no longer clone `Gd` pointer

Author: Bromeon

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<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.
@@ -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> {

godot-core/src/obj/gd.rs

@@ -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.

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<T: GodotClass> 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<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,
                 }
             }
@@ -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
             }
         }
     };
@@ -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,
                 }
             }
@@ -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
             }
         }
     };

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::*;
 

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<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.

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)
     }
 }
 

godot-core/src/obj/traits.rs

@@ -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.
@@ -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();
+
         // SAFETY:
         // - We have a `Gd<Self>` so, provided that `storage_unbounded` succeeds, the associated instance
         //   storage has been created.
@@ -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)
     }
 }
 

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<Baseless>, Base<Node2D>) {
     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<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;