Provide error context for typed array clone check

[mivort]

This patch adds output of ConvertError's Display to debug-only check's panic message which provides additional context when type mismatch happens.

Panic message would include the intended type name and what was given instead of it.

The output would look roughly like this:

copied array should have same type as original array:
expected array of type Builtin(DICTIONARY), got Untyped: []

[GodotRust]

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

[TitanNano]

In which scenario did you encounter this expect actually failing?

[mivort]

@TitanNano I've started getting this exception in debug builds after upgrade from gdext 0.3.5 to 0.4.0, with both Godot 4.4 and 4.5. I've noticed there's another panic which provides the similar context and message, so this patch is probably not needed:

Reason: function panicked: assertion `left == right` failed: Array/Dictionary element type validation failed: cached as Untyped but FFI reports DICTIONARY. This indicates that another extension modified the type after godot-rust cached it.
  left: DICTIONARY
  right: NIL
ERROR: [.../crates/gdext/godot-core/src/meta/element_type.rs:141]

For context: I have a node with gdext-exported Array<Dictionary>:

#[export]
pub equipment: Array<Dictionary>,

In signal handler, I do equipment.clone() to pass the shared reference and get this exception. I don't have any other extensions in that project and don't set the array type manually.

I'd like to try and reproduce it on a smaller project to check if if happens consistently, or to bisect the changes which happened between 0.3.5 and 0.4.

[Yarwin]

I'd like to try and reproduce it on a smaller project to check if if happens consistently, or to bisect the changes which happened between 0.3.5 and 0.4.

Please, do, if you will be able to replicate it then we should be able to provide a fix (or at least an explanation 😅)

[mivort]

I've bisected and found out that the issue began to appear after #1304. I'll try to reproduce it with a smaller project to understand better why type check might fail.

[mivort]

I think I've managed to reproduce the issue in the controlled environment. The problem appears to be happening right after duplicate_shallow()/duplicate_deep() gets called on the typed array - maybe a type cache mismatch happens in that case? Here's the snippet which produces a panic in debug builds:

let mut equipment = Array::<Dictionary>::new();
godot_warn!("Type before duplicate: {:?}", equipment.element_type()); // shows the correct type
equipment = equipment.duplicate_shallow(); // or duplicate_deep()
godot_warn!("Type after duplicate: {:?}", equipment.element_type()); // <- panics

[mivort]

So, it appears that Array's .as_inner().duplicate(...) method produces a VariantArray with cached_element_type OnceCell being already initialized to Untyped. So, cached type doesn't get updated after that, and then the debug check notices the mismatch in the later calls.

As a workaround, resetting OnceCell cache in Array in duplicate_(shallow/deep) methods will cause OnceCell to be re-initialized:

// godot-core/src/builtin/collections/array.rs:553
let mut duplicate: VariantArray = unsafe { self.as_inner().duplicate(true) };
duplicate.cached_element_type = OnceCell::new();

...so subsequent call to .element_type() will properly fetch the engine type. But maybe there's a way to avoid OnceCell from being initialized with Untyped in the first place?

UPD: Ah, so there's transfer_cache method which should apply the proper cached value. And it doesn't transfer the cache - because OnceCell is already set to Untyped, so set method doesn't update the value. Resetting the OnceCell helps to make it perform the transfer, but that would kinda invalidate the intended init-once cache policy.

[Yarwin]

I think the right thing to do would be changing the return type of duplicate method – which return value is initialized by us before the call – from VariantArray to Array

i.e. going from

    pub unsafe fn duplicate(&self, deep: bool,) -> VariantArray {
        type CallRet = VariantArray;
        type CallParams = (bool,);
        let args = (deep,);
        unsafe {
            let method_bind = sys::builtin_method_table() . fptr_by_index(...);
            Signature::< CallParams, CallRet > ::out_builtin_ptrcall(method_bind, "Array", "duplicate", self.sys_ptr, args)
        }
    }

to

    pub unsafe fn duplicate<T>(&self, deep: bool,) -> Array<T> {
        type CallRet = Array<T>;
        type CallParams = (bool,);
        let args = (deep,);
        unsafe {
            let method_bind = sys::builtin_method_table() . fptr_by_index(...);
            Signature::< CallParams, CallRet > ::out_builtin_ptrcall(method_bind, "Array", "duplicate", self.sys_ptr, args)
        }
    }

[mivort]

pub unsafe fn duplicate is auto-generated based on extension API JSON, isn't it? Godot's JSON doesn't provide much info on how generated array typing can be improved (typing info is available in GDScript, but it looks like the API requires to call a method to know the resulting type for sure):

{
	"name": "duplicate",
	"return_type": "Array",
	"is_vararg": false,
	"is_const": true,
	"is_static": false,
	"hash": 636440122,
	"arguments": [
		{
			"name": "deep",
			"type": "bool",
			"default_value": "false"
		}
	]
},

...unless there's a way to have a special handling for this codegen. Otherwise, it seems like either initial type cache value should be non-initialized for all VariantArray-returning calls (like from_opaque does), or duplicate_deep/shallow wrappers should reset it, IIUC.

[Yarwin]

this is fully on our side, so we would have to make special exception

I'll look into it tomorrow/on Tuesday

[Bromeon]

I think the right thing to do would be changing the return type of duplicate method – which return value is initialized by us before the call – from VariantArray to Array

GDExtension doesn't support generic signatures, so cases like these cannot be detected by the code generator. In the past, using VariantArray at the codegen level worked, because the higher-level Array impl explicitly overrode the typing (unsafe).

Your idea is a bit cleaner but may add more complex special casing 🤔 I think both are valid approaches.

In Godot itself this isn't a problem, because GDScript doesn't care about getting variance correct, so you can always convert Array[T] to Array. However, this is not possible in Rust because it would allow untyped_array.push(123) if the array in fact contained strings. See #727.

Are other methods affected by this as well? Immediately coming to mind are subarray_*. We'd also need to keep a lookout once we make Dictionary generic...

[Bromeon]

PR itself looks good, but we should additionally fix the real issue 🙂 thanks!