Add the ability to make reentrant &mut self calls

[lilizoey]

This PR allows for situations where a &mut self is held, then a call is made to godot which then calls back to the same rust object and a new &mut self is taken. For instance this code adapted from #338:

#[godot_api]
impl INode for Bar {
    fn ready(&mut self) {
        let foo = Node::new_alloc();
        self.base_mut().add_child(foo.clone().upcast());
    }

    fn on_notification(&mut self, what: NodeNotification) {}
}

The Problem

The issue with the code above originally is that our current instance storage uses RefCell (or RwLock for threads) to track borrows. And this places a strict restriction on any code coming from Godot: If there exists a &mut T reference, then no other references can be taken.

This is a simple and useful rule, however it doesn't entirely match with how rust works in general, take this code for instance:

fn first(&mut self) {
  self.second();
}

fn second(&mut self) {}

In pure rust, this is an entirely ok thing to do. This compiles without issue. However take this code which emulates how we do it currently:

// in `SomeClass`
fn first(&mut self) {
  self.godot.call(Self::second);
}

fn second(&mut self) {}

// in `Godot`
fn call(&self, f: fn(&mut SomeClass)) {
  let mut instance = self.instance.borrow_mut();
  f(&mut instance);
}

This code must fail, because there already exists a &mut SomeClass reference and we thus cannot get a new one from RefCell.

However there is no good reason to actually reject this pattern, the only reason we do is because we're coming from ffi and RefCell has no way of knowing that we actually can take a new &mut T reference here.

The idea

So to enable code like the above we must create a different kind of cell that can be made aware that a new &mut SomeClass reference is fine to create.

The question then is, when can we do so? The simple answer is: as long as it doesn't alias. But that answer is almost entirely useless as rust doesn't have a defined aliasing model yet. There are however some things we can be confident about, as both current proposed aliasing models (tree borrows and stacked borrows) guarantee this and any future aliasing model almost certainly will continue to work like this. Since this is how &mut references work in safe rust.

1. Aliasing is based on accesses. i.e merely creating a new &mut T while one already exists is not aliasing, it must actually be written to or read from in some way to cause UB
2. Assuming b: &mut T is derived from a a: &mut T. Then it is fine to:

• Stop accessing a
• Start accessing b
• Stop accessing b, and never access b again
• Start accessing a again

So if we can create a cell that is able to essentially lock down some reference a: &mut T, preventing it from being written to or read from. Then that cell is free to hand out a new b: &mut T, which is derived from a, for as long as it can guarantee that a is not being accessed.

The way to ensure this is with a guard, we create a guard that takes this a: &mut T and makes it inaccessible while the guard exists. Then rust will helpfully ensure that no other references to the same object can exist. As to the rust compiler, if we've passed a to this guard, then that means we cannot use a or any other derived reference until we drop this guard. Because a is a mutable reference. Thus, as long as we can inform the cell when such a guard is created, and hand that cell a new pointer derived from a, then the cell can hand out a new &mut T reference again.

Implementation details

godot-cell

This PR adds a new crate godot-cell. This is added as a separate crate to make it easier to get the unsafety right, as we can then evaluate the cell's safety on its own, completely independent of all the other gdext code which has lots of other unsafe that could interact in weird ways.

The public API of godot-cell is: GdCell, MutGuard, NonAliasingGuard, RefGuard. And it exposes no unsafe functions currently.

The borrowing logic is implemented in borrow_state.rs, this has a BorrowState struct that contains all the data needed to keep track of when various borrows are available. It is entirely safe on its own, as it's purely data tracking the state of the borrows, but doesn't actually maintain any pointers on its own. I used proptest to fuzz all important invariants that this struct must uphold.

guards.rs contains the three guards we expose there, MutGuard, NonAliasingGuard, RefGuard. MutGuard and RefGuard function very similar to RefCell's Ref and RefMut. In fact if we ignore NonAliasingGuard then this cell is basically just a thread-safe less optimized RefCell. NonAliasingGuard is the guard that stores the &mut reference and makes it inaccessible so that the cell can hand out a new &mut reference.

