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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
c766e7c
impl `SystemParam` for `PhantomData`
JoJoJet Mar 10, 2023
8994d13
ensure state types don't collide for `SystemParam`
JoJoJet Mar 10, 2023
92d7921
add a dedicated method for generating identifiers with no collision
JoJoJet Mar 10, 2023
1c6d01f
add a comment describing the collision algorithm
JoJoJet Mar 10, 2023
a25fb60
use explicit lifetimes in the `SystemParam` derive
JoJoJet Mar 10, 2023
65f332e
remove `#system_param(ignore)`
JoJoJet Mar 10, 2023
43507a6
clean up state struct lifetimes
JoJoJet Mar 10, 2023
6df2d0a
Merge remote-tracking branch 'upstream/main' into system-param-phanto…
JoJoJet Mar 10, 2023
4e5797c
remove unused attributes
JoJoJet Mar 10, 2023
6eac07a
use a descriptive name for a macro variable
JoJoJet Mar 10, 2023
653fc46
implement `WorldQuery` for `PhantomData`
JoJoJet Mar 10, 2023
e18e7af
add a compile test
JoJoJet Mar 10, 2023
715f8ec
remove `#[world_query(ignore)]`
JoJoJet Mar 10, 2023
f60909b
make `world_query(ignore)` an error
JoJoJet Mar 10, 2023
4ee9978
add comments to `IS_DENSE` and `IS_ARCHETYPAL`
JoJoJet Mar 10, 2023
3a55ca1
update a comment
JoJoJet Mar 10, 2023
c4005ac
disallow `#[system_param]` attributes
JoJoJet Mar 11, 2023
136033c
Merge remote-tracking branch 'upstream/main' into system-param-phanto…
JoJoJet Mar 11, 2023
eaddf9a
remove an unused const
JoJoJet Mar 11, 2023
b2f4309
undo an overlapping change
JoJoJet Mar 13, 2023
0ab464c
add an example demonstrating `PhantomData` queries
JoJoJet Mar 17, 2023
978437a
import worldquery
JoJoJet Mar 17, 2023
08d6b60
remove collision code
JoJoJet Mar 18, 2023
244a589
remove needless whitespace
JoJoJet Mar 18, 2023
6f5d446
Merge remote-tracking branch 'upstream/main' into system-param-phanto…
JoJoJet Mar 18, 2023
3f0bd19
remove an unused import
JoJoJet Mar 18, 2023
47040f2
Merge remote-tracking branch 'upstream/main' into system-param-phanto…
JoJoJet Mar 21, 2023
988ec67
fix formatting for a `let ... else` block
JoJoJet Mar 21, 2023
986f10c
Merge remote-tracking branch 'upstream/main' into system-param-phanto…
JoJoJet Mar 22, 2023
aebb03d
Merge remote-tracking branch 'upstream/main' into system-param-phanto…
JoJoJet Mar 22, 2023
589361e
remove a space
JoJoJet Mar 22, 2023
5dfc332
use non-conflicting lifetime names
JoJoJet Mar 23, 2023
93624c3
add a test case for invariant lifetimes
JoJoJet Mar 24, 2023
eb3e17d
re-add `#[system_param(ignore)]`
JoJoJet Mar 28, 2023
f3572b1
re-add `#[world_query(ignore)]`
JoJoJet Mar 28, 2023
bcfee71
restore some docs
JoJoJet Mar 28, 2023
10edef0
cargo fmt
JoJoJet Mar 28, 2023
f8469c5
re-order an example
JoJoJet Mar 28, 2023
2579709
allow unlimited ignored fields
JoJoJet Mar 28, 2023
6a3cfce
reduce churn
JoJoJet Mar 28, 2023
8ff0178
Update crates/bevy_ecs/macros/src/lib.rs
JoJoJet Mar 29, 2023
464e095
deduplicate regression tests
JoJoJet Mar 29, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions crates/bevy_ecs/macros/src/fetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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>,
Expand Down
69 changes: 44 additions & 25 deletions crates/bevy_ecs/macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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 {
Expand All @@ -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;
Expand All @@ -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();
Expand All @@ -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);
Expand All @@ -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);
Expand Down
100 changes: 99 additions & 1 deletion crates/bevy_ecs/src/query/fetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down Expand Up @@ -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,)*)> {}

};
}

Expand Down Expand Up @@ -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;
JoJoJet marked this conversation as resolved.
Show resolved Hide resolved
// `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::*;
Expand All @@ -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]
Expand Down
34 changes: 33 additions & 1 deletion crates/bevy_ecs/src/system/system_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use bevy_utils::{all_tuples, synccell::SyncCell};
use std::{
borrow::Cow,
fmt::Debug,
marker::PhantomData,
ops::{Deref, DerefMut},
};

Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -1465,7 +1471,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 ()>
/// }
Expand Down Expand Up @@ -1531,6 +1536,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;
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 :)


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::*;
Expand Down Expand Up @@ -1605,6 +1630,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.
Expand Down Expand Up @@ -1642,4 +1668,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, ()>,)>,
}
}