Make ScriptInstance.call and company re-entrant

[Mercerenies]

Related to #501, though ScriptInstance is a bit of a special case.

Godot actually passes ScriptInstance around as void* for some reason, so on the Rust side we implement a trait called ScriptInstance whose methods take a &mut self. In particular, the method of most concern is ScriptInstance::call

fn call(
    &mut self,
    method: StringName,
    args: &[&Variant]
) -> Result<Variant, u32>

This method documentation makes the following note.

It’s important that the script does not cause a second call to this function while executing a method call. This would result in a panic.

Which makes any attempts to implement recursion in a Rust-side scripting language a non-starter. I think we can make this method re-entrant, and I'd like to start the discussion on how to do that.

Currently, we implement the GDExtension ScriptInterface type as ScriptInstanceData<T: ScriptInstance>, which contains a RefCell<T>. When we need to invoke a trait function like ScriptInstance::call, we take the ScriptInstanceData<T> and do

borrow_instance_mut(instance).call(method.clone(), args)

That is, we borrow the contents of the RefCell<T>, instance.inner, for the duration of the call. That makes it impossible to invoke call again on the same instance recursively from Godot until the original call exits.

Proposed Solution 1

My first thought is this. ScriptInstance is never used as a trait object, so it needn't be object-safe. So one solution is to change the signature of all of the ScriptInstance methods from

fn call(&mut self, ...) -> ...

to

fn call(instance: RefCell<Self>, ...) -> ...

Then the implementor of call can borrow mutably, decide what it needs to do, and then release the borrow if it's going to make a potentially-recursive call. This just puts the control (and responsibility) of borrowing correctly in the hands of the implementor. I'm not entirely sure how I feel about that, as instance: RefCell<Self> is a much more obtuse API than &mut self, but it definitely would get the job done.

Proposed Solution 2

We may be able to apply the GodotCell trick to ScriptInstance. I don't claim to fully understand the trickery exhibited by this new cell type, but it looks like it hinges on the fact that the self in these calls is actually owned by a Gd<Self>. In our case with ScriptInstance, the self we have is always owned by a ScriptInstanceData<Self>, so a similar trick may be viable.

[TitanNano]

I believe the script instance should either adopt GodotCell once it is ready, or an alternative I already proposed in the initial PR, is to make the entire script instance &self and delegate all mutation to the script instance implementation as interior mutability.

That said, script recursion should not require going back through the engine and should be contained in the script runtime.

EDIT: I missed that GodotCell has already been merged.

[Mercerenies]

That said, script recursion should not require going back through the engine and should be contained in the script runtime.

I'm not sure I agree with that. If I have an unknown object of type Variant, I'm going to call back into the Godot engine to invoke methods on it, not handle that myself. Sure, if it's explicit, direct recursion then I could do something clever and never leave Rust, but that doesn't handle mutual recursion, recursion that goes through GDScript, or calling something like Array.map or another higher-order Godot function with a function argument that calls back into self somehow. There are lots of legitimate reasons to recurse through Godot.

[TitanNano]

@Mercerenies All right, I was more thinking of direct recursion. But what you are describing is definitely an issue. Unfortunately, it is a general issue of gdext.

If I'm not mistaken, even with GodotCell call stack like
Class::method(&mut self) -> EngineClass::api_method() -> Class::method(&self)will cause a panic.

[Bromeon]

Unfortunately, it is a general issue of gdext.

If I'm not mistaken, even with GodotCell call stack like Class::method(&mut self) -> EngineClass::api_method() -> Class::method(&self)will cause a panic.

No, GodotCell should solve this 🙂

You can look at an integration test to get a feel. Here you have the chain first_calls -> call -> second.

gdext/itest/rust/src/object_tests/reentrant_test.rs