GdCell is the actual cell itself, it stores the BorrowState, the value that the cell contains, as well as a stack of pointers. This stack of pointers is pushed onto whenever a NonAliasingGuard is created, and we create new references from the top of the stack. This is to ensure that any reference we create is derived from the most recent reference. This also makes it so that putting an unrelated reference into the NonAliasingGuard would still be safe to do, however we do error if that happens as that is almost certainly a logic error.

I set up a mock in tests/mock.rs which tries to emulate some of the relevant logic we do in gdext using pure rust, so that we can run miri on this mock to check if miri is happy with the pattern. I also added a new job to the full-ci, which runs miri on godot-cell using both stacked and tree borrows.

Changes to godot-core

The single-threaded and multi-threaded storage was largely just adapted to use GdCell instead of RefCell/RwLock. But i did need to add an extra methods to be able to call set_non_aliasing on the GdCell,

Referring to the change to Base described below, i made base_mut() use the set_non_aliasing on the instance storage. This required me to extend the lifetime of the returned storage. To do this i added a pub(crate) unsafe fn storage_unbounded method in Gd. As otherwise there was no way to convince rust that the returned reference was in fact not referencing a local variable. Since Gd::storage's definition makes rust believe the instance storage pointer is derived from the &self pointer when in reality it outlives the &self pointer.

Of course storage_unbounded must be unsafe as it could be used to make a reference that outlives the instance storage, but at least in the case i used it it never will.

Refactorings in this PR

These refactorings can be split off if desired.

Base no longer Derefs

Now Base has no public api, instead users are expected to use the trait WithBaseField to access the base object, by using to_gd(), base() or base_mut() instead depending on use-case.

This means that code such as:

let position = self.base.get_position();
self.base.set_position(position + Vector2::new(1.0, 0.0));

Becomes:

let position = self.base().get_position();
self.base_mut().set_position(position + Vector2::new(1.0, 0.0));

Storage trait

There is now a trait called Storage, and an internal trait StorageRefCounted. These are used to provide a common API for instance storage implementations. It was used while developing this PR to ensure parity with the old instance storages, but we could now revert that to the old style if preferred.

Advantages

By making all accesses to instance storages use one of the trait methods, we no longer need to worry about signature mismatches due to cfg-differences.

It's easier to ensure parity between our two instance storages, as we just need to ensure they conform to the trait's definition.

We have a more obvious place to document the instance storage, as well as requirements and safety invariants. Rather than needing to duplicate documentation among both our instance storages.

We can more easily reduce the amount of cfg needed outside of storage.rs, as we can now refer to associated types of the Storage trait. Rather than needing to choose a type concretely via cfg. See guards.rs for example of that.

Disadvantages

Now calling a storage method in our macros is a bit more complicated, we need to either import the trait or use ::godot::private::Storage::method(..). We only do this three places however so it isn't a huge concern.

Changing visibility of methods is less convenient, as all methods have the same visibility as the trait.

We dont need the polymorphism that the traits enable. We may in the future want this, but this trait is not designed for that use-case.

fixes #338

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-501

[Bromeon]

Thanks so much! This is amazing, I didn't expect it would be possible to bend the Rust type system so much 🎸

Also, I really appreciate the thorough documentation and explanations in the code. I commented quite a lot on it -- not because it's lacking, but rather because I consider this matter rather complex, and having very well-defined terminology will help the understanding, even in one year 😎

About mutexes and .lock().unwrap() statements:

1. Can it happen that panics poison the lock, grinding things to a halt?
2. If another thread (later, once we implement that) holds a Mutex, the lock call itself (not the unwrap) will panic. In methods where you return Result, could it make sense to use try_lock() instead? You can still treat TryLockResult::Err as an error, even in case of poisons.
3. You made a comment about possibly removing Mutex. I guess you are thinking of RefCell in single-threaded contexts? For multi-threaded ones, it might be that RwLock is more flexible, allowing coexisting shared borrows.

FIrst review, just covering part of the code. 🙈

[Bromeon]

