Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document/improve DynGd<_, D> type inference #1026

Open
Bromeon opened this issue Jan 23, 2025 · 2 comments
Open

Document/improve DynGd<_, D> type inference #1026

Bromeon opened this issue Jan 23, 2025 · 2 comments
Labels
c: core Core components documentation Improvements or additions to documentation quality-of-life No new functionality, but improves ergonomics/internals

Comments

@Bromeon
Copy link
Member

Bromeon commented Jan 23, 2025

TLDR: we need to find ways to improve or deal with error messages arising from failed type inference of D in DynGd<T, D>.

Originally posted by @Yarwin in #1022 (comment)

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();. 
  1. Enforce lifetime bound on original trait:
trait Health: 'static {}
  1. Explicitly declare lifetime bound in function signature: fn deal_20_damage(h: &mut (dyn Health + 'static)) {…}. (ugh)
@Bromeon Bromeon added documentation Improvements or additions to documentation quality-of-life No new functionality, but improves ergonomics/internals c: core Core components labels Jan 23, 2025
@Bromeon
Copy link
Member Author

Bromeon commented Jan 23, 2025

We should check what the implications would be if:

  • DynGd<T, D> gets a new bound D: 'static
  • AsDyn<Trait> gets a new bound Trait: 'static

@Bromeon Bromeon changed the title Document DynGd<_, D> type inference Document/improve DynGd<_, D> type inference Jan 23, 2025
@lilizoey
Copy link
Member

lilizoey commented Feb 8, 2025

We should check what the implications would be if:

* `DynGd<T, D>` gets a new bound `D: 'static`

* `AsDyn<Trait>` gets a new bound `Trait: 'static`

Honestly this just seems like the right thing to do to me either way. If a trait object has a non-static lifetime that means the trait might get invalidated at some point. Which really doesn't fit with how we're doing DynGd in the first place. We are basically relying on being able to access these traits at any point in time. i cant think of a way we'd accurately be able to track the lifetime of objects we pass into this mechanism.

The actual consequence to the user should basically just be that they can't use a non-static lifetime on references in their trait, and may need to specify some : 'static bounds on some generics. though in most cases the lifetime can be elided. even in a case like this:

trait Foo<'a> {
  ...
}

Attempting to use this in a place that requires D: ?Sized + 'static will make rust just infer that the lifetime must be static.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Core components documentation Improvements or additions to documentation quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Development

No branches or pull requests

2 participants