Bugfix – initialize&cache proper return value for generic, typed array. #1357

Yarwin · 2025-10-06T07:40:21Z

What Problem does this PR solve?

One mentioned in #1348. Long story short, we are responsible for initializing a return value from function; instead of initializing Array<T> we were initializing VariantArray and cached its type as untyped. Oopsie.

In other words, this code would panic:

#[itest]
fn array_inner_type() {
    let primary = Array::<Dictionary>::new();
    let secondary = primary.duplicate_shallow();
    assert_eq!(secondary.element_type(), primary.element_type()); // Panics!
    let secondary = primary.duplicate_deep();
    assert_eq!(secondary.element_type(), primary.element_type()) // Panics!
}

   -- array_inner_type ... ERROR: [panic /home/irwin/apps/godot/opensource-contr/missing_docs/gdext/godot-core/src/meta/element_type.rs:141]
  assertion `left == right` failed: Array/Dictionary element type validation failed: cached as Untyped but FFI reports DICTIONARY. This indicates that another extension modified the type after godot-rust cached it.              
    left: DICTIONARY                                                                                                                                                                                                                
   right: NIL                                                                                                                                                                                                                       
  Context: itest `array_inner_type` failed

This PR tweaks codegen, so instead of generating:

    pub unsafe fn duplicate(&self, deep: bool,) -> VariantArray {
        type CallRet = VariantArray;
        type CallParams = (bool,);
        let args = (deep,);
        unsafe {
            let method_bind = sys::builtin_method_table() . fptr_by_index(...);
            Signature::< CallParams, CallRet > ::out_builtin_ptrcall(method_bind, "Array", "duplicate", self.sys_ptr, args)
        }
    }

we generate:

    pub fn duplicate < Ret > (&self, deep: bool,) -> Array < Ret > where Ret: crate::meta::ArrayElement, {
        type CallRet < Ret > = Array < Ret >;
        type CallParams = (bool,);
        let args = (deep,);
        unsafe {
            let method_bind = sys::builtin_method_table() . fptr_by_index(705usize);
            Signature::< CallParams, CallRet < Ret > > ::out_builtin_ptrcall(method_bind, "Array", "duplicate", self.sys_ptr, args)
        }
    }

I haven't been tweaking code too much (albeit I removed some dead code). I think it is OK and should hold for dictionaries as well, as long as nothing drastically changes.

GodotRust · 2025-10-06T07:59:16Z

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

Bromeon · 2025-10-06T13:34:33Z

godot-codegen/src/generator/builtins.rs

+        && (&method.return_value().type_tokens().to_string() == "VariantArray"
+            || method.is_generic())


Should swap || operands, as is_generic is cheaper to compute.

side note – it might be good to check if we can set optimization level 1\2 for codegen or macros – it would allow compiler to optimize such things automatically and properly recognize no-ops 🤔.