Suggested change

	- uses: actions/checkout@v3
	- uses: actions/checkout@v4

In the meantime I updated 😉

[Bromeon]

Could this be passed as a new input to https://github.com/godot-rust/gdext/blob/master/.github/composite/rust/action.yml ?

I guess it won't work as the components: input, due to nightly and the extra step 🤔

[Bromeon]

proptest has quite a few dependencies:

proptest v1.4.0
├── bit-set v0.5.3
│   └── bit-vec v0.6.3
├── bit-vec v0.6.3
├── bitflags v2.4.1
├── lazy_static v1.4.0
├── num-traits v0.2.17
│   └── libm v0.2.8
│   [build-dependencies]
│   └── autocfg v1.1.0
├── rand v0.8.5
│   ├── rand_chacha v0.3.1
│   │   ├── ppv-lite86 v0.2.17
│   │   └── rand_core v0.6.4
│   │       └── getrandom v0.2.11
│   │           └── cfg-if v1.0.0
│   └── rand_core v0.6.4 (*)
├── rand_chacha v0.3.1 (*)
├── rand_xorshift v0.3.0
│   └── rand_core v0.6.4 (*)
├── regex-syntax v0.8.2
├── rusty-fork v0.3.0
│   ├── fnv v1.0.7
│   ├── quick-error v1.2.3
│   ├── tempfile v3.8.1
│   │   ├── cfg-if v1.0.0
│   │   ├── fastrand v2.0.1
│   │   └── windows-sys v0.48.0
│   │       └── windows-targets v0.48.5
│   │           └── windows_x86_64_msvc v0.48.5
│   └── wait-timeout v0.2.0
├── tempfile v3.8.1 (*)
└── unarray v0.1.4

It's only a dev dependency, but will still need to be run in CI -- do you know what the miri job duration is?

Also, it looks like it comes with quite a few features on-by-default, do you think some could be turned off?

[lilizoey]

you can look here for last time i ran the full ci on my fork:
https://github.com/lilizoey/gdextension/actions/runs/6997929311
miri-test took 47 seconds there.

[Bromeon]

29s of which were spent on dependencies and compilation 😛

No, 47s for that job is fine. But we need to keep in mind that [dev-dependencies] is not only used for miri, but also for unit tests (and examples or benchmarks, which are not applicable here). So this will have a ripple effect on all 3 unit-test jobs, at least when they're not cached.

Could we use something like [target.'cfg(...)'.dev-dependencies]?

[lilizoey]

honestly the easiest way to make sure this doesn't have this ripple effect is to just add a feature which enables proptest and a specific CI job that runs proptest. so i might just do that.

[Bromeon]

This requirement is circular, in new() you write:

    /// # Safety
    ///
    /// It must be safe to call [`as_ref()`](NonNull::as_ref) on `value` for as long as the guard is not
    /// dropped.

Maybe the causality is twisted, and this would rather be a consequence than a cause of upholding the new() safety invariants?

[lilizoey]

I'm not sure what you're saying here. If a is a consequence of b, then b is a cause of a. To me it seems the same to say "as a consequence of new(), it is safe to call as_ref()" and "because of new(), it is safe to call as_ref()"

[Bromeon]

The wording of # Safety in new():

It must be safe to call as_ref() on value for as long as the guard is not dropped.

however sounds very much like "you must ensure that ... is safe to call" (i.e. precondition), and not a consequence (i.e. postcondition). This could be reworded, and listed at the end (after the preconditions), along the lines "if you ensure above invariants, then ...".

[Bromeon]

You probably meant:

Suggested change

	/// No other mutable borrows to the same value will be created while this guard exists.
	/// No mutable borrows to the same value can be created while this guard exists.

[lilizoey]

sure, i don't think there's a big difference between saying "will" or "can" here.

[Bromeon]

My first thought with "will" was that it's possible to create others, but the current implementation doesn't.
"can" is stronger in the sense that "it's enforced".

Also, omitting "other" is slightly clearer, as this borrow is not mutable.

[Bromeon]

since set_non_aliasing ensures any references matches the ones this one would return

