From 4a5dfd7c9350c0ab5907b78d9ddb85591b898e4a Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Mon, 15 Sep 2025 23:27:27 +0200 Subject: [PATCH] Remove lifetime from `PassiveGd` The lifetime's intention was to express the "outlives" relation from `&self` to `PassiveGd<'_, T>`. This worked, however it also introduced a breaking change in the `BaseRef` + `BaseMut` guards. Code such as the following now caused borrow errors: for i in 0..self.base().get_child_count() { self.base_mut().rotate(10.0); } This isn't very intuitive, nor particularly helpful for soundness. This reverts the `PartialGd<'gd, T>` to `PartialGd` with unsafe constructors. In practice, quite some unsafety was still needed anyway (e.g. `BaseMut` needed an unsafe `'static` intermediate reference). --- godot-core/src/obj/base.rs | 26 +++------ godot-core/src/obj/guards.rs | 8 +-- godot-core/src/obj/passive_gd.rs | 69 ++++++++++++++++-------- godot-core/src/obj/traits.rs | 58 +++++++++++--------- itest/rust/src/object_tests/base_test.rs | 12 +++++ 5 files changed, 102 insertions(+), 71 deletions(-) diff --git a/godot-core/src/obj/base.rs b/godot-core/src/obj/base.rs index 3b5f6432a..c467e2f63 100644 --- a/godot-core/src/obj/base.rs +++ b/godot-core/src/obj/base.rs @@ -306,7 +306,7 @@ impl Base { } /// 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 { #[cfg(debug_assertions)] assert_eq!( self.init_state.get(), @@ -314,7 +314,8 @@ impl Base { "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` is currently in the initializing state. @@ -337,33 +338,20 @@ impl Base { /// 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 { #[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()) } } } diff --git a/godot-core/src/obj/guards.rs b/godot-core/src/obj/guards.rs index 33b3f36e1..d35db2689 100644 --- a/godot-core/src/obj/guards.rs +++ b/godot-core/src/obj/guards.rs @@ -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, _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, instance: &'a T) -> Self { Self { passive_gd, _instance: instance, @@ -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, _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, inaccessible_guard: InaccessibleGuard<'a, T>, ) -> Self { Self { diff --git a/godot-core/src/obj/passive_gd.rs b/godot-core/src/obj/passive_gd.rs index deef78357..522e2f2af 100644 --- a/godot-core/src/obj/passive_gd.rs +++ b/godot-core/src/obj/passive_gd.rs @@ -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`, it does not increment/decrement +/// `PassiveGd` provides an unsafe 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 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 { 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()`. +impl PassiveGd { + /// Creates a passive reference from a strong `Gd` 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) -> 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`. /// - /// 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) -> Self { + /// - The caller must ensure that the underlying object remains valid for the entire lifetime of this `PassiveGd`. + unsafe fn new(weak_gd: Gd) -> Self { Self { weak_gd: ManuallyDrop::new(weak_gd), - _phantom: PhantomData, } } } -impl Drop for PassiveGd<'_, T> { +impl Drop for PassiveGd { fn drop(&mut self) { // SAFETY: Only extracted once, in Drop. let weak = unsafe { ManuallyDrop::take(&mut self.weak_gd) }; @@ -62,7 +85,7 @@ impl Drop for PassiveGd<'_, T> { } } -impl Deref for PassiveGd<'_, T> { +impl Deref for PassiveGd { type Target = Gd; fn deref(&self) -> &Self::Target { @@ -70,7 +93,7 @@ impl Deref for PassiveGd<'_, T> { } } -impl DerefMut for PassiveGd<'_, T> { +impl DerefMut for PassiveGd { fn deref_mut(&mut self) -> &mut Self::Target { &mut self.weak_gd } diff --git a/godot-core/src/obj/traits.rs b/godot-core/src/obj/traits.rs index 7706a960b..068018ec9 100644 --- a/godot-core/src/obj/traits.rs +++ b/godot-core/src/obj/traits.rs @@ -363,7 +363,7 @@ pub trait WithBaseField: GodotClass + Bounds { #[doc(hidden)] fn base_field(&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. @@ -374,7 +374,7 @@ pub trait WithBaseField: GodotClass + Bounds { /// use godot::prelude::*; /// /// #[derive(GodotClass)] - /// #[class(init, base = Node)] + /// #[class(init, base=Node)] /// struct MyClass { /// base: Base, /// } @@ -386,11 +386,6 @@ pub trait WithBaseField: GodotClass + Bounds { /// 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 @@ -422,25 +417,24 @@ pub trait WithBaseField: GodotClass + Bounds { /// /// 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 { @@ -463,11 +457,10 @@ pub trait WithBaseField: GodotClass + Bounds { /// /// 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, /// } @@ -484,17 +477,32 @@ pub trait WithBaseField: GodotClass + Bounds { /// #[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, + /// # } + /// # 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(); diff --git a/itest/rust/src/object_tests/base_test.rs b/itest/rust/src/object_tests/base_test.rs index acb91b118..57ebf74f0 100644 --- a/itest/rust/src/object_tests/base_test.rs +++ b/itest/rust/src/object_tests/base_test.rs @@ -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)]