ScriptInstance::call Panics When Using Callable

[Pspritechologist]

Obtaining and calling a Callable from SiMut::base_mut causes a panic if done during the call callback on a ScriptInstance.
Below are the files used to replicate this issue.

The actual code to replicate is extremely small, but the files are large due to the amount of functions in the Traits. I've pushed any actually relevant functions to the top of their impl blocks.

Needlessly large code files

// lib.rs
use godot::{classes::Engine, prelude::*};

mod script;
mod language;
mod instance;

pub struct Extension;

#[gdextension]
unsafe impl ExtensionLibrary for Extension {
	fn on_level_init(level: InitLevel) {
		if level == InitLevel::Scene {
			Engine::singleton().register_script_language(&language::ReplLanguage::new_alloc());
		}
	}
}

// language.rs
use crate::script::ReplScript;
use godot::{classes::{IScriptExtension, IScriptLanguageExtension}, prelude::*};

#[derive(GodotClass)]
#[class(tool, init, base = ScriptLanguageExtension)]
pub struct ReplLanguage;

#[godot_api]
impl IScriptLanguageExtension for ReplLanguage {
	fn create_script(&self) -> Option<Gd<godot::classes::Object>> {
		Some(Gd::from_init_fn(ReplScript::init).upcast())
	}

	fn get_name(&self) -> GString { "Repl".into() }
	fn get_type(&self) -> GString { "Repl".into() }
	fn get_extension(&self) -> GString { "repl".into() }
	fn get_reserved_words(&self) -> PackedStringArray { Default::default() }
	fn is_control_flow_keyword(&self, _keyword: GString) -> bool { false }
	fn get_comment_delimiters(&self) -> PackedStringArray { Default::default() }
	fn get_doc_comment_delimiters(&self) -> PackedStringArray { Default::default() }
	fn get_string_delimiters(&self) -> PackedStringArray { Default::default() }
	fn make_template(&self, _template: GString, _class_name: GString, _base_class_name: GString) -> Option<Gd<godot::classes::Script>> { None }
	fn get_built_in_templates(&self, _object: StringName) -> Array<Dictionary> { Default::default() }
	fn validate(&self, _script: GString, _path: GString, _validate_functions: bool, _validate_errors: bool, _validate_warnings: bool, _validate_safe_lines: bool) -> Dictionary { Default::default() }
	fn validate_path(&self, _path: GString) -> GString { Default::default() }
	fn is_using_templates(&mut self) -> bool { false }
	fn has_named_classes(&self) -> bool { false }
	fn supports_builtin_mode(&self) -> bool { false }
	fn supports_documentation(&self) -> bool { false }
	fn can_inherit_from_file(&self) -> bool { false }
	fn can_make_function(&self) -> bool { false }
	fn overrides_external_editor(&mut self) -> bool { false }
	fn find_function(&self, _function: GString, _code: GString) -> i32 { 1 }
	fn make_function(&self, _class_name: GString, _function_name: GString, _function_args: PackedStringArray) -> GString { Default::default() }
	fn open_in_external_editor(&mut self, _script: Option<Gd<godot::classes::Script>>, _line: i32, _column: i32) -> godot::global::Error { godot::global::Error::FAILED }
	fn preferred_file_name_casing(&self) -> godot::classes::script_language::ScriptNameCasing { godot::classes::script_language::ScriptNameCasing::SNAKE_CASE }
	fn complete_code(&self, _code: GString, _path: GString, _owner: Option<Gd<godot::classes::Object>>) -> Dictionary { Default::default() }
	fn lookup_code(&self, _code: GString, _symbol: GString, _path: GString, _owner: Option<Gd<godot::classes::Object>>) -> Dictionary { Default::default() }
	fn auto_indent_code(&self, code: GString, _from_line: i32, _to_line: i32) -> GString { code }
	fn add_global_constant(&mut self, _name: StringName, _value: Variant) { }
	fn add_named_global_constant(&mut self, _name: StringName, _value: Variant) { }
	fn remove_named_global_constant(&mut self, _name: StringName) { }
	fn reload_all_scripts(&mut self) { }
	fn reload_tool_script(&mut self, _script: Option<Gd<godot::classes::Script>>, _soft_reload: bool) { }
	fn get_recognized_extensions(&self) -> PackedStringArray { [self.get_extension()].into() }
	fn get_public_functions(&self) -> Array<Dictionary> { Default::default() }
	fn get_public_constants(&self) -> Dictionary { Default::default() }
	fn get_public_annotations(&self) -> Array<Dictionary> { Default::default() }
	fn handles_global_class_type(&self, type_: GString) -> bool { self.get_type() == type_ }
	fn get_global_class_name(&self, _path: GString) -> Dictionary { Default::default() }
	fn init_ext(&mut self) { }
	fn finish(&mut self) { }
	fn thread_enter(&mut self) { }
	fn thread_exit(&mut self) { }
	fn frame(&mut self) { }
	fn debug_get_error(&self) -> GString { Default::default() }
	fn debug_get_stack_level_count(&self) -> i32 { 0 }
	fn debug_get_stack_level_line(&self, _level: i32) -> i32 { 1 }
	fn debug_get_stack_level_function(&self, _level: i32) -> GString { Default::default() }
	fn debug_get_stack_level_source(&self, _level: i32) -> GString { Default::default() }
	fn debug_get_stack_level_locals(&mut self, _level: i32, _max_subitems: i32, _max_depth: i32) -> Dictionary { Default::default() }
	fn debug_get_stack_level_members(&mut self, _level: i32, _max_subitems: i32, _max_depth: i32) -> Dictionary { Default::default() }
	unsafe fn debug_get_stack_level_instance(&mut self, _level: i32) -> * mut std::ffi::c_void { std::ptr::null_mut() }
	fn debug_get_globals(&mut self, _max_subitems: i32, _max_depth: i32) -> Dictionary { Default::default() }
	fn debug_parse_stack_level_expression(&mut self, _level: i32, _expression: GString, _max_subitems: i32, _max_depth: i32) -> GString { Default::default() }
	fn debug_get_current_stack_info(&mut self) -> Array<Dictionary> { Default::default() }
	fn profiling_start(&mut self) { }
	fn profiling_stop(&mut self) { }
	fn profiling_set_save_native_calls(&mut self, _enable: bool) { }
	unsafe fn profiling_get_accumulated_data(&mut self, _info_array: * mut godot::classes::native::ScriptLanguageExtensionProfilingInfo, _info_max: i32) -> i32 { 0 }
	unsafe fn profiling_get_frame_data(&mut self, _info_array: * mut godot::classes::native::ScriptLanguageExtensionProfilingInfo, _info_max: i32) -> i32 { 0 }
}

