Document `DynGd<_, D>` type inference. by Yarwin · Pull Request #1142 · godot-rust/gdext

Yarwin · 2025-04-27T11:48:50Z

Closes: #1026

Document when type inference works and when it doesn't.
Going with lilizoey suggestion – Explicitly enforce Trait: 'static for DynGd<T, Trait> and AsDyn<Trait> (we have no means to track lifetimes of given object either way which means it might be invalidated at any point – albeit creating such DynGd is hard, since GodotClass must be 'static either way).

Despite messing with it for some time I couldn't force compiler to properly infer given types – thus I limited myself to merely documenting it 😒.

GodotRust · 2025-04-27T12:00:28Z

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

Bromeon

Thanks!

Apart from the : 'static bound users need to add, are there any other ergonomic downsides of this change?

: 'static is arguably a bit annoying since that is very unusual for most traits, but I guess if it makes inference clearer, that's probably worth it 🤔

Bromeon · 2025-04-27T12:18:34Z

godot-core/src/obj/dyn_gd.rs

+/// // Type can't be inferred.
+/// // Would result in confusing compilation error
+/// // since compiler would try to enforce 'static **lifetime** on our reference.
+/// // let mut dyn_gd = Monster::new_gd().into_dyn();
+/// // no_inference(&mut *dyn_gd.dyn_bind_mut());


This could be a separate block with compile_fail.

