Array<T> can hold values that are invalid representations of T

[Bromeon]

Problem

Array<T> and Array<U> are indirectly convertible, via Variant.

Integer conversions

They are even equal if T and U map to the same variant type, like integers:

#[itest(focus)]
fn variant_array_conversions_ok() {
    let i32_array: Array<i32> = array![1, 2, 4, 300, 500];
    let i64_array: Array<i64> = array![1, 2, 4, 300, 500];

    let i32_variant = i32_array.to_variant();
    let i64_variant = i64_array.to_variant();

    assert_eq!(i32_variant, i64_variant);
}

However, this can be exploited to craft arrays with values that are not valid for the static element types.

#[itest(focus)]
fn variant_array_conversions() {
    let i32_array: Array<i32> = array![1, 2, 4, 300, 500];
    let i8_back: Array<i8> = i32_variant.try_to();
    //assert!(i8_back.is_err()); // NOT TRUE

    dbg!(&i8_back); // Ok( "[1, 2, 4, 300, 500]" )
    let i8_back = i8_back.unwrap();

    let num = i8_back.get(4);
    // ^ Panics only now: FromGodot::from_variant() failed: `i8` cannot store the given value: 500
}

Class casting

I have not tested yet if a similar issue exists with subclassing, i.e. Array<Gd<T>> and Array<Gd<U>>. Although this is probably more rigorously checked, since we have access to the class name as part of the array type -- whereas the exact integer type is lost, it's just VariantType::INT.

Note that "array upcasting" is not safe, because arrays aren't covariant. If we allow converting Array<Gd<Node3D>> to Array<Gd<Node>>, then someone could attempt to store different Node types in the Node3D array.

---

Possible solutions

1. One option is to limit ArrayElement to the canonical type, which matches the Godot representation (i.e. i64 for integers). This would then only allow 1 type per VariantType (except for objects), but it would be more explicit.

2. We could iterate through each element and check if conversion is possible. This can be expensive, but we could do it a bit smarter:

   • only in cases where there are multiple ArrayElement types mapping to the same VariantType (e.g. integers)
   • only if the type is not already the canonical type (storing as i64 will always succeed)
     This would mean we would still allow non-canonical Array<T> usages, however with a slight performance hit. People who care about this could then fall back to the canonical types.

3. A complex system tracing arrays-in-variants via global metadata (then e.g. i16 -> i32 would be safe). It's not really something I'd like to get into though, as it has a million backdoors and edge cases, and I don't even know if it's possible to identify arrays in GDExtension.

4. We accept the lowered type safety and resort to panics when accessing elements. Variant conversions are quite robust so they should catch these cases, but it's still not very nice to postpone/hide such logic errors.

[Houtamelo]

Inside Godot, do Array<i32> and Array<i8> have the same memory layout?
Are Rust i8s occupying the same amount of space as i32s?

If the only advantage between Array<i32> and Array<i8> would be type safety, then I think it makes sense to simply restrict to types that are represented in Godot as well. (In this case, Array<i8> wouldn't be valid).

[Bromeon]

Inside Godot, do Array<i32> and Array<i8> have the same memory layout?

Yes, all arrays use Variant as their physical element type. The runtime type is just meta-information.

Are Rust i8s occupying the same amount of space as i32s?

Not sure what exactly you mean, but i8 needs 8 and i32 32 bits 🙂
But inside Variant, only i64 is stored, so we on Rust side promote it to the larger integer.

If the only advantage between Array<i32> and Array<i8> would be type safety, then I think it makes sense to simply restrict to types that are represented in Godot as well. (In this case, Array<i8> wouldn't be valid).

That's an option, but we also support these integer types in other places: parameters and return types of #[func], converting from/to Variant... so it seems like a limitation that isn't very clear.

[lilizoey]

i think we should go for option 1 for a couple of reasons

i think builtin types should match types that godot can represent. this is likely what people will assume anyway. with integer/float types there's a bit more leeway since those are rust types that we map to godot types, but even there godot does in fact have some cases where it tracks the different types.

runtime type checking should ideally be cheap imo, constant time preferably. this is doable with i8 for instance, but with arrays we'd need to iterate over the entire array, which is linear time. this would be extra surprising if people aren't fully familiar with the details of what types are representable by godot and then seemingly arbitrarily get performance hits.

with rust collections, you generally do have the property that any value stored in the collection will be a value of that type. so for Array<T> all values should be of type T.

i also think option 3 isn't something we can really hope to support. it'd make our arrays so much more complicated to deal with from a development and maintenance perspective.

finally, people can always just make their own newtype array if they really need it for other types. both 2 and 4 should be doable with that.

[Bromeon]

Here's the upstream PR that adds validates integer element types at runtime: godotengine/godot#95492

And here the local one: #855