Add the ability to make reentrant &mut self calls by lilizoey · Pull Request #501 · godot-rust/gdext (original) (raw)

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:

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