Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OutArray, a possibly typed array. #806

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Conversation

Bromeon
Copy link
Member

@Bromeon Bromeon commented Jul 21, 2024

Quite a fundamental change to the way how we handle arrays, which should also fix #727. Still an early draft version and up for discussion.

This PR adds a third array type to the 2 existing ones:

  1. Array<T>, typed arrays
  2. VariantArray == Array<Variant>, untyped arrays
  3. OutArray, a possibly typed array

Out variance

"Out" here means out variance. In short, this means that such a type supports all operations where data flows out of the array, because those are covariant:

let a: Array<i32> = array![1, 2, 3]; 
let mut b: OutArray = a.into_out_array(); // loses static type info

let v: Variant = b.get(1); // works

However, it does not allow any data flow in to the array:

b.set(1, 5.to_variant()); // forbidden
b.set(1, "str".to_variant()); // forbidden

As you see in the 2nd example, this would otherwise ruin type safety. This is btw a problem that GDScript is fighting with.

It is important to understand that "out" does not mean "read". It's perfectly fine to perform write operations, as long as they don't involve data flow in to the array.

let v: Variant = b.pop(); // no problem
b.shuffle(); // no problem either

Engine APIs

One area where more research is needed are the Godot class/method APIs. So far, we have used either VariantArray or Array<T> in the signatures, and I think it has worked quite well. One commit changes VariantArray to OutArray, but I'm not yet sure if we should really do this.

Some thoughts:

  • OutArray is more flexible when you have to provide it, because you can now pass Array<T> (typed arrays) and convert them. Today, one has to use an untyped array.
    • For parameters to engine functions.
    • For return types in virtual functions.
  • However, OutArray is more limiting when receiving it, as one first needs to (fallibly) convert to VariantArray.
    • For return types of engine functions.
    • For parameters in virtual functions.
  • One option would be to choose OutArray when returning, and VariantArray when receiving.
  • It's not completely clear to me which guarantees Godot gives us today. If the signature is returning Array, could it technically point to a (runtime-)typed array or are we guaranteed that it's untyped?

Drawbacks

While it does solve some type-safety problems, an extra OutArray also brings quite a bit of mental overhead. It can also require conversions in more places. We should consider that GDScript doesn't have this, and few people are complaining about the covariance problem.

This feature could pull in more APIs just to be useful. Something like impl AsArrayArg or impl Into<OutArray>, similar to AsObjectArg, may be necessary. Then there's array!/varray! macros, etc.

As soon as Dictionaries become typed (which will happen), we'll have the same situation there, with both key/value variance. An OutDictionary would be the pendant.

A lighter way to address this would be to allow converting from Array<T> to VariantArray. If we do this, we can still panic on "in data flow" operations for VariantArray. This would have less strict guarantees, but might be more ergonomic for gamedev code.

@Bromeon Bromeon added feature Adds functionality to the library c: core Core components labels Jul 21, 2024
@Bromeon Bromeon added this to the 0.2 milestone Jul 21, 2024
@Bromeon Bromeon added the breaking-change Requires SemVer bump label Jul 21, 2024
@GodotRust
Copy link

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

@lilizoey
Copy link
Member

with typed dictionaries in 4.4, should we do something similar to this for typed dictionaries?

@Bromeon
Copy link
Member Author

Bromeon commented Sep 20, 2024

Yes, definitely. Would be good to do it for arrays first, to see if the concept works out 🙂

I'd like to try out your idea of Deref from Array<T> to &OutArray for parameter passing...

There are still some open questions on the engine API side. I'm not sure if it's safe to assume that a return type Array (without a type) also implies that it's untyped at runtime (thus representable as Array<Variant> in Rust). Theoretically, this is possible:

# GDScript instead of C++ to show principle
func some_godot_api(condition) -> Array:
    if condition:
        var ints: Array[int] = [1, 2, 3]
        return ints
    else:
        var floats: Array[float] = [1, 2, 3]
        return floats

But I'm not sure if it happens in practice. But if we want to be conservative, we need to treat Array return types as OutArray.

In general, I'm not sure how much friction OutArray would cause in practice -- if it turns out to force conversions all over the place, maybe it's better to stay with Godot's model instead of fighting it 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Requires SemVer bump c: core Core components feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Converting Typed Arrays to Variant Arrays Panics
3 participants