// script.rs
use crate::{instance::ReplInstance, language::ReplLanguage};
use godot::{classes::{IScriptExtension, ScriptExtension}, obj::script::create_script_instance, prelude::*};

#[derive(GodotClass)]
#[class(tool, init, base = ScriptExtension)]
pub struct ReplScript {
	base: Base<ScriptExtension>,
}

#[godot_api]
impl IScriptExtension for ReplScript  {
	unsafe fn instance_create(&self, for_object: Gd<godot::classes::Object>) -> *mut std::ffi::c_void {
		create_script_instance(ReplInstance { script_ref: self.to_gd().upcast() }, for_object)
	}
	
	fn get_language(&self) -> Option<Gd<godot::classes::ScriptLanguage>> {
		Some(Gd::from_object(ReplLanguage).upcast())
	}
	
	unsafe fn placeholder_instance_create(&self, _for_object: Gd<godot::classes::Object>) -> *mut std::ffi::c_void { std::ptr::null_mut() }
	fn editor_can_reload_from_file(&mut self) -> bool { false }
	unsafe fn placeholder_erased(&mut self, _placeholder: *mut std::ffi::c_void) { }
	fn instance_has(&self, _object: Gd<godot::classes::Object>) -> bool { false }
	fn get_instance_base_type(&self) -> StringName { "Object".into() }
	fn has_source_code(&self) -> bool { false }
	fn get_source_code(&self) -> GString { Default::default() }
	fn set_source_code(&mut self, _code: GString) { Default::default() }
	fn reload(&mut self, _keep_state: bool) -> godot::global::Error { godot::global::Error::OK }
	fn can_instantiate(&self) -> bool { true }
	fn get_base_script(&self) -> Option<Gd<godot::classes::Script>> { None }
	fn get_global_name(&self) -> StringName { Default::default() }
	fn inherits_script(&self, _script: Gd<godot::classes::Script>) -> bool { false }
	fn get_documentation(&self) -> Array<Dictionary> { Default::default() }
	fn get_class_icon_path(&self) -> GString { Default::default() }
	fn get_script_method_argument_count(&self, _method: StringName) -> Variant { Variant::nil() }
	fn get_method_info(&self, _method: StringName) -> Dictionary { Default::default() }
	fn is_valid(&self) -> bool { true }
	fn is_tool(&self) -> bool { false }
	fn is_abstract(&self) -> bool { false }
	fn has_method(&self, _method: StringName) -> bool { false }
	fn has_static_method(&self, _method: StringName) -> bool { false }
	fn has_script_signal(&self, _signal: StringName) -> bool { false }
	fn get_script_signal_list(&self) -> Array<Dictionary> { Default::default() }
	fn has_property_default_value(&self, _property: StringName) -> bool { false }
	fn get_property_default_value(&self, _property: StringName) -> Variant { Variant::nil() }
	fn update_exports(&mut self) { }
	fn get_script_method_list(&self) -> Array<Dictionary> { Default::default() }
	fn get_script_property_list(&self) -> Array<Dictionary> { Default::default() }
	fn get_member_line(&self, _member: StringName) -> i32 { 1 }
	fn get_constants(&self) -> Dictionary { Default::default() }
	fn get_members(&self) -> Array<StringName> { Default::default() }
	fn is_placeholder_fallback_enabled(&self) -> bool { false }
	fn get_rpc_config(&self) -> Variant { Variant::nil() }
	fn setup_local_to_scene(&mut self) { }
}

