Skip to content

Commit b0ba40e

Browse files
authored
Merge pull request #1312 from godot-rust/bugfix/base-borrows
Remove lifetime from `PassiveGd`
2 parents a6859b9 + 4a5dfd7 commit b0ba40e

File tree

5 files changed

+102
-71
lines changed

5 files changed

+102
-71
lines changed

godot-core/src/obj/base.rs

Lines changed: 7 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -306,15 +306,16 @@ impl<T: GodotClass> Base<T> {
306306
}
307307

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

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

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

338339
/// Returns a [`PassiveGd`] referencing the base object, assuming the derived object is fully constructed.
339340
///
340-
/// This method directly creates a PassiveGd from a weak reference, providing clean lifetime management
341-
/// without the need for manual `drop_weak()` calls.
342-
pub(crate) fn constructed_passive(&self) -> PassiveGd<'_, T> {
343-
// SAFETY: returned lifetime here is re-bound to self. Covariant lifetime conversion 'static -> 'self.
344-
unsafe { self.constructed_passive_unbounded() }
345-
}
346-
347-
/// Returns a weak [`Gd`] referencing the base object, assuming the derived object is fully constructed.
348-
///
349341
/// Unlike [`Self::__constructed_gd()`], this does not increment the reference count for ref-counted `T`s.
350342
/// The returned weak reference is safe to use only as long as the associated instance remains alive.
351343
///
352344
/// # Safety
353-
/// This method disconnects the lifetime, as opposed to [`Self::constructed_passive()]. Caller is responsible of re-binding the
354-
/// lifetime to the instance.
355-
pub(crate) unsafe fn constructed_passive_unbounded(&self) -> PassiveGd<'static, T> {
345+
/// Caller must ensure that the underlying object remains valid for the entire lifetime of the returned `PassiveGd`.
346+
pub(crate) unsafe fn constructed_passive(&self) -> PassiveGd<T> {
356347
#[cfg(debug_assertions)] // debug_assert! still checks existence of symbols.
357348
assert!(
358349
!self.is_initializing(),
359350
"WithBaseField::base(), base_mut() can only be called on fully-constructed objects, after I*::init() or Gd::from_init_fn()"
360351
);
361352

362-
// Create weak reference from the same object pointer without cloning (incrementing refcount).
363-
let weak_gd = unsafe { Gd::from_obj_sys_weak(self.obj.obj_sys()) };
364-
365-
// SAFETY: weak_gd is a weakly created Gd, and remains valid as long as self is alive (per safety precondition of this fn).
366-
unsafe { PassiveGd::from_weak_owned(weak_gd) }
353+
// SAFETY: object pointer is valid and remains valid as long as self is alive (per safety precondition of this fn).
354+
unsafe { PassiveGd::from_obj_sys(self.obj.obj_sys()) }
367355
}
368356
}
369357

godot-core/src/obj/guards.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -198,12 +198,12 @@ macro_rules! make_base_ref {
198198
#[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")]
199199
#[doc = concat!("See [`", stringify!($doc_type), "::base()`](", stringify!($doc_path), "::base()) for usage.")]
200200
pub struct $ident<'a, T: $bound> {
201-
passive_gd: PassiveGd<'a, T::Base>,
201+
passive_gd: PassiveGd<T::Base>,
202202
_instance: &'a T,
203203
}
204204

