forestia/bevy
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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

Browse Source 
# 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.
This commit is contained in:
JoJoJet 2023-03-30 11:12:26 -04:00 committed by GitHub
parent f219a08907
commit d9113cca6f
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 178 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
crates/bevy_ecs/macros/src/fetch.rs
View File

@ -396,7 +396,7 @@ pub fn derive_world_query_impl(input: TokenStream) -> TokenStream {
#[automatically_derived]
#visibility struct #state_struct_name #user_impl_generics #user_where_clauses {
#(#field_idents: <#field_types as #path::query::WorldQuery>::State,)*
#(#ignored_field_idents: #ignored_field_types,)*
#(#ignored_field_idents: ::std::marker::PhantomData<fn() -> #ignored_field_types>,)*
}
#mutable_impl
@ -437,7 +437,7 @@ pub fn derive_world_query_impl(input: TokenStream) -> TokenStream {
}
struct WorldQueryFieldInfo {
/// Has `#[fetch(ignore)]` or `#[filter_fetch(ignore)]` attribute.
/// Has the `#[world_query(ignore)]` attribute.
is_ignored: bool,
/// All field attributes except for `world_query` ones.
attrs: Vec<Attribute>,

69
crates/bevy_ecs/macros/src/lib.rs
View File

@ -14,7 +14,7 @@ use proc_macro2::Span;
use quote::{format_ident, quote};
use syn::{
parse::ParseStream, parse_macro_input, parse_quote, punctuated::Punctuated, spanned::Spanned,
ConstParam, DeriveInput, Field, GenericParam, Ident, Index, Meta, MetaList, NestedMeta, Token,
ConstParam, DeriveInput, GenericParam, Ident, Index, Meta, MetaList, NestedMeta, Token,
TypeParam,
};
@ -264,7 +264,7 @@ static SYSTEM_PARAM_ATTRIBUTE_NAME: &str = "system_param";
pub fn derive_system_param(input: TokenStream) -> TokenStream {
let token_stream = input.clone();
let ast = parse_macro_input!(input as DeriveInput);
let syn::Data::Struct(syn::DataStruct { fields: field_definitions, ..}) = ast.data else {
let syn::Data::Struct(syn::DataStruct { fields: field_definitions, .. }) = ast.data else {
return syn::Error::new(ast.span(), "Invalid `SystemParam` type: expected a `struct`")
.into_compile_error()
.into();
@ -295,7 +295,8 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
}),
)
})
.collect::<Vec<(&Field, SystemParamFieldAttributes)>>();
.collect::<Vec<_>>();
let mut field_locals = Vec::new();
let mut fields = Vec::new();
let mut field_types = Vec::new();
@ -346,11 +347,7 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
.filter(|g| !matches!(g, GenericParam::Lifetime(_)))
.collect();
let mut shadowed_lifetimes: Vec<_> = generics.lifetimes().map(|x| x.lifetime.clone()).collect();
for lifetime in &mut shadowed_lifetimes {
let shadowed_ident = format_ident!("_{}", lifetime.ident);
lifetime.ident = shadowed_ident;
}
let shadowed_lifetimes: Vec<_> = generics.lifetimes().map(|_| quote!('_)).collect();
let mut punctuated_generics = Punctuated::<_, Token![,]>::new();
punctuated_generics.extend(lifetimeless_generics.iter().map(|g| match g {
@ -372,9 +369,27 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
_ => unreachable!(),
}));
let punctuated_generics_no_bounds: Punctuated<_, Token![,]> = lifetimeless_generics
.iter()
.map(|&g| match g.clone() {
GenericParam::Type(mut g) => {
g.bounds.clear();
GenericParam::Type(g)
}
g => g,
})
.collect();
let mut tuple_types: Vec<_> = field_types.iter().map(|x| quote! { #x }).collect();
let mut tuple_patterns: Vec<_> = field_locals.iter().map(|x| quote! { #x }).collect();
tuple_types.extend(
ignored_field_types
.iter()
.map(|ty| parse_quote!(::std::marker::PhantomData::<#ty>)),
);
tuple_patterns.extend(ignored_field_types.iter().map(|_| parse_quote!(_)));
// If the number of fields exceeds the 16-parameter limit,
// fold the fields into tuples of tuples until we are below the limit.
const LIMIT: usize = 16;
@ -385,6 +400,7 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
let end = Vec::from_iter(tuple_patterns.drain(..LIMIT));
tuple_patterns.push(parse_quote!( (#(#end,)*) ));
}
// Create a where clause for the `ReadOnlySystemParam` impl.
// Ensure that each field implements `ReadOnlySystemParam`.
let mut read_only_generics = generics.clone();
@ -395,6 +411,9 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
.push(syn::parse_quote!(#field_type: #path::system::ReadOnlySystemParam));
}
let fields_alias =
ensure_no_collision(format_ident!("__StructFieldsAlias"), token_stream.clone());
let struct_name = &ast.ident;
let state_struct_visibility = &ast.vis;
let state_struct_name = ensure_no_collision(format_ident!("FetchState"), token_stream);
@ -404,41 +423,41 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
// The struct can still be accessed via SystemParam::State, e.g. EventReaderState can be accessed via
// <EventReader<'static, 'static, T> as SystemParam>::State
const _: () = {
// Allows rebinding the lifetimes of each field type.
type #fields_alias <'w, 's, #punctuated_generics_no_bounds> = (#(#tuple_types,)*);
#[doc(hidden)]
#state_struct_visibility struct #state_struct_name <'w, 's, #(#lifetimeless_generics,)*>
#state_struct_visibility struct #state_struct_name <#(#lifetimeless_generics,)*>
#where_clause {
state: (#(<#tuple_types as #path::system::SystemParam>::State,)*),
marker: std::marker::PhantomData<(
<#path::prelude::Query<'w, 's, ()> as #path::system::SystemParam>::State,
#(fn() -> #ignored_field_types,)*
)>,
state: <#fields_alias::<'static, 'static, #punctuated_generic_idents> as #path::system::SystemParam>::State,
}
unsafe impl<'w, 's, #punctuated_generics> #path::system::SystemParam for #struct_name #ty_generics #where_clause {
type State = #state_struct_name<'static, 'static, #punctuated_generic_idents>;
type Item<'_w, '_s> = #struct_name <#(#shadowed_lifetimes,)* #punctuated_generic_idents>;
unsafe impl<#punctuated_generics> #path::system::SystemParam for
#struct_name <#(#shadowed_lifetimes,)* #punctuated_generic_idents> #where_clause
{
type State = #state_struct_name<#punctuated_generic_idents>;
type Item<'w, 's> = #struct_name #ty_generics;
fn init_state(world: &mut #path::world::World, system_meta: &mut #path::system::SystemMeta) -> Self::State {
#state_struct_name {
state: <(#(#tuple_types,)*) as #path::system::SystemParam>::init_state(world, system_meta),
marker: std::marker::PhantomData,
state: <#fields_alias::<'_, '_, #punctuated_generic_idents> as #path::system::SystemParam>::init_state(world, system_meta),
}
}
fn new_archetype(state: &mut Self::State, archetype: &#path::archetype::Archetype, system_meta: &mut #path::system::SystemMeta) {
<(#(#tuple_types,)*) as #path::system::SystemParam>::new_archetype(&mut state.state, archetype, system_meta)
<#fields_alias::<'_, '_, #punctuated_generic_idents> as #path::system::SystemParam>::new_archetype(&mut state.state, archetype, system_meta)
}
fn apply(state: &mut Self::State, system_meta: &#path::system::SystemMeta, world: &mut #path::world::World) {
<(#(#tuple_types,)*) as #path::system::SystemParam>::apply(&mut state.state, system_meta, world);
<#fields_alias::<'_, '_, #punctuated_generic_idents> as #path::system::SystemParam>::apply(&mut state.state, system_meta, world);
}
unsafe fn get_param<'w2, 's2>(
state: &'s2 mut Self::State,
unsafe fn get_param<'w, 's>(
state: &'s mut Self::State,
system_meta: &#path::system::SystemMeta,
world: &'w2 #path::world::World,
world: &'w #path::world::World,
change_tick: #path::component::Tick,
) -> Self::Item<'w2, 's2> {
) -> Self::Item<'w, 's> {
let (#(#tuple_patterns,)*) = <
(#(#tuple_types,)*) as #path::system::SystemParam
>::get_param(&mut state.state, system_meta, world, change_tick);

100
crates/bevy_ecs/src/query/fetch.rs
View File

@ -278,6 +278,24 @@ use std::{cell::UnsafeCell, marker::PhantomData};
/// # bevy_ecs::system::assert_is_system(my_system);
/// ```
///
/// # Generic Queries
///
/// When writing generic code, it is often necessary to use [`PhantomData`]
/// to constrain type parameters. Since `WorldQuery` is implemented for all
/// `PhantomData<T>` types, this pattern can be used with this macro.
///
/// ```
/// # use bevy_ecs::{prelude::*, query::WorldQuery};
/// # use std::marker::PhantomData;
/// #[derive(WorldQuery)]
/// pub struct GenericQuery<T> {
/// id: Entity,
/// marker: PhantomData<T>,
/// }
/// # fn my_system(q: Query<GenericQuery<()>>) {}
/// # bevy_ecs::system::assert_is_system(my_system);
/// ```
///
/// # Safety
///
/// Component access of `Self::ReadOnly` must be a subset of `Self`
@ -1315,7 +1333,6 @@ macro_rules! impl_anytuple_fetch {
/// SAFETY: each item in the tuple is read only
unsafe impl<$($name: ReadOnlyWorldQuery),*> ReadOnlyWorldQuery for AnyOf<($($name,)*)> {}
};
}
@ -1389,6 +1406,71 @@ unsafe impl<Q: WorldQuery> WorldQuery for NopWorldQuery<Q> {
/// SAFETY: `NopFetch` never accesses any data
unsafe impl<Q: WorldQuery> ReadOnlyWorldQuery for NopWorldQuery<Q> {}
/// SAFETY: `PhantomData` never accesses any world data.
unsafe impl<T: ?Sized> WorldQuery for PhantomData<T> {
type Item<'a> = ();
type Fetch<'a> = ();
type ReadOnly = Self;
type State = ();
fn shrink<'wlong: 'wshort, 'wshort>(_item: Self::Item<'wlong>) -> Self::Item<'wshort> {}
unsafe fn init_fetch<'w>(
_world: &'w World,
_state: &Self::State,
_last_run: Tick,
_this_run: Tick,
) -> Self::Fetch<'w> {
}
unsafe fn clone_fetch<'w>(_fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {}
// `PhantomData` does not match any components, so all components it matches
// are stored in a Table (vacuous truth).
const IS_DENSE: bool = true;
// `PhantomData` matches every entity in each archetype.
const IS_ARCHETYPAL: bool = true;
unsafe fn set_archetype<'w>(
_fetch: &mut Self::Fetch<'w>,
_state: &Self::State,
_archetype: &'w Archetype,
_table: &'w Table,
) {
}
unsafe fn set_table<'w>(_fetch: &mut Self::Fetch<'w>, _state: &Self::State, _table: &'w Table) {
}
unsafe fn fetch<'w>(
_fetch: &mut Self::Fetch<'w>,
_entity: Entity,
_table_row: TableRow,
) -> Self::Item<'w> {
}
fn update_component_access(_state: &Self::State, _access: &mut FilteredAccess<ComponentId>) {}
fn update_archetype_component_access(
_state: &Self::State,
_archetype: &Archetype,
_access: &mut Access<ArchetypeComponentId>,
) {
}
fn init_state(_world: &mut World) -> Self::State {}
fn matches_component_set(
_state: &Self::State,
_set_contains_id: &impl Fn(ComponentId) -> bool,
) -> bool {
true
}
}
/// SAFETY: `PhantomData` never accesses any world data.
unsafe impl<T: ?Sized> ReadOnlyWorldQuery for PhantomData<T> {}
#[cfg(test)]
mod tests {
use super::*;
@ -1397,6 +1479,22 @@ mod tests {
#[derive(Component)]
pub struct A;
// Compile test for https://github.com/bevyengine/bevy/pull/8030.
#[test]
fn world_query_phantom_data() {
#[derive(WorldQuery)]
pub struct IgnoredQuery<Marker> {
id: Entity,
#[world_query(ignore)]
_marker: PhantomData<Marker>,
_marker2: PhantomData<Marker>,
}
fn ignored_system(_: Query<IgnoredQuery<()>>) {}
crate::system::assert_is_system(ignored_system);
}
// Ensures that each field of a `WorldQuery` struct's read-only variant
// has the same visibility as its corresponding mutable field.
#[test]

34
crates/bevy_ecs/src/system/system_param.rs
View File

@ -19,6 +19,7 @@ use bevy_utils::{all_tuples, synccell::SyncCell};
use std::{
borrow::Cow,
fmt::Debug,
marker::PhantomData,
ops::{Deref, DerefMut},
};
@ -65,6 +66,11 @@ use std::{
/// # bevy_ecs::system::assert_is_system(my_system::<()>);
/// ```
///
/// ## `PhantomData`
///
/// [`PhantomData`] is a special type of `SystemParam` that does nothing.
/// This is useful for constraining generic types or lifetimes.
///
/// # Generic `SystemParam`s
///
/// When using the derive macro, you may see an error in the form of:
@ -1466,7 +1472,6 @@ pub mod lifetimeless {
/// #[derive(SystemParam)]
/// struct GenericParam<'w, 's, T: SystemParam> {
/// field: T,
/// #[system_param(ignore)]
/// // Use the lifetimes in this type, or they will be unbound.
/// phantom: core::marker::PhantomData<&'w &'s ()>
/// }
@ -1532,6 +1537,26 @@ unsafe impl<P: SystemParam + 'static> SystemParam for StaticSystemParam<'_, '_,
}
}
// SAFETY: No world access.
unsafe impl<T: ?Sized> SystemParam for PhantomData<T> {
type State = ();
type Item<'world, 'state> = Self;
fn init_state(_world: &mut World, _system_meta: &mut SystemMeta) -> Self::State {}
unsafe fn get_param<'world, 'state>(
_state: &'state mut Self::State,
_system_meta: &SystemMeta,
_world: &'world World,
_change_tick: Tick,
) -> Self::Item<'world, 'state> {
PhantomData
}
}
// SAFETY: No world access.
unsafe impl<T: ?Sized> ReadOnlySystemParam for PhantomData<T> {}
#[cfg(test)]
mod tests {
use super::*;
@ -1606,6 +1631,7 @@ mod tests {
_foo: Res<'w, T>,
#[system_param(ignore)]
marker: PhantomData<&'w Marker>,
marker2: PhantomData<&'w Marker>,
}
// Compile tests for https://github.com/bevyengine/bevy/pull/6957.
@ -1643,4 +1669,10 @@ mod tests {
#[derive(Resource)]
pub struct FetchState;
// Regression test for https://github.com/bevyengine/bevy/issues/8192.
#[derive(SystemParam)]
pub struct InvariantParam<'w, 's> {
_set: ParamSet<'w, 's, (Query<'w, 's, ()>,)>,
}
}