Crash caused by Godot Editor allowing inserting `null` into `Array<Gd<T>>`

[gg-yb]

See also #282.

Apparently the editor allows null values in arrays in all cases. I did not check if this is an array-specific thing.

gdext Version: 8990464 (master)

How to reproduce: Export a Array<Gd<Texture2D>> property. In the editor panel, increase the array length, save the scene, then reopen the scene. The editor crashes with the following message:

thread '<unnamed>' panicked at 'failed to convert from variant Variant(<null>) to godot_core::obj::gd::Gd<godot_core::gen::classes::texture_2d::re_export::Texture2D>; VariantConversionError', /home/####/Repositories/####/gdext/godot-core/src/builtin/variant/variant_traits.rs:20:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
fatal runtime error: failed to initiate panic, error 5

The project can no longer be opened, and changes to the Scene file have to be made by external means to restore the project.

[gg-yb]

Regarding what the right thing to do is, I think exporting Array<Gd<T>> is inherently dangerous when Godot allows inserting null values in all cases. If we cannot stop Godot from allowing null values here, we might want to require the user to use Array<Option<Gd<T>>> (once that is implemented, I think #247 is relevant here?), as as far as I understand gdext considers Option<Gd> to be the choice for nullable pointers?

[lilizoey]

We should probably just make Array<T> ignore null values unless the T allows for null values (such as Option<T> or Variant). I'm not sure if we should then remove the null values or keep them. Keeping them is more in line with what godot does and more zero-cost, but it's very non-idiomatic for Array<T> to contain values that can't be represented by T. But then again we don't need to actually expose it to the user.

I'd prefer removing the null values though, unless that makes the editor act really weirdly.

[gg-yb]

While there likely is no use in having null elements in an Array that cannot represent them, I think silently dropping those elements (and thus changing contents, and array length) might subtly subvert user expectations of receiving on the Rust end what they entered on the Godot end, and might lead to bugs (even if only semantic ones) where people are not aware of the non-null nature of e.g. Gd<T>.

Maybe we should automatically treat Array<T> as if all elements were Option<T> (i.e. indexing returns Option<T>) if the editor allows for T to be null?

[Bromeon]

We would also need to test how GDScript handles Array and Array[T] in terms of null insertions. Both in local variables as well as @export ones.

[lilizoey]

Maybe we should automatically treat Array<T> as if all elements were Option<T> (i.e. indexing returns Option<T>) if the editor allows for T to be null?

I'm not sure if this would work for value types. But we should first see how godot treats this as Bromeon mentioned.

[gg-yb]

I ran the following code:

Via the editor I pre-filled "foo" with 1, 2; and "bar" with a texture. Then I added another element in both arrays and did not change the added element (i.e. took the default the editor would insert).

The output is as follows:

Foo(3): [1, 2, 0]
Bar(2): [<CompressedTexture2D#-9223372013410558334>, <null>]
Foo(3): [1, 2, 0]
Bar(3): [<CompressedTexture2D#-9223372013410558334>, <null>, <null>]
Baz(2): [1, 2]
Baz(2): [1, 2]
Qux(2): [1, "abc"]
Qux(3): [1, "abc", <null>]

[lilizoey]

So the only case where our logic should fail here would be for inserting null objects into an Array<Gd<T>>. I think i'd still prefer removing them automatically here since Gd<T> is supposed to be non-nullable, and the alternatives seem to either be potentially breaking the project or breaking null-safety for Gd<T> (though admittedly fairly weakly).

[Bromeon]

Gd<T> containing null is a non-starter, this would just introduce the Billion Dollar Mistake.

@gg-yb so it looks like Array (aka Array[Variant]) and Array[Object] (the texture one) can contain nulls, but integer ones cannot. Maybe we should check how this behaves for other types, e.g. Strings.

So I see two options:

1. We allow exporting Array<T>, but certain T must be wrapped as Option<T> (namely the nullable ones)
2. We skip/remove nulls in Array<T> but allow them in Array<Option<T>>.

(2) is more ergonomic, as it prevents unwrapping every time a user wants to access elements. We would need to define how we skip/remove the nulls though.

[lilizoey]

Gd<T> containing null is a non-starter, this would just introduce the Billion Dollar Mistake.

it would be Array<Gd<T>> containing null values, but not allowing you to get a null-Gd<T>. so it'd just return Nones. thus why it would only weakly be breaking null safety. but i dont think i like this solution. it'd require a lot of reworking of various stuff, including indexing.

[gg-yb]

Is it possible for us to raise an error (one that does not crash the editor) when we get null for a Gd<T> in an array (TODO: does it happen for exported Gd<T> without arrays too?)

That would prevent pseudo-nullable Gd<T> (or any option requirement), and also would not break user expectations (I add an element via the editor, it shows up in Rust).

Silently dropping elements on the rust side feels like something that may cause a lot of pain in the future, because that is not what people expect to happen when they add an element, especially if its the default element the editor inserts. I'd rather see an explicit error warning the user early of the issue, then they can decide whether they need to support null (thus requiring them to use Option<Gd<T>>) or whether to just keep disallowing null by not changing their code (thus raising errors that do NOT crash the engine whenever null is in the array)

[Arrekin]

Please do not drop nulls. If your decision is to not support them, just throw an error, forcing the developer to clean his array in GdScript. Getting your array with elements randomly displaced will be a terrible experience. (Basically +1 to the comment above)

[Bromeon]

Is it possible for us to raise an error (one that does not crash the editor) when we get null for a Gd<T> in an array (TODO: does it happen for exported Gd<T> without arrays too?)

But would this then not always cause an error whenever someone adds an element via editor, because it's initially null?

Silently dropping elements on the rust side feels like something that may cause a lot of pain in the future, because that is not what people expect to happen when they add an element, especially if its the default element the editor inserts

But what do people expect if the backing storage is declared as Array<Gd<Object>>? Certainly not that it retains nulls, either.

[gg-yb]

Is it possible for us to raise an error (one that does not crash the editor) when we get null for a Gd<T> in an array (TODO: does it happen for exported Gd<T> without arrays too?)

But would this then not always cause an error whenever someone adds an element via editor, because it's initially null?

Ideally this would only occur once the user somehow shows intent to leave it that way (e.g. focus loss). I do not know when Godot actually tries to bring the editor contents into Rust, but if we cannot prevent the error from happening immediately, we should likely still show it.

Silently dropping elements on the rust side feels like something that may cause a lot of pain in the future, because that is not what people expect to happen when they add an element, especially if its the default element the editor inserts

But what do people expect if the backing storage is declared as Array<Gd<Object>>? Certainly not that it retains nulls, either.

We maybe should define people here: As this involves not only Rust code but the editor as well, we should accept that not everyone will be familiar with Rust, or how that DLL actually stores the array's contents. Even if it was only Rust code, we cannot expect everyone working on a project to be well-versed in Rust and Memory Safety Concerns/The Null Question.

So we should approach the topic in a way that is agnostic to even programming:

When I do something, I either succeed or it clearly does not work.

In this case: When I add an element to an array, it either is added, or an error is raised. This is my expectation as a dev, too. I call some method, it either does what it says it does, or it raises an error.

And in a way, adding elements via the editor is like pushing into an array, complete with the same fear of "I can see it in the editor, why can't I see it from the code?"

[lilizoey]

Is it possible for us to raise an error (one that does not crash the editor) when we get null for a Gd<T> in an array (TODO: does it happen for exported Gd<T> without arrays too?)

But would this then not always cause an error whenever someone adds an element via editor, because it's initially null?

Ideally this would only occur once the user somehow shows intent to leave it that way (e.g. focus loss). I do not know when Godot actually tries to bring the editor contents into Rust, but if we cannot prevent the error from happening immediately, we should likely still show it.

Silently dropping elements on the rust side feels like something that may cause a lot of pain in the future, because that is not what people expect to happen when they add an element, especially if its the default element the editor inserts

But what do people expect if the backing storage is declared as Array<Gd<Object>>? Certainly not that it retains nulls, either.

We maybe should define people here: As this involves not only Rust code but the editor as well, we should accept that not everyone will be familiar with Rust, or how that DLL actually stores the array's contents. Even if it was only Rust code, we cannot expect everyone working on a project to be well-versed in Rust and Memory Safety Concerns/The Null Question.

I feel like it's kind of an expectation that rust developers are familiar with nullability and how that's treated in rust. And if you're creating a gdext library then it's your responsibility to ensure that it works well when used as intended. So you shouldn't be using Array<Gd<T>> if you expect people to put nulls into it like youre suggesting.

So we should approach the topic in a way that is agnostic to even programming:

When I do something, I either succeed or it clearly does not work.

In this case: When I add an element to an array, it either is added, or an error is raised. This is my expectation as a dev, too. I call some method, it either does what it says it does, or it raises an error.

And in a way, adding elements via the editor is like pushing into an array, complete with the same fear of "I can see it in the editor, why can't I see it from the code?"

I think that if the nulls were just dropped then they wouldn't show up in the editor on a reload. but we should definitely check out the ergonomics of doing it this way. as well as the other suggestions.

My preference in order from most to least preferred is roughly:

1. Drop nulls
2. Dont allow exporting Array<Gd<T>>, i.e require using Array<Option<Gd<T>>> for exports
3. Allow null values in a Array<Gd<T>>, but prevent you from getting a null-gd out of such an array.

I think option 1 would be best but it depends entirely on how well it works with the editor. If it creates a confusing user-experience it'd make it very unusable, and possibly more of a footgun than anything else.

option 2 certainly works but I'd like for people to be able to export Array<Gd<T>> if they want to have a guarantee that every element of the array isn't null.

option 3 does work but it's a bit strange from a rust perspective to have an array with elements you can't really access. and it'd make Array<Gd<T>> and Array<Option<Gd<T>>> behave identically which is also weird. it's also a bit unclear to me if we can limit this to just Gd<T> or if it'd impact every other type too.

there may be other alternatives but those are the main three i can think of.

[lilizoey]

From what i can tell, this doesn't crash the editor now using godot 4.2.2 at least. Instead you get a panic when you try to access the null objects from the array.