Support associated types in #[godot_dyn]

[Yarwin]

closes #1021.

WIP - adds support for associated types for #[godot_dyn] macro. Requires some more extensive unit testing - I'm not sure if there are any possible side effects.

(also my macros aren't great, feel free to critique them)

[GodotRust]

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

[Bromeon]

Thanks a lot, this was very quick 😲

In the future, it might also happen that such constraints are extended to associated consts (rust-lang/rust#92827), but I don't think this is happening soon. If someone uses nightly Rust and needs this, they would be welcome to submit another PR 🙂

[Bromeon]

"Impl" is a bit misleading, as there's no impl block involved.

Also, unless we need to distinguish Some/None in some logic, godot-rust typically uses the empty token stream.

Suggested change

	let associated_impl = if associated_types.is_empty() {
	None
	} else {
	Some(quote! { <#(#associated_types, )*>})
	};
	let assoc_type_constraints = if associated_types.is_empty() {
	TokenStream::new()
	} else {
	quote! { < #(#associated_types),* > }
	};

Please apply suggestion verbatim (also spacing + , changed).

[Yarwin]

it seems that I'll pretty much double the tests in dyn_gd_test.rs; not optimal albeit necessary.

[Bromeon]

it seems that I'll pretty much double the tests in dyn_gd_test.rs; not optimal albeit necessary.

Why? Can't you just add another trait with an associated type, and implement it for one type with #[godot_dyn]? We don't need to re-test all the possible interactions, especially since this is a compile-time fix.

[Yarwin]

Hmm, you are right – I can reduce amount of tests, since I haven't spot any weird side effects 🤔 – I'll leave two implementors and reduce amount of test cases.

EDIT: done. I removed dyn_gd_multiple_traits since we are already check for multiple trait registrations & access to base via dyn trait (in foreign::NodeHealth).

[Yarwin]

Rebased on current master, resolved conflicts

[Bromeon]

Implementation looks good, but tests decrease coverage slightly and are a bit harder to understand now. I made some suggestions on how to simplify things, while still testing the full associated type mechanics.

[Bromeon]

All these tests also checked type inference: when there's only one candidate of a trait, the compiler is smart enough to figure it out. Explicitly specifying types defeats this.

Would this still be necessary with my IdProvider proposal below? Would be cool if you could revert changes to existing tests regarding type inference; these should keep running as-is.

[Bromeon]

Maybe you can instead extend the InstanceIdProvider, e.g. like this?

trait IdProvider {
    type Id;
    fn get_id_dynamic(&self) -> Self::Id;
}

That would require less changes and keep the two traits easier to distinguish.

(Also, if you move it to its original declaration location, the diff is easier to inspect).

[Yarwin]

It is worth noting that type inference doesn't work – and results in very confusing errors – in most cases (sic!) if there are multiple possible DynGd<_, D> trait implementations.

In this example foreign::NodeHealth is implementor of two dyn_gd traits: Health and InstanceProvider. Trying to compile following code:

fn dyn_gd_creation_deref() {
    let obj = foreign::NodeHealth::new_alloc();
    let original_id = obj.instance_id();

    let mut obj = obj.into_dyn();

    let dyn_id = obj.instance_id();
    assert_eq!(dyn_id, original_id);

    deal_20_damage(&mut *obj.dyn_bind_mut());
    assert_eq!(obj.dyn_bind().get_hitpoints(), 80);

    obj.free();
}

fn deal_20_damage(h: &mut Health) {
    h.deal_damage(20);
}

trait Health {
    (…)
}

Results in bunch of very confusing errors:

    Checking itest v0.0.0 (/home/irwin/apps/godot/opensource-contr/missing_docs/gdext/itest/rust)
error[E0597]: `obj` does not live long enough
  --> itest/rust/src/object_tests/dyn_gd_test.rs:68:26
   |
63 |     let mut obj = obj.into_dyn();
   |         ------- binding `obj` declared here
...
68 |     deal_20_damage(&mut *obj.dyn_bind_mut());
   |                          ^^^---------------
   |                          |
   |                          borrowed value does not live long enough
   |                          argument requires that `obj` is borrowed for `'static`
...
72 | }
   | - `obj` dropped here while still borrowed

error[E0716]: temporary value dropped while borrowed
  --> itest/rust/src/object_tests/dyn_gd_test.rs:68:26
   |
68 |     deal_20_damage(&mut *obj.dyn_bind_mut());
   |     ---------------------^^^^^^^^^^^^^^^^^^-- temporary value is freed at the end of this statement
   |     |                    |
   |     |                    creates a temporary value which is freed while still in use
   |     argument requires that borrow lasts for `'static`

error[E0502]: cannot borrow `obj` as immutable because it is also borrowed as mutable
  --> itest/rust/src/object_tests/dyn_gd_test.rs:69:16
   |
68 |     deal_20_damage(&mut *obj.dyn_bind_mut());
   |                          ------------------
   |                          |
   |                          mutable borrow occurs here
   |                          argument requires that `obj` is borrowed for `'static`
69 |     assert_eq!(obj.dyn_bind().get_hitpoints(), 80);
   |                ^^^ immutable borrow occurs here

error[E0505]: cannot move out of `obj` because it is borrowed
  --> itest/rust/src/object_tests/dyn_gd_test.rs:71:5
   |
63 |     let mut obj = obj.into_dyn();
   |         ------- binding `obj` declared here
...
68 |     deal_20_damage(&mut *obj.dyn_bind_mut());
   |                          ------------------
   |                          |
   |                          borrow of `obj` occurs here
   |                          argument requires that `obj` is borrowed for `'static`
...
71 |     obj.free();
   |     ^^^ move out of `obj` occurs here

It might be that &mut (dyn Health + 'static) - a return type declared in NodeHealth's AsDyn implementation for dyn_upcasts– is different from &mut Health and compiler tries to enforce/satisfy this lifetime bound by making a reference static?

This issue should be mentioned and documented in the book. The errors are unhelpful and confusing. It is next to impossible to know what is going on without knowledge about an inner implementation (which might require some digging – AsDyn is hidden behind macro invocation for example, checking descriptions of dyn_binds doesn't yield anything either…).

Current solutions to this problem:

1. Just explicitly specify given dyn trait (recommended):

let mut obj = obj.into_dyn::<dyn Health>();
let mut obj: DynGd<_, dyn Health> = obj.into_dyn();. 

2. Enforce lifetime bound on original trait:

trait Health: 'static {
…
}

3. Explicitly declare lifetime bound in function signature: fn deal_20_damage(h: &mut (dyn Health + 'static)) {…}. (ugh)

[Bromeon]

Good points. I moved them to their own issue, since they have nothing to do with associated type.

Small things left, then this should be ready! Please also squash commits once ready from your side 🙂

[Bromeon]

Suggested change

	// type can be safely inferred.
	// Type can be safely inferred.

[Bromeon]

Suggested change

	// trait must be explicitly declared if multiple AsDyn<...> trait implementations exists.
	// `dyn Health` must be explicitly declared if multiple AsDyn<...> trait implementations exist.

[Bromeon]

Thanks a lot!