Lines 27 to 44 in e3644a0

	#[func]
	fn first_calls(&mut self) {
	self.first_called_pre = true;
	self.base_mut().call("second".into(), &[]);
	self.first_called_post = true;
	}
	#[func]
	fn first_signal(&mut self) {
	self.first_called_pre = true;
	self.base_mut().emit_signal("some_signal".into(), &[]);
	self.first_called_post = true;
	}
	#[func]
	fn second(&mut self) {
	self.second_called = true;
	}

[TitanNano]

@Bromeon sorry I didn't make my example super clear.

So yes, the new GodotCell implementation allows for re-entrance when making calls to the API if the base class. What I was trying to express though is any other scenario where a call to some engine API happens to call back into the same rust class again.

Something like this:

CustomScriptLanguage::method(&mut self) -> Engine::api_method() -> CustomScriptLanguage::get_type(&self)

Or a class calling a GDScript that calls back into the class due to some circular references:

impl CustomNode {
    pub fn method(&mut self) {
        self.ref_gdscript_child_node.call("some_method", &[]);
    }

    pub fn on_child_signal(&self) {...}
}

func some_method():
    self.child_signal.emit()

[TitanNano]

This discussion leads me to this idea of performing a "downgrade" of &mut self to &self before calling an engine API:

let base = self.base_mut();
let self_gd = base.to_godot().cast::<Self>();
let new_self = self_gd.bind();

[TitanNano]

@Mercerenies what are your thoughts on making the entire ScriptInstance take &self? I still believe it would give implementors the highest amount of control and flexibility.

[Mercerenies]

Can we do that? To be honest, I would've written that idea off as impossible, but I suppose if everything really is just clever interior mutability, then we could probably get away with it. If &selfing everything is remotely possible, then I'm on board with it. And (again, assuming there's no unexpected hiccups) it would certainly be the most straightforward thing to implement.

I also feel like we have too much direct access to ScriptInstance. Every other Godot type is a Gd<T>, so when we pass them around and use very non-Rust semantics to work with them, it's not surprising (since it's clearly a wrapper type). ScriptInstance is weird in that Godot gives us direct pointers to it, so I wonder if it should also be some kind of custom wrapper type (which we could then, of course, take by immutable reference).

[TitanNano]

Experimental branch where ScriptInstance is completely immutable: https://github.com/TitanNano/gdext/tree/jovan/script_instance_no_mut

I also feel like we have too much direct access to ScriptInstance.

I don't really follow. ScriptInstance is an abstraction for the GDExtension interface the engine provides / expects to expose a script instance to it. The engine is the only one making calls into an instance, there shouldn't be anyone else holding a reference to it. Where do you see it being passed around?

I would say ScriptInstance is not that much different from implementing something like IScriptExtension.

[Bromeon]

Using &self everywhere feels a bit like a cop-out: while it makes us no longer need to think about mutability, it pushes the responsibility onto the user, through interior mutability.

Furthermore, it doesn't follow our API design anywhere else -- Array, Dictionary etc. also allow &mut self access, even though they are internally reference-counted. It would also weaken type safety if ScriptInstance::set() would take &self: you can no longer pass around a &ScriptInstance to prevent accidental modification.

[Mercerenies]

I don't really follow. ScriptInstance is an abstraction for the GDExtension interface the engine provides / expects to expose a script instance to it. The engine is the only one making calls into an instance, there shouldn't be anyone else holding a reference to it. Where do you see it being passed around?

Let me put it another way. Our ScriptInstance trait isn't a Godot interface. It's something rust-gdextension made up. The Godot interface is actually GDExtensionScriptInstancePtr, which we effectively "subclass" on the Rust side as

struct ScriptInstanceData<T: ScriptInstance> {
    inner: RefCell<T>,
    gd_instance_ptr: *mut sys::GDExtensionScriptInstanceInfo,
}

And then whenever a method is called on that, we do inner.borrow().whatever() or inner.borrow_mut().whatever(). But ScriptInstance (the Rust-side trait that takes self) is our invention. So maybe those methods shouldn't be taking &self or &mut self at all and should instead be taking either ScriptInstanceData<Self> or something that resembles it. It still pushes the responsibility for interior mutability onto the user, but it makes it incredibly explicit that that's the case.

