Implement `GodotConvert` for `Vec<T>`, `[T; N]` and `&[T]` #795

Houtamelo · 2024-07-12T05:51:12Z

Rationale:
In multithreaded contexts, I've come across a use case where it was useful to express that a multithreaded operation would return a type that implements ToGodot.

This is one of the cases, in my crate gdext_coroutines:

fn async_task<R>(
	&self,
	f: impl std::future::Future<Output = R> + Send + 'static,
) -> CoroutineBuilder<R>
	where
		R: 'static + ToGodot + Send;

Rust vectors and arrays fit all bounds of R except ToGodot, even if the element type implements ToGodot.

The workaround is to use wrapper types and manually implement GodotConvert for them, but I believe this blanket implementation is simple enough to be worth improving the user experience.

GodotRust · 2024-07-12T07:01:09Z

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

Bromeon

Thank you! 👍

One thing to note is that we make the Godot equivalent of Vec<T> officially Array<T>, even though PackedTArray might be closer in some cases (same memory layout). But I think that's OK given that Array is more versatile; people could still explicitly convert to packed arrays.

Could you add integration tests for the conversions, e.g. in builtin_tests/convert_test.rs?

bend-n

Array => [T; N] should be done without a Vec allocation

godot-core/src/meta/godot_convert/impls.rs

bend-n · 2024-07-14T12:39:36Z

godot-core/src/meta/godot_convert/impls.rs