205205
impl<'a, T: $bound> $ident<'a, T> {
206-
pub(crate) fn new(passive_gd: PassiveGd<'a, T::Base>, instance: &'a T) -> Self {
206+
pub(crate) fn new(passive_gd: PassiveGd<T::Base>, instance: &'a T) -> Self {
207207
Self {
208208
passive_gd,
209209
_instance: instance,
@@ -231,13 +231,13 @@ macro_rules! make_base_mut {
231231
///
232232
#[doc = concat!("See [`", stringify!($doc_type), "::base_mut()`](", stringify!($doc_path), "::base_mut()) for usage.\n")]
233233
pub struct $ident<'a, T: $bound> {
234-
passive_gd: PassiveGd<'a, T::Base>,
234+
passive_gd: PassiveGd<T::Base>,
235235
_inaccessible_guard: InaccessibleGuard<'a, T>,
236236
}
237237

238238
impl<'a, T: $bound> $ident<'a, T> {
239239
pub(crate) fn new(
240-
passive_gd: PassiveGd<'a, T::Base>,
240+
passive_gd: PassiveGd<T::Base>,
241241
inaccessible_guard: InaccessibleGuard<'a, T>,
242242
) -> Self {
243243
Self {

godot-core/src/obj/passive_gd.rs

Lines changed: 46 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -5,55 +5,78 @@
55
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
66
*/
77

8-
use std::marker::PhantomData;
98
use std::mem::ManuallyDrop;
109
use std::ops::{Deref, DerefMut};
1110

1211
use crate::obj::{Gd, GodotClass};
12+
use crate::sys;
1313

1414
/// Passive (non-owning) reference to a Godot object.
1515
///
16-
/// `PassiveGd<'gd, T>` provides a safe abstraction for weak references to Godot objects. Unlike `Gd<T>`, it does not increment/decrement
16+
/// `PassiveGd<T>` provides an unsafe abstraction for weak references to Godot objects. Unlike `Gd<T>`, it does not increment/decrement
1717
/// the reference count for `RefCounted` objects, and its `Drop` impl only cleans up metadata, not the Godot object.
1818
///
19-
/// 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.
19+
/// This type is for internal use only, to access base objects in guards and traits, and to wrap manual [`Gd::clone_weak()`] and
20+
/// [`Gd::drop_weak()`] patterns.
2021
///
21-
/// This type is primarily used internally for base object access in guards and traits, providing a clean alternative to manual
22-
/// [`Gd::clone_weak()`] and [`Gd::drop_weak()`] patterns.
23-
pub(crate) struct PassiveGd<'gd, T: GodotClass> {
22+
/// # Why no lifetime?
23+
/// Previous versions used `PassiveGd<'gd, T>` with an explicit lifetime parameter. This caused subtle borrow-checking issues due to Rust's
24+
/// [drop check](https://doc.rust-lang.org/nomicon/dropck.html) rules. When a type has drop obligations (implements `Drop` or contains fields
25+
/// that do), the borrow checker conservatively assumes the destructor might access borrowed data reachable through that value, forcing all
26+
/// such borrows to strictly outlive the value. This created false conflicts when creating both shared and mutable base references from the
27+
/// same object, even though our `Drop` implementation never accesses the lifetime-bound data.
28+
///
29+
/// By removing the lifetime parameter and making construction `unsafe`, we eliminate these false-positive borrow conflicts while maintaining
30+
/// memory safety through explicit caller contracts.
31+
///
32+
/// In nightly Rust, `#[may_dangle]` on the lifetime parameter might be an alternative, to tell the compiler that our `Drop` implementation
33+
/// won't access the borrowed data, but this attribute requires careful safety analysis to ensure it's correctly applied.
34+
pub(crate) struct PassiveGd<T: GodotClass> {
2435
weak_gd: ManuallyDrop<Gd<T>>,
25-
26-
// Covariant lifetime: PassiveGd<'a, T> can be used wherever PassiveGd<'b, T> is needed, if 'a: 'b.
27-
_phantom: PhantomData<&'gd ()>,
2836
}
2937

30-
impl<'gd, T: GodotClass> PassiveGd<'gd, T> {
31-
pub fn from_strong_ref(gd: &Gd<T>) -> Self {
32-
// SAFETY:
33-
// - `clone_weak()` creates a pointer conforming to `from_weak_gd()` requirements.
34-
// - PassiveGd will destroy the pointer with `drop_weak()`.
38+
impl<T: GodotClass> PassiveGd<T> {
39+
/// Creates a passive reference from a strong `Gd<T>` shared reference.
40+
///
41+
/// # Safety
42+
/// The caller must ensure that the underlying object remains valid for the entire lifetime of this `PassiveGd`.
43+
pub unsafe fn from_strong_ref(gd: &Gd<T>) -> Self {
44+
// SAFETY: clone_weak() creates valid weak reference; caller ensures object validity.
45+
let weak_gd = gd.clone_weak();
46+
unsafe { Self::new(weak_gd) }
47+
}
48+
49+
/// Creates a passive reference directly from a raw object pointer.
50+
///
51+
/// This is a direct constructor that avoids the intermediate `Gd::from_obj_sys_weak()` step,
52+
/// providing better performance for the common pattern of creating PassiveGd from raw pointers.
53+
///
54+
/// # Safety
55+
/// - `obj_ptr` must be a valid, live object pointer.
56+
/// - The caller must ensure that the underlying object remains valid for the entire lifetime of this `PassiveGd`.
57+
pub unsafe fn from_obj_sys(obj_ptr: sys::GDExtensionObjectPtr) -> Self {
58+
// SAFETY: from_obj_sys_weak() creates valid weak reference from obj_ptr; caller ensures object validity.
3559
unsafe {
36-
let weak_gd = gd.clone_weak();
37-
Self::from_weak_owned(weak_gd)
60+
let weak_gd = Gd::from_obj_sys_weak(obj_ptr);
61+
Self::new(weak_gd)
3862
}
3963
}
4064

4165
/// Creates a passive reference directly from a weak `Gd<T>`.
4266
///
43-
/// Will invoke `Gd::drop_weak()` when dropped. Since the parameter has no lifetime, you need to provide the lifetime `'gd` explicitly.
67+
/// Will invoke `Gd::drop_weak()` when dropped.
4468
///
4569
/// # Safety
4670
/// - `weak_gd` must be a weakly created `Gd`, e.g. from [`Gd::clone_weak()`] or [`Gd::from_obj_sys_weak()`].
47-
/// - The caller must ensure that the `weak_gd` remains valid for the lifetime `'gd`.
48-
pub unsafe fn from_weak_owned(weak_gd: Gd<T>) -> Self {
71+
/// - The caller must ensure that the underlying object remains valid for the entire lifetime of this `PassiveGd`.
72+
unsafe fn new(weak_gd: Gd<T>) -> Self {
4973
Self {
5074
weak_gd: ManuallyDrop::new(weak_gd),
51-
_phantom: PhantomData,
5275
}
5376
}
5477
}
5578

56-
impl<T: GodotClass> Drop for PassiveGd<'_, T> {
79+
impl<T: GodotClass> Drop for PassiveGd<T> {
5780
fn drop(&mut self) {
5881
// SAFETY: Only extracted once, in Drop.
5982
let weak = unsafe { ManuallyDrop::take(&mut self.weak_gd) };
@@ -62,15 +85,15 @@ impl<T: GodotClass> Drop for PassiveGd<'_, T> {
6285
}
6386
}
6487

65-
impl<T: GodotClass> Deref for PassiveGd<'_, T> {
88+
impl<T: GodotClass> Deref for PassiveGd<T> {
6689
type Target = Gd<T>;
6790

6891
fn deref(&self) -> &Self::Target {
6992
&self.weak_gd
7093
}
7194
}
7295

73-
impl<T: GodotClass> DerefMut for PassiveGd<'_, T> {
96+
impl<T: GodotClass> DerefMut for PassiveGd<T> {
7497
fn deref_mut(&mut self) -> &mut Self::Target {
7598
&mut self.weak_gd
7699
}

godot-core/src/obj/traits.rs

Lines changed: 33 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -363,7 +363,7 @@ pub trait WithBaseField: GodotClass + Bounds<Declarer = bounds::DeclUser> {
363363
#[doc(hidden)]
364364
fn base_field(&self) -> &Base<Self::Base>;
365365

366-
/// Returns a shared reference suitable for calling engine methods on this object.
366+
/// Returns a shared reference guard, suitable for calling `&self` engine methods on this object.
367367
///
368368
/// Holding a shared guard prevents other code paths from obtaining a _mutable_ reference to `self`, as such it is recommended to drop the
369369
/// guard as soon as you no longer need it.
@@ -374,7 +374,7 @@ pub trait WithBaseField: GodotClass + Bounds<Declarer = bounds::DeclUser> {
374374
/// use godot::prelude::*;
375375
///
376376
/// #[derive(GodotClass)]
377-
/// #[class(init, base = Node)]
377+
/// #[class(init, base=Node)]
378378
/// struct MyClass {
379379
/// base: Base<Node>,
380380
/// }
@@ -386,11 +386,6 @@ pub trait WithBaseField: GodotClass + Bounds<Declarer = bounds::DeclUser> {
386386
/// godot_print!("name is {name}");
387387
/// }
388388
/// }
389-
///
390-
/// # pub struct Test;
391-
///
392-
/// # #[gdextension]
393-
/// # unsafe impl ExtensionLibrary for Test {}
394389
/// ```
395390
///
396391
/// However, we cannot call methods that require `&mut Base`, such as
@@ -422,25 +417,24 @@ pub trait WithBaseField: GodotClass + Bounds<Declarer = bounds::DeclUser> {
422417
///
423418
/// For this, use [`base_mut()`](WithBaseField::base_mut()) instead.
424419
fn base(&self) -> BaseRef<'_, Self> {
425-
let passive_gd = self.base_field().constructed_passive();
420+
// SAFETY: lifetime is bound to self through BaseRef, ensuring the object remains valid.
421+
let passive_gd = unsafe { self.base_field().constructed_passive() };
426422
BaseRef::new(passive_gd, self)
427423
}
428424

429-
/// Returns a mutable reference suitable for calling engine methods on this object.
425+
/// Returns an exclusive reference guard, suitable for calling `&self`/`&mut self` engine methods on this object.
430426
///
431-
/// This method will allow you to call back into the same object from Godot, unlike what would happen
432-
/// if you used [`to_gd()`](WithBaseField::to_gd). You have to keep the `BaseRef` guard bound for the entire duration the engine might
433-
/// re-enter a function of your class. The guard temporarily absorbs the `&mut self` reference, which allows for an additional mutable
434-
/// reference to be acquired.
427+
/// This method will allow you to call back into the same object from Godot -- something that [`to_gd()`][Self::to_gd] does not allow.
428+
/// You have to keep the `BaseMut` guard bound for the entire duration the engine might re-enter a function of your class. The guard
429+
/// temporarily absorbs the `&mut self` reference, which allows for an additional exclusive (mutable) reference to be acquired.
435430
///
436-
/// Holding a mutable guard prevents other code paths from obtaining _any_ reference to `self`, as such it is recommended to drop the
431+
/// Holding an exclusive guard prevents other code paths from obtaining _any_ reference to `self`, as such it is recommended to drop the
437432
/// guard as soon as you no longer need it.
438433
///
439434
/// # Examples
440435
///
441436
/// ```no_run
442-
/// use godot::prelude::*;
443-
///
437+
/// # use godot::prelude::*;
444438
/// #[derive(GodotClass)]
445439
/// #[class(init, base = Node)]
446440
/// struct MyClass {
@@ -463,11 +457,10 @@ pub trait WithBaseField: GodotClass + Bounds<Declarer = bounds::DeclUser> {
463457
///
464458
/// We can call back into `self` through Godot:
465459
///
466-
/// ```
467-
/// use godot::prelude::*;
468-
///
460+
/// ```no_run
461+
/// # use godot::prelude::*;
469462
/// #[derive(GodotClass)]
470-
/// #[class(init, base = Node)]
463+
/// #[class(init, base=Node)]
471464
/// struct MyClass {
472465
/// base: Base<Node>,
473466
/// }
@@ -484,17 +477,32 @@ pub trait WithBaseField: GodotClass + Bounds<Declarer = bounds::DeclUser> {
484477
/// #[func]
485478
/// fn other_method(&mut self) {}
486479
/// }
480+
/// ```
487481
///
488-
/// # pub struct Test;
489-
///
490-
/// # #[gdextension]
491-
/// # unsafe impl ExtensionLibrary for Test {}
482+
/// Rust's borrow checking rules are enforced if you try to overlap `base_mut()` calls:
483+
/// ```compile_fail
484+
/// # use godot::prelude::*;
485+
/// # #[derive(GodotClass)]
486+
/// # #[class(init)]
487+
/// # struct MyStruct {
488+
/// # base: Base<RefCounted>,
489+
/// # }
490+
/// # impl MyStruct {
491+
/// // error[E0499]: cannot borrow `*self` as mutable more than once at a time
492+
///
493+
/// fn method(&mut self) {
494+
/// let mut a = self.base_mut();
495+
/// // ---- first mutable borrow occurs here
496+
/// let mut b = self.base_mut();
497+
/// // ^^^^ second mutable borrow occurs here
498+
/// }
499+
/// # }
492500
/// ```
493501
#[allow(clippy::let_unit_value)]
494502
fn base_mut(&mut self) -> BaseMut<'_, Self> {
495503
// We need to construct this first, as the mut-borrow below will block all other access.
496504
// SAFETY: lifetime is re-established at the bottom BaseMut construction, since return type of this fn has lifetime bound to instance.
497-
let passive_gd = unsafe { self.base_field().constructed_passive_unbounded() };
505+
let passive_gd = unsafe { self.base_field().constructed_passive() };
498506

499507
let gd = self.to_gd();
500508

itest/rust/src/object_tests/base_test.rs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,18 @@ impl Based {
260260
use godot::obj::WithBaseField as _;
261261
self.to_gd()
262262
}
263+
264+
// Regression compile test for https://github.com/godot-rust/gdext/pull/1312, causing overly restrictive borrow errors.
265+
// base() + base_mut() guards' lifetime must not be extended too much.
266+
fn _borrow_checks(&mut self) {
267+
for _child in self.base().get_children().iter_shared() {
268+
self.base_mut().rotate(10.0);
269+
}
270+
271+
for i in 0..self.base().get_child_count() {
272+
self.base_mut().rotate(i as real);
273+
}
274+
}
263275
}
264276

265277
#[derive(GodotClass)]

0 commit comments

Comments
 (0)