From 8f80e55f6b9c03be1e901c9536dd772a1c53d5ef Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Mon, 16 Feb 2026 23:07:02 +0100 Subject: [PATCH] `InnerArray`: remove generic + unsafe methods in favor of `AnyArray` --- godot-codegen/src/generator/builtins.rs | 35 +------- godot-codegen/src/generator/classes.rs | 1 - .../src/generator/functions_common.rs | 33 +++----- .../src/generator/utility_functions.rs | 1 - godot-codegen/src/generator/virtual_traits.rs | 1 - godot-codegen/src/models/domain.rs | 49 +----------- godot-codegen/src/models/domain_mapping.rs | 36 +++++---- .../special_cases/codegen_special_cases.rs | 1 - .../src/special_cases/special_cases.rs | 23 +----- .../src/builtin/collections/any_array.rs | 48 ++++++++--- .../src/builtin/collections/any_dictionary.rs | 50 ++++++++---- godot-core/src/builtin/collections/array.rs | 67 ++++++++++------ .../collections/array_functional_ops.rs | 6 +- .../src/builtin/collections/dictionary.rs | 80 +++++++++---------- .../builtin_tests/containers/array_test.rs | 39 +++++++-- .../containers/dictionary_test.rs | 36 ++++++--- 16 files changed, 248 insertions(+), 258 deletions(-) diff --git a/godot-codegen/src/generator/builtins.rs b/godot-codegen/src/generator/builtins.rs index a06b0acf0..e6aa04b56 100644 --- a/godot-codegen/src/generator/builtins.rs +++ b/godot-codegen/src/generator/builtins.rs @@ -232,34 +232,6 @@ 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 { - 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() == "VarArray" { - return Some(quote! { - /// # Safety - /// - /// You must ensure that the returned array fulfils the safety invariants of [`Array`](crate::builtin::Array). - }); - } - } - - None -} - fn make_builtin_method_definition( builtin_class: &BuiltinClass, variant_shout_name: &Ident, @@ -302,13 +274,11 @@ 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::::out_builtin_ptrcall( + Signature::::out_builtin_ptrcall( method_bind, #builtin_name_str, #method_name_str, @@ -330,8 +300,6 @@ fn make_builtin_method_definition( ) }; - let safety_doc = method_safety_doc(builtin_class.name(), method); - functions_common::make_function_definition( method, &FnCode { @@ -341,7 +309,6 @@ fn make_builtin_method_definition( is_virtual_required: false, is_varcall_fallible: false, }, - safety_doc, &TokenStream::new(), ) } diff --git a/godot-codegen/src/generator/classes.rs b/godot-codegen/src/generator/classes.rs index 4d15c200a..96994299c 100644 --- a/godot-codegen/src/generator/classes.rs +++ b/godot-codegen/src/generator/classes.rs @@ -635,7 +635,6 @@ fn make_class_method_definition( is_virtual_required: false, is_varcall_fallible: true, }, - None, cfg_attributes, ) } diff --git a/godot-codegen/src/generator/functions_common.rs b/godot-codegen/src/generator/functions_common.rs index dd191fecd..83b8681d3 100644 --- a/godot-codegen/src/generator/functions_common.rs +++ b/godot-codegen/src/generator/functions_common.rs @@ -103,7 +103,6 @@ pub struct FnParamTokens { pub fn make_function_definition( sig: &dyn Function, code: &FnCode, - safety_doc: Option, cfg_attributes: &TokenStream, ) -> FnDefinition { let has_default_params = default_parameters::function_uses_default_params(sig); @@ -120,20 +119,18 @@ pub fn make_function_definition( // to only use `unsafe` for pointers in parameters (for outbound calls), and in return values (for virtual calls). Or technically more // correct, make the entire trait unsafe as soon as one function can return pointers, but that's very unergonomic and non-local. // Thus, let's keep things simple and more conservative. - let (maybe_unsafe, maybe_safety_doc) = if let Some(safety_doc) = safety_doc { - (quote! { unsafe }, safety_doc) - } else if sig.common().is_unsafe { - ( - quote! { unsafe }, - quote! { - /// # Safety - /// - /// This method has automatically been marked `unsafe` because it accepts raw pointers as parameters. - /// If Godot does not document any safety requirements, make sure you understand the underlying semantics. - }, - ) + let (maybe_unsafe, maybe_safety_doc); + if sig.common().is_unsafe { + maybe_unsafe = quote! { unsafe }; + maybe_safety_doc = quote! { + /// # Safety + /// + /// This method has automatically been marked `unsafe` because it accepts raw pointers as parameters. + /// If Godot does not document any safety requirements, make sure you understand the underlying semantics. + }; } else { - (TokenStream::new(), TokenStream::new()) + maybe_unsafe = TokenStream::new(); + maybe_safety_doc = TokenStream::new(); }; let FnParamTokens { @@ -173,15 +170,13 @@ 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 (maybe_deprecated, _maybe_expect_deprecated) = make_deprecation_attribute(sig); let call_sig_decl = { let return_ty = &sig.return_value().type_tokens(); quote! { - type CallRet #maybe_func_generic_params = #return_ty; + type CallRet = #return_ty; type CallParams #callsig_lifetime_args = (#(#param_types,)*); } }; @@ -286,11 +281,10 @@ pub fn make_function_definition( quote! { #maybe_deprecated #maybe_safety_doc - #vis #maybe_unsafe fn #primary_fn_name #maybe_func_generic_params ( + #vis #maybe_unsafe fn #primary_fn_name ( #receiver_param #( #params, )* ) #return_decl - #maybe_func_generic_bounds { #call_sig_decl @@ -510,7 +504,6 @@ 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> }); diff --git a/godot-codegen/src/generator/utility_functions.rs b/godot-codegen/src/generator/utility_functions.rs index 5d453cc7e..cc1717934 100644 --- a/godot-codegen/src/generator/utility_functions.rs +++ b/godot-codegen/src/generator/utility_functions.rs @@ -75,7 +75,6 @@ pub(crate) fn make_utility_function_definition(function: &UtilityFunction) -> To is_virtual_required: false, is_varcall_fallible: false, }, - None, &TokenStream::new(), ); diff --git a/godot-codegen/src/generator/virtual_traits.rs b/godot-codegen/src/generator/virtual_traits.rs index 1014164cf..8f76a02dc 100644 --- a/godot-codegen/src/generator/virtual_traits.rs +++ b/godot-codegen/src/generator/virtual_traits.rs @@ -204,7 +204,6 @@ fn make_virtual_method( is_virtual_required, is_varcall_fallible: true, }, - None, &TokenStream::new(), ); diff --git a/godot-codegen/src/models/domain.rs b/godot-codegen/src/models/domain.rs index c45b605d3..bf575f73b 100644 --- a/godot-codegen/src/models/domain.rs +++ b/godot-codegen/src/models/domain.rs @@ -332,10 +332,6 @@ pub trait Function: fmt::Display { 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 } @@ -681,13 +677,6 @@ impl FnReturn { Self::with_enum_replacements(return_value, &[], flow, 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, replacements: EnumReplacements, @@ -730,14 +719,6 @@ impl FnReturn { } } - pub fn generic_params(&self) -> Option { - self.type_.as_ref()?.generic_params() - } - - pub fn where_clause(&self) -> Option { - 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> } @@ -781,11 +762,6 @@ pub enum RustTy { /// Untyped arrays are either `BuiltinIdent("AnyArray")` for outbound methods, or `BuiltinIdent("Array")` for virtual methods. BuiltinArray { elem_type: TokenStream }, - /// Will be included as `Array` 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, is_const: bool }, @@ -840,10 +816,7 @@ impl RustTy { } pub fn return_decl(&self) -> TokenStream { - match self { - Self::GenericArray => quote! { -> Array }, - _ => quote! { -> #self }, - } + quote! { -> #self } } /// Returns tokens without `Option` wrapper, even for nullable engine classes. @@ -857,25 +830,6 @@ impl RustTy { } } - pub fn generic_params(&self) -> Option { - if matches!(self, Self::GenericArray) { - Some(quote! { < Ret > }) - } else { - None - } - } - - pub fn where_clause(&self) -> Option { - 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; @@ -924,7 +878,6 @@ impl ToTokens for RustTy { } } RustTy::ExtenderReceiver { tokens: path } => path.to_tokens(tokens), - RustTy::GenericArray => quote! { Array }.to_tokens(tokens), RustTy::SysPointerType { tokens: path } => path.to_tokens(tokens), } } diff --git a/godot-codegen/src/models/domain_mapping.rs b/godot-codegen/src/models/domain_mapping.rs index e3867aae1..bdca63ab1 100644 --- a/godot-codegen/src/models/domain_mapping.rs +++ b/godot-codegen/src/models/domain_mapping.rs @@ -367,17 +367,28 @@ impl BuiltinMethod { return None; } - let return_value = match special_cases::builtin_method_generic_ret(builtin_name, method) { - Some(generic) => generic, - _ => { - let return_value = &method - .return_type - .as_deref() - .map(JsonMethodReturn::from_type_no_meta); - - // Builtin methods are always outbound (not virtual), thus flow for return type is Godot -> Rust. - FnReturn::new(return_value, FlowDirection::GodotToRust, ctx) - } + let is_exposed_in_outer = + special_cases::is_builtin_method_exposed(builtin_name, &method.name); + + let return_value = { + let return_value = &method + .return_type + .as_deref() + .map(JsonMethodReturn::from_type_no_meta); + + // Builtin methods are always outbound (not virtual), thus flow for return type is Godot -> Rust. + // Exception: Inner{Array,Dictionary} methods return Any{Array,Dictionary} instead of Var{Array,Dictionary}. Reason is that + // arrays/dicts can be generic and store type info, thus typing returned collections differently. Thus use RustToGodot flow. + let flow = if !is_exposed_in_outer + && matches!(builtin_name.godot_ty.as_str(), "Array" | "Dictionary") + && matches!(method.return_type.as_deref(), Some("Array" | "Dictionary")) + { + FlowDirection::RustToGodot // AnyArray + AnyDictionary. + } else { + FlowDirection::GodotToRust + }; + + FnReturn::new(return_value, flow, ctx) }; // For parameters in builtin methods, flow is always Rust -> Godot. @@ -385,9 +396,6 @@ impl BuiltinMethod { let parameters = FnParam::builder().build_many(&method.arguments, FlowDirection::RustToGodot, ctx); - let is_exposed_in_outer = - special_cases::is_builtin_method_exposed(builtin_name, &method.name); - // Construct surrounding_class with correct type names: // * godot_ty: Always the real Godot type (e.g. "String"). // * rust_ty: Rust struct where the method is declared ("GString" for exposed, "InnerString" for private one). diff --git a/godot-codegen/src/special_cases/codegen_special_cases.rs b/godot-codegen/src/special_cases/codegen_special_cases.rs index a97fd4fa3..8a810814e 100644 --- a/godot-codegen/src/special_cases/codegen_special_cases.rs +++ b/godot-codegen/src/special_cases/codegen_special_cases.rs @@ -48,7 +48,6 @@ 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::SysPointerType { .. } => true, RustTy::EngineArray { elem_class, .. } => is_class_excluded(elem_class.as_str()), diff --git a/godot-codegen/src/special_cases/special_cases.rs b/godot-codegen/src/special_cases/special_cases.rs index 7f76c3f40..c2a45d547 100644 --- a/godot-codegen/src/special_cases/special_cases.rs +++ b/godot-codegen/src/special_cases/special_cases.rs @@ -34,7 +34,7 @@ use proc_macro2::Ident; use crate::Context; use crate::conv::to_enum_type_uncached; use crate::models::domain::{ - ClassCodegenLevel, Enum, EnumReplacements, FnReturn, RustTy, TyName, VirtualMethodPresence, + ClassCodegenLevel, Enum, EnumReplacements, RustTy, TyName, VirtualMethodPresence, }; use crate::models::json::{JsonBuiltinMethod, JsonClassMethod, JsonSignal, JsonUtilityFunction}; use crate::special_cases::codegen_special_cases; @@ -823,27 +823,6 @@ 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` – 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). -#[rustfmt::skip] -pub fn builtin_method_generic_ret( - class_name: &TyName, - method: &JsonBuiltinMethod, -) -> Option { - match ( - class_name.rust_ty.to_string().as_str(), - method.name.as_str(), - ) { - | ("Array", "duplicate") - | ("Array", "slice") - | ("Array", "filter") - - => 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. diff --git a/godot-core/src/builtin/collections/any_array.rs b/godot-core/src/builtin/collections/any_array.rs index 45cd92896..54ef08873 100644 --- a/godot-core/src/builtin/collections/any_array.rs +++ b/godot-core/src/builtin/collections/any_array.rs @@ -209,17 +209,17 @@ impl AnyArray { /// 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) -> AnyArray { - self.array.duplicate_shallow().upcast_any_array() + self.array.as_inner().duplicate(false) } /// Returns a deep copy, duplicating nested `Array`/`Dictionary` elements but keeping `Object` elements shared. /// /// This operation retains the dynamic [element type][Self::element_type]: copying `Array` will yield another `Array`. /// - /// To create a shallow copy, use [`duplicate_shallow()`][Self::duplicate_shallow] instead. + /// 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_deep(&self) -> Self { - self.array.duplicate_deep().upcast_any_array() + self.array.as_inner().duplicate(true) } /// Returns a sub-range as a new array. @@ -231,8 +231,7 @@ impl AnyArray { /// _Godot equivalent: `slice`_ #[doc(alias = "slice")] pub fn subarray_shallow(&self, range: impl meta::SignedRange, step: Option) -> Self { - let sliced = self.array.subarray_shallow(range, step); - sliced.upcast_any_array() + self.subarray_impl(range, step, false) } /// Returns a sub-range as a new `Array`. @@ -244,8 +243,16 @@ impl AnyArray { /// _Godot equivalent: `slice` with `deep: true`_ #[doc(alias = "slice")] pub fn subarray_deep(&self, range: impl meta::SignedRange, step: Option) -> Self { - let sliced = self.array.subarray_deep(range, step); - sliced.upcast_any_array() + self.subarray_impl(range, step, true) + } + + fn subarray_impl(&self, range: impl meta::SignedRange, step: Option, deep: bool) -> Self { + let step = step.unwrap_or(1); + assert_ne!(step, 0, "subarray: step cannot be zero"); + let (begin, end) = range.signed(); + let end = end.unwrap_or(i32::MAX as i64); + + self.array.as_inner().slice(begin, end, step as i64, deep) } /// Returns an non-exclusive iterator over the elements of the `Array`. @@ -436,7 +443,23 @@ impl AnyArray { self.try_cast_array::() } - // If we add direct-conversion methods that panic, we can use meta::element_godot_type_name::() to mention type in case of mismatch. + /// Converts to `Array` if the runtime type matches, panics otherwise. + /// + /// This is a convenience method that panics with a descriptive message if the cast fails. + /// Use [`try_cast_array()`][Self::try_cast_array] for a non-panicking version. + /// + /// # Panics + /// If the array's dynamic element type does not match `T`. + pub(crate) fn cast_array(self) -> Array { + let from_type = self.element_type(); + self.try_cast_array::().unwrap_or_else(|_| { + panic!( + "cast_array_or_panic: expected element type {:?}, got {:?}", + ElementType::of::(), + from_type, + ) + }) + } } // ---------------------------------------------------------------------------------------------------------------------------------------------- @@ -449,11 +472,12 @@ impl AnyArray { unsafe impl GodotFfi for AnyArray { const VARIANT_TYPE: sys::ExtVariantType = sys::ExtVariantType::Concrete(VariantType::ARRAY); - // No Default trait, thus manually defining this and ffi_methods!. + // Constructs a valid Godot array as ptrcall destination, without caching the element type. + // See Array::new_with_init for rationale. unsafe fn new_with_init(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self { - let mut result = Self::new_untyped(); - init_fn(result.sys_mut()); - result + let array = VarArray::new_uncached_type(init_fn); + + Self { array } } // Manually forwarding these, since no Opaque. diff --git a/godot-core/src/builtin/collections/any_dictionary.rs b/godot-core/src/builtin/collections/any_dictionary.rs index 2c0e6d0c7..66ceef46b 100644 --- a/godot-core/src/builtin/collections/any_dictionary.rs +++ b/godot-core/src/builtin/collections/any_dictionary.rs @@ -49,7 +49,7 @@ use crate::registry::property::SimpleVar; #[derive(PartialEq)] #[repr(transparent)] // Guarantees same layout as VarDictionary, enabling Deref from Dictionary (K/V have no influence on layout). pub struct AnyDictionary { - dict: VarDictionary, + pub(crate) dict: VarDictionary, } impl AnyDictionary { @@ -62,13 +62,6 @@ impl AnyDictionary { Self { dict: inner } } - /// Creates an empty untyped `AnyDictionary`. - pub(crate) fn new_untyped() -> Self { - Self { - dict: VarDictionary::default(), - } - } - fn from_opaque(opaque: sys::types::OpaqueDictionary) -> Self { Self { dict: VarDictionary::from_opaque(opaque), @@ -171,7 +164,7 @@ impl AnyDictionary { pub fn keys_array(&self) -> AnyArray { // Array can still be typed; so AnyArray is the only sound return type. // Do not use dict.keys_array() which assumes Variant typing. - self.dict.as_inner().keys().upcast_any_array() + self.dict.as_inner().keys() } /// Creates a new `Array` containing all the values currently in the dictionary. @@ -181,7 +174,7 @@ impl AnyDictionary { pub fn values_array(&self) -> AnyArray { // Array can still be typed; so AnyArray is the only sound return type. // Do not use dict.values_array() which assumes Variant typing. - self.dict.as_inner().values().upcast_any_array() + self.dict.as_inner().values() } /// Returns a shallow copy, sharing reference types (`Array`, `Dictionary`, `Object`...) with the original dictionary. @@ -191,7 +184,10 @@ impl AnyDictionary { /// To create a deep copy, use [`duplicate_deep()`][Self::duplicate_deep] instead. /// To create a new reference to the same dictionary data, use [`clone()`][Clone::clone]. pub fn duplicate_shallow(&self) -> AnyDictionary { - self.dict.duplicate_shallow().upcast_any_dictionary() + self.dict + .as_inner() + .duplicate(false) + .upcast_any_dictionary() } /// Returns a deep copy, duplicating nested `Array`/`Dictionary` elements but keeping `Object` elements shared. @@ -201,7 +197,7 @@ impl AnyDictionary { /// To create a shallow copy, use [`duplicate_shallow()`][Self::duplicate_shallow] instead. /// To create a new reference to the same dictionary data, use [`clone()`][Clone::clone]. pub fn duplicate_deep(&self) -> Self { - self.dict.duplicate_deep().upcast_any_dictionary() + self.dict.as_inner().duplicate(true).upcast_any_dictionary() } /// Returns an iterator over the key-value pairs of the `Dictionary`. @@ -304,6 +300,27 @@ impl AnyDictionary { pub fn try_cast_var_dictionary(self) -> Result { self.try_cast_dictionary::() } + + /// Converts to `Dictionary` if the runtime types match, panics otherwise. + /// + /// This is a convenience method that panics with a descriptive message if the cast fails. + /// Use [`try_cast_dictionary()`][Self::try_cast_dictionary] for a non-panicking version. + /// + /// # Panics + /// If the dictionary's dynamic key or value types do not match `K` and `V`. + pub fn cast_dictionary(self) -> Dictionary { + let from_key = self.key_element_type(); + let from_value = self.value_element_type(); + self.try_cast_dictionary::().unwrap_or_else(|_| { + panic!( + "cast_dictionary_or_panic: expected key type {:?} and value type {:?}, got {:?} and {:?}", + ElementType::of::(), + ElementType::of::(), + from_key, + from_value, + ) + }) + } } // ---------------------------------------------------------------------------------------------------------------------------------------------- @@ -317,11 +334,12 @@ unsafe impl GodotFfi for AnyDictionary { const VARIANT_TYPE: sys::ExtVariantType = sys::ExtVariantType::Concrete(VariantType::DICTIONARY); - // No Default trait, thus manually defining this and ffi_methods!. + // Constructs a valid Godot dictionary as ptrcall destination, without caching element types. + // See Array::new_with_init for rationale. unsafe fn new_with_init(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self { - let mut result = Self::new_untyped(); - init_fn(result.sys_mut()); - result + let dict = VarDictionary::new_uncached_type(init_fn); + + Self { dict } } // Manually forwarding these, since no Opaque. diff --git a/godot-core/src/builtin/collections/array.rs b/godot-core/src/builtin/collections/array.rs index a3361f753..51a4db9d6 100644 --- a/godot-core/src/builtin/collections/array.rs +++ b/godot-core/src/builtin/collections/array.rs @@ -500,11 +500,8 @@ impl Array { /// 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: 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) }; - - // Note: cache is being set while initializing the duplicate as a return value for above call. - duplicate + // duplicate() returns a typed array with the same type as Self, and all values are taken from `self` so have the right type. + self.as_inner().duplicate(false).cast_array::() } /// Returns a deep copy, duplicating nested `Array`/`Dictionary` elements but keeping `Object` elements shared. @@ -512,11 +509,8 @@ impl Array { /// 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: 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) }; - - // Note: cache is being set while initializing the duplicate as a return value for above call. - duplicate + // duplicate() returns a typed array with the same type as Self, and all values are taken from `self` so have the right type. + self.as_inner().duplicate(true).cast_array::() } /// Returns a sub-range `begin..end` as a new `Array`. @@ -551,10 +545,9 @@ impl Array { let (begin, end) = range.signed(); let end = end.unwrap_or(i32::MAX as i64); - // 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) }; - - subarray + self.as_inner() + .slice(begin, end, step as i64, deep) + .cast_array::() } /// Returns an non-exclusive iterator over the elements of the `Array`. @@ -815,11 +808,10 @@ impl Array { /// Returns a pointer to the element at the given index, or null if out of bounds. fn ptr_or_null(&self, index: usize) -> sys::GDExtensionConstVariantPtr { + let index = to_i64(index); + // SAFETY: array_operator_index_const returns null for invalid indexes. - let variant_ptr = unsafe { - let index = to_i64(index); - interface_fn!(array_operator_index_const)(self.sys(), index) - }; + let variant_ptr = unsafe { interface_fn!(array_operator_index_const)(self.sys(), index) }; // Signature is wrong in GDExtension, semantically this is a const ptr sys::SysPtr::as_const(variant_ptr) @@ -842,15 +834,13 @@ impl Array { /// Returns a pointer to the element at the given index, or null if out of bounds. fn ptr_mut_or_null(&mut self, index: usize) -> sys::GDExtensionVariantPtr { + let index = to_i64(index); + // SAFETY: array_operator_index returns null for invalid indexes. - unsafe { - let index = to_i64(index); - interface_fn!(array_operator_index)(self.sys_mut(), index) - } + unsafe { interface_fn!(array_operator_index)(self.sys_mut(), index) } } /// # Safety - /// /// This has the same safety issues as doing `self.assume_type::()` and so the relevant safety invariants from /// [`assume_type`](Self::assume_type) must be upheld. /// @@ -873,7 +863,6 @@ impl Array { /// that take a variant array even though we want to pass a typed one. /// /// # Safety - /// /// - 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 `U`. /// @@ -1013,6 +1002,18 @@ impl Array { } } + /// Creates a new array for [`GodotFfi::new_with_init()`], without setting a type yet. + pub(super) fn new_uncached_type(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self { + let mut result = unsafe { + Self::new_with_uninit(|self_ptr| { + let ctor = sys::builtin_fn!(array_construct_default); + ctor(self_ptr, std::ptr::null_mut()); + }) + }; + init_fn(result.sys_mut()); + result + } + /// # Safety /// Does not validate the array element type; `with_checked_type()` should be called afterward. // Visibility: shared with AnyArray. @@ -1111,7 +1112,23 @@ impl VarArray { unsafe impl GodotFfi for Array { const VARIANT_TYPE: ExtVariantType = ExtVariantType::Concrete(VariantType::ARRAY); - ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. } + ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; + fn new_from_sys; + fn new_with_uninit; + fn sys; + fn sys_mut; + fn from_arg_ptr; + fn move_return_ptr; + } + + /// Constructs a valid Godot array as ptrcall destination, without caching the element type. + /// + /// Ptrcall may replace the underlying data with an array of a different type (e.g. `duplicate()` on a typed + /// array returns `VarArray` in codegen, but the actual data is typed). The cache is lazily populated on first + /// access from the actual Godot data. + unsafe fn new_with_init(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self { + Self::new_uncached_type(init_fn) + } } // Only implement for untyped arrays; typed arrays cannot be nested in Godot. diff --git a/godot-core/src/builtin/collections/array_functional_ops.rs b/godot-core/src/builtin/collections/array_functional_ops.rs index 7a76cfee9..b8679d94f 100644 --- a/godot-core/src/builtin/collections/array_functional_ops.rs +++ b/godot-core/src/builtin/collections/array_functional_ops.rs @@ -50,8 +50,7 @@ impl<'a, T: ArrayElement> ArrayFunctionalOps<'a, T> { /// ``` #[must_use] pub fn filter(&self, callable: &Callable) -> Array { - // SAFETY: filter() returns array of same type as self. - unsafe { self.array.as_inner().filter(callable) } + self.array.as_inner().filter(callable).cast_array::() } /// Returns a new untyped array with each element transformed by the callable. @@ -72,8 +71,7 @@ impl<'a, T: ArrayElement> ArrayFunctionalOps<'a, T> { /// ``` #[must_use] pub fn map(&self, callable: &Callable) -> VarArray { - // SAFETY: map() returns an untyped array (element type Variant). - unsafe { self.array.as_inner().map(callable) } + self.array.as_inner().map(callable).cast_array() } /// Reduces the array to a single value by iteratively applying the callable. diff --git a/godot-core/src/builtin/collections/dictionary.rs b/godot-core/src/builtin/collections/dictionary.rs index 3318b3671..0a7bf3a72 100644 --- a/godot-core/src/builtin/collections/dictionary.rs +++ b/godot-core/src/builtin/collections/dictionary.rs @@ -136,6 +136,18 @@ impl Dictionary { } } + /// Creates a new dictionary for [`GodotFfi::new_with_init()`], without setting a type yet. + pub(super) fn new_uncached_type(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self { + let mut result = unsafe { + Self::new_with_uninit(|self_ptr| { + let ctor = sys::builtin_fn!(dictionary_construct_default); + ctor(self_ptr, std::ptr::null_mut()); + }) + }; + init_fn(result.sys_mut()); + result + } + /// Constructs an empty typed `Dictionary`. pub fn new() -> Self { let mut dict = Self::default(); @@ -353,9 +365,7 @@ impl Dictionary { /// _Godot equivalent: `keys`_ #[doc(alias = "keys")] pub fn keys_array(&self) -> Array { - // SAFETY: keys() returns an untyped array with element type Variant. - let out_array = self.as_inner().keys(); - unsafe { out_array.assume_type() } + self.as_inner().keys().cast_array() } /// Creates a new `Array` containing all the values currently in the dictionary. @@ -363,9 +373,7 @@ impl Dictionary { /// _Godot equivalent: `values`_ #[doc(alias = "values")] pub fn values_array(&self) -> Array { - // SAFETY: values() returns an untyped array with element type Variant. - let out_array = self.as_inner().values(); - unsafe { out_array.assume_type() } + self.as_inner().values().cast_array() } /// Copies all keys and values from `other` into `self`. @@ -392,10 +400,10 @@ impl Dictionary { /// /// _Godot equivalent: `dict.duplicate(true)`_ pub fn duplicate_deep(&self) -> Self { - let dup = self.as_inner().duplicate(true); - // SAFETY: duplicate() returns a typed dictionary with the same type as Self, and all values are taken from `self` so have the right type. - let result = unsafe { Self::assume_type(dup) }; - result.with_cache(self) + self.as_inner() + .duplicate(true) + .upcast_any_dictionary() + .cast_dictionary::() } /// Shallow copy, copying elements but sharing nested collections. @@ -408,38 +416,10 @@ impl Dictionary { /// /// _Godot equivalent: `dict.duplicate(false)`_ pub fn duplicate_shallow(&self) -> Self { - let dup = self.as_inner().duplicate(false); - // SAFETY: duplicate() returns a typed dictionary with the same type as Self, and all values are taken from `self` so have the right type. - let result = unsafe { Self::assume_type(dup) }; - result.with_cache(self) - } - - /// Changes the type parameter without runtime checks, consuming the dictionary. - /// - /// # Safety - /// - Values written to dictionary must match runtime type. - /// - Values read must be convertible to types `K` and `V`. - /// - If runtime type matches `K` and `V`, both conditions hold automatically. - /// - /// This method has the same memory layout requirements as [`Array::assume_type`]. - // TODO(v0.5): fragile manual field move + mem::forget; if a field is added, it must be moved here too. - // Consider transmute (requires #[repr(C)]) or ManuallyDrop + ptr::read. Same issue in Array::assume_type. - unsafe fn assume_type(dict: VarDictionary) -> Self { - let result = Self { - opaque: dict.opaque, - _phantom: PhantomData, - cached_key_type: OnceCell::new(), - cached_value_type: OnceCell::new(), - }; - - // Transfer cached types to avoid redundant FFI calls. - ElementType::transfer_cache(&dict.cached_key_type, &result.cached_key_type); - ElementType::transfer_cache(&dict.cached_value_type, &result.cached_value_type); - - // Prevent drop of dict since we moved opaque. - std::mem::forget(dict); - - result + self.as_inner() + .duplicate(false) + .upcast_any_dictionary() + .cast_dictionary::() } /// Returns an iterator over the key-value pairs of the `Dictionary`. @@ -760,7 +740,21 @@ impl Dictionary { unsafe impl GodotFfi for Dictionary { const VARIANT_TYPE: ExtVariantType = ExtVariantType::Concrete(sys::VariantType::DICTIONARY); - ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. } + ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; + fn new_from_sys; + fn new_with_uninit; + fn sys; + fn sys_mut; + fn from_arg_ptr; + fn move_return_ptr; + } + + /// Constructs a valid Godot dictionary as ptrcall destination, without caching the element types. + /// + /// See `Array::new_with_init` for rationale. + unsafe fn new_with_init(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self { + Self::new_uncached_type(init_fn) + } } impl std::ops::Deref for Dictionary { diff --git a/itest/rust/src/builtin_tests/containers/array_test.rs b/itest/rust/src/builtin_tests/containers/array_test.rs index 017b5f643..f29893506 100644 --- a/itest/rust/src/builtin_tests/containers/array_test.rs +++ b/itest/rust/src/builtin_tests/containers/array_test.rs @@ -94,7 +94,7 @@ fn array_hash() { } #[itest] -fn array_share() { +fn array_clone() { let mut array = array![1, 2]; let shared = array.clone(); array.set(0, 3); @@ -104,25 +104,50 @@ fn array_share() { #[itest] fn array_duplicate_shallow() { let subarray = array![2, 3]; + assert_eq!( + subarray.duplicate_shallow().element_type(), + ElementType::Builtin(VariantType::INT) + ); + let array = varray![1, subarray]; let duplicate = array.duplicate_shallow(); - Array::::try_from_variant(&duplicate.at(1)) - .unwrap() - .set(0, 4); + assert_eq!(duplicate.element_type(), ElementType::Untyped); + + Array::::from_variant(&duplicate.at(1)).set(0, 4); assert_eq!(subarray.at(0), 4); } #[itest] fn array_duplicate_deep() { let subarray = array![2, 3]; + assert_eq!( + subarray.duplicate_deep().element_type(), + ElementType::Builtin(VariantType::INT) + ); + let array = varray![1, subarray]; let duplicate = array.duplicate_deep(); - Array::::try_from_variant(&duplicate.at(1)) - .unwrap() - .set(0, 4); + assert_eq!(duplicate.element_type(), ElementType::Untyped); + + Array::::from_variant(&duplicate.at(1)).set(0, 4); assert_eq!(subarray.at(0), 2); } +#[itest] +fn array_any_duplicate_deep() { + let typed = array![2, 3].upcast_any_array(); + assert_eq!( + typed.duplicate_deep().element_type(), + ElementType::Builtin(VariantType::INT) + ); + + let untyped = varray![1, typed].upcast_any_array(); + assert_eq!( + untyped.duplicate_deep().element_type(), + ElementType::Untyped + ); +} + #[itest] #[allow(clippy::reversed_empty_ranges)] fn array_subarray_shallow() { diff --git a/itest/rust/src/builtin_tests/containers/dictionary_test.rs b/itest/rust/src/builtin_tests/containers/dictionary_test.rs index c4731b888..788d10e73 100644 --- a/itest/rust/src/builtin_tests/containers/dictionary_test.rs +++ b/itest/rust/src/builtin_tests/containers/dictionary_test.rs @@ -889,7 +889,7 @@ func variant_script_dict() -> Dictionary[Variant, CustomScriptForDictionaries]: #[cfg(since_api = "4.4")] mod typed_dictionary_tests { - use godot::builtin::dict; + use godot::builtin::{array, dict}; use super::*; @@ -943,18 +943,36 @@ mod typed_dictionary_tests { assert_eq!(d.get("num"), Some(23.to_variant())); } - #[itest(skip)] // TODO(v0.5): fix {keys,values}_array and re-enable. - #[expect(clippy::dbg_macro)] + #[itest] fn dictionary_typed_kv_array() { - // Value type needs to be specified for now, due to GString/StringName/NodePath ambiguity. - let dict: Dictionary = dict! { + // Key type needs to be specified for now, due to GString/StringName/NodePath ambiguity. + let dict: Dictionary = dict! { "key1": 10, "key2": 20, }; - dbg!(dict.keys_array()); - dbg!(dict.keys_array().element_type()); - dbg!(dict.values_array()); - dbg!(dict.values_array().element_type()); + assert_eq!(dict.keys_array(), array!["key1", "key2"]); + assert_eq!( + dict.keys_array().element_type(), + ElementType::Builtin(VariantType::STRING) + ); + assert_eq!(dict.values_array(), array![10, 20]); + assert_eq!( + dict.values_array().element_type(), + ElementType::Builtin(VariantType::INT) + ); + } + + #[itest] + fn dictionary_typed_duplicate() { + let dict = Dictionary::::new(); + + let shallow = dict.duplicate_shallow(); + assert_eq!(shallow.key_element_type(), dict.key_element_type()); + assert_eq!(shallow.value_element_type(), dict.value_element_type()); + + let deep = dict.duplicate_deep(); + assert_eq!(deep.key_element_type(), dict.key_element_type()); + assert_eq!(deep.value_element_type(), dict.value_element_type()); } }