`Array<T>` -> `VariantArray` conversion panics (aka "OutArray problem")

[Pspritechologist]

Passing a typed Array from Godot and attempting to convert it to a VariantArray currently panics (in a very unclear manner).
This is unintuitive and unhelpful behavior, and makes dealing with Variants that could be typed Arrays extremely annoying, as you'd need to check against every possible contained type.
Attached is a minimum viable reproduction that shows off the issue well.

array_test.zip

I apologize if there's an intended way around this already (or if it's simply not intended), I wasn't able to find any workaround that didn't involve matching to each contained type.

[lilizoey]

there's a constructor we can use to turn arrays into a differently typed array array_from_base_type_class_name_script, i tried that here https://github.com/lilizoey/gdextension/tree/feature/array-coerce, i havent tested this yet but the basic idea should work

This adds a constructor of coerce_from(&Variant) -> Result<Self, ConvertError> which only errors when the array is the wrong type, not when the array is the wrong type

[Dheatly23]

Can i also ask for making VariantArray be a supertype of all typed arrays? It will solve it and make the behavior more coherent with Godot's. One problem i can see is set/insert operation becomes fallible, as the element type might be not compatible.

Also there might be problem with Drop impl as the demo abort instead of panic.

[Bromeon]

Attached is a minimum viable reproduction that shows off the issue well.

Please avoid external projects unless absolutely necessary. This requires extra hurdles for anyone who wants to participate in the discussion. Instead, write a short code snippet demonstrating the relevant problem (and only that). Thanks! 🙂

---

Can i also ask for making VariantArray be a supertype of all typed arrays?

What do you mean concretely with supertype in Rust? That they can be convertible?

---

It will solve it and make the behavior more coherent with Godot's. One problem i can see is set/insert operation becomes fallible, as the element type might be not compatible.

Godot's relation between Array and Array[T] is known to have several issues, see godotengine/godot-proposals#7364. One problem in particular is incorrect contravariance (Array is a supertype of Array[T] for reading, not for writing). We'd also need to make sure equality checks are not confusing.

There were some great ideas here to address some of these issues, but it brings complexity that may not be justified, and it's not yet clear in which direction Godot is heading -- also with regards to typed dictionaries.

[Dheatly23]

What do you mean concretely with supertype in Rust? That they can be convertible?

Yes, as in typed array can be assigned into VariantArray type (but not vice versa).

Godot's relation between Array and Array[T] is known to have several issues, see godotengine/godot-proposals#7364. One problem in particular is incorrect contravariance (Array is a supertype of Array[T] for reading, not for writing). We'd also need to make sure equality checks are not confusing.

There were some great ideas here to address some of these issues, but it brings complexity that may not be justified, and it's not yet clear in which direction Godot is heading -- also with regards to typed dictionaries.

Thanks for the link! But at least for ergonomic and coherence, it's best to copy Godot's behavior as it is currently.

Like OP said, it's impossible to handle typed arrays without cast for every element type. That's kinda the reason why i'm using typed array only sparingly and read-only. Making VariantArray be the supertype would ease by far most common operation, reading, at a cost of fallible writes.

[Pspritechologist]

Please avoid external projects unless absolutely necessary. This requires extra hurdles for anyone who wants to participate in the discussion. Instead, write a short code snippet demonstrating the relevant problem (and only that). Thanks! 🙂

The following is an example of the problem. The function reveal_error accepts any Variant, only acting if said variant is of type Array. This function will panic if passed a typed Array in spite of there being no case where the contents of a typed Array are insufficient to fill in for an untyped one.

#[func]
fn reveal_error(array: Variant) {
    if array.get_type() == VariantType::ARRAY {
        let converted = array.to::<Array<Variant>>(); // Or <VariantArray>
        godot_print!("Array contents: {}", converted);
    }
}

Here is an example of the function being used in GDScript. The first call will pass fine, while the second one will panic. Working around this with an API in a convenient manner is extremely difficult.

var untyped_array: Array = ["Hello", "World"]

var typed_array: Array[String] = ["Hello", "World"]

MyStruct.reveal_error(untyped_array)

MyStruct.reveal_error(typed_array)

[Dheatly23]

After further testing, it seem the problem is even more deeper than typed -> untyped conversion.

For example this code:

#[func]
fn dict_error(v: Variant) {
    println!("{}", v.to::<Dictionary>());
}

Cause panic-in-panic if fed typed array. From what i see, the problem goes like this:

1. Try to convert into untyped array.
2. Failed, so panic.
3. Print erroring value using Debug.
4. Variant Debug implementation tries to dispatch.
5. Back to step 1.

[Bromeon]

If we allow Array<T> to be converted into VariantArray, this means all the "input" methods of VariantArray can now fail:

• set
• push
• push_front
• insert
• fill
• resize
• extend_array
• Extend::extend

We'd also need to see how this affects #[var] and #[export], in case GDScript/the editor try to set individual elements of the array.

Note

It's important that I mentioned "input", not "write". More precisely, operations where T is used in contravariant position.

Mutating methods such as clear, pop, pop_back, remove, erase, shrink, reverse, shuffle, sort_unstable and sort_unstable_custom can be safely invoked on a VariantArray that is a Array<T> in disguise.

This reduces type safety quite a bit, but maybe it's an interim solution until we find a design that models this better. For example, providing separate VariantArray and VariantReadArray/VariantOutArray types, where the latter doesn't provide the above methods.

[lilizoey]

the solution i suggested above uses an array-constructor to construct a new untyped array from the old one. it doesn't merely treat the old array as a VariantArray.

[Bromeon]

Thanks for clarifying, makes sense.

There are probably use cases for both 🤔 GDScript also allows handling Array[T] typed arrays polymorphically as Array. It's in fact the only way to implement functions supporting different element types (e.g. custom print), because GDScript doesn't have generic functions.

Since we can implement generic functions on Array<T> in Rust, this is maybe less pressing (although still valid)?

[lilizoey]

i think having a workaround to doing a match over every single array type when wanting to convert a Variant into an array would be useful. which is why i suggested we add an array constructor that takes a Variant, and (if it is an array) converts it into a new VariantArray with the same contents.

[Dheatly23]

I did some impl here: https://github.com/Dheatly23/gdext/tree/variant-array-supertype

Making fallible writes also has an advantage with read-only array. I only started testing it, so i don't know much about it's full implication.

[lilizoey]

i feel like we shouldn't just make VariantArray be able to be any other array under the hood. i think it'd be nice to have the current array type which works as it currently does, but we could also add a new array that is like a VariantArray but it can be any kind of array under the hood.

maybe we could do like Gd and have RawArray, then we can build on top of that two different kinds of array types: StaticArray<T> and RuntimeArray (names up for bikeshedding). Where StaticArray has a safety invariant where its type must actually be T (how the current Array works). While RuntimeArray can have any type in it, and any checks happen only at runtime.

We could with this allow a StaticArray<T> to be converted to a RuntimeArray, and we could have a function to coerce from RuntimeArray back to a StaticArray<T> as well.

We could possible use the names Array<T> for the static one, and VariantArray for runtime. This would then make Array<Variant> different from VariantArray, but i think usually when people reach for VariantArray they want something like gdscripts weakly typed array.