Codegen for Array<T> - required to properly initialize and cache Array runtime type.

-----

Methods such as `duplicate_...` were initializing and caching typed `VariantArray` as a return value for outgoing calls. We fix it by initializing proper `Array<T>` in the first place.

Author: Yarwin

godot-codegen/src/generator/builtins.rs

@@ -197,14 +197,27 @@ fn make_special_builtin_methods(class_name: &TyName, _ctx: &Context) -> TokenStr
 
 /// Get the safety docs of an unsafe method, or `None` if it is safe.
 fn method_safety_doc(class_name: &TyName, method: &BuiltinMethod) -> Option<TokenStream> {
-    if class_name.godot_ty == "Array"
-        && &method.return_value().type_tokens().to_string() == "VariantArray"
-    {
-        return Some(quote! {
-            /// # Safety
-            ///
-            /// You must ensure that the returned array fulfils the safety invariants of [`Array`](crate::builtin::Array).
-        });
+    if class_name.godot_ty == "Array" {
+        if method.is_generic() {
+            return Some(quote! {
+               /// # Safety
+               /// You must ensure that the returned array fulfils the safety invariants of [`Array`](crate::builtin::Array), this being:
+               /// - Any values written to the array must match the runtime type of the array.
+               /// - Any values read from the array must be convertible to the type `T`.
+               ///
+               /// If the safety invariant of `Array` is intact, which it must be for any publicly accessible arrays, then `T` must match
+               /// the runtime type of the array. This then implies that both of the conditions above hold. This means that you only need
+               /// to keep the above conditions in mind if you are intentionally violating the safety invariant of `Array`.
+               ///
+               /// In the current implementation, both cases will produce a panic rather than undefined behavior, but this should not be relied upon.
+            });
+        } else if &method.return_value().type_tokens().to_string() == "VariantArray" {
+            return Some(quote! {
+                /// # Safety
+                ///
+                /// You must ensure that the returned array fulfils the safety invariants of [`Array`](crate::builtin::Array).
+            });
+        }
     }
 
     None
@@ -252,11 +265,13 @@ fn make_builtin_method_definition(
     let receiver = functions_common::make_receiver(method.qualifier(), ffi_arg_in);
     let object_ptr = &receiver.ffi_arg;
 
+    let maybe_generic_params = method.return_value().generic_params();
+
     let ptrcall_invocation = quote! {
         let method_bind = sys::builtin_method_table().#fptr_access;
 
 
-        Signature::<CallParams, CallRet>::out_builtin_ptrcall(
+        Signature::<CallParams, CallRet #maybe_generic_params>::out_builtin_ptrcall(
             method_bind,
             #builtin_name_str,
             #method_name_str,

godot-codegen/src/generator/default_parameters.rs

@@ -11,7 +11,7 @@ use quote::{format_ident, quote};
 
 use crate::generator::functions_common;
 use crate::generator::functions_common::{
-    make_arg_expr, make_param_or_field_type, FnArgExpr, FnCode, FnKind, FnParamDecl, FnParamTokens,
+    make_arg_expr, make_param_or_field_type, FnArgExpr, FnCode, FnKind, FnParamDecl,
 };
 use crate::models::domain::{FnParam, FnQualifier, Function, RustTy, TyName};
 use crate::util::{ident, safe_ident};

godot-codegen/src/generator/functions_common.rs

@@ -142,7 +142,6 @@ pub fn make_function_definition(
         callsig_param_types: param_types,
         callsig_lifetime_args,
         arg_exprs: arg_names,
-        func_general_lifetime: fn_lifetime,
     } = if sig.is_virtual() {
         make_params_exprs_virtual(sig.params().iter(), sig)
     } else {
@@ -175,11 +174,14 @@ pub fn make_function_definition(
         default_structs_code = TokenStream::new();
     };
 
+    let maybe_func_generic_params = sig.return_value().generic_params();
+    let maybe_func_generic_bounds = sig.return_value().where_clause();
+
     let call_sig_decl = {
         let return_ty = &sig.return_value().type_tokens();
 
         quote! {
-            type CallRet = #return_ty;
+            type CallRet #maybe_func_generic_params = #return_ty;
             type CallParams #callsig_lifetime_args = (#(#param_types,)*);
         }
     };
@@ -279,10 +281,12 @@ pub fn make_function_definition(
 
         quote! {
             #maybe_safety_doc
-            #vis #maybe_unsafe fn #primary_fn_name #fn_lifetime (
+            #vis #maybe_unsafe fn #primary_fn_name #maybe_func_generic_params (
                 #receiver_param
                 #( #params, )*
-            ) #return_decl {
+            ) #return_decl
+            #maybe_func_generic_bounds
+            {
                 #call_sig_decl
 
                 let args = (#( #arg_names, )*);
@@ -489,6 +493,7 @@ pub(crate) fn make_param_or_field_type(
             ..
         }
         | RustTy::BuiltinArray { .. }
+        | RustTy::GenericArray
         | RustTy::EngineArray { .. } => {
             let lft = lifetimes.next();
             special_ty = Some(quote! { RefArg<#lft, #ty> });

godot-codegen/src/models/domain.rs

@@ -300,28 +300,40 @@ pub trait Function: fmt::Display {
     fn name(&self) -> &str {
         &self.common().name
     }
+
     /// Rust name as `Ident`. Might be cached in future.
     fn name_ident(&self) -> Ident {
         safe_ident(self.name())
     }
+
     fn godot_name(&self) -> &str {
         &self.common().godot_name
     }
+
     fn params(&self) -> &[FnParam] {
         &self.common().parameters
     }
+
     fn return_value(&self) -> &FnReturn {
         &self.common().return_value
     }
+
     fn is_vararg(&self) -> bool {
         self.common().is_vararg
     }
+
     fn is_private(&self) -> bool {
         self.common().is_private
     }
+
     fn is_virtual(&self) -> bool {
         matches!(self.direction(), FnDirection::Virtual { .. })
     }
+
+    fn is_generic(&self) -> bool {
+        matches!(self.return_value().type_, Some(RustTy::GenericArray))
+    }
+
     fn direction(&self) -> FnDirection {
         self.common().direction
     }
@@ -621,6 +633,13 @@ impl FnReturn {
         Self::with_enum_replacements(return_value, &[], ctx)
     }
 
+    pub fn with_generic_builtin(generic_type: RustTy) -> Self {
+        Self {
+            decl: generic_type.return_decl(),
+            type_: Some(generic_type),
+        }
+    }
+
     pub fn with_enum_replacements(
         return_value: &Option<JsonMethodReturn>,
         replacements: EnumReplacements,
@@ -669,6 +688,14 @@ impl FnReturn {
         }
     }
 
+    pub fn generic_params(&self) -> Option<TokenStream> {
+        self.type_.as_ref()?.generic_params()
+    }
+
+    pub fn where_clause(&self) -> Option<TokenStream> {
+        self.type_.as_ref()?.where_clause()
+    }
+
     pub fn call_result_decl(&self) -> TokenStream {
         let ret = self.type_tokens();
         quote! { -> Result<#ret, crate::meta::error::CallError> }
@@ -705,6 +732,11 @@ pub enum RustTy {
     /// Note that untyped arrays are mapped as `BuiltinIdent("Array")`.
     BuiltinArray { elem_type: TokenStream },
 
+    /// Will be included as `Array<T>` in the generated source.
+    ///
+    /// Set by [`builtin_method_generic_ret`](crate::special_cases::builtin_method_generic_ret)
+    GenericArray,
+
     /// C-style raw pointer to a `RustTy`.
     RawPointer { inner: Box<RustTy>, is_const: bool },
 
@@ -755,10 +787,30 @@ impl RustTy {
     pub fn return_decl(&self) -> TokenStream {
         match self {
             Self::EngineClass { tokens, .. } => quote! { -> Option<#tokens> },
+            Self::GenericArray => quote! { -> Array<Ret> },
             other => quote! { -> #other },
         }
     }
 
+    pub fn generic_params(&self) -> Option<TokenStream> {
+        if matches!(self, Self::GenericArray) {
+            Some(quote! { < Ret > })
+        } else {
+            None
+        }
+    }
+
+    pub fn where_clause(&self) -> Option<TokenStream> {
+        if matches!(self, Self::GenericArray) {
+            Some(quote! {
+                where
+                    Ret: crate::meta::ArrayElement,
+            })
+        } else {
+            None
+        }
+    }
+
     pub fn is_integer(&self) -> bool {
         let RustTy::BuiltinIdent { ty, .. } = self else {
             return false;
@@ -789,6 +841,7 @@ impl ToTokens for RustTy {
             RustTy::EngineEnum { tokens: path, .. } => path.to_tokens(tokens),
             RustTy::EngineClass { tokens: path, .. } => path.to_tokens(tokens),
             RustTy::ExtenderReceiver { tokens: path } => path.to_tokens(tokens),
+            RustTy::GenericArray => quote! { Array<Ret> }.to_tokens(tokens),
         }
     }
 }

godot-codegen/src/models/domain_mapping.rs

@@ -366,10 +366,17 @@ impl BuiltinMethod {
             return None;
         }
 
-        let return_value = method
-            .return_type
-            .as_deref()
-            .map(JsonMethodReturn::from_type_no_meta);
+        let return_value = if let Some(generic) =
+            special_cases::builtin_method_generic_ret(builtin_name, method)
+        {
+            generic
+        } else {
+            let return_value = &method
+                .return_type
+                .as_deref()
+                .map(JsonMethodReturn::from_type_no_meta);
+            FnReturn::new(return_value, ctx)
+        };
 
         Some(Self {
             common: FunctionCommon {
@@ -381,7 +388,7 @@ impl BuiltinMethod {
                 parameters: FnParam::builder()
                     .no_defaults()
                     .build_many(&method.arguments, ctx),
-                return_value: FnReturn::new(&return_value, ctx),
+                return_value,
                 is_vararg: method.is_vararg,
                 is_private: false, // See 'exposed' below. Could be special_cases::is_method_private(builtin_name, &method.name),
                 is_virtual_required: false,

godot-codegen/src/special_cases/codegen_special_cases.rs

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

godot-codegen/src/special_cases/special_cases.rs

@@ -33,7 +33,7 @@ use proc_macro2::Ident;
 
 use crate::conv::to_enum_type_uncached;
 use crate::models::domain::{
-    ClassCodegenLevel, Enum, EnumReplacements, RustTy, TyName, VirtualMethodPresence,
+    ClassCodegenLevel, Enum, EnumReplacements, FnReturn, RustTy, TyName, VirtualMethodPresence,
 };
 use crate::models::json::{JsonBuiltinMethod, JsonClassMethod, JsonSignal, JsonUtilityFunction};
 use crate::special_cases::codegen_special_cases;
@@ -778,6 +778,24 @@ pub fn is_builtin_method_deleted(_class_name: &TyName, method: &JsonBuiltinMetho
     codegen_special_cases::is_builtin_method_excluded(method)
 }
 
+/// Returns some generic type – such as `GenericArray` representing `Array<T>` – if method is marked as generic, `None` otherwise.
+///
+/// Usually required to initialize the return value and cache its type (see also https://github.com/godot-rust/gdext/pull/1357).
+pub fn builtin_method_generic_ret(
+    class_name: &TyName,
+    method: &JsonBuiltinMethod,
+) -> Option<FnReturn> {
+    match (
+        class_name.rust_ty.to_string().as_str(),
+        method.name.as_str(),
+    ) {
+        ("Array", "duplicate") | ("Array", "slice") => {
+            Some(FnReturn::with_generic_builtin(RustTy::GenericArray))
+        }
+        _ => None,
+    }
+}
+
 /// True if signal is absent from codegen (only when surrounding class is excluded).
 pub fn is_signal_deleted(_class_name: &TyName, signal: &JsonSignal) -> bool {
     // If any argument type (a class) is excluded.

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

@@ -533,12 +533,11 @@ impl<T: ArrayElement> Array<T> {
     /// To create a deep copy, use [`duplicate_deep()`][Self::duplicate_deep] instead.
     /// To create a new reference to the same array data, use [`clone()`][Clone::clone].
     pub fn duplicate_shallow(&self) -> Self {
-        // SAFETY: We never write to the duplicated array, and all values read are read as `Variant`.
-        let duplicate: VariantArray = unsafe { self.as_inner().duplicate(false) };
+        // SAFETY: duplicate() returns a typed array with the same type as Self, and all values are taken from `self` so have the right type
+        let duplicate: Self = unsafe { self.as_inner().duplicate(false) };
 
-        // SAFETY: duplicate() returns a typed array with the same type as Self, and all values are taken from `self` so have the right type.
-        let result = unsafe { duplicate.assume_type() };
-        result.with_cache(self)
+        // Note: cache is being set while initializing the duplicate as a return value for above call.
+        duplicate
     }
 
     /// Returns a deep copy of the array. All nested arrays and dictionaries are duplicated and
@@ -548,12 +547,11 @@ impl<T: ArrayElement> Array<T> {
     /// To create a shallow copy, use [`duplicate_shallow()`][Self::duplicate_shallow] instead.
     /// To create a new reference to the same array data, use [`clone()`][Clone::clone].
     pub fn duplicate_deep(&self) -> Self {
-        // SAFETY: We never write to the duplicated array, and all values read are read as `Variant`.
-        let duplicate: VariantArray = unsafe { self.as_inner().duplicate(true) };
+        // SAFETY: duplicate() returns a typed array with the same type as Self, and all values are taken from `self` so have the right type
+        let duplicate: Self = unsafe { self.as_inner().duplicate(true) };
 
-        // SAFETY: duplicate() returns a typed array with the same type as Self, and all values are taken from `self` so have the right type.
-        let result = unsafe { duplicate.assume_type() };
-        result.with_cache(self)
+        // Note: cache is being set while initializing the duplicate as a return value for above call.
+        duplicate
     }
 
     /// Returns a sub-range `begin..end` as a new `Array`.
@@ -588,13 +586,10 @@ impl<T: ArrayElement> Array<T> {
         let (begin, end) = range.signed();
         let end = end.unwrap_or(i32::MAX as i64);
 
-        // SAFETY: The type of the array is `T` and we convert the returned array to an `Array<T>` immediately.
-        let subarray: VariantArray =
-            unsafe { self.as_inner().slice(begin, end, step as i64, deep) };
+        // SAFETY: slice() returns a typed array with the same type as Self, and all values are taken from `self` so have the right type.
+        let subarray: Self = unsafe { self.as_inner().slice(begin, end, step as i64, deep) };
 
-        // SAFETY: slice() returns a typed array with the same type as Self.
-        let result = unsafe { subarray.assume_type() };
-        result.with_cache(self)
+        subarray
     }
 
     /// Returns an iterator over the elements of the `Array`. Note that this takes the array
@@ -950,8 +945,7 @@ impl<T: ArrayElement> Array<T> {
         }
     }
 
-    /// 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, and for API functions
+    /// Changes the generic type on this array, without changing its contents. Needed for API functions
     /// that take a variant array even though we want to pass a typed one.
     ///
     /// # Safety
@@ -966,13 +960,6 @@ 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.
-    unsafe fn assume_type<U: ArrayElement>(self) -> Array<U> {
-        // 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)