[TitanNano]

It would also weaken type safety if ScriptInstance::set() would take &self: you can no longer pass around a &ScriptInstance to prevent accidental modification.

@Bromeon I think this is a fair point. We would then still have to provide a way to make &mut ScriptInstance inaccessible (via GodotCell or otherwise) and integrators of scripting languages would have to work this into their implementation. Every call that is passed from a script to an engine API has to ensure that there is no mutable reference as there is always a change of reentrance and having the constraints of this library leaking into the script language itself is not desirable.

Do you have a suggestion how to approach this instead?

And then whenever a method is called on that, we do inner.borrow().whatever() or inner.borrow_mut().whatever(). But ScriptInstance (the Rust-side trait that takes self) is our invention. So maybe those methods shouldn't be taking &self or &mut self at all and should instead be taking either ScriptInstanceData<Self> or something that resembles it.

@Mercerenies I don't entirely agree with this. ScriptInstance is a safe version of the GDExtensionScriptInstance struct. ScriptInstanceData<Self> is a necessary wrapper to uphold all the safety requirements, and it should not be exposed to the public API in the same way none of the other GDExtension* types are directly exposed.

Exposing just a cell like &GodotCell<T: ScriptInstance> definitely would have its advantages, though.

[TitanNano]

Another option where ScriptInstance takes a GdCell<Self> instead of self. https://github.com/TitanNano/gdext/tree/jovan/script_instance_gd_cell Haven't tested it yet, though.

[lilizoey]

impl CustomNode {
    pub fn method(&mut self) {
        self.ref_gdscript_child_node.call("some_method", &[]);
    }

    pub fn on_child_signal(&self) {...}
}

func some_method():
    self.child_signal.emit()

You can make this work with GodotCell it just is a bit cumbersome and doesn't come for free:

impl CustomNode {
    pub fn method(&mut self) {
        let child_node = self.ref_gdscript_child_node.clone();
        let guard = self.base_mut();
        child_node.call("some_method", &[]);
        std::mem::drop(guard);
    }

    pub fn on_child_signal(&self) {...}
}

[Mercerenies]

@lilizoey Yeah, that's the sort of care that I would like to be possible for people like me implementing a scripting language. Most godot-rust users won't need or care about re-entrancy, so they shouldn't pay the cost of noisy syntax. But it should be possible for those of us who need it. And now, thanks to your work, it is. I hope a similar solution is viable for ScriptInstance.

[TitanNano]

@Bromeon regrading your comment in #647 (comment):
I have so far updated https://github.com/TitanNano/gdext/tree/jovan/script_instance_gd_cell to actually work and extended the itests to include a re-entrance test. This does indeed currently make GdCell public, something you wrote you would like to avoid. A ScriptInstance implementation requires access to borrow(), borrow_mut() and make_inaccessible().

[Bromeon]

@TitanNano Thanks a lot for looking into it and getting a PoC running! 👍

Since you've worked with this for a while, you probably know some stuff that I don't and might clarify some things.
I'm especially worried about this change:

Which is a considerable downgrade in user-friendlyness, in multiple ways:

• this: Pin<&GdCell<Self>> instead of &self
• this.borrow().unwrap().field instead of self.field
• change to associated function instead of method with receiver

So, a few questions:

1. Is the double-borrow really a problem everywhere? In the initial post of this issue, mostly call() was mentioned for recursion.
2. Why is Pin needed?
3. Can we not consider a pattern like base() + base_mut(), where calls could be explicitly made re-entrant where needed?

Basically, we should not expose GdCell at all (too low-level), and we should keep the default usage simple. If someone implements ScriptInstance and doesn't need recursion or other re-entrancy, their life is now suddenly 10 times harder -- how do we justify this? I'd rather provide some power tools where truly needed.

[TitanNano]

1. Is the double-borrow really a problem everywhere? In the initial post of this issue, mostly call() was mentioned for recursion.

Most functions do not execute arbitrary logic defined by script authors. So fn call(...) would be the most definite case for having re-entrant logic / causing it. get_property / set_property in theory could end up in the same situation if script languages support property setter and getter methods that allow arbitrary calls to other function.

2. Why is Pin needed?

Pin is required by GdCell so if we can get it out of the public interface it won't be needed anymore.

3. Can we not consider a pattern like base() + base_mut(), where calls could be explicitly made re-entrant where needed?

base() + base_mut() heavily relies upon the Godot inheritance system to gain access to the underlying GdCell to make the current mutable reference inaccessible. ScriptInstance on the other hand, uses a struct that is then wrapped by the engine and called into when necessary. Kind of the other way around than the usual inheritance system.

We perhaps could introduce a custom wrapper for &mut self that is only passed to fn call(...) (and possibly set_property and even get_property) and implements Deref<Self>. This wrapper could hold a reference to both &mut self and Pin<&GdCell<Self>> and expose methods to make the reference inaccessible. This could be implemented as fn owner() / mut_owner() instead of base. The script owner / host object is currently something that has to be handled by the ScriptInstance implementor and should be stored as weak references to avoid creating cyclic references (something I only noticed recently).

Alternatively, we would have to somehow become part of the instantiation of the ScriptInstance type to store some value inside of it which can later be accessed. This sounds much more complex to me.