for example – herestd::mem::take will be no-op for -C opt-level=2 (handy link: https://rust.godbolt.org/) and almost no-op for opt-level 1 (don't ask me why almost). (black box and indirection because compiler was behaving silly and differently to normal usecases)

fn foobaz(somefoo: &mut SomeFoo) { let mut vec = std::mem::take(&mut somefoo.foo); for val in &mut vec { *val += 1; } somefoo.foo = vec; } pub fn main(vec: Vec<u32>) { std::hint::black_box({ let mut foo = SomeFoo {foo: vec, bar: 4}; foobaz(&mut foo) }); }

Bromeon · 2025-10-06T13:35:45Z

godot-codegen/src/generator/builtins.rs

    let receiver = functions_common::make_receiver(method.qualifier(), ffi_arg_in);
    let object_ptr = &receiver.ffi_arg;

+    let generic_params = method.return_value().generic_params();


Suggested change

let generic_params = method.return_value().generic_params();

let maybe_generic_params = method.return_value().generic_params();

if it's often empty, that makes this a bit clearer...

Bromeon · 2025-10-06T13:38:22Z

godot-codegen/src/generator/default_parameters.rs

-    // ExBuilder::new() constructor signature.
-    let FnParamTokens {
-        func_general_lifetime: simple_fn_lifetime,
-        ..
-    } = fns::make_params_exprs(
-        required_fn_params.iter().cloned(),
-        FnKind::ExBuilderConstructor,
-    );


This change is unrelated (the dead code you mentioned), right? Why exactly?

Could you extract it to its own commit so it's easier to isolate the changes in case of regressions?

Why exactly?

func_general_lifetime was never set, thus it was always empty – effectively this code was doing nothing.

Bromeon · 2025-10-06T13:40:20Z

godot-codegen/src/models/domain.rs

+    fn is_generic(&self) -> bool {
+        matches!(self.return_value().type_, Some(RustTy::GenericArray))
+    }


Always empty lines between multi-line fn decls

Bromeon · 2025-10-06T13:41:45Z

godot-codegen/src/models/domain.rs

+    /// `Array<T>`
+    ///
+    /// Set by [`builtin_method_generic_ret`](crate::special_cases::builtin_method_generic_ret)
+    GenericArray,


Maybe make clear that this is actually generic in the generated source, and T is not just a placeholder here 🤔

Bromeon · 2025-10-06T14:21:06Z

godot-codegen/src/special_cases/special_cases.rs


+/// Returns some generic type – such as `GenericArray` representing `Array<T>` – if method is marked as generic, `None` otherwise.
+///
+/// Usually required to properly initialize the return value & cache its type.


Suggested change

/// Usually required to properly initialize the return value & cache its type.

/// Usually required to initialize the return value and cache its type (see also https://github.com/godot-rust/gdext/pull/1357).

Bromeon · 2025-10-06T14:32:09Z

godot-codegen/src/special_cases/special_cases.rs

+    match (
+        class_name.rust_ty.to_string().as_str(),
+        method.name.as_str(),
+    ) {
+        ("Array", "duplicate") | ("Array", "slice") => Some(FnReturn {
+            decl: RustTy::GenericArray.return_decl(),
+            type_: Some(RustTy::GenericArray),
+        }),
+        _ => None,


Might be worth dedicated constructor maybe? FnReturn::with_generic_builtin()

Bromeon · 2025-10-06T14:32:57Z

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

        // 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) };
+        let duplicate: Self = unsafe { self.as_inner().duplicate(false) };


Safety statement needs update.

ended rewriting all the methods, since std:mem::transmute was going from Array<T> to Array<T>, and cache was already initialized 😄

Bromeon · 2025-10-06T14:33:46Z

itest/rust/src/builtin_tests/containers/array_test.rs

    assert_eq!(script.get_global_name(), "CustomScriptForArrays".into());
 }

+// Test that proper type has been set&cached while creating new Array.


For regression tests, add link to PR/issue, here e.g.

https://github.com/godot-rust/gdext/pull/1357

Bromeon · 2025-10-06T14:34:26Z

itest/rust/src/builtin_tests/containers/array_test.rs

+#[itest]
+fn array_inner_type() {
+    let primary = Array::<Dictionary>::new();
+    let secondary = primary.duplicate_shallow();
+    assert_eq!(secondary.element_type(), primary.element_type());
+    let secondary = primary.duplicate_deep();
+    assert_eq!(secondary.element_type(), primary.element_type());
+    let subarray = primary.subarray_deep(.., None);
+    assert_eq!(subarray.element_type(), primary.element_type());
+    let subarray = primary.subarray_shallow(.., None);
+    assert_eq!(subarray.element_type(), primary.element_type());
+}


Nitpick, group subtests by introducing empty lines:

Suggested change

#[itest]

fn array_inner_type() {

let primary = Array::<Dictionary>::new();

let secondary = primary.duplicate_shallow();

assert_eq!(secondary.element_type(), primary.element_type());

let secondary = primary.duplicate_deep();

assert_eq!(secondary.element_type(), primary.element_type());

let subarray = primary.subarray_deep(.., None);

assert_eq!(subarray.element_type(), primary.element_type());

let subarray = primary.subarray_shallow(.., None);

assert_eq!(subarray.element_type(), primary.element_type());

}

#[itest]

fn array_inner_type() {

let primary = Array::<Dictionary>::new();

let secondary = primary.duplicate_shallow();

assert_eq!(secondary.element_type(), primary.element_type());

let secondary = primary.duplicate_deep();

assert_eq!(secondary.element_type(), primary.element_type());

let subarray = primary.subarray_deep(.., None);

assert_eq!(subarray.element_type(), primary.element_type());

let subarray = primary.subarray_shallow(.., None);

assert_eq!(subarray.element_type(), primary.element_type());

}

Bromeon · 2025-10-06T14:38:15Z

Nice work, thank you!

Note that this requires all generic functions to be unsafe, as they're only sound with one specific generic argument, and UB with all others.

…ray 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.

…set. Remove `ExBuilderConstructor` – it was doing nothing, and in practice it was supposed to do the same thing as `ExBuilderConstructorLifetimed`.

Yarwin · 2025-10-06T18:06:16Z

Note that this requires all generic functions to be unsafe, as they're only sound with one specific generic argument, and UB with all others.

That's correct, I also created/moved separate safety docs for generated generic methods returning Array<T> which is a little more specific.

Bromeon · 2025-10-08T01:25:41Z

godot-codegen/src/generator/builtins.rs

-            ///
-            /// You must ensure that the returned array fulfils the safety invariants of [`Array`](crate::builtin::Array).
-        });
+    if class_name.godot_ty == "Array" {


This is quite nice, but does anyone ever see these docs?
I guess the show up in IDE hints? 🤔

(currently) contributors are the only ones seeing it.

Still nice to have and might come in handy one day – I think it might be useful providing some context while debugging some UB (if any ever happens) too

screenshoot from previous version

Yarwin added bug c: core Core components labels Oct 6, 2025

Bromeon reviewed Oct 6, 2025

View reviewed changes

Yarwin added 2 commits October 6, 2025 20:01

Codegen for Array<T> - required to properly initialize and cache Ar…

46003b5

…ray 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.

Remove func_general_lifetime from FnParamTokens since it was never …

aaffcb5

…set. Remove `ExBuilderConstructor` – it was doing nothing, and in practice it was supposed to do the same thing as `ExBuilderConstructorLifetimed`.

Yarwin force-pushed the fix-wrongly-assumed-array-type branch from d8577c9 to aaffcb5 Compare October 6, 2025 18:01

Bromeon approved these changes Oct 8, 2025

View reviewed changes

Yarwin added this pull request to the merge queue Oct 10, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 10, 2025

		&& (&method.return_value().type_tokens().to_string() == "VariantArray"
		\|\| method.is_generic())

	let generic_params = method.return_value().generic_params();
	let maybe_generic_params = method.return_value().generic_params();

	/// Usually required to properly initialize the return value & cache its type.
	/// Usually required to initialize the return value and cache its type (see also https://github.com/godot-rust/gdext/pull/1357).

Uh oh!

Bugfix – initialize&cache proper return value for generic, typed array. #1357

Are you sure you want to change the base?

Bugfix – initialize&cache proper return value for generic, typed array. #1357

Conversation

Yarwin commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What Problem does this PR solve?

Uh oh!

GodotRust commented Oct 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bromeon commented Oct 6, 2025

Uh oh!

Yarwin commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yarwin Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Yarwin commented Oct 6, 2025 •

edited

Loading

Yarwin commented Oct 6, 2025 •

edited

Loading

Yarwin Oct 8, 2025 •

edited

Loading