Add support for typed arrays #101

ttencate · 2023-01-31T14:22:08Z

No description provided.

Bromeon · 2023-02-01T10:03:12Z

Possibly interesting: commit 2ff08c8 added the array_set_typed function in the C header.

bors try

bors · 2023-02-01T10:34:39Z

try

Build failed:

full-ci

Bromeon

Thanks a lot for another great pull request! 😀

1. I thought a bit more about the naming, especially about your comments on Discord:

I hope that typed arrays will be the majority, so #2 is nicer to work with. It also aligns with gdextension's VariantArray. [...]

At that boundary, in godot-core/src/gen, I count 87 places where Array is used as argument or return type, and 209 places where TypedArray is used instead. And some of those Array uses might be cases where the Godot devs haven't added extra type annotations yet.

Idiomatic would probably be to make the "right" way (type-safe) easier to use? That is,

struct Array<T> {...}
type VariantArray = Array<Variant>;

I think this might be worth a try. If it doesn't work, we can still do something like VArray or even rename everything.

(Don't bother with all the text re-alignment if you replace identifiers 😬)

2. The current design does mean that in case of type Variant, we create extra copies by calling Variant::to_variant(). Which I think is fine for now, they're designed to be cheaply copyable, but I thought it would be worth mentioning.

3. The reordering of the methods makes it quite hard to see actual changes, is there any particular reason for that?

Bromeon · 2023-02-01T12:11:31Z

godot-core/src/builtin/array.rs

+impl<T: VariantMetadata> PartialEq for TypedArray<T> {
+    #[inline]
+    fn eq(&self, other: &Self) -> bool {
+        unsafe {
+            let mut result = false;
+            sys::builtin_call! {
+                array_operator_equal(self.sys(), other.sys(), result.sys_mut())
+            };
+            result
+        }
+    }
+}
+
+impl<T: VariantMetadata> PartialOrd for TypedArray<T> {
+    #[inline]
+    fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+        let op_less = |lhs, rhs| unsafe {
+            let mut result = false;
+            sys::builtin_call! {
+                array_operator_less(lhs, rhs, result.sys_mut())
+            };
+            result
+        };
+
+        if op_less(self.sys(), other.sys()) {
+            Some(std::cmp::Ordering::Less)
+        } else if op_less(other.sys(), self.sys()) {
+            Some(std::cmp::Ordering::Greater)
+        } else if self.eq(other) {
+            Some(std::cmp::Ordering::Equal)
+        } else {
+            None
+        }
+    }
+}


It's a pity we can't use the macro for generics, but it's probably also quite a big effort to support...
Let's leave it as is, and reconsider in case this happens in many places.

It's not too bad to support that actually. Just did it the quick and dirty way for now, but I can DRY it up before we merge this.

As you wish. For me, it's also fine to do that later.

Bromeon · 2023-02-01T21:11:24Z

godot-core/src/builtin/array.rs

+    /// Changes the generic type on this array, without changing its contents. Needed for API
+    /// functions that return a variant array even though we know its type.
+    fn assume_type<U: VariantMetadata>(self) -> TypedArray<U> {
+        // SAFETY: The memory layout of `TypedArray<T>` does not depend on `T`.
+        unsafe { std::mem::transmute(self) }
+    }


This should probably call check_type().

Even if the layout is the same, the entire invariant of the class is that each element has type U.
It isn't unimaginable that future code relies on that property, without thinking of this (unchecked yet marked safe) boundary.

Note that assume_typed is not public. I think we should have a public wrapper like to_typed() that does check the type (and returns Result).

Public/private is generally orthogonal to safe/unsafe. This method can be used to subvert the type system in a way that can cause UB. Not in the current implementation (as far as I can see at least), but as soon as any code starts relying on the fact that a Array<T> holds indeed variants of type T. Which is a generally reasonable assumption*.

Imagine we optimize one of the accesses in the future:

pub fn get(&self, index: usize) -> T { ... T::from_variant(variant) }

becomes

pub fn get(&self, index: usize) -> T { ... // SAFETY: every Variant in Array<T> has T unsafe { T::from_variant_unchecked(variant) } }

Now you can argue that this unsafe block introduced UB. But it's not true, it merely triggered UB. Its // SAFETY assumption relies on the invariant of the type Array<T>. The function that can violate the invariant in the first place is assume_typed(), if used wrongly.

So there are two options:

assume_typed() must be unsafe, as the caller must ensure that it is always called on arrays with the correct type (an external invariant).

assume_typed() verifies that invariant and is thus safe.

* As you brought up, there are edge cases where "Array<T> always holds T variants" is hard to enforce, but I'd like to try. Even if this turned out to be wrong, I wouldn't see it as a reason to open more doors that can violate invariants.

It's also depending on how we define the invariants. If we shift from "Array<T> holds T variants" towards "each access to variants is separately runtime-checked", things look again different. But I'd like to give up strong guarantees only if there's absolutely no way around it.

Fair point. In an ideal world, I would mark assume_typed() as unsafe because all current call sites actually do uphold the invariant. However, we don't know what curve balls Godot's typing will throw us in the future, so an extra check_type() is the safer route.

Turns out, we actually need to break the invariant sometimes. InnerArray::append_array() for example takes any Array, not necessarily a TypedArray of the same type as self. So I'm going to mark this unsafe instead.

But why does that make it unsafe? The method assume_typed() would just panic on check_type(), no?

In other words, it's a logic error if we don't allow to append Array<String> to Array<Variant>, but not a memory safety issue.

We can't call check_type() inside assume_typed() because it would panic for a legitimate, but internal, use case: conversion from TypedArray<T> to TypedArray<Variant> in order to pass it to InnerArray::append_array(). You mentioned two options above; we can't use the second, so I went with the first.

godot-core/src/builtin/array.rs

godot-core/src/builtin/meta/class_name.rs

godot-core/src/builtin/variant/impls.rs

godot/src/lib.rs

ttencate · 2023-02-02T08:24:13Z

2. The current design does mean that in case of type Variant, we create extra copies by calling Variant::to_variant(). Which I think is fine for now, they're designed to be cheaply copyable, but I thought it would be worth mentioning.

True, I didn't think of that. Agreed that we can live with it for now.

3. The reordering of the methods makes it quite hard to see actual changes, is there any particular reason for that?

There are three impl blocks now: one for T: VariantMetadata, one for T: VariantMetadata + FromVariant and one for T: VariantMetadata + ToVariant. Seemed like the Rusty way to do it. Not sure how useful it is; maybe it would allow e.g. TypedArray<&str>, but of course it would be write-only.

If you think it's not worth it, I can easily put it back in a single impl in the original order.

ttencate · 2023-02-02T08:55:54Z

Looks like set_typed was removed from the JSON API, but there's still array_set_typed in the header file. I'll update this when I have more time.

They also added array_ref, which just points the array's data to another array without checking or touching any types. Not sure when you'd want that, but it's good to know it exists.

Bromeon · 2023-02-03T20:59:19Z

There are three impl blocks now: one for T: VariantMetadata, one for T: VariantMetadata + FromVariant and one for T: VariantMetadata + ToVariant. Seemed like the Rusty way to do it. Not sure how useful it is; maybe it would allow e.g. TypedArray<&str>, but of course it would be write-only.

If you think it's not worth it, I can easily put it back in a single impl in the original order.

Ah, good thinking! We may not be able to keep that up in case we try to tighten the invariants, but for this PR it should be fine.

Looks like set_typed was removed from the JSON API, but there's still array_set_typed in the header file.

Yep, you might have missed my reply here 🙂
This was changed in godotengine/godot#71989, motivation being that it shouldn't be accessible from GDScript.

Any further inputs about the name?
From my earlier review:

1. I thought a bit more about the naming, especially about your comments on Discord:

I hope that typed arrays will be the majority, so #2 is nicer to work with. It also aligns with gdextension's VariantArray. [...]
At that boundary, in godot-core/src/gen, I count 87 places where Array is used as argument or return type, and 209 places where TypedArray is used instead. And some of those Array uses might be cases where the Godot devs haven't added extra type annotations yet.

Idiomatic would probably be to make the "right" way (type-safe) easier to use? That is,
struct Array<T> {...}
type VariantArray = Array<Variant>;
I think this might be worth a try. If it doesn't work, we can still do something like VArray or even rename everything.

(Don't bother with all the text re-alignment if you replace identifiers 😬)

ttencate · 2023-02-04T12:52:05Z

Any further inputs about the name?

I would like to rename it as you suggested, but would prefer to do that in a separate PR if that's okay.

ttencate · 2023-02-04T13:00:26Z

Current status: need to add more integration tests; also blocked on #108 which is causing the first few integration tests to fail.

Bromeon · 2023-02-04T16:01:25Z

Sorry for merge conflicts, doctests were broken on master and somehow slipped CI.

I would like to rename it as you suggested, but would prefer to do that in a separate PR if that's okay.

Fine with me. With your proposal of UnknownArray, we might also consider how that would possibly integrate into our naming scheme.

Current status: need to add more integration tests; also blocked on #108 which is causing the first few integration tests to fail.

I have a branch that fixes the issue, unfortunately it introduces memory leaks elsewhere 🙈 I'll keep debugging.

ttencate · 2023-02-05T13:17:34Z

Alright, this PR has been stewing for long enough, so I'm going to leave generics support in impl_builtin_traits! for a future PR. I've added some more integration tests and I have some confidence now that there are some cases in which this stuff actually works 😜

Bromeon · 2023-02-05T13:21:29Z

Integration tests only have access to a limited set of classes, due to compile-time / dev-cycle reasons.

Would it be possible to use some of the available classes here?

ttencate · 2023-02-05T13:57:36Z

Right, I was still using an old command line (not check.sh itest) locally. Might be difficult to find a replacement... I'll take a closer look tonight (CEST).

Bromeon

Thanks a lot for the update, and the huge number of tests!

godot-codegen/src/class_generator.rs

godot-codegen/src/util.rs

godot-core/src/builtin/array.rs

godot-core/src/builtin/meta/class_name.rs

itest/rust/src/array_test.rs

Bromeon

Thanks a lot for another great addition!

bors r+

101: Add support for typed arrays r=Bromeon a=ttencate Co-authored-by: Thomas ten Cate <ttencate@gmail.com>

bors · 2023-02-08T19:59:13Z

Build failed:

full-ci

Bromeon

Small suggestion, and hopefully we can additionally integrate godotengine/godot#72926 for type safety. But otherwise we can also make a change that works with current Godot.

godot-core/src/builtin/array.rs

ttencate · 2023-02-09T08:05:22Z

Let's deal with that later, once it's merged upstream. Applied your suggestion for now.

- Rename `Array` to `TypedArray<T>` - Check/set runtime type of the underlying Godot `Array` - Make all parameters and return values `T` instead of `Variant` - Add `Array` as an alias for `TypedArray<Variant>` - Add `array!` macro and use it in tests See godot-rust#33 for design discussion

ttencate · 2023-02-09T20:14:29Z

Rebased and resolved conflicts.

Bromeon · 2023-02-09T21:35:49Z

bors r+

bors · 2023-02-09T21:46:42Z

Build succeeded:

full-ci

Bromeon · 2023-02-09T21:49:17Z

Finally 😁 thanks for the great job!

bors bot added a commit that referenced this pull request Feb 1, 2023

Try #101:

eeae6d0

Bromeon force-pushed the master branch from 7d4582d to c1a36da Compare February 1, 2023 18:12

Bromeon reviewed Feb 1, 2023

View reviewed changes

Bromeon mentioned this pull request Feb 1, 2023

Add iteration methods to Dictionary #99

Merged

ttencate force-pushed the feature/typed_array branch 2 times, most recently from 6a147fd to f145278 Compare February 2, 2023 08:43

Bromeon added feature Adds functionality to the library c: core Core components labels Feb 3, 2023

ttencate mentioned this pull request Feb 5, 2023

Converting RefCounted to Variant causes premature free #108

Closed

ttencate force-pushed the feature/typed_array branch 3 times, most recently from 74b729a to 8950852 Compare February 5, 2023 13:16

ttencate marked this pull request as ready for review February 5, 2023 13:18

Bromeon reviewed Feb 5, 2023

View reviewed changes

ttencate force-pushed the feature/typed_array branch 4 times, most recently from b7fe10e to f1e85d9 Compare February 5, 2023 21:08

ttencate mentioned this pull request Feb 5, 2023

Array support #33

Closed

ttencate force-pushed the feature/typed_array branch 3 times, most recently from 037e3f3 to cff8ff6 Compare February 6, 2023 08:13

ttencate mentioned this pull request Feb 6, 2023

Segfault in variant_conversion_fails test #117

Closed

ttencate force-pushed the feature/typed_array branch 3 times, most recently from 3577314 to 82a6779 Compare February 7, 2023 16:11

Bromeon approved these changes Feb 8, 2023

View reviewed changes

bors bot added a commit that referenced this pull request Feb 8, 2023

Merge #101

a8df462

101: Add support for typed arrays r=Bromeon a=ttencate Co-authored-by: Thomas ten Cate <ttencate@gmail.com>

Bromeon reviewed Feb 8, 2023

View reviewed changes

godot-core/src/builtin/array.rs Outdated Show resolved Hide resolved

ttencate force-pushed the feature/typed_array branch 2 times, most recently from ac0c8c9 to 897ad60 Compare February 9, 2023 08:02

ttencate force-pushed the feature/typed_array branch from 897ad60 to 42f12f5 Compare February 9, 2023 20:14

bors bot merged commit e594365 into godot-rust:master Feb 9, 2023

Bromeon mentioned this pull request Feb 11, 2023

Project status overview #24

Open

Bromeon mentioned this pull request Mar 9, 2023

Bitsets implement Default #162

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for typed arrays #101

Add support for typed arrays #101

ttencate commented Jan 31, 2023

Bromeon commented Feb 1, 2023 •

edited

Loading

bors bot commented Feb 1, 2023

Bromeon left a comment

Bromeon Feb 1, 2023

ttencate Feb 2, 2023

Bromeon Feb 3, 2023

Bromeon Feb 1, 2023 •

edited

Loading

ttencate Feb 2, 2023

Bromeon Feb 2, 2023 •

edited

Loading

ttencate Feb 2, 2023

ttencate Feb 3, 2023

Bromeon Feb 3, 2023

ttencate Feb 4, 2023

ttencate commented Feb 2, 2023

ttencate commented Feb 2, 2023 •

edited

Loading

Bromeon commented Feb 3, 2023 •

edited

Loading

ttencate commented Feb 4, 2023

ttencate commented Feb 4, 2023

Bromeon commented Feb 4, 2023

ttencate commented Feb 5, 2023 •

edited

Loading

Bromeon commented Feb 5, 2023

ttencate commented Feb 5, 2023

Bromeon left a comment

Bromeon left a comment

bors bot commented Feb 8, 2023

Bromeon left a comment

ttencate commented Feb 9, 2023

ttencate commented Feb 9, 2023

Bromeon commented Feb 9, 2023

bors bot commented Feb 9, 2023

Bromeon commented Feb 9, 2023 •

edited

Loading

Add support for typed arrays #101

Add support for typed arrays #101

Conversation

ttencate commented Jan 31, 2023

Bromeon commented Feb 1, 2023 • edited Loading

bors bot commented Feb 1, 2023

try

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttencate commented Feb 2, 2023

ttencate commented Feb 2, 2023 • edited Loading

Bromeon commented Feb 3, 2023 • edited Loading

ttencate commented Feb 4, 2023

ttencate commented Feb 4, 2023

Bromeon commented Feb 4, 2023

ttencate commented Feb 5, 2023 • edited Loading

Bromeon commented Feb 5, 2023

ttencate commented Feb 5, 2023

Bromeon left a comment

Choose a reason for hiding this comment

Bromeon left a comment

Choose a reason for hiding this comment

bors bot commented Feb 8, 2023

Bromeon left a comment

Choose a reason for hiding this comment

ttencate commented Feb 9, 2023

ttencate commented Feb 9, 2023

Bromeon commented Feb 9, 2023

bors bot commented Feb 9, 2023

Bromeon commented Feb 9, 2023 • edited Loading

Bromeon commented Feb 1, 2023 •

edited

Loading

Bromeon Feb 1, 2023 •

edited

Loading

Bromeon Feb 2, 2023 •

edited

Loading

ttencate commented Feb 2, 2023 •

edited

Loading

Bromeon commented Feb 3, 2023 •

edited

Loading

ttencate commented Feb 5, 2023 •

edited

Loading

Bromeon commented Feb 9, 2023 •

edited

Loading