(I haven't tested any of this, just some thoughts)

[lilizoey]

Is this really much simpler of a solution than just making every method take &self and forcing people to use interior mutability where relevant? both Pin and GdCell are rather complicated and require a lot of in depth understanding of rust to use properly, but with &self the user can decide how intricate they want to be with the interior mutability if they even need it at all.

[Bromeon]

We perhaps could introduce a custom wrapper for &mut self that is only passed to fn call(...) (and possibly set_property and even get_property) and implements Deref<Self>. This wrapper could hold a reference to both &mut self and Pin<&GdCell<Self>> and expose methods to make the reference inaccessible. This could be implemented as fn owner() / mut_owner() instead of base. The script owner / host object is currently something that has to be handled by the ScriptInstance implementor and should be stored as weak references to avoid creating cyclic references (something I only noticed recently).

That sounds interesting, and nicer than a Pin<&GdCell<Self>> in user signatures. 👍

Could something like self.owner_mut().some_godot_function() be used to do re-entrant calls again?

---

Alternatively, we would have to somehow become part of the instantiation of the ScriptInstance type to store some value inside of it which can later be accessed. This sounds much more complex to me.

Not sure what exactly you mean here -- but if you think it's more complex, we can gladly explore the other avenue first 🙂

---

Is this really much simpler of a solution than just making every method take &self and forcing people to use interior mutability where relevant?

Also fair point -- it's a bit hard for me to judge how often one could live with the "basic &self/&mut self" and when one would need to resort to interior mutability. I had the impression that many typical ScriptInstance implementations require re-entrant calls, or are these rather special cases?

---

Thanks a lot for the all the input, it's appreciated!

[TitanNano]

@lilizoey i agree but it was dismissed in #554 (comment)

[TitanNano]

Could something like self.owner_mut().some_godot_function() be used to do re-entrant calls again?

In short, yes. The owner of the script instance is just a Gd<T> that is unrelated to the script instance reference.

I will see if I can get a rough draft of this concept implemented.

[Bromeon]

At the time I didn't know that the alternative would be so much more complex, to be honest 😁

So I'm no longer generally against &self + interior mutability, but I'd really want to make sure we have considered multiple possible designs first. And we have to approach this from a user-friendliness perspective, not just implementation complexity. An important point here is that such complexity also affects users that do not require re-entrant calls. They don't benefit from it.

To summarize different suggestions:

Everywhere &self:

• pushes mutability handling onto the user (more complex code)
• different API from Array, Dictionary, Gd etc.
• loss of const-correctness: passing &MyScriptInstance argument doesn't guarantee it's not changed by a function

Custom type Pin<&GdCell<Self>>:

• very hard to understand for someone who just wants to get stuff done
• requires publishing of GdCell, which is currently an implementation detail
• code to access self becomes much more verbose

Something with owner() + owner_mut()

• ??

Thanks for looking into it @TitanNano. Feel free to keep the PoC small, no need to map the entire ScriptInstance API initially...

[TitanNano]

@Bromeon here is the latest POC based on what we discussed: https://github.com/TitanNano/gdext/compare/jovan/script_instance_gd_cell...TitanNano:gdext:jovan/script_instance_mut_wrapper?expand=1

One draw back of this solution is that owner APIs are only available to ScriptInstance functions which receive the wrapped mutable reference to self.

[lilizoey]

I think it'd be possible to more closely mirror GodotClass here with its base. Maybe do something like

pub trait ScriptInstance {
  fn init(owner: Owner<Object>) -> Self;
  fn owner_field(&self) -> &Owner<Object>;
  fn owner(&self) -> OwnerRef<Object> {
    let gd = self.owner_field().to_gd();

    OwnerRef::new(gd, self)
  }
  fn owner_mut(&mut self) -> OwnerMut<Object> {
    let owner_gd = self.owner_field().to_gd();
    let storage = self.owner_field().get_storage_somehow();
    
    let guard = storage.get_inaccessible(self);
    OwnerMut::new(owner_gd, guard)
  }
  // The rest of the methods
}

Could maybe even split it into two traits, and have a ScriptInstanceWithOwnerField trait, though i feel like most implementations would want access to the owner field. Worst case you can always just implement owner_field as unimplemented!() if you know for sure you'll never use owner()/owner_mut().

[Bromeon]

The API for ScriptInstance starts to become rather heavy... 🤔

Now, to implement a custom script extension, the user needs to know:

• ScriptInstance trait
• create_script_instance function
• ScriptExtension class
• IScriptExtension trait
  • its unsafe method instance_create
• Owner<T> type
• OwnerRef + OwnerMut types
  • even if they don't need to be named in typical cases, there's still the owner() + owner_mut() methods
• Conversions between that all that stuff and Gd<T> or &T to interact with Godot APIs
• ...

Which looks like a very steep learning curve. Sure, there is quite a bit of inherent complexity in the way how script extensions work, but maybe we should also check if/how other bindings implement those and if we could simplify something without abandoning safety (probably not, but worth checking).

Additionally, if we build a parallel Owner<T> mechanism, we should check if Gd<T> cannot be reused (in particular the guard types) or at least make sure the duplication is not too big, or localized. In contrast to this API, Gd has an extreme versatility and can be used all over the place. Owner<T> would be a highly specialized smart pointer that has no use outside ScriptInstance.

I'm also a bit worried since there are likely already additional smart pointer types necessary for the multi-threaded case, and a proliferation of those will leave us with gdnative-level complexity that is near-impossible to understand for casual game developers. Again, sometimes complexity is inevitable, and maybe Owner<T> is indeed the simplest. But let's keep the user perspective in mind here 🙂 if things work very similar to Gd & Co., at least previously learned concepts can be adopted.

[lilizoey]

as-is, we cannot directly reuse the existing logic from Gd for doing the same thing with ScriptInstance. Since the current implementation would require the script instance to derive GodotClass, as instance storage requires that. But im gonna check if it's possible to generalize the instance storage a little bit to where the instance doesn't need to implement GodotClass. Then maybe we can just share the existing code between both?

[Bromeon]

I think that could work, but only if there's enough common code. If we end up needing traits with 2 separate impls, then there's not much code sharing.

Also this would then be more an implementation detail, not the user facing API. But if the latter has conceptual + naming parallels between Owner<T> and Gd<T>, that would also help learning.

[TitanNano]

I think it'd be possible to more closely mirror GodotClass here with its base. Maybe do something like

pub trait ScriptInstance {
  fn init(owner: Owner<Object>) -> Self;
  fn owner_field(&self) -> &Owner<Object>;
  fn owner(&self) -> OwnerRef<Object> {
    let gd = self.owner_field().to_gd();

    OwnerRef::new(gd, self)
  }
  fn owner_mut(&mut self) -> OwnerMut<Object> {
    let owner_gd = self.owner_field().to_gd();
    let storage = self.owner_field().get_storage_somehow();
    
    let guard = storage.get_inaccessible(self);
    OwnerMut::new(owner_gd, guard)
  }
  // The rest of the methods
}

Could maybe even split it into two traits, and have a ScriptInstanceWithOwnerField trait, though i feel like most implementations would want access to the owner field. Worst case you can always just implement owner_field as unimplemented!() if you know for sure you'll never use owner()/owner_mut().

A Problem I see with this solution is that you now have taken over the type instantiation of what ever implements the trait which will be quite inconvenient for who ever wants to use ScriptInstance. GodotClass solves this with proc-macros and multiple APIs to create a new instance of a type, but any of this seems quite complex for ScriptInstance.

You also cannot get the script instance storage via the script owner object unless we start maintaining a mapping between godot objects and their script instances. The engine does not expose any way to access script instances.

[TitanNano]

Additionally, if we build a parallel Owner mechanism, we should check if Gd cannot be reused (in particular the guard types) or at least make sure the duplication is not too big, or localized. In contrast to this API, Gd has an extreme versatility and can be used all over the place. Owner would be a highly specialized smart pointer that has no use outside ScriptInstance.

I don't see how Owner<T> could be replaced with Gd<T> as it basically replicates Base<T>. Looking at Base<T> again it seems that the only real difference is that it relies on GodotClass to access T::Base. If we can factor the associated base type into a common trait we can reuse the Base types here.

[TitanNano]

https://github.com/TitanNano/gdext/compare/jovan/script_instance_gd_cell...TitanNano:gdext:jovan/script_instance_mut_wrapper?expand=1

I updated this with a commit that implements a POC for using Base<T>, BaseRef<T>, BaseMut<T> instead of some custom owner types.

[lilizoey]

That should probably work yeah, it's a lot less effort than trying to mirror GodotClass exactly which is a benefit. Using a Base to represent the owner does sort of make semantic sense actually, because the owner of a script instance is effectively equivalent to the base object of a rust object. In fact, godot even refers to it as a base_object at least in one place: https://docs.godotengine.org/en/stable/classes/class_script.html#class-script-method-instance-has. We could consider even just naming the methods base/base_mut?

Needing to add an extra trait in WithBase is a bit unfortunate but you do need to get a hold of the base type somehow so if we wanna reuse the existing code then that does need to go somewhere, and an extra trait makes sense to me at least.

One thing to consider (which we could do another time too) that i just thought of is: In IScriptExtension::instance_create the user must always return a pointer created with create_script_instance. This means that IScriptExtension must actually be an unsafe trait. Currently instance_create is an unsafe function, which it doesn't actually need to be afaik since you shouldn't be able to cause UB merely by calling the method. However one way to fix that would be to overwrite instance_create with our own custom method that has this signature instead:

fn instance_create(&self) -> impl ScriptInstance;

Since then we can just call create_script_instance when we call this method.

I think this is a bigger issue actually with how we generate bindings currently. Since we just declare any method that takes or returns a pointer unsafe. But usually methods that return a pointer and doesn't take a pointer doesn't need to be unsafe, but rather the trait itself must be unsafe. I'll look into that a bit more and open a separate issue for that.

[TitanNano]

If there are no further comments on the topic, I will now start cleaning up the POC and create an PR.

---

Are there any opinions how the &mut ScriptInstance ref guard should be named?