From 3ef99cf82c46579f3dd87305012d9325601ca7f7 Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Wed, 18 Dec 2024 13:30:46 -0500 Subject: [PATCH] Replace `impl_param_set` proc macro with a `macro_rules` macro (#16847) # Objective Simplify the code by using `macro_rules` instead of a proc macro where possible. ## Solution Replace `impl_param_set` proc macro with a `macro_rules` macro. --- crates/bevy_app/Cargo.toml | 2 +- crates/bevy_ecs/Cargo.toml | 2 +- crates/bevy_ecs/macros/src/lib.rs | 133 +-------------------- crates/bevy_ecs/src/system/system_param.rs | 104 +++++++++++++++- crates/bevy_reflect/Cargo.toml | 2 +- crates/bevy_render/Cargo.toml | 2 +- crates/bevy_state/Cargo.toml | 2 +- 7 files changed, 107 insertions(+), 140 deletions(-) diff --git a/crates/bevy_app/Cargo.toml b/crates/bevy_app/Cargo.toml index 74c00dd78a807..a42deb8efd74b 100644 --- a/crates/bevy_app/Cargo.toml +++ b/crates/bevy_app/Cargo.toml @@ -30,7 +30,7 @@ bevy_tasks = { path = "../bevy_tasks", version = "0.15.0-dev" } # other downcast-rs = "1.2.0" thiserror = { version = "2", default-features = false } -variadics_please = "1.0" +variadics_please = "1.1" [target.'cfg(not(target_arch = "wasm32"))'.dependencies] ctrlc = "3.4.4" diff --git a/crates/bevy_ecs/Cargo.toml b/crates/bevy_ecs/Cargo.toml index df8d850d7ac87..ba5893046a376 100644 --- a/crates/bevy_ecs/Cargo.toml +++ b/crates/bevy_ecs/Cargo.toml @@ -119,7 +119,7 @@ nonmax = { version = "0.5", default-features = false } arrayvec = { version = "0.7.4", default-features = false, optional = true } smallvec = { version = "1", features = ["union"] } indexmap = { version = "2.5.0", default-features = false } -variadics_please = { version = "1.0", default-features = false } +variadics_please = { version = "1.1", default-features = false } critical-section = { version = "1.2.0", optional = true } portable-atomic = { version = "1", default-features = false, features = [ "fallback", diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index 0d4103d890399..0893e721aef8e 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -12,12 +12,11 @@ mod world_query; use crate::{query_data::derive_query_data_impl, query_filter::derive_query_filter_impl}; use bevy_macro_utils::{derive_label, ensure_no_collision, get_struct_fields, BevyManifest}; use proc_macro::TokenStream; -use proc_macro2::Span; use proc_macro2::TokenStream as TokenStream2; use quote::{format_ident, quote}; use syn::{ parse_macro_input, parse_quote, punctuated::Punctuated, spanned::Spanned, token::Comma, - ConstParam, DeriveInput, GenericParam, Ident, Index, TypeParam, + ConstParam, DeriveInput, GenericParam, Index, TypeParam, }; enum BundleFieldKind { @@ -284,136 +283,6 @@ pub fn derive_visit_entities(input: TokenStream) -> TokenStream { }) } -fn get_idents(fmt_string: fn(usize) -> String, count: usize) -> Vec { - (0..count) - .map(|i| Ident::new(&fmt_string(i), Span::call_site())) - .collect::>() -} - -#[proc_macro] -pub fn impl_param_set(_input: TokenStream) -> TokenStream { - let mut tokens = TokenStream::new(); - let max_params = 8; - let params = get_idents(|i| format!("P{i}"), max_params); - let metas = get_idents(|i| format!("m{i}"), max_params); - let mut param_fn_muts = Vec::new(); - for (i, param) in params.iter().enumerate() { - let fn_name = Ident::new(&format!("p{i}"), Span::call_site()); - let index = Index::from(i); - let ordinal = match i { - 1 => "1st".to_owned(), - 2 => "2nd".to_owned(), - 3 => "3rd".to_owned(), - x => format!("{x}th"), - }; - let comment = - format!("Gets exclusive access to the {ordinal} parameter in this [`ParamSet`]."); - param_fn_muts.push(quote! { - #[doc = #comment] - /// No other parameters may be accessed while this one is active. - pub fn #fn_name<'a>(&'a mut self) -> SystemParamItem<'a, 'a, #param> { - // SAFETY: systems run without conflicts with other systems. - // Conflicting params in ParamSet are not accessible at the same time - // ParamSets are guaranteed to not conflict with other SystemParams - unsafe { - #param::get_param(&mut self.param_states.#index, &self.system_meta, self.world, self.change_tick) - } - } - }); - } - - for param_count in 1..=max_params { - let param = ¶ms[0..param_count]; - let meta = &metas[0..param_count]; - let param_fn_mut = ¶m_fn_muts[0..param_count]; - tokens.extend(TokenStream::from(quote! { - // SAFETY: All parameters are constrained to ReadOnlySystemParam, so World is only read - unsafe impl<'w, 's, #(#param,)*> ReadOnlySystemParam for ParamSet<'w, 's, (#(#param,)*)> - where #(#param: ReadOnlySystemParam,)* - { } - - // SAFETY: Relevant parameter ComponentId and ArchetypeComponentId access is applied to SystemMeta. If any ParamState conflicts - // with any prior access, a panic will occur. - unsafe impl<'_w, '_s, #(#param: SystemParam,)*> SystemParam for ParamSet<'_w, '_s, (#(#param,)*)> - { - type State = (#(#param::State,)*); - type Item<'w, 's> = ParamSet<'w, 's, (#(#param,)*)>; - - // Note: We allow non snake case so the compiler don't complain about the creation of non_snake_case variables - #[allow(non_snake_case)] - fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { - #( - // Pretend to add each param to the system alone, see if it conflicts - let mut #meta = system_meta.clone(); - #meta.component_access_set.clear(); - #meta.archetype_component_access.clear(); - #param::init_state(world, &mut #meta); - // The variable is being defined with non_snake_case here - let #param = #param::init_state(world, &mut system_meta.clone()); - )* - // Make the ParamSet non-send if any of its parameters are non-send. - if false #(|| !#meta.is_send())* { - system_meta.set_non_send(); - } - #( - system_meta - .component_access_set - .extend(#meta.component_access_set); - system_meta - .archetype_component_access - .extend(&#meta.archetype_component_access); - )* - (#(#param,)*) - } - - unsafe fn new_archetype(state: &mut Self::State, archetype: &Archetype, system_meta: &mut SystemMeta) { - // SAFETY: The caller ensures that `archetype` is from the World the state was initialized from in `init_state`. - unsafe { <(#(#param,)*) as SystemParam>::new_archetype(state, archetype, system_meta); } - } - - fn apply(state: &mut Self::State, system_meta: &SystemMeta, world: &mut World) { - <(#(#param,)*) as SystemParam>::apply(state, system_meta, world); - } - - fn queue(state: &mut Self::State, system_meta: &SystemMeta, mut world: DeferredWorld) { - <(#(#param,)*) as SystemParam>::queue(state, system_meta, world.reborrow()); - } - - #[inline] - unsafe fn validate_param<'w, 's>( - state: &'s Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell<'w>, - ) -> bool { - <(#(#param,)*) as SystemParam>::validate_param(state, system_meta, world) - } - - #[inline] - unsafe fn get_param<'w, 's>( - state: &'s mut Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell<'w>, - change_tick: Tick, - ) -> Self::Item<'w, 's> { - ParamSet { - param_states: state, - system_meta: system_meta.clone(), - world, - change_tick, - } - } - } - - impl<'w, 's, #(#param: SystemParam,)*> ParamSet<'w, 's, (#(#param,)*)> - { - #(#param_fn_mut)* - } - })); - } - - tokens -} - /// Implement `SystemParam` to use a struct as a parameter in a system #[proc_macro_derive(SystemParam, attributes(system_param))] pub fn derive_system_param(input: TokenStream) -> TokenStream { diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 36f6da9d50588..2b38fc6201146 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -17,7 +17,6 @@ use crate::{ }, }; use alloc::{borrow::ToOwned, boxed::Box, vec::Vec}; -use bevy_ecs_macros::impl_param_set; pub use bevy_ecs_macros::{Resource, SystemParam}; use bevy_ptr::UnsafeCellDeref; use bevy_utils::synccell::SyncCell; @@ -32,7 +31,7 @@ use core::{ use disqualified::ShortName; use super::Populated; -use variadics_please::all_tuples; +use variadics_please::{all_tuples, all_tuples_enumerated}; /// A parameter that can be used in a [`System`](super::System). /// @@ -674,7 +673,106 @@ pub struct ParamSet<'w, 's, T: SystemParam> { change_tick: Tick, } -impl_param_set!(); +macro_rules! impl_param_set { + ($(($index: tt, $param: ident, $system_meta: ident, $fn_name: ident)),*) => { + // SAFETY: All parameters are constrained to ReadOnlySystemParam, so World is only read + unsafe impl<'w, 's, $($param,)*> ReadOnlySystemParam for ParamSet<'w, 's, ($($param,)*)> + where $($param: ReadOnlySystemParam,)* + { } + + // SAFETY: Relevant parameter ComponentId and ArchetypeComponentId access is applied to SystemMeta. If any ParamState conflicts + // with any prior access, a panic will occur. + unsafe impl<'_w, '_s, $($param: SystemParam,)*> SystemParam for ParamSet<'_w, '_s, ($($param,)*)> + { + type State = ($($param::State,)*); + type Item<'w, 's> = ParamSet<'w, 's, ($($param,)*)>; + + // Note: We allow non snake case so the compiler don't complain about the creation of non_snake_case variables + #[allow(non_snake_case)] + fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { + $( + // Pretend to add each param to the system alone, see if it conflicts + let mut $system_meta = system_meta.clone(); + $system_meta.component_access_set.clear(); + $system_meta.archetype_component_access.clear(); + $param::init_state(world, &mut $system_meta); + // The variable is being defined with non_snake_case here + let $param = $param::init_state(world, &mut system_meta.clone()); + )* + // Make the ParamSet non-send if any of its parameters are non-send. + if false $(|| !$system_meta.is_send())* { + system_meta.set_non_send(); + } + $( + system_meta + .component_access_set + .extend($system_meta.component_access_set); + system_meta + .archetype_component_access + .extend(&$system_meta.archetype_component_access); + )* + ($($param,)*) + } + + unsafe fn new_archetype(state: &mut Self::State, archetype: &Archetype, system_meta: &mut SystemMeta) { + // SAFETY: The caller ensures that `archetype` is from the World the state was initialized from in `init_state`. + unsafe { <($($param,)*) as SystemParam>::new_archetype(state, archetype, system_meta); } + } + + fn apply(state: &mut Self::State, system_meta: &SystemMeta, world: &mut World) { + <($($param,)*) as SystemParam>::apply(state, system_meta, world); + } + + fn queue(state: &mut Self::State, system_meta: &SystemMeta, mut world: DeferredWorld) { + <($($param,)*) as SystemParam>::queue(state, system_meta, world.reborrow()); + } + + #[inline] + unsafe fn validate_param<'w, 's>( + state: &'s Self::State, + system_meta: &SystemMeta, + world: UnsafeWorldCell<'w>, + ) -> bool { + <($($param,)*) as SystemParam>::validate_param(state, system_meta, world) + } + + #[inline] + unsafe fn get_param<'w, 's>( + state: &'s mut Self::State, + system_meta: &SystemMeta, + world: UnsafeWorldCell<'w>, + change_tick: Tick, + ) -> Self::Item<'w, 's> { + ParamSet { + param_states: state, + system_meta: system_meta.clone(), + world, + change_tick, + } + } + } + + impl<'w, 's, $($param: SystemParam,)*> ParamSet<'w, 's, ($($param,)*)> + { + $( + /// Gets exclusive access to the parameter at index + #[doc = stringify!($index)] + /// in this [`ParamSet`]. + /// No other parameters may be accessed while this one is active. + pub fn $fn_name<'a>(&'a mut self) -> SystemParamItem<'a, 'a, $param> { + // SAFETY: systems run without conflicts with other systems. + // Conflicting params in ParamSet are not accessible at the same time + // ParamSets are guaranteed to not conflict with other SystemParams + unsafe { + $param::get_param(&mut self.param_states.$index, &self.system_meta, self.world, self.change_tick) + } + } + )* + } + } +} + +all_tuples_enumerated!(impl_param_set, 1, 8, P, m, p); /// A type that can be inserted into a [`World`] as a singleton. /// diff --git a/crates/bevy_reflect/Cargo.toml b/crates/bevy_reflect/Cargo.toml index 5879c1cac7de8..193a0b7c86b62 100644 --- a/crates/bevy_reflect/Cargo.toml +++ b/crates/bevy_reflect/Cargo.toml @@ -75,7 +75,7 @@ uuid = { version = "1.0", default-features = false, optional = true, features = "v4", "serde", ] } -variadics_please = "1.0" +variadics_please = "1.1" wgpu-types = { version = "23", features = ["serde"], optional = true } [dev-dependencies] diff --git a/crates/bevy_render/Cargo.toml b/crates/bevy_render/Cargo.toml index 01031e983bb8d..d584a4bc044ff 100644 --- a/crates/bevy_render/Cargo.toml +++ b/crates/bevy_render/Cargo.toml @@ -92,7 +92,7 @@ async-channel = "2.3.0" nonmax = "0.5" smallvec = { version = "1.11", features = ["const_new"] } offset-allocator = "0.2" -variadics_please = "1.0" +variadics_please = "1.1" [target.'cfg(not(target_arch = "wasm32"))'.dependencies] # Omit the `glsl` feature in non-WebAssembly by default. diff --git a/crates/bevy_state/Cargo.toml b/crates/bevy_state/Cargo.toml index a4e93255f7c6e..30ddcfd6eddc2 100644 --- a/crates/bevy_state/Cargo.toml +++ b/crates/bevy_state/Cargo.toml @@ -23,7 +23,7 @@ bevy_utils = { path = "../bevy_utils", version = "0.15.0-dev" } bevy_reflect = { path = "../bevy_reflect", version = "0.15.0-dev", optional = true } bevy_app = { path = "../bevy_app", version = "0.15.0-dev", optional = true } bevy_hierarchy = { path = "../bevy_hierarchy", version = "0.15.0-dev", optional = true } -variadics_please = "1.0" +variadics_please = "1.1" [lints] workspace = true