// instance.rs
use crate::{language::ReplLanguage, script::ReplScript};
use godot::{classes::Script, global::MethodFlags, meta::{MethodInfo, PropertyInfo}, obj::script::{ScriptInstance, SiMut}, prelude::*};

pub struct ReplInstance {
	pub script_ref: Gd<Script>,
}

impl ScriptInstance for ReplInstance {
	type Base = Object;

	fn call(mut this: SiMut<Self>, method: StringName, args: &[&Variant]) -> Result<Variant, godot::sys::GDExtensionCallErrorType> {
		if !method.to_string().eq("do_name") {
			return Err(godot::sys::GDEXTENSION_CALL_ERROR_INVALID_METHOD);
		}

		let (name, safe): (_, bool) = (args[0].clone(), args[1].to());

		if safe {
			this.base_mut().call("set_name", &[name]);
		} else {
			let callable: Callable = this.base_mut().get("set_name").to();
			callable.call(&[name]);
		}

		Ok(Variant::nil())
	}

	fn get_method_list(&self) -> Vec<godot::meta::MethodInfo> {
		vec![
			MethodInfo {
				id: 0,
				method_name: "do_name".into(),
				class_name: ReplScript::class_name(),
				return_type: PropertyInfo::new_var::<Variant>("return"),
				arguments: Default::default(),
				default_arguments: Default::default(),
				flags: MethodFlags::NORMAL,
			}
		]
	}

	fn has_method(&self, method: StringName) -> bool { method.to_string() == "do_name" }

	fn get_script(&self) -> &Gd<godot::classes::Script> { &self.script_ref }
	fn on_refcount_decremented(&self) -> bool { false }
	fn on_refcount_incremented(&self) { }
	fn set_property(_this: godot::obj::script::SiMut<Self>, _name: StringName, _value: &Variant) -> bool { false }
	fn get_property(&self, _name: StringName) -> Option<Variant> { None }
	fn get_property_list(&self) -> Vec<godot::meta::PropertyInfo> { Default::default() }
	fn get_property_type(&self, _name: StringName) -> VariantType { VariantType::NIL }
	fn get_property_state(&self) -> Vec<(StringName, Variant)> { Default::default() }
	fn property_get_fallback(&self, _name: StringName) -> Option<Variant> { None }
	fn property_set_fallback(_this: godot::obj::script::SiMut<Self>, _name: StringName, _value: &Variant) -> bool { false }
	fn get_method_argument_count(&self, _method: StringName) -> Option<u32> { None }
	fn to_string(&self) -> GString { Default::default() }
	fn class_name(&self) -> GString { Default::default() }
	fn get_language(&self) -> Gd<godot::classes::ScriptLanguage> { Gd::from_object(ReplLanguage).upcast() }
	fn is_placeholder(&self) -> bool { false }
}

# main.gd (Attached to the root Node of a main.tscn)
extends Node

func _ready() -> void:
	var script = ReplScript.new()
	self.set_script(script)
	
	self.call("do_name", "Owo", true)
	
	self.call("do_name", "Awa", false)

Yields the following panic:

ERROR: Rust function panicked: [panic]
  ScriptInstance borrow failed, already bound; T = repl::instance::ReplInstance.
    Make sure to use `SiMut::base_mut()` when possible.
    Details: cannot borrow while accessible mutable borrow exists.
  at /path/checkouts/gdext-76630c89719e160c/6da1bbf/godot-core/src/obj/script.rs:211
  Context: error when calling repl::instance::ReplInstance::call
   at: godot_core::private::handle_panic_with_print (/path/checkouts/gdext-76630c89719e160c/6da1bbf/godot-core/src/private.rs:405)

Of note is that both of the above calls actually have the desired effect (presumably once the call fails due to panic Godot calls the base method as intended), the second one simply also yields a panic.
I assume this would not be the case for calls to class methods rather than base methods.

[Pspritechologist]

@TitanNano I was told to mention you on this as you worked on most of this API.

[TitanNano]

@Pspritechologist thanks, I will try to take a look.

[TitanNano]

@Pspritechologist after a brief look I think this part of the code is the issue:

let callable: Callable = this.base_mut().get("set_name").to();
callable.call(&[name]);

The ScriptBaseMut<'_, T> that is returned by base_mut() will be dropped at the end of the line because it's never bound to a variable. So this, which is essentially &mut self, is now available again (as soon as you drop the ScriptBaseMut). When you call the callable, it tries to rebind &mut self of the script instance and it fails. You have to keep ScriptBaseMut around as long as you want to re-enter the script.

Like this:

{
    let base = this.base_mut();
    let callable: Callable = base.get("set_name").to();
    callable.call(&[name]);
}

Let me know if this works for you.

[Pspritechologist]

@TitanNano Wow, I had no idea I'd need to keep it bound while performing unrelated operations, thank you a ton, the panics have gone away :)

Feel free to close this seeing as it doesn't appear to be a bug.

[Bromeon]

Do you think this is something we can/should mention in the docs? If so, what would be a good way?

[Pspritechologist]

I think so yeah, it was unintuitive to me that keeping the SiMut bound would have side effects. Likely just a note in the type docs that it's not just important to use base_mut whenever possible (clarifying not only for mut operations but operations with potentially mut side effects), but also to keep the mut binding alive for as long as operations may be performed on the instance. This could include Callables, probably triggering signals, other such stuff as well.

[Pspritechologist]

I'd probably have a lot of general notes on this API, I wonder if I'm the first person to use it to implement a full scripting runtime 🤔

[Bromeon]

Would you like to add a PR improving the docs? 🙂

[TitanNano]

@Bromeon @Pspritechologist I'm also considering if we should replace fn base_mut() with fn with_base_mut<R>(closure: FnOnce(ScriptBaseMut<'_, T>) -> R) -> R. This might incentivize users to do perform their operations inside the closure and also prevent the base from being dropped unintentionally. Thoughts?

[Bromeon]

This is what gdnative did with map() + map_mut().

I didn't like it because:

• it makes any control flow impossible (return, continue, break)
• it complicates error handling significantly (returning Result, ? operator)
• it requires extra indentation (not a big deal, but worth mentioning)

It's also not given that it would prevent the above issue; someone could still do

let callable: Callable = this.with_base_mut(|b| b.get("set_name").to());
callable.call(&[name]);

What we should probably do is

1. document caveats of the guards -- they are so central to the library, also with Gd::base() + Gd::base_mut(), that users must understand them properly
2. make error messages as clear as possible.

Regarding 2), the error is currently

ERROR: Rust function panicked: [panic]
  ScriptInstance borrow failed, already bound; T = repl::instance::ReplInstance.
    Make sure to use `SiMut::base_mut()` when possible.
    Details: cannot borrow while accessible mutable borrow exists. 

Here, we could additionally mention

Use SiMut::base_mut() when possible, and double-check that the guard is still alive during a re-entrant call.

Furthermore, one idea I once had for Gd's own guards is to obtain a backtrace whenever guards are retrieved (e.g. bind()), and on double-borrow error, print both locations. This is expensive, but can be limited to Debug mode. Not sure if that would help here, too 🤔

[Pspritechologist]

I personally like the closure option, I agree it's a little messy but it serves to clarify the usage. Instead of appearing like a standard Rust borrow it looks more like 'entering a context', which seems to be accurate. Mlua does a similar thing with scopes which is actually what I'm using to expose the Struct in the first place.

[Bromeon]

Not that we're already using guards for these APIs:

• Gd::base
• Gd::base_mut
• Gd::bind
• Gd::bind_mut
• DynGd::dyn_bind
• DynGd::dyn_bind_mut

Rust locks/mutexes follow the same. This pattern has generally worked very well.

Is this usage here really different enough to choose another approach? Genuine question, maybe it is.
But it seems to me that there's some general confusion around

• base_mut being re-entrant, but base not
• the name base[_mut] not indicating re-entrancy
• unclear guard semantics

and it's not obvious to which extent the guard/closure contributes to the confusion here.

It's also noteworthy that for people who need the explicitness, it's easy to build an (extension) function taking a closure when there is a guard API, but it's impossible to build a guard around a closure API.

[lilizoey]

i want to note that the same behavior also happens for self.base_mut(), so if we're documenting this here it might be useful for base_mut as well.

also another note is that keeping guards around in rust basically always has side effects. it's not unique to this, for instance a Mutex will be locked for as long as the guard is around. that is in some sense the entire reason for guards to exist, the side effect allows for some operations to happen safely (for instance when a mutex is locked you can safely give out a single &mut reference to it).

[Pspritechologist]

also another note is that keeping guards around in rust basically always has side effects...

I meant more that the side effects of a Mutex are contained within that Mutex and lifetimes related to the guards generated. You can't lock a Mutex and have that affect something with no binding to said Mutex (via the Mutex itself or a lifetime). I totally understand why it works this way (even though I don't know the low level of how it works) but it's not inherently intuitive that the guard needs to be kept past when it's valid to drop in order to interact with data that has no formal connection to the guard itself.
Like Bromeon said, it's just that the Callable gets around the compile time guarantee of the lock.

[Bromeon]

But as I said, a closure API isn't bulletproof at preventing this, either.
Users can still do this, and for someone unfamiliar there's nothing obviously wrong.

let callable: Callable = this.with_base_mut(|b| b.get("set_name").to());
callable.call(&[name]);

Imo what we should really do is a combination of two things:

• Documentation, to mention potential pitfalls.
• Better diagnostics, so that people who run into borrow errors have context (e.g. memorizing first borrow and showing stacktrace).