Implement basic Array functionality

[ttencate]

This implements the "easy" stuff on variant arrays:

• FromIterator, From on slices and Rust arrays
• IntoIterator, try_to_vec
• basic modification methods
• most utility methods

Still missing:

• utility methods that require Callable
• Eq, Ord (requires generated wrappers for operators)
• everything involving "typed" and "readonly"

See #33

[Bromeon]

This is a fantastic contribution, thanks a lot! ❤️

Very sensible choice of rust-idiomatic vs. true-to-GDScript signatures, something that's not easy. Also great test coverage! 🙂

You seem to use a lot of try_into().unwrap(), maybe extract that into functions to_i64/to_isize? We could then later refactor it to unwrap_unchecked if at some point we can't afford the unwrap anymore.

bors try

[Bromeon]

Thanks a lot for adding this section! 👍

Concurrent reads on different threads are also safe, but any writes must be externally synchronized.

Isn't that always true, for any kind of data structure? If all the accesses are read-only, threading is never a problem, many functional languages are built on this principle.

Is the safety-related information specific to Array or should we maybe move it one level up, to the builtin module documentation? I'm also writing an entire article about safety in godot-rust, so the API doc can really go into specifics and can take high-level assumptions (don't mess up threading from GDScript) as a base. I guess it doesn't hurt to repeat some parts, maybe later we can link to that article.

[Bromeon]

One thing worth mentioning is that each Array instance lives independently and has copy semantics (in fact CoW, but from the user perspective, copying an array creates an independent on).

Can you imagine any scenario where this leads to safety issues? E.g. the old instance is iterated over, but a copy (yet-to-be-CoW-written) is now modified.

[ttencate]

Isn't that always true, for any kind of data structure? If all the accesses are read-only, threading is never a problem, many functional languages are built on this principle.

It's true that concurrent reads are normally safe, but concurrent writes can also be made safe internally (using mutexes or whatever). The main point is that we don't do that.

Is the safety-related information specific to Array or should we maybe move it one level up, to the builtin module documentation?

I just wrote it here for now because I felt bad about ignoring GDScript threads altogether, whereas gdnative's VariantArray handles them more explicitly. But I didn't feel qualified to make such safety claims for other parts of the gdextension :) If it needs to go somewhere, I'd expect it even higher up, at the top level.

One thing worth mentioning is that each Array instance lives independently and has copy semantics (in fact CoW, but from the user perspective, copying an array creates an independent on).

Array is a strange beast. It's refcounted, so writes are reflected in all copies, but the refcounted thing is Vector<Variant> which itself is a refcounted copy-on-write array.

So the question is: can it ever happen that the inner Vector<T> is shared between two seemingly independent instances of Array? I don't think so, but I wouldn't bet on it.

And the other question is: does it matter? Surely CowData would create a copy before applying any mutations, right?

[Bromeon]

I don't see through the entire Array implementation in Godot... Would need to spend more time on it.

What I noticed:

• Two instances share state (the ref-counter), when one instance is copied from the other. But it looks like Godot uses the SafeRefCount class, which uses std::atomic internally. So concurrent modifications of the ref-counter should not cause race conditions or UB.
• Array has different methods that look like copying:
  • _assign(const Array&) -> bool
  • `typed_assign(const Array&) -> bool)
  • duplicate() -> Array
  • Array(const Array &p_from)
    I'm not sure which one is used by the FFI copy constructor method, would need to run the debugger.

I think we're good for now. Let's get this initial implementation running.

[Bromeon]

Very nice touch to change the name! 👍

This made me think: do we have variant types which can be "equivalent but not equal"?
Most types are Copy, but maybe OBJECT?

Would probably need some digging in Godot to be sure, but this name is a safe start...

[ttencate]

Maybe int(1) and float(1.0)? Or StringName("") and String("")?

[bors]

try

Build succeeded:

• full-ci

[ttencate]

You seem to use a lot of try_into().unwrap(), maybe extract that into functions to_i64/to_isize? We could then later refactor it to unwrap_unchecked if at some point we can't afford the unwrap anymore.

Done! More readable too.

All other comments addressed as well.

[Bromeon]

Thanks a lot for the updates!

bors r+

[bors]

Build succeeded:

• full-ci