Also, instead of **lifetime**, you could write *lifetime* (&'static mut ...).

Bromeon · 2025-04-27T12:21:34Z

godot-core/src/obj/dyn_gd.rs

+/// fn deal_damage(h: &mut dyn Health) { /* ... */ }
+///
+/// fn no_inference(i: &mut dyn NoInference) { /* ... */ }


Suggested change

/// fn deal_damage(h: &mut dyn Health) { /* ... */ }

///

/// fn no_inference(i: &mut dyn NoInference) { /* ... */ }

/// // Two example functions accepting trait object, to check type inference.

/// fn deal_damage(h: &mut dyn Health) { /* ... */ }

/// fn no_inference(i: &mut dyn NoInference) { /* ... */ }

Bromeon · 2025-04-27T12:24:17Z

godot-core/src/obj/dyn_gd.rs

+/// If class implements more than one `#[godot_dyn]` trait, type inference only works when the trait we dereference to explicitly declares
+/// `'static` as its supertrait.


Suggested change

/// If class implements more than one `#[godot_dyn]` trait, type inference only works when the trait we dereference to explicitly declares

/// `'static` as its supertrait.

/// If a class implements more than one `AsDyn` relation (usually via `#[godot_dyn]`), type inference will only work when the trait

/// used for `D` explicitly declares a `: 'static` bound.

Bromeon · 2025-04-27T12:25:24Z

godot-core/src/obj/dyn_gd.rs

+///
+/// If class implements more than one `#[godot_dyn]` trait, type inference only works when the trait we dereference to explicitly declares
+/// `'static` as its supertrait.
+/// Otherwise, if only one `#[godot_dyn]` trait is implemented for a given class, the type can be properly inferred.


Suggested change

/// Otherwise, if only one `#[godot_dyn]` trait is implemented for a given class, the type can be properly inferred.

/// Otherwise, if only one `impl AsDyn` is present for a given class, the type can always be inferred.

Yarwin · 2025-04-29T10:18:08Z

Apart from the : 'static bound users need to add, are there any other ergonomic downsides of this change?

Personally I haven't found any downsides 🤔.

I'll try to make another deep dive in a month or two to figure out what is going on – but for now, especially for 0.3, I just want to have this quirk documented since compile errors related to this case are very confusing if one doesn't know the internals.

I also updated comment related to #1145 (I was too focused to make it work ASAP to take care of such details 😅).

Bromeon · 2025-04-30T09:05:49Z

itest/rust/src/object_tests/dyn_gd_test.rs

+    // Type can be inferred because `Health` explicitly declares
+    // 'static as its supertrait.
+    let mut obj = original_obj.clone().into_dyn();


Suggested change

// Type can be inferred because `Health` explicitly declares

// 'static as its supertrait.

let mut obj = original_obj.clone().into_dyn();

// Type can be inferred because `Health` explicitly declares a 'static bound.

let mut obj = original_obj.clone().into_dyn();

Line width 140-150 (see code style)

Also, 'static isn't a trait, so it's not a supertrait either.

Bromeon · 2025-04-30T09:20:20Z

godot-core/src/obj/dyn_gd.rs

+/// # Type inference
+///
+/// If a class implements more than one `AsDyn<D>` relation (usually via `#[godot_dyn]`), type inference will only work when the trait
+/// used for `D` explicitly declares a `: 'static` bound.


This section could probably come last, no?

Exporting is definitely more important (arguably also more than dyn-re-enrichment).

hmm, I think you are right – albeit I'm finding Polymorphic dyn re-enrichment a bit more important 🤔 (reasoning – the first two sections explain how to use it and how it works, while following ones focus on documenting various quirks)

I put Type inference as the last section

Bromeon · 2025-04-30T09:24:24Z

godot-core/src/obj/dyn_gd.rs

+/// fn deal_damage(h: &mut dyn Health) { /* ... */ }
+/// fn no_inference(i: &mut dyn NoInference) { /* ... */ }
+///
+/// // Type can be inferred since `static supertrait is explicitly declared for Health trait.


Suggested change

/// // Type can be inferred since `static supertrait is explicitly declared for Health trait.

/// // Type can be inferred since 'static bound is explicitly declared for Health trait.

No backtick, but apostrophe -- and no supertrait

Bromeon · 2025-04-30T09:24:56Z

itest/rust/src/object_tests/dyn_gd_test.rs

 // Example symbols

-trait Health {
+// 'static supertrait must be explicitly declared to make type inference work.


Suggested change

// 'static supertrait must be explicitly declared to make type inference work.

// 'static bound must be explicitly declared to make type inference work.

Bromeon · 2025-04-30T09:25:54Z

itest/rust/src/object_tests/dyn_gd_test.rs

+
+    // Not recommended – for presentational purposes only.
+    // Works because 'static bound on type is enforced in function signature.
+    // i.e. This wouldn't work with fn get_instance_id(...).


Suggested change

// i.e. This wouldn't work with fn get_instance_id(...).

// I.e. this wouldn't work with fn get_instance_id(...).

Bromeon · 2025-04-30T09:28:50Z

godot-core/src/registry/callbacks.rs

+/// # Safety
+///
+/// - `r_is_valid` must be assignable.
+/// - `r_is_valid` comes uninitialized and must be set.
 pub unsafe extern "C" fn to_string<T: cap::GodotToString>(


Safety sections are the requirements a caller must fulfill, not the requirements on the function implementation (2nd bullet point).

But let's remove it altogether, as only Godot calls it and none of the other callbacks has it. You can still mention the fact that

is_valid comes uninitialized and must be set by this function

as a regular (non-safety) comment.

Bromeon · 2025-04-30T09:30:56Z

godot-core/src/registry/callbacks.rs

 pub unsafe extern "C" fn to_string<T: cap::GodotToString>(
    instance: sys::GDExtensionClassInstancePtr,
-    is_valid: *mut sys::GDExtensionBool,
+    r_is_valid: *mut sys::GDExtensionBool,


Please revert the rename. While it follows Godot convention, it's inconsistent with all the other callback signatures here. It's even inconsistent with the other parameter in this exact function (would be p_instance in Godot).

- Document when type inference works and when it doesn't. - Explicitly enforce `Trait: 'static` for `DynGd<T, D>` and `AsDyn<D>` – mostly for clarity, creating non-static `DynGd<T, D>` was already pretty much impossible, and `T` – GodotClass – must be 'static either way.

Bromeon reviewed Apr 27, 2025

View reviewed changes

Yarwin force-pushed the document-type-inference-for-dyngd branch from 86bd36a to a098d0a Compare April 29, 2025 10:13

Yarwin force-pushed the document-type-inference-for-dyngd branch from a098d0a to 901e79a Compare April 29, 2025 10:28

Yarwin added documentation Improvements or additions to documentation c: core Core components labels Apr 30, 2025

Bromeon reviewed Apr 30, 2025

View reviewed changes

Yarwin added 2 commits April 30, 2025 12:07

Update comment in to_string.

3529708

Yarwin force-pushed the document-type-inference-for-dyngd branch from 901e79a to 3529708 Compare April 30, 2025 10:10

Bromeon added this to the 0.3 milestone May 1, 2025

Bromeon approved these changes May 1, 2025

View reviewed changes

Bromeon added this pull request to the merge queue May 1, 2025

Merged via the queue into godot-rust:master with commit 2bc0872 May 1, 2025
17 checks passed

		/// If class implements more than one `#[godot_dyn]` trait, type inference only works when the trait we dereference to explicitly declares
		/// `'static` as its supertrait.

	/// Otherwise, if only one `#[godot_dyn]` trait is implemented for a given class, the type can be properly inferred.
	/// Otherwise, if only one `impl AsDyn` is present for a given class, the type can always be inferred.

	/// // Type can be inferred since `static supertrait is explicitly declared for Health trait.
	/// // Type can be inferred since 'static bound is explicitly declared for Health trait.

	// 'static supertrait must be explicitly declared to make type inference work.
	// 'static bound must be explicitly declared to make type inference work.

	// i.e. This wouldn't work with fn get_instance_id(...).
	// I.e. this wouldn't work with fn get_instance_id(...).

Uh oh!

Conversation

Yarwin commented Apr 27, 2025

Uh oh!

GodotRust commented Apr 27, 2025

Uh oh!

Bromeon left a comment

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!

Yarwin commented Apr 29, 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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants