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
26 changes: 7 additions & 19 deletions godot-core/src/obj/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -306,15 +306,16 @@ impl<T: GodotClass> Base<T> {
}

/// Returns a passive reference to the base object, for use in script contexts only.
pub(crate) fn to_script_passive(&self) -> PassiveGd<'_, T> {
pub(crate) fn to_script_passive(&self) -> PassiveGd<T> {
#[cfg(debug_assertions)]
assert_eq!(
self.init_state.get(),
InitState::Script,
"to_script_passive() can only be called on script-context Base objects"
);

PassiveGd::from_strong_ref(&self.obj)
// SAFETY: the object remains valid for script contexts as per the assertion above.
unsafe { PassiveGd::from_strong_ref(&self.obj) }
}

/// Returns `true` if this `Base<T>` is currently in the initializing state.
Expand All @@ -337,33 +338,20 @@ impl<T: GodotClass> Base<T> {

/// 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> {
/// Caller must ensure that the underlying object remains valid for the entire lifetime of the returned `PassiveGd`.
pub(crate) unsafe fn constructed_passive(&self) -> PassiveGd<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) }
// SAFETY: object pointer is valid and remains valid as long as self is alive (per safety precondition of this fn).
unsafe { PassiveGd::from_obj_sys(self.obj.obj_sys()) }
}
}

Expand Down
8 changes: 4 additions & 4 deletions godot-core/src/obj/guards.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,12 +198,12 @@ 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> {
passive_gd: PassiveGd<'a, T::Base>,
passive_gd: PassiveGd<T::Base>,
_instance: &'a T,
}

impl<'a, T: $bound> $ident<'a, T> {
pub(crate) fn new(passive_gd: PassiveGd<'a, T::Base>, instance: &'a T) -> Self {
pub(crate) fn new(passive_gd: PassiveGd<T::Base>, instance: &'a T) -> Self {
Self {
passive_gd,
_instance: instance,
Expand Down Expand Up @@ -231,13 +231,13 @@ 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> {
passive_gd: PassiveGd<'a, T::Base>,
passive_gd: PassiveGd<T::Base>,
_inaccessible_guard: InaccessibleGuard<'a, T>,
}

impl<'a, T: $bound> $ident<'a, T> {
pub(crate) fn new(
passive_gd: PassiveGd<'a, T::Base>,
passive_gd: PassiveGd<T::Base>,
inaccessible_guard: InaccessibleGuard<'a, T>,
) -> Self {
Self {
Expand Down
69 changes: 46 additions & 23 deletions godot-core/src/obj/passive_gd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,55 +5,78 @@
* 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};
use crate::sys;

/// 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
/// `PassiveGd<T>` provides an unsafe 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 for internal use only, to access base objects in guards and traits, and to wrap manual [`Gd::clone_weak()`] and
/// [`Gd::drop_weak()`] patterns.
///
/// 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> {
/// # Why no lifetime?
/// Previous versions used `PassiveGd<'gd, T>` with an explicit lifetime parameter. This caused subtle borrow-checking issues due to Rust's
/// [drop check](https://doc.rust-lang.org/nomicon/dropck.html) rules. When a type has drop obligations (implements `Drop` or contains fields
/// that do), the borrow checker conservatively assumes the destructor might access borrowed data reachable through that value, forcing all
/// such borrows to strictly outlive the value. This created false conflicts when creating both shared and mutable base references from the
/// same object, even though our `Drop` implementation never accesses the lifetime-bound data.
///
/// By removing the lifetime parameter and making construction `unsafe`, we eliminate these false-positive borrow conflicts while maintaining
/// memory safety through explicit caller contracts.
///
/// In nightly Rust, `#[may_dangle]` on the lifetime parameter might be an alternative, to tell the compiler that our `Drop` implementation
/// won't access the borrowed data, but this attribute requires careful safety analysis to ensure it's correctly applied.
pub(crate) struct PassiveGd<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()`.
impl<T: GodotClass> PassiveGd<T> {
/// Creates a passive reference from a strong `Gd<T>` shared reference.
///
/// # Safety
/// The caller must ensure that the underlying object remains valid for the entire lifetime of this `PassiveGd`.
pub unsafe fn from_strong_ref(gd: &Gd<T>) -> Self {
// SAFETY: clone_weak() creates valid weak reference; caller ensures object validity.
let weak_gd = gd.clone_weak();
unsafe { Self::new(weak_gd) }
}

/// Creates a passive reference directly from a raw object pointer.
///
/// This is a direct constructor that avoids the intermediate `Gd::from_obj_sys_weak()` step,
/// providing better performance for the common pattern of creating PassiveGd from raw pointers.
///
/// # Safety
/// - `obj_ptr` must be a valid, live object pointer.
/// - The caller must ensure that the underlying object remains valid for the entire lifetime of this `PassiveGd`.
pub unsafe fn from_obj_sys(obj_ptr: sys::GDExtensionObjectPtr) -> Self {
// SAFETY: from_obj_sys_weak() creates valid weak reference from obj_ptr; caller ensures object validity.
unsafe {
let weak_gd = gd.clone_weak();
Self::from_weak_owned(weak_gd)
let weak_gd = Gd::from_obj_sys_weak(obj_ptr);
Self::new(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.
/// Will invoke `Gd::drop_weak()` when dropped.
///
/// # 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 {
/// - The caller must ensure that the underlying object remains valid for the entire lifetime of this `PassiveGd`.
unsafe fn new(weak_gd: Gd<T>) -> Self {
Self {
weak_gd: ManuallyDrop::new(weak_gd),
_phantom: PhantomData,
}
}
}

impl<T: GodotClass> Drop for PassiveGd<'_, T> {
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) };
Expand All @@ -62,15 +85,15 @@ impl<T: GodotClass> Drop for PassiveGd<'_, T> {
}
}

impl<T: GodotClass> Deref for PassiveGd<'_, T> {
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> {
impl<T: GodotClass> DerefMut for PassiveGd<T> {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.weak_gd
}
Expand Down
58 changes: 33 additions & 25 deletions godot-core/src/obj/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ pub trait WithBaseField: GodotClass + Bounds<Declarer = bounds::DeclUser> {
#[doc(hidden)]
fn base_field(&self) -> &Base<Self::Base>;

/// Returns a shared reference suitable for calling engine methods on this object.
/// Returns a shared reference guard, suitable for calling `&self` engine methods on this object.
///
/// Holding a shared guard prevents other code paths from obtaining a _mutable_ reference to `self`, as such it is recommended to drop the
/// guard as soon as you no longer need it.
Expand All @@ -374,7 +374,7 @@ pub trait WithBaseField: GodotClass + Bounds<Declarer = bounds::DeclUser> {
/// use godot::prelude::*;
///
/// #[derive(GodotClass)]
/// #[class(init, base = Node)]
/// #[class(init, base=Node)]
/// struct MyClass {
/// base: Base<Node>,
/// }
Expand All @@ -386,11 +386,6 @@ pub trait WithBaseField: GodotClass + Bounds<Declarer = bounds::DeclUser> {
/// godot_print!("name is {name}");
/// }
/// }
///
/// # pub struct Test;
///
/// # #[gdextension]
/// # unsafe impl ExtensionLibrary for Test {}
/// ```
///
/// However, we cannot call methods that require `&mut Base`, such as
Expand Down Expand Up @@ -422,25 +417,24 @@ pub trait WithBaseField: GodotClass + Bounds<Declarer = bounds::DeclUser> {
///
/// For this, use [`base_mut()`](WithBaseField::base_mut()) instead.
fn base(&self) -> BaseRef<'_, Self> {
let passive_gd = self.base_field().constructed_passive();
// SAFETY: lifetime is bound to self through BaseRef, ensuring the object remains valid.
let passive_gd = unsafe { self.base_field().constructed_passive() };
BaseRef::new(passive_gd, self)
}

/// Returns a mutable reference suitable for calling engine methods on this object.
/// Returns an exclusive reference guard, suitable for calling `&self`/`&mut self` engine methods on this object.
///
/// This method will allow you to call back into the same object from Godot, unlike what would happen
/// if you used [`to_gd()`](WithBaseField::to_gd). You have to keep the `BaseRef` guard bound for the entire duration the engine might
/// re-enter a function of your class. The guard temporarily absorbs the `&mut self` reference, which allows for an additional mutable
/// reference to be acquired.
/// This method will allow you to call back into the same object from Godot -- something that [`to_gd()`][Self::to_gd] does not allow.
/// You have to keep the `BaseMut` guard bound for the entire duration the engine might re-enter a function of your class. The guard
/// temporarily absorbs the `&mut self` reference, which allows for an additional exclusive (mutable) reference to be acquired.
///
/// Holding a mutable guard prevents other code paths from obtaining _any_ reference to `self`, as such it is recommended to drop the
/// Holding an exclusive guard prevents other code paths from obtaining _any_ reference to `self`, as such it is recommended to drop the
/// guard as soon as you no longer need it.
///
/// # Examples
///
/// ```no_run
/// use godot::prelude::*;
///
/// # use godot::prelude::*;
/// #[derive(GodotClass)]
/// #[class(init, base = Node)]
/// struct MyClass {
Expand All @@ -463,11 +457,10 @@ pub trait WithBaseField: GodotClass + Bounds<Declarer = bounds::DeclUser> {
///
/// We can call back into `self` through Godot:
///
/// ```
/// use godot::prelude::*;
///
/// ```no_run
/// # use godot::prelude::*;
/// #[derive(GodotClass)]
/// #[class(init, base = Node)]
/// #[class(init, base=Node)]
/// struct MyClass {
/// base: Base<Node>,
/// }
Expand All @@ -484,17 +477,32 @@ pub trait WithBaseField: GodotClass + Bounds<Declarer = bounds::DeclUser> {
/// #[func]
/// fn other_method(&mut self) {}
/// }
/// ```
///
/// # pub struct Test;
///
/// # #[gdextension]
/// # unsafe impl ExtensionLibrary for Test {}
/// Rust's borrow checking rules are enforced if you try to overlap `base_mut()` calls:
/// ```compile_fail
/// # use godot::prelude::*;
/// # #[derive(GodotClass)]
/// # #[class(init)]
/// # struct MyStruct {
/// # base: Base<RefCounted>,
/// # }
/// # impl MyStruct {
/// // error[E0499]: cannot borrow `*self` as mutable more than once at a time
///
/// fn method(&mut self) {
/// let mut a = self.base_mut();
/// // ---- first mutable borrow occurs here
/// let mut b = self.base_mut();
/// // ^^^^ second mutable borrow occurs here
/// }
/// # }
/// ```
#[allow(clippy::let_unit_value)]
fn base_mut(&mut self) -> BaseMut<'_, Self> {
// 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 passive_gd = unsafe { self.base_field().constructed_passive() };

let gd = self.to_gd();

Expand Down
12 changes: 12 additions & 0 deletions itest/rust/src/object_tests/base_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,18 @@ impl Based {
use godot::obj::WithBaseField as _;
self.to_gd()
}

// Regression compile test for https://github.com/godot-rust/gdext/pull/1312, causing overly restrictive borrow errors.
// base() + base_mut() guards' lifetime must not be extended too much.
fn _borrow_checks(&mut self) {
for _child in self.base().get_children().iter_shared() {
self.base_mut().rotate(10.0);
}

for i in 0..self.base().get_child_count() {
self.base_mut().rotate(i as real);
}
}
}

#[derive(GodotClass)]
Expand Down
Loading