+	fn try_from_godot(via: Self::Via) -> Result<Self, ConvertError> {
+		let vec = via.iter_shared().collect::<Vec<T>>();
+
+		Self::try_from(vec)
+			.map_err(|vec| {
+				let message = format!(
+					"Array length mismatch, expected: `{}`, got: `{}`", LEN, vec.len()
+				);
+
+				ConvertError::with_kind_value(ErrorKind::Custom(Some(message.into())), vec)
+			})


This can, and ought, to be done without a collection.

Suggested change

fn try_from_godot(via: Self::Via) -> Result<Self, ConvertError> {

let vec = via.iter_shared().collect::<Vec<T>>();

Self::try_from(vec)

.map_err(|vec| {

let message = format!(

"Array length mismatch, expected: `{}`, got: `{}`", LEN, vec.len()

);

ConvertError::with_kind_value(ErrorKind::Custom(Some(message.into())), vec)

})

fn try_from_godot(via: Self::Via) -> Result<Self, ConvertError> {

use std::mem::{ManuallyDrop, MaybeUninit};

/// [`MaybeUninit::assume_init_array`] polyfill.

///

/// # Safety

///

/// It is up to the caller to guarantee that all elements of the array are

/// in an initialized state.

const unsafe fn aai<T, const N: usize>(array: [MaybeUninit<T>; N]) -> [T; N] {

/// [`core::intrinsics::transmute_unchecked`] polyfill.

/// # Safety

///

/// Same as [`core::mem::transmute()`].

const unsafe fn transmute_unchecked<T, U>(value: T) -> U {

// Create union type that can store a `T` or a `U`.

// We can then use this to convert between them.

//

// The repr(C) layout forces no offset between `t` and `u` as talked about here

// https://rust-lang.github.io/unsafe-code-guidelines/layout/unions.html#c-compatible-layout-repr-c

#[repr(C)]

union Transmute<T, U> {

t: ManuallyDrop<T>,

u: ManuallyDrop<U>,

}

// Create the union in the `T` state.

let value = Transmute {

t: ManuallyDrop::new(value),

};

// Read from the union in the `U` state.

// SAFETY: This is safe because the caller has promised that `T` can be transmuted to `U`.

// The following reference link talks about repr(C) unions being used this way.

// https://doc.rust-lang.org/reference/items/unions.html#reading-and-writing-union-fields

unsafe { ManuallyDrop::into_inner(value.u) }

}

// SAFETY: the caller must guarantee that `self` is initialized.

unsafe { transmute_unchecked(array) }

}

// Make sure the length of the array matches what its copied from.

if via.len() != LEN {

let message = format!(

"Array length mismatch, expected: `{}`, got: `{}`",

LEN,

via.len()

);

return Err(ConvertError::with_kind_value(

ErrorKind::Custom(Some(message.into())),

via,

));

}

// Create an uninit array for the output.

let mut out = [const { MaybeUninit::uninit() }; LEN];

// Copy each element from `via` to the returned value.

for (item, copy) in via.iter_shared().by_ref().zip(&mut out) {

copy.write(item);

}

// SAFETY: all items must have been written to, at this point.

// if a panic has occurred, which I dont think it can, the items in `out` are leaked.

Ok(unsafe { aai(out) })

If you prefer not to write out all that unsafe you can use my car crate and just do car::try_from_array_option(|_| iter.next()).map_err(...)

@bend-n Thanks for this implementation! Since this is quite involved and may require its own review, I'd rather not push the responsibility on @Houtamelo for this change and instead suggest you later open a separate issue/PR where we can discuss this.

If people agree that the extra unsafety is worth it, it probably makes sense to extract the polyfills to global functions, e.g. in godot-ffi/src/toolbox.rs. So they could be added independently of GodotConvert and also get their own unit tests.

Bromeon · 2024-07-21T08:28:04Z

(Rebased onto latest master, just to make sure everything's up-to-date)

Bromeon · 2024-07-28T18:32:45Z

Seems like the last change broke CI, you should probably revert the suggestion you applied from GitHub web UI...

Houtamelo · 2024-07-30T10:21:35Z

Seems like the last change broke CI, you should probably revert the suggestion you applied from GitHub web UI...

Speaking of which, would you like me to finish this PR? Or wait until the OutArray implementation is done?

Bromeon · 2024-08-02T10:38:20Z

I'm not sure where the OutArray proposal is heading, and I haven't received much feedback, so I think we can move ahead here.

Houtamelo · 2024-08-02T14:19:30Z

I'm not sure where the OutArray proposal is heading, and I haven't received much feedback, so I think we can move ahead here.

Alright then, I'll try to have this done by the weekend.
I think moving forward makes sense, if anything, we can just make the necessary edits to this when the OutArray impl is done.

Bromeon · 2024-08-10T22:32:53Z

Any update on this? (No rush on my side, just thought I'd check back since you mentioned last weekend 🙂 )

Houtamelo · 2024-08-11T04:05:55Z

Any update on this? (No rush on my side, just thought I'd check back since you mentioned last weekend 🙂 )

I resumed it today, was almost done until I ran into #851

For now, I've commented the tests that fail due to those issues, I've also cleaned up the commit and ensured check.sh runs successfully.

Also, I edited the implementation of Slice::from_godot which now uses unsafe code but slightly different from the suggestion of @bend-n

bend-n

Not a fan of this ultimately pointless unsafe, the options could just be unwrapped normally, as theres no uninit memory happening here, and the whole thing would be safe, and it would be fine, only using a few more bytes than the unsafe solution.

bend-n · 2024-08-11T05:49:41Z

godot-core/src/meta/godot_convert/impls.rs

+            // SAFETY: We checked that the length of via matches `LEN`, therefore idx would only be out of bounds if
+            // the array is modified during iteration, which should never be done and if done is caught in debug mode
+            // by the assertion above.
+            unsafe {


This unsafe is pointless-- the you can simply use iterators, and do via.iter_shared().zip(&mut option_array), and just *out = element it like that, or put an assert!(LEN == via_len) at the top and index both of them?

bend-n · 2024-08-11T05:50:30Z

godot-core/src/meta/godot_convert/impls.rs

+        let array = option_array.map(|some|
+			// SAFETY: We checked that the length of via matches `LEN`, therefore all options are some.
+			// This could be done with MaybeUninit, but that may cause leaks if a panic occurs in one of the FFI calls
+			// in `iter_shared()`


If you dont want it to leak, you can add a panic guard to properly drop the initialized elements, however if iter_shared() panics isnt that a bigger problem?
Remember, leaking is completely fine.

bend-n · 2024-08-11T05:52:34Z

godot-core/src/meta/godot_convert/impls.rs

+            debug_assert!(
+                idx < LEN,
+                "Elements were added to Array during `iter_shared()`, this is not allowed."
+            );


This is possible? Isnt it already UB if this happens? Im not sure how iter_shared() works, but this is rather dubious.

If it is possible, it should be a real assert!, and not a debug_assert!, as this is an integral safety condition with that get_unchecked.

If we add any such checks, they should happen inside Iter::next() (for arrays and dictionaries). Because otherwise, each call site has to repeat them, and we'll end up with some validated and some not.

Modify-while-iterate should be safe because it uses indexes internally, and these are bounds-checked. So it only causes logic errors, not memory unsafety, and thus debug_assert! (inside iter_shared() implementation) is fine.

Modify-while-iterate is fine, but if it changes the length of the array, its a problem, and thats what the debug_assert is checking, and if idx >= LEN then get_unchecked_mut is UB.

Bromeon

Thanks!

The "check if iter_shared works" assertions are well-intended but if we do those, they should be inside the iteration implementations (which have access to the array/dictionary) and not on each call-site. This could be done in a separate PR -- we can focus on this one first.

Bromeon · 2024-08-11T06:45:55Z

godot-core/src/meta/godot_convert/impls.rs

+			.map(|v| v.to_variant())
+			.unwrap_or_else(|_| {
+				panic!("to_variant(): u64 value {} is not representable inside Variant, which can only store i64 integers", self)
+			})


Please change back to spaces 🙂

Please change back to spaces 🙂

Weird, I just ran cargo fmt, will revert

Bromeon · 2024-08-11T06:54:13Z

godot-core/src/meta/godot_convert/impls.rs

+            debug_assert!(
+                idx < LEN,
+                "Elements were added to Array during `iter_shared()`, this is not allowed."
+            );


If we add any such checks, they should happen inside Iter::next() (for arrays and dictionaries). Because otherwise, each call site has to repeat them, and we'll end up with some validated and some not.

Modify-while-iterate should be safe because it uses indexes internally, and these are bounds-checked. So it only causes logic errors, not memory unsafety, and thus debug_assert! (inside iter_shared() implementation) is fine.

Bromeon · 2024-08-11T06:58:51Z

itest/rust/src/builtin_tests/convert_test.rs

+// Ensure valid conversions from Array to Rust collection types work
+#[itest]
+fn array_to_rust_valid() {


The different collection types (Vec, array) have independent implementations and should thus have separate #[itest]. It's fine to have multiple element types in one though (f64, GString etc.).

We are likely going to remove support for non-i64 integers and non-f64 floats as element types due to #805 -- so could you maybe use i64, GString and e.g. Vector2 or so?

Also nitpick, comments should generally stop with a period 🙂

Bromeon · 2024-08-11T07:02:06Z

itest/rust/src/builtin_tests/convert_test.rs

+    let valid_array_type_2 = godot::builtin::array! {
+        2_f64,
+        -64_f64,
+    };


You can import godot::builtin::array!.

Houtamelo · 2024-08-11T09:15:42Z

@Bromeon Should be all good now

Bromeon

There are now 500 LoC of tests, which is quite a lot for basically 2 conversions. I think it's good to test different element types, but there's a lot of duplication. Could you maybe try to extract some into generic helper functions?

There is some prior art in roundtrip:

gdext/itest/rust/src/common.rs

Lines 12 to 23 in 05446f6

    
           pub fn roundtrip<T>(value: T) 
        
           where 
        
               T: FromGodot + ToGodot + PartialEq + Debug, 
        
           { 
        
               // TODO test other roundtrip (first FromGodot, then ToGodot) 
        
               // Some values can be represented in Variant, but not in T (e.g. Variant(0i64) -> Option<InstanceId> -> Variant is lossy) 
        
               let variant = value.to_variant(); 
        
               let back = T::try_from_variant(&variant).unwrap(); 
        
               assert_eq!(value, back); 
        
           }

Or also several helper functions from here downward...

godot-core/src/meta/godot_convert/impls.rs

itest/rust/src/builtin_tests/convert_test.rs

Houtamelo · 2024-08-11T10:53:24Z

There are now 500 LoC of tests, which is quite a lot for basically 2 conversions. I think it's good to test different element types, but there's a lot of duplication. Could you maybe try to extract some into generic helper functions?

There is some prior art in roundtrip:

gdext/itest/rust/src/common.rs

Lines 12 to 23 in 05446f6

pub fn roundtrip<T>(value: T)

where

T: FromGodot + ToGodot + PartialEq + Debug,

{

// TODO test other roundtrip (first FromGodot, then ToGodot)

// Some values can be represented in Variant, but not in T (e.g. Variant(0i64) -> Option<InstanceId> -> Variant is lossy)

let variant = value.to_variant();

let back = T::try_from_variant(&variant).unwrap();

assert_eq!(value, back);

}

Or also several helper functions from here downward...

I could but I don't know when I'll have time to work on this again

Co-authored-by: Jan Haller <bromeon@gmail.com>

Bromeon · 2024-08-12T20:47:39Z

Took me too long to refactor all the tests, so I rewrote them. I didn't test all the details like error message, but the core functionality (conversion) is covered, and I find it quite maintainable now.

Thanks for implementing this feature! 🙂

Implement GodotConvert for Vec<T>, [T] and &[T], where T: ArrayElement.

Bromeon added feature Adds functionality to the library c: core Core components labels Jul 12, 2024

Bromeon reviewed Jul 12, 2024

View reviewed changes

bend-n suggested changes Jul 14, 2024

View reviewed changes

Bromeon mentioned this pull request Jul 22, 2024

Add OnReady::node() + #[init(node = "...")] attribute #807

Merged

Houtamelo marked this pull request as ready for review August 11, 2024 04:07

bend-n suggested changes Aug 11, 2024

View reviewed changes

Bromeon reviewed Aug 11, 2024

View reviewed changes

godot-core/src/meta/godot_convert/impls.rs Outdated Show resolved Hide resolved

itest/rust/src/builtin_tests/convert_test.rs Outdated Show resolved Hide resolved

Bromeon linked an issue Aug 11, 2024 that may be closed by this pull request

Implement ToGodot/FromGodot for Vec<T> #263

Closed

Implement GodotConvert for Vec<T>, [T; N] and &[T]

c4be58a

Co-authored-by: Jan Haller <bromeon@gmail.com>

Bromeon added this pull request to the merge queue Aug 12, 2024

github-merge-queue bot pushed a commit that referenced this pull request Aug 12, 2024

Merge pull request #795 from Houtamelo/master

98045f0

Implement GodotConvert for Vec<T>, [T] and &[T], where T: ArrayElement.

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 12, 2024

Bump MSRV: 1.78 -> 1.80

7de3d38

Bromeon enabled auto-merge August 12, 2024 21:00

Bromeon changed the title ~~Implement GodotConvert for Vec<T>, [T] and &[T], where T: ArrayElement.~~ Implement GodotConvert for Vec<T>, [T; N] and &[T] Aug 12, 2024

Bromeon added this pull request to the merge queue Aug 12, 2024

Merged via the queue into godot-rust:master with commit 6bcaa3c Aug 12, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `GodotConvert` for `Vec<T>`, `[T; N]` and `&[T]` #795

Implement `GodotConvert` for `Vec<T>`, `[T; N]` and `&[T]` #795

Houtamelo commented Jul 12, 2024

GodotRust commented Jul 12, 2024

Bromeon left a comment

bend-n left a comment

bend-n Jul 14, 2024

bend-n Jul 14, 2024

Bromeon Jul 14, 2024

Bromeon commented Jul 21, 2024

Bromeon commented Jul 28, 2024

Houtamelo commented Jul 30, 2024

Bromeon commented Aug 2, 2024

Houtamelo commented Aug 2, 2024

Bromeon commented Aug 10, 2024

Houtamelo commented Aug 11, 2024 •

edited

Loading

bend-n left a comment •

edited

Loading

bend-n Aug 11, 2024

bend-n Aug 11, 2024

bend-n Aug 11, 2024 •

edited

Loading

Bromeon Aug 11, 2024

bend-n Aug 11, 2024 •

edited

Loading

Bromeon left a comment

Bromeon Aug 11, 2024

Houtamelo Aug 11, 2024

Bromeon Aug 11, 2024

Bromeon Aug 11, 2024

Bromeon Aug 11, 2024

Houtamelo commented Aug 11, 2024

Bromeon left a comment

Houtamelo commented Aug 11, 2024

Bromeon commented Aug 12, 2024

-	fn try_from_godot(via: Self::Via) -> Result<Self, ConvertError> {
-		let vec = via.iter_shared().collect::<Vec<T>>();
-		Self::try_from(vec)
-			.map_err(|vec| {
-				let message = format!(
-					"Array length mismatch, expected: `{}`, got: `{}`", LEN, vec.len()
-				);
-				ConvertError::with_kind_value(ErrorKind::Custom(Some(message.into())), vec)
-			})
+	fn try_from_godot(via: Self::Via) -> Result<Self, ConvertError> {
+        use std::mem::{ManuallyDrop, MaybeUninit};
+        /// [`MaybeUninit::assume_init_array`] polyfill.
+        ///
+        /// # Safety
+        ///
+        /// It is up to the caller to guarantee that all elements of the array are
+        /// in an initialized state.
+        const unsafe fn aai<T, const N: usize>(array: [MaybeUninit<T>; N]) -> [T; N] {
+            /// [`core::intrinsics::transmute_unchecked`] polyfill.
+            /// # Safety
+            ///
+            /// Same as [`core::mem::transmute()`].
+            const unsafe fn transmute_unchecked<T, U>(value: T) -> U {
+                // Create union type that can store a `T` or a `U`.
+                // We can then use this to convert between them.
+                //
+                // The repr(C) layout forces no offset between `t` and `u` as talked about here
+                // https://rust-lang.github.io/unsafe-code-guidelines/layout/unions.html#c-compatible-layout-repr-c
+                #[repr(C)]
+                union Transmute<T, U> {
+                    t: ManuallyDrop<T>,
+                    u: ManuallyDrop<U>,
+                }
+                // Create the union in the `T` state.
+                let value = Transmute {
+                    t: ManuallyDrop::new(value),
+                };
+                // Read from the union in the `U` state.
+                // SAFETY: This is safe because the caller has promised that `T` can be transmuted to `U`.
+                // The following reference link talks about repr(C) unions being used this way.
+                // https://doc.rust-lang.org/reference/items/unions.html#reading-and-writing-union-fields
+                unsafe { ManuallyDrop::into_inner(value.u) }
+            }
+            // SAFETY: the caller must guarantee that `self` is initialized.
+            unsafe { transmute_unchecked(array) }
+        }
+        // Make sure the length of the array matches what its copied from.
+        if via.len() != LEN {
+            let message = format!(
+                "Array length mismatch, expected: `{}`, got: `{}`",
+                LEN,
+                via.len()
+            );
+            return Err(ConvertError::with_kind_value(
+                ErrorKind::Custom(Some(message.into())),
+                via,
+            ));
+        }
+        // Create an uninit array for the output.
+        let mut out = [const { MaybeUninit::uninit() }; LEN];
+        // Copy each element from `via` to the returned value.
+        for (item, copy) in via.iter_shared().by_ref().zip(&mut out) {
+            copy.write(item);
+        }
+        // SAFETY: all items must have been written to, at this point.
+        // if a panic has occurred, which I dont think it can, the items in `out` are leaked.
+        Ok(unsafe { aai(out) })

	pub fn roundtrip<T>(value: T)
	where
	T: FromGodot + ToGodot + PartialEq + Debug,
	{
	// TODO test other roundtrip (first FromGodot, then ToGodot)
	// Some values can be represented in Variant, but not in T (e.g. Variant(0i64) -> Option<InstanceId> -> Variant is lossy)

	let variant = value.to_variant();
	let back = T::try_from_variant(&variant).unwrap();

	assert_eq!(value, back);
	}

Implement GodotConvert for Vec<T>, [T; N] and &[T] #795

Implement GodotConvert for Vec<T>, [T; N] and &[T] #795

Conversation

Houtamelo commented Jul 12, 2024

GodotRust commented Jul 12, 2024

Bromeon left a comment

Choose a reason for hiding this comment

bend-n 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 commented Jul 21, 2024

Bromeon commented Jul 28, 2024

Houtamelo commented Jul 30, 2024

Bromeon commented Aug 2, 2024

Houtamelo commented Aug 2, 2024

Bromeon commented Aug 10, 2024

Houtamelo commented Aug 11, 2024 • edited Loading

bend-n left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bend-n Aug 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bend-n Aug 11, 2024 • edited Loading

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Houtamelo commented Aug 11, 2024

Bromeon left a comment

Choose a reason for hiding this comment

Houtamelo commented Aug 11, 2024

Bromeon commented Aug 12, 2024

Implement `GodotConvert` for `Vec<T>`, `[T; N]` and `&[T]` #795

Implement `GodotConvert` for `Vec<T>`, `[T; N]` and `&[T]` #795

Houtamelo commented Aug 11, 2024 •

edited

Loading

bend-n left a comment •

edited

Loading

bend-n Aug 11, 2024 •

edited

Loading

bend-n Aug 11, 2024 •

edited

Loading