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

Make #[system_param(ignore)] and #[world_query(ignore)] unnecessary #8030

Merged
merged 42 commits into from
Mar 30, 2023

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Mar 10, 2023

Objective

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.

Solution

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.


Changelog

  • Implemented SystemParam and WorldQuery for PhantomData<T>.
  • Fixed a miscompilation caused when invariant lifetimes were used with the SystemParam macro.

@JoJoJet JoJoJet added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Mar 10, 2023
@alice-i-cecile
Copy link
Member

This also prevents you from using any other weirder types in here. But I've never seen a case for that, and can't imagine one, so I think that's net good.

// SAFETY: No world access.
unsafe impl<T: ?Sized> SystemParam for PhantomData<T> {
type State = ();
type Item<'world, 'state> = Self;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is better than just returning () because it's easier to debug.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also necessary, since SystemParams have to be reflexive :)

Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not totally sure that using PhantomData is a 1:1 replacement for world_query(ignore) and system_param(ignore) for all T: Default, though removing that functionality seems like a good idea to me to encourage users to use Local<T> instead.

crates/bevy_ecs/macros/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/macros/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/query/fetch.rs Outdated Show resolved Hide resolved
@JoJoJet JoJoJet added the C-Bug An unexpected or incorrect behavior label Mar 24, 2023
@JoJoJet JoJoJet removed the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Mar 28, 2023
@alice-i-cecile alice-i-cecile added this to the 0.10.1 milestone Mar 28, 2023
@JoJoJet
Copy link
Member Author

JoJoJet commented Mar 28, 2023

I figured out how to do this without removing the ignore attributes, so this PR is now non-breaking. I will remove the attributes in a follow-up PR.

@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 30, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 30, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 30, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 30, 2023
Merged via the queue into bevyengine:main with commit d9113cc Mar 30, 2023
@JoJoJet JoJoJet deleted the system-param-phantom-data branch March 30, 2023 15:56
james7132 pushed a commit that referenced this pull request Mar 30, 2023
# Objective

Follow-up to #8030.

Now that `SystemParam` and `WorldQuery` are implemented for
`PhantomData`, the `ignore` attributes are now unnecessary.

---

## Changelog

- Removed the attributes `#[system_param(ignore)]` and
`#[world_query(ignore)]`.

## Migration Guide

The attributes `#[system_param(ignore)]` and `#[world_query]` ignore
have been removed. If you were using either of these with `PhantomData`
fields, you can simply remove the attribute:

```rust
#[derive(SystemParam)]
struct MyParam<'w, 's, Marker> {
    ...
    // Before:
    #[system_param(ignore)
    _marker: PhantomData<Marker>,

    // After:
    _marker: PhantomData<Marker>,
}
#[derive(WorldQuery)]
struct MyQuery<Marker> {
    ...
    // Before:
    #[world_query(ignore)
    _marker: PhantomData<Marker>,

    // After:
    _marker: PhantomData<Marker>,
}
```

If you were using this for another type that implements `Default`,
consider wrapping that type in `Local<>` (this only works for
`SystemParam`):

```rust
#[derive(SystemParam)]
struct MyParam<'w, 's> {
    // Before:
    #[system_param(ignore)]
    value: MyDefaultType, // This will be initialized using `Default` each time `MyParam` is created.

    // After:
    value: Local<MyDefaultType>, // This will be initialized using `Default` the first time `MyParam` is created.
}
```

If you are implementing either trait and need to preserve the exact
behavior of the old `ignore` attributes, consider manually implementing
`SystemParam` or `WorldQuery` for a wrapper struct that uses the
`Default` trait:

```rust
// Before:

#[derive(WorldQuery)
struct MyQuery {
   #[world_query(ignore)]
    str: String,
}

// After:

#[derive(WorldQuery)
struct MyQuery {
    str: DefaultQuery<String>,
}

pub struct DefaultQuery<T: Default>(pub T);

unsafe impl<T: Default> WorldQuery for DefaultQuery<T> {
    type Item<'w> = Self;
    ...
    unsafe fn fetch<'w>(...) -> Self::Item<'w> {
        Self(T::default())
    }
}
```
JoJoJet added a commit to JoJoJet/bevy that referenced this pull request 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 pull request 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 pull request 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 C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SystemParam: lifetime may not live long enough
3 participants