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

SystemParam: lifetime may not live long enough #8192

Closed
mbolt35 opened this issue Mar 24, 2023 · 7 comments · Fixed by #8030
Closed

SystemParam: lifetime may not live long enough #8192

mbolt35 opened this issue Mar 24, 2023 · 7 comments · Fixed by #8030
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior
Milestone

Comments

@mbolt35
Copy link

mbolt35 commented Mar 24, 2023

Bevy version

v0.10.0

What you did

Upgrading a bevy 0.9 project to the latest and greatest. I have a few #[derive(SystemParam)] implementations that were working great with 0.9, but now give me an error: lifetime may not live long enough which appears to all occur within the SystemParam attribute itself. Here's the struct definition in question:

/// TileEntities is a helper system parameter that provides pulling
/// component references directly from the tile positions.
#[allow(clippy::complexity)]
#[derive(SystemParam)]
pub struct TileEntities<'w, 's, T: EntityRef + Component>
    where <T as EntityRef>::ComponentType: Component,
{
    references: ParamSet<'w, 's, (
        Query<'w, 's, &'static mut T>,
        Query<'w, 's, &'static T>)>,
    component: ParamSet<'w, 's, (
        Query<'w, 's, &'static mut T::ComponentType>,
        Query<'w, 's, &'static T::ComponentType>)>,
    map: TileQuery<'w, 's, SimulationLayer>,
}

The TileQuery itself is also a SystemParam:

#[derive(SystemParam)]
pub struct TileQuery<'w, 's, T: Component + Layer> {
    map: Query<'w, 's, &'static TileStorage, With<T>>,
    world_manager: Res<'w, WorldManager>,
    tile_mapper: TileMapper<'w>,
}

/// TileMapper can be included in systems and used to map tile positions back into world
/// coordinates and other tile related conversions.
#[derive(SystemParam)]
pub struct TileMapper<'w> {
    config: Res<'w, MapConfig>
}

The EntityRef trait is defined as:

/// EntityRef is a trait used to reference a single entity from multiple tiles
pub trait EntityRef {
    /// The `ComponentType` refers to the internal type of component the `Entity`
    /// refers to. This is especially useful information to have for building
    /// generic bevy `Query` parameters for systems.
    type ComponentType;

    /// This method returns the `UVec2` tile position of the entity set.
    fn tile(&self) -> UVec2;

    /// This method returns the `Entity` identifier.
    fn get(&self) -> Entity;
}

What went wrong

I'm getting 4 nearly identical errors all pointing to the #[derive(SystemParam)].

Here's the first error, followed by the 3 differences

error: lifetime may not live long enough
   --> src\tiles\tile_entities.rs:155:10
    |
155 | #[derive(SystemParam)]
    |          ^^^^^^^^^^^
    |          |
    |          lifetime `'w` defined here
    |          lifetime `'w2` defined here
    |          associated function was supposed to return data with lifetime `'w2` but it is returning data with lifetime `'w`
    |
    = help: consider adding the following bound: `'w: 'w2`
    = note: requirement occurs because of the type `tile_entities::TileEntities<'_, '_, T>`, which makes the generic argument `'_` invariant
    = note: the struct `tile_entities::TileEntities<'w, 's, T>` is invariant over the parameter `'w`
    = help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance
    = note: this error originates in the derive macro `SystemParam` (in Nightly builds, run with -Z macro-backtrace for more info)

Second:

...
155 | #[derive(SystemParam)]
    |          ^^^^^^^^^^^
    |          |
    |          lifetime `'s` defined here
    |          lifetime `'s2` defined here
    |          associated function was supposed to return data with lifetime `'s2` but it is returning data with lifetime `'s`
    |
    = help: consider adding the following bound: `'s: 's2`
...

Third:

...
155 | #[derive(SystemParam)]
    |          ^^^^^^^^^^^
    |          |
    |          lifetime `'s2` defined here
    |          lifetime `'s` defined here
    |          associated function was supposed to return data with lifetime `'s` but it is returning data with lifetime `'s2`
    |
    = help: consider adding the following bound: `'s2: 's`
...

Fourth:

...
155 | #[derive(SystemParam)]
    |          ^^^^^^^^^^^
    |          |
    |          lifetime `'w2` defined here
    |          lifetime `'w` defined here
    |          associated function was supposed to return data with lifetime `'w` but it is returning data with lifetime `'w2`
    |
    = help: consider adding the following bound: `'w2: 'w`
...

Additional information

I will work to see if I can get a reproducible case with less dependencies than the current project I am working in. Apologies if this is unclear in anyway. Will be happy to assist anyway possible!

Thanks for an incredible open source project

@mbolt35 mbolt35 added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Mar 24, 2023
@alice-i-cecile
Copy link
Member

What happens if you change those lifetimes to 'static?

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events and removed S-Needs-Triage This issue needs to be labelled labels Mar 24, 2023
@mbolt35
Copy link
Author

mbolt35 commented Mar 24, 2023

What happens if you change those lifetimes to 'static?

@alice-i-cecile Just to be clear, do you mean a 'static constraint on 'w and 's? Or do you mean dropping the 'w and 's in my type and replacing with 'static? (I'm assuming there could be a difference there do to the way the param parses the struct body)

Will give this a shot ASAP! Thanks for the response

@JoJoJet
Copy link
Member

JoJoJet commented Mar 24, 2023

Ah, seems like this issue pops up when a SystemParam has invariant lifetimes. This used to work, but it must have regressed during the 0.9-0.10 cycle (likely my fault) since none of our tests use invariant lifetimes.

Here's a minimum reproducible example:

#[derive(SystemParam)]
pub struct InvariantParam<'w, 's> {
    set: ParamSet<'w, 's, (Query<'w, 's, ()>,)>,
}

Seems like this will be fixed by #8030.

@mbolt35
Copy link
Author

mbolt35 commented Mar 25, 2023

@alice-i-cecile Adding the 'static constraint on 'w and 's did seem to solve 2 of the errors, but looks as if now it's expecting a lifetime to outlive static:

error[E0478]: lifetime bound not satisfied
   --> src\tiles\tile_entities.rs:155:10
    |
155 | #[derive(SystemParam)]
    |          ^^^^^^^^^^^
    |
note: lifetime parameter instantiated with the lifetime `'_w` as defined here
   --> src\tiles\tile_entities.rs:155:10
    |
155 | #[derive(SystemParam)]
    |          ^^^^^^^^^^^
    = note: but lifetime parameter must outlive the static lifetime
    = note: this error originates in the derive macro `SystemParam` 

and

error[E0478]: lifetime bound not satisfied
   --> src\tiles\tile_entities.rs:155:10
    |
155 | #[derive(SystemParam)]
    |          ^^^^^^^^^^^
    |
note: lifetime parameter instantiated with the lifetime `'_s` as defined here
   --> src\tiles\tile_entities.rs:155:10
    |
155 | #[derive(SystemParam)]
    |          ^^^^^^^^^^^
    = note: but lifetime parameter must outlive the static lifetime
    = note: this error originates in the derive macro `SystemParam`

@JoJoJet Thanks for chiming in here, do you have any insight on when we might expect #8030 ? Possible there will be a patch release, or are we talking 0.11?

@JoJoJet
Copy link
Member

JoJoJet commented Mar 25, 2023

#8030 has a breaking change, so in order for the fix to land in a patch release I'd have to make a new PR with just that fix and nothing else. I can look into that later but I'm not sure how that would work out.

As a temporary workaround, try changing just the lifetimes inside of the ParamSet to 'static:

Meaning if you have a struct like this: ParamSet<'w, 's, Query<'w, 's, ...>>
Change it to this: ParamSet<'w, 's, Query<'static, 'static, ...>>

Changing the lifetimes in this specific way seems to make it compile.

@mbolt35
Copy link
Author

mbolt35 commented Mar 25, 2023

Meaning if you have a struct like this: ParamSet<'w, 's, Query<'w, 's, ...>> Change it to this: ParamSet<'w, 's, Query<'static, 'static, ...>>

Changing the lifetimes in this specific way seems to make it compile.

@JoJoJet Thank you, that works. Appreciate the help 👍

@JoJoJet
Copy link
Member

JoJoJet commented Mar 25, 2023

Happy I could help :)

@JoJoJet JoJoJet added this to the 0.10.1 milestone Mar 28, 2023
JoJoJet added a commit to JoJoJet/bevy that referenced this issue Mar 31, 2023
…ry (bevyengine#8030)

When using `PhantomData` fields with the `#[derive(SystemParam)]` or
`#[derive(WorldQuery)]` macros, the user is required to add the
`#[system_param(ignore)]` attribute so that the macro knows to treat
that field specially. This is undesirable, since it makes the macro more
fragile and less consistent.

Implement `SystemParam` and `WorldQuery` for `PhantomData`. This makes
the `ignore` attributes unnecessary.

Some internal changes make the derive macro compatible with types that
have invariant lifetimes, which fixes bevyengine#8192. From what I can tell, this
fix requires `PhantomData` to implement `SystemParam` in order to ensure
that all of a type's generic parameters are always constrained.

---

+ Implemented `SystemParam` and `WorldQuery` for `PhantomData<T>`.
+ Fixed a miscompilation caused when invariant lifetimes were used with
the `SystemParam` macro.
cart pushed a commit that referenced this issue Mar 31, 2023
…ry (#8030)

When using `PhantomData` fields with the `#[derive(SystemParam)]` or
`#[derive(WorldQuery)]` macros, the user is required to add the
`#[system_param(ignore)]` attribute so that the macro knows to treat
that field specially. This is undesirable, since it makes the macro more
fragile and less consistent.

Implement `SystemParam` and `WorldQuery` for `PhantomData`. This makes
the `ignore` attributes unnecessary.

Some internal changes make the derive macro compatible with types that
have invariant lifetimes, which fixes #8192. From what I can tell, this
fix requires `PhantomData` to implement `SystemParam` in order to ensure
that all of a type's generic parameters are always constrained.

---

+ Implemented `SystemParam` and `WorldQuery` for `PhantomData<T>`.
+ Fixed a miscompilation caused when invariant lifetimes were used with
the `SystemParam` macro.
UkoeHB pushed a commit to UkoeHB/bevy that referenced this issue Sep 10, 2023
…ry (bevyengine#8030)

When using `PhantomData` fields with the `#[derive(SystemParam)]` or
`#[derive(WorldQuery)]` macros, the user is required to add the
`#[system_param(ignore)]` attribute so that the macro knows to treat
that field specially. This is undesirable, since it makes the macro more
fragile and less consistent.

Implement `SystemParam` and `WorldQuery` for `PhantomData`. This makes
the `ignore` attributes unnecessary.

Some internal changes make the derive macro compatible with types that
have invariant lifetimes, which fixes bevyengine#8192. From what I can tell, this
fix requires `PhantomData` to implement `SystemParam` in order to ensure
that all of a type's generic parameters are always constrained.

---

+ Implemented `SystemParam` and `WorldQuery` for `PhantomData<T>`.
+ Fixed a miscompilation caused when invariant lifetimes were used with
the `SystemParam` macro.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants