Merge pull request #853 from godot-rust/qol/array-element-conversions

Best-effort checks for `Array<Integer>` conversions; fix `Debug` for variants containing typed arrays

Author: Bromeon

godot-codegen/src/conv/type_conversions.rs

@@ -217,7 +217,9 @@ fn to_rust_type_uncached(full_ty: &GodotTy, ctx: &mut Context) -> RustTy {
     } else if let Some(elem_ty) = ty.strip_prefix("typedarray::") {
         let rust_elem_ty = to_rust_type(elem_ty, None, ctx);
         return if ctx.is_builtin(elem_ty) {
-            RustTy::BuiltinArray(quote! { Array<#rust_elem_ty> })
+            RustTy::BuiltinArray {
+                elem_type: quote! { Array<#rust_elem_ty> },
+            }
         } else {
             RustTy::EngineArray {
                 tokens: quote! { Array<#rust_elem_ty> },

godot-codegen/src/models/domain.rs

@@ -588,11 +588,13 @@ pub struct GodotTy {
 
 #[derive(Clone, Debug)]
 pub enum RustTy {
-    /// `bool`, `Vector3i`
+    /// `bool`, `Vector3i`, `Array`
     BuiltinIdent(Ident),
 
     /// `Array<i32>`
-    BuiltinArray(TokenStream),
+    ///
+    /// Note that untyped arrays are mapped as `BuiltinIdent("Array")`.
+    BuiltinArray { elem_type: TokenStream },
 
     /// C-style raw pointer to a `RustTy`.
     RawPointer { inner: Box<RustTy>, is_const: bool },
@@ -674,7 +676,7 @@ impl ToTokens for RustTy {
     fn to_tokens(&self, tokens: &mut TokenStream) {
         match self {
             RustTy::BuiltinIdent(ident) => ident.to_tokens(tokens),
-            RustTy::BuiltinArray(path) => path.to_tokens(tokens),
+            RustTy::BuiltinArray { elem_type } => elem_type.to_tokens(tokens),
             RustTy::RawPointer {
                 inner,
                 is_const: true,

godot-codegen/src/special_cases/codegen_special_cases.rs

@@ -37,7 +37,7 @@ fn is_type_excluded(ty: &str, ctx: &mut Context) -> bool {
     fn is_rust_type_excluded(ty: &RustTy) -> bool {
         match ty {
             RustTy::BuiltinIdent(_) => false,
-            RustTy::BuiltinArray(_) => false,
+            RustTy::BuiltinArray { .. } => false,
             RustTy::RawPointer { inner, .. } => is_rust_type_excluded(inner),
             RustTy::EngineArray { elem_class, .. } => is_class_excluded(elem_class.as_str()),
             RustTy::EngineEnum {

godot-core/src/builtin/collections/array.rs

@@ -34,6 +34,9 @@ use sys::{ffi_methods, interface_fn, GodotFfi};
 /// `Array<T>`, where the type `T` must implement `ArrayElement`. Some types like `Array<T>` cannot
 /// be stored inside arrays, as Godot prevents nesting.
 ///
+/// If you plan to use any integer or float types apart from `i64` and `f64`, read
+/// [this documentation](../meta/trait.ArrayElement.html#integer-and-float-types).
+///
 /// # Reference semantics
 ///
 /// Like in GDScript, `Array` acts as a reference type: multiple `Array` instances may
@@ -716,19 +719,57 @@ impl<T: ArrayElement> Array<T> {
     ///
     /// Note also that any `GodotType` can be written to a `Variant` array.
     ///
-    /// In the current implementation, both cases will produce a panic rather than undefined
-    /// behavior, but this should not be relied upon.
+    /// In the current implementation, both cases will produce a panic rather than undefined behavior, but this should not be relied upon.
     unsafe fn assume_type<U: ArrayElement>(self) -> Array<U> {
-        // SAFETY: The memory layout of `Array<T>` does not depend on `T`.
-        unsafe { std::mem::transmute(self) }
+        // The memory layout of `Array<T>` does not depend on `T`.
+        std::mem::transmute::<Array<T>, Array<U>>(self)
+    }
+
+    /// # Safety
+    /// See [`assume_type`](Self::assume_type).
+    unsafe fn assume_type_ref<U: ArrayElement>(&self) -> &Array<U> {
+        // The memory layout of `Array<T>` does not depend on `T`.
+        std::mem::transmute::<&Array<T>, &Array<U>>(self)
+    }
+
+    #[cfg(debug_assertions)]
+    pub(crate) fn debug_validate_elements(&self) -> Result<(), ConvertError> {
+        // SAFETY: every element is internally represented as Variant.
+        let canonical_array = unsafe { self.assume_type_ref::<Variant>() };
+
+        // If any element is not convertible, this will return an error.
+        for elem in canonical_array.iter_shared() {
+            elem.try_to::<T>().map_err(|_err| {
+                FromGodotError::BadArrayTypeInt {
+                    expected: self.type_info(),
+                    value: elem
+                        .try_to::<i64>()
+                        .expect("origin must be i64 compatible; this is a bug"),
+                }
+                .into_error(self.clone())
+            })?;
+        }
+
+        Ok(())
+    }
+
+    // No-op in Release. Avoids O(n) conversion checks, but still panics on access.
+    #[cfg(not(debug_assertions))]
+    pub(crate) fn debug_validate_elements(&self) -> Result<(), ConvertError> {
+        Ok(())
     }
 
     /// Returns the runtime type info of this array.
     fn type_info(&self) -> ArrayTypeInfo {
         let variant_type = VariantType::from_sys(
             self.as_inner().get_typed_builtin() as sys::GDExtensionVariantType
         );
-        let class_name = self.as_inner().get_typed_class_name();
+
+        let class_name = if variant_type == VariantType::OBJECT {
+            Some(self.as_inner().get_typed_class_name())
+        } else {
+            None
+        };
 
         ArrayTypeInfo {
             variant_type,
@@ -765,12 +806,23 @@ impl<T: ArrayElement> Array<T> {
         if type_info.is_typed() {
             let script = Variant::nil();
 
+            // A bit contrived because empty StringName is lazy-initialized but must also remain valid.
+            #[allow(unused_assignments)]
+            let mut empty_string_name = None;
+            let class_name = if let Some(class_name) = &type_info.class_name {
+                class_name.string_sys()
+            } else {
+                empty_string_name = Some(StringName::default());
+                // as_ref() crucial here -- otherwise the StringName is dropped.
+                empty_string_name.as_ref().unwrap().string_sys()
+            };
+
             // SAFETY: The array is a newly created empty untyped array.
             unsafe {
                 interface_fn!(array_set_typed)(
                     self.sys_mut(),
                     type_info.variant_type.sys(),
-                    type_info.class_name.string_sys(),
+                    class_name, // must be empty if variant_type != OBJECT.
                     script.var_sys(),
                 );
             }
@@ -786,6 +838,19 @@ impl<T: ArrayElement> Array<T> {
     }
 }
 
+impl VariantArray {
+    /// # Safety
+    /// - Variant must have type `VariantType::ARRAY`.
+    /// - Subsequent operations on this array must not rely on the type of the array.
+    pub(crate) unsafe fn from_variant_unchecked(variant: &Variant) -> Self {
+        // See also ffi_from_variant().
+        Self::new_with_uninit(|self_ptr| {
+            let array_from_variant = sys::builtin_fn!(array_from_variant);
+            array_from_variant(self_ptr, sys::SysPtr::force_mut(variant.var_sys()));
+        })
+    }
+}
+
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // Traits
 
@@ -838,14 +903,16 @@ impl<T: ArrayElement> ToGodot for Array<T> {
 
 impl<T: ArrayElement> FromGodot for Array<T> {
     fn try_from_godot(via: Self::Via) -> Result<Self, ConvertError> {
+        T::debug_validate_elements(&via)?;
         Ok(via)
     }
 }
 
 impl<T: ArrayElement> fmt::Debug for Array<T> {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         // Going through `Variant` because there doesn't seem to be a direct way.
-        write!(f, "{:?}", self.to_variant().stringify())
+        // Reuse Display.
+        write!(f, "{}", self.to_variant().stringify())
     }
 }
 
@@ -878,17 +945,21 @@ impl<T: ArrayElement + fmt::Display> fmt::Display for Array<T> {
 impl<T: ArrayElement> Clone for Array<T> {
     fn clone(&self) -> Self {
         // SAFETY: `self` is a valid array, since we have a reference that keeps it alive.
-        let array = unsafe {
+        let copy = unsafe {
             Self::new_with_uninit(|self_ptr| {
                 let ctor = sys::builtin_fn!(array_construct_copy);
                 let args = [self.sys()];
                 ctor(self_ptr, args.as_ptr());
             })
         };
 
-        array
-            .with_checked_type()
-            .expect("copied array should have same type as original array")
+        // Double-check copy's runtime type in Debug mode.
+        if cfg!(debug_assertions) {
+            copy.with_checked_type()
+                .expect("copied array should have same type as original array")
+        } else {
+            copy
+        }
     }
 }
 
@@ -1000,6 +1071,7 @@ impl<T: ArrayElement> GodotFfiVariant for Array<T> {
     }
 
     fn ffi_from_variant(variant: &Variant) -> Result<Self, ConvertError> {
+        // First check if the variant is an array. The array conversion shouldn't be called otherwise.
         if variant.get_type() != Self::variant_type() {
             return Err(FromVariantError::BadType {
                 expected: Self::variant_type(),
@@ -1015,6 +1087,7 @@ impl<T: ArrayElement> GodotFfiVariant for Array<T> {
             })
         };
 
+        // Then, check the runtime type of the array.
         array.with_checked_type()
     }
 }

godot-core/src/builtin/collections/dictionary.rs

@@ -606,7 +606,7 @@ impl<'a> Keys<'a> {
         TypedKeys::from_untyped(self)
     }
 
-    /// Returns an array of the keys
+    /// Returns an array of the keys.
     pub fn array(self) -> VariantArray {
         // Can only be called
         assert!(self.iter.is_first);

godot-core/src/builtin/collections/packed_array.rs

@@ -548,7 +548,7 @@ macro_rules! impl_packed_array {
             fn export_hint() -> $crate::meta::PropertyHintInfo {
                 // In 4.3 Godot can (and does) use type hint strings for packed arrays, see https://github.com/godotengine/godot/pull/82952.
                 if sys::GdextBuild::since_api("4.3") {
-                    $crate::meta::PropertyHintInfo::export_array_element::<$Element>()
+                    $crate::meta::PropertyHintInfo::export_packed_array_element::<$Element>()
                 } else {
                     $crate::meta::PropertyHintInfo::type_name::<$PackedArray>()
                 }

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

@@ -5,7 +5,9 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/.
  */
 
-use crate::builtin::{GString, StringName, VariantDispatch, VariantOperator, VariantType};
+use crate::builtin::{
+    GString, StringName, VariantArray, VariantDispatch, VariantOperator, VariantType,
+};
 use crate::meta::error::ConvertError;
 use crate::meta::{ArrayElement, FromGodot, ToGodot};
 use godot_ffi as sys;
@@ -448,6 +450,16 @@ impl fmt::Display for Variant {
 
 impl fmt::Debug for Variant {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        VariantDispatch::from_variant(self).fmt(f)
+        // Special case for arrays: avoids converting to VariantArray (the only Array type in VariantDispatch), which fails
+        // for typed arrays and causes a panic. This can cause an infinite loop with Debug, or abort.
+        // Can be removed if there's ever a "possibly typed" Array type (e.g. OutArray) in the library.
+
+        if self.get_type() == VariantType::ARRAY {
+            // SAFETY: type is checked, and only operation is print (out data flow, no covariant in access).
+            let array = unsafe { VariantArray::from_variant_unchecked(self) };
+            array.fmt(f)
+        } else {
+            VariantDispatch::from_variant(self).fmt(f)
+        }
     }
 }

godot-core/src/meta/array_type_info.rs

@@ -16,17 +16,27 @@ use std::fmt;
 /// We ignore the `script` parameter because it has no impact on typing in Godot.
 #[derive(Eq, PartialEq)]
 pub(crate) struct ArrayTypeInfo {
+    /// The builtin type; always set.
     pub variant_type: VariantType,
 
+    /// If [`variant_type`] is [`VariantType::OBJECT`], then the class name; otherwise `None`.
+    ///
     /// Not a `ClassName` because some values come from Godot engine API.
-    pub class_name: StringName,
+    pub class_name: Option<StringName>,
 }
 
 impl ArrayTypeInfo {
     pub fn of<T: GodotType>() -> Self {
+        let variant_type = <T::Via as GodotType>::Ffi::variant_type();
+        let class_name = if variant_type == VariantType::OBJECT {
+            Some(T::Via::class_name().to_string_name())
+        } else {
+            None
+        };
+
         Self {
-            variant_type: <T::Via as GodotType>::Ffi::variant_type(),
-            class_name: T::Via::class_name().to_string_name(),
+            variant_type,
+            class_name,
         }
     }
 
@@ -38,18 +48,17 @@ impl ArrayTypeInfo {
         self.variant_type
     }
 
-    pub fn class_name(&self) -> &StringName {
-        &self.class_name
+    pub fn class_name(&self) -> Option<&StringName> {
+        self.class_name.as_ref()
     }
 }
 
 impl fmt::Debug for ArrayTypeInfo {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        let class = self.class_name.to_string();
-        let class_str = if class.is_empty() {
-            String::new()
-        } else {
+        let class_str = if let Some(class) = &self.class_name {
             format!(" (class={class})")
+        } else {
+            String::new()
         };
 
         write!(f, "{:?}{}", self.variant_type, class_str)

godot-core/src/meta/error/convert_error.rs

@@ -169,12 +169,19 @@ impl fmt::Display for ErrorKind {
 /// Conversion failed during a [`FromGodot`](crate::meta::FromGodot) call.
 #[derive(Eq, PartialEq, Debug)]
 pub(crate) enum FromGodotError {
+    /// Destination `Array<T>` has different type than source's runtime type.
     BadArrayType {
         expected: ArrayTypeInfo,
         actual: ArrayTypeInfo,
     },
+
+    /// Special case of `BadArrayType` where a custom int type such as `i8` cannot hold a dynamic `i64` value.
+    BadArrayTypeInt { expected: ArrayTypeInfo, value: i64 },
+
     /// InvalidEnum is also used by bitfields.
     InvalidEnum,
+
+    /// `InstanceId` cannot be 0.
     ZeroInstanceId,
 }
 
@@ -208,17 +215,22 @@ impl fmt::Display for FromGodotError {
                     };
                 }
 
+                let exp_class = expected.class_name().expect("lhs class name present");
+                let act_class = actual.class_name().expect("rhs class name present");
                 assert_ne!(
-                    expected.class_name(),
-                    actual.class_name(),
+                    exp_class, act_class,
                     "BadArrayType with expected == got, this is a gdext bug"
                 );
 
                 write!(
                     f,
-                    "expected array of class {}, got array of class {}",
-                    expected.class_name(),
-                    actual.class_name()
+                    "expected array of class {exp_class}, got array of class {act_class}"
+                )
+            }
+            Self::BadArrayTypeInt { expected, value } => {
+                write!(
+                    f,
+                    "integer value {value} does not fit into Array of type {expected:?}"
                 )
             }
             Self::InvalidEnum => write!(f, "invalid engine enum value"),