Not sure here what you meant exactly, would it be "any reference matches" or "any references match"?

Also "pass in" -- where?

And only one mutable reference to the same value can exist since we cannot have any other aliasing mutable references around to pass in.

This sounds a bit circular, although it probably is not: "only one reference can exist because there are no other ones". Maybe differentiate more clearly between the thing that holds the value and the thing that's passed in (maybe we should coin/use some consistent terms) 🙂

[Bromeon]

Will error if there is no current possibly aliasing mutable borrow

Do you mean "if there is an aliasining mutable borrow"?
What does "possibly" mean here?

[lilizoey]

Possibly aliasing means that the reference may be used in an aliasing manner in the future. But isn't actually guaranteed to.

And no, there needs to be a current mutable borrow that may alias other borrows. That is because we are trying to stop that borrow in particular from aliasing any borrows.

We cannot set some borrow to be non-aliasing, if it's already non-aliasing. So we panic if there is no borrow around to set as non-aliasing.

[Bromeon]

Is "aliasing" used in the "another reference can exist" sense of Rust, or in the "non-aliasing" property that is part of this crate? If the latter, we should update it to the new term (whatever we converge on), to make things clearer.

If "possibly" just means "can be in the future", then I would omit it. This is already implied by "current".
Otherwise, this word hints that there is another condition playing into this, without mentioning said condition.

[lilizoey]

Aliasing is consistently used as in the sense in rust. i.e that the reference exists and is accessed in an aliasing manner.

If two mutable references exist concurrently then they may alias if used wrong. But not necessarily.

A reference is set as non-aliasing if we prevent any accesses to that reference in an aliasing manner with future references.

[Bromeon]

Do you think it would make sense to name the lifetimes?

E.g. 'cell and 'val or so...

[Bromeon]

Keep in mind that in multithreaded code it is still possible for this to return true, and then the cell hands out a new borrow before it is destroyed. So we still need to ensure that this cannot happen at the same time.

Yes, very good observation.

In multithreaded contexts, any follow-up actions need to be protected by the mutex itself, which would likely lead to some API like self.if_currently_bound(|| action). Alternatively, surrounding mutexes (e.g. storage) could take over this job.

No need to do anything along those lines now, but maybe you could add TODO(threads). If we then get around the whole multi-threaded topic and revisit this code, it's easier to spot such issues.

[lilizoey]

They dont need to be protected by the mutex itself necessarily, since if we have a &mut GdCell then we know that calling is_currently_bound() on it cannot race with another value. Since we have a &mut reference to the cell.

[Bromeon]

I'm not following here; how do we get a &mut GdCell if we have a Pin<GdCell> at the call site?
(since it doesn't implement Unpin)

[lilizoey]

Well more accurately we'd have a &mut Pin<Box<GdCell>>, same applies to that.

[Bromeon]

Suggested change

	// SAFETY:
	// `T` is sync so we can return references to it on different threads.
	// Additionally all internal state is synchronized via a mutex, so we wont have race conditions when trying
	// to use it from multiple threads.
	unsafe impl<T: Sync> Sync for GdCell<T> {}
	// SAFETY: `T` is sync so we can return references to it on different threads.
	// Additionally all internal state is synchronized via a mutex, so we won't have race conditions when trying
	// to use it from multiple threads.
	unsafe impl<T: Sync> Sync for GdCell<T> {}

[Bromeon]

There are now quite some indirections, e.g.

• GdMut -> MutGuard -> Mutex<CellState<T>>
• GdRef -> RefGuard -> Mutex<CellState<T>>
• BaseMut -> BaseMutGuard -> NonAliasingGuard
• BaseGuard -> NonAliasingGuard

Are they really all necessary? If yes, can you document this list and other relations somewhere (not public docs, but e.g. crate docs of godot-cell, or godot-core/obj/guards.rs)?

Additionally we have quite some code duplication in single/multi-threaded Storage impls -- which the new trait does not seem to be able to eliminate. Or is that pending a future refactoring? I see the point of enforcing signatures with the traits, but maybe we should also rethink if this actually simplify things (not in this PR). Otherwise we're really stacking layers of abstraction 😅

[Bromeon]

Does this not test the same as prevent_mut_mut above?

At least from the name it's not quite clear...

[lilizoey]

oh yeah huh

[Bromeon]

Is the order here relevant?

If not, I'd group the binding/guard/assert together, after key.

[lilizoey]

I guess it wouldn't make a big difference, but the reason for this order is that i dont wanna increment the counter unless i actually can create a new instance, and i only know i can create a new instance once the lock has been acquired.

Though since this is just a mock test it shouldn't really matter in practice

[Bromeon]

but the reason for this order is that i dont wanna increment the counter unless i actually can create a new instance, and i only know i can create a new instance once the lock has been acquired.

I see. Could you document these thoughts in a brief comment? 🙂

[Bromeon]

Could these be separate tests?

And/or extract the common structure in a function?

[Bromeon]

Is the : GodotClass bound truly required?

I usually try to get the minimum possible bounds -- we also don't add UserClass etc here, even though they'd be present. Since a user cannot create GdRef/GdMut, we don't lose type safety, but the type system becomes a bit simpler.

[lilizoey]

I mean it's either the cfg from before or we use the Storage trait which does require GodotClass atm. i can check if we can remove the GodotClass bound but i doubt it.

[Bromeon]

Thanks for checking. Maybe it's also possible to have the bound only on individual methods rather than the type.

Otherwise not that important.

[lilizoey]

It doesn't really seem possible, we need a : GodotClass bound on the instancestorages themselves, which requires us to implement Storage for T: GodotClass which in the end means that T must implement GodotClass in the guards anyway.

[Bromeon]

OK, fair enough! 🙂

[Bromeon]

Suggested change

	/// This can be used to call methods on the base object of a rust object that take `&self` or `&mut self` as
	/// the receiver.
	/// This can be used to call methods on the base object of a Rust object that takes `&self` or `&mut self` as
	/// the receiver.

[Lamby777]

isn't "take" correct here, since you're talking about the methods (plural) ?

[Bromeon]

True, it's the methods.

Maybe "methods on a Rust object's base" would help. Thanks!

[Bromeon]

Suggested change

	/// See [`WithBaseField::base_mut()`](super::WithBaseField::base_mut()) for usage.
	/// See [`WithBaseField::base_mut()`](super::WithBaseField::base_mut) for usage.

The () is usually only needed in the "to-linked" part when disambiguating, e.g. when there's a field of same name.

[Bromeon]

29s of which were spent on dependencies and compilation 😛

No, 47s for that job is fine. But we need to keep in mind that [dev-dependencies] is not only used for miri, but also for unit tests (and examples or benchmarks, which are not applicable here). So this will have a ripple effect on all 3 unit-test jobs, at least when they're not cached.

Could we use something like [target.'cfg(...)'.dev-dependencies]?

[Bromeon]

See discussion here.

Suggested change

	ptr: Option<NonNull<T>>,
	}
	impl<T> CellState<T> {
	/// Create a new uninitialized state. Use [`initialize_ptr()`](CellState::initialize_ptr()) to initialize
	/// it.
	fn new() -> Self {
	Self {
	borrow_state: BorrowState::new(),
	ptr: None,
	}
	}
	/// Initialize the pointer if it is `None`.
	fn initialize_ptr(&mut self, value: &UnsafeCell<T>) {
	if self.ptr.is_none() {
	self.set_ptr(NonNull::new(value.get()).unwrap());
	} else {
	panic!("Cannot initialize pointer as it is already initialized.")
	}
	}
	/// Returns the current pointer. Panics if uninitialized.
	fn get_ptr(&self) -> NonNull<T> {
	self.ptr.unwrap()
	}
	/// Set the current pointer to the new pointer.
	fn set_ptr(&mut self, new_ptr: NonNull<T>) {
	self.ptr = Some(new_ptr);
	}
	ptr: *mut T,
	}
	impl<T> CellState<T> {
	/// Create a new uninitialized state. Use [`initialize_ptr()`](CellState::initialize_ptr()) to initialize
	/// it.
	fn new() -> Self {
	Self {
	borrow_state: BorrowState::new(),
	ptr: std::ptr::null_mut(),
	}
	}
	/// Initialize the pointer if it is `null`.
	fn initialize_ptr(&mut self, value: &UnsafeCell<T>) {
	assert!(self.ptr.is_null(), "pointer is already initialized.");
	self.ptr = value.get();
	}
	/// Returns the current pointer. Panics if uninitialized.
	fn get_ptr(&self) -> NonNull<T> {
	NonNull::new(self.ptr).expect("pointer is uninitialized.")
	}
	/// Set the current pointer to the new pointer.
	fn set_ptr(&mut self, new_ptr: NonNull<T>) {
	self.ptr = new_ptr.as_ptr();
	}

[lilizoey]

There are now quite some indirections, e.g.

* `GdMut` -> `MutGuard` -> `Mutex<CellState<T>>`

* `GdRef` -> `RefGuard` -> `Mutex<CellState<T>>`

* `BaseMut` -> `BaseMutGuard` -> `NonAliasingGuard`

* `BaseGuard` -> `NonAliasingGuard`

Are they really all necessary? If yes, can you document this list and other relations somewhere (not public docs, but e.g. crate docs of godot-cell, or godot-core/obj/guards.rs)?

The list is actually more like

gdext API	type alias	godot-cell API	internal
GdMut<'a, T>	MutGuard<'a, T>	MutGuard<'a, T>	Mutex<CellState<T>>
GdRef<'a, T>	RefGuard<'a, T>	RefGuard<'a, T>	Mutex<CellState<T>>
BaseMut<'a, T>	BaseMutGuard<'a, T>	InaccessibleGuard<'a, T>	Mutex<CellState<T>>
BaseRef<'a, T>	-	-	Gd<T>

The type alias column is to avoid #[cfg] stuff in the guards, by just having them refer to an associated type alias in InstanceStorage. But if we commit to making all the guards use the same godot-cell types then we can just remove this type alias and hardcode storages to using those types then we can avoid both #[cfg] and type aliases. However that would make it harder to then make two different kinds of guards in godot-cell for multi-threaded vs single-threaded code. In which case we either need to choose between #[cfg] stuff or type aliases. And imo using type aliases here is nicer. Obviously we still need #[cfg] somewhere, but i think the code is a lot simpler if we just keep it in one place.

The godot-cell API column and internal column is definitely needed in order to have good encapsulation of godot-cell.

If we remove the gdext API column and use the godot-cell types directly then im not entirely sure how we would implement BaseRef, additionally it'd make the godot-cell types public.

Additionally we have quite some code duplication in single/multi-threaded Storage impls -- which the new trait does not seem to be able to eliminate. Or is that pending a future refactoring? I see the point of enforcing signatures with the traits, but maybe we should also rethink if this actually simplify things (not in this PR). Otherwise we're really stacking layers of abstraction 😅

I'll see if we can remove more duplication now

[lilizoey]

Additionally we have quite some code duplication in single/multi-threaded Storage impls -- which the new trait does not seem to be able to eliminate. Or is that pending a future refactoring? I see the point of enforcing signatures with the traits, but maybe we should also rethink if this actually simplify things (not in this PR). Otherwise we're really stacking layers of abstraction 😅

I'll see if we can remove more duplication now

I think most of the duplication comes from the fact that the single-threaded and multi-threaded storage uses the same godot-cell types atm. If we later add specific single-threaded godot-cell types then there wouldn't be as much duplication.

[Bromeon]

Thanks, this table is very helpful! 👍

If you want to keep the type aliases, I would suggest to name them the same as the public godot-cell types (already the case for 2 out of 3).

In the future, threaded object pointers will likely change a bit. If we went for a separate type Gds<T> ("Gd sync"), we'd likely also have separate types than GdRef and GdMut. In that case, would there still be value in the type aliases? If not, we can also remove this layer now and only add it when truly needed, rather than prematurely.