From 7a40865e68643b307d2b69deef3904f5643068d5 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Thu, 9 Mar 2023 20:20:46 -0500 Subject: [PATCH 01/17] add a regression test --- crates/bevy_ecs/src/system/system_param.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 018c79630f455..54fb8ff7bc011 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1636,4 +1636,10 @@ mod tests { { _q: Query<'w, 's, Q, ()>, } + + pub trait FetchState {} + + // regression test for https://github.com/bevyengine/bevy/issues/8010. + #[derive(SystemParam)] + pub struct Collide(T); } From 0bf2c7f1e32cc7e202d91a35316050a5d9c19780 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Thu, 9 Mar 2023 20:33:48 -0500 Subject: [PATCH 02/17] fix the regression test --- crates/bevy_ecs/src/system/system_param.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 54fb8ff7bc011..e77611831a1c0 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1641,5 +1641,8 @@ mod tests { // regression test for https://github.com/bevyengine/bevy/issues/8010. #[derive(SystemParam)] - pub struct Collide(T); + pub struct Collide { + #[system_param(ignore)] + _marker: PhantomData, + } } From 7ad905cc24c9084073e1204a820713aea0a37e14 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Thu, 9 Mar 2023 22:18:41 -0500 Subject: [PATCH 03/17] ensure state types don't collide for `SystemParam` --- crates/bevy_ecs/macros/src/lib.rs | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index dbafe0d9e3ead..a71a84303956f 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -7,7 +7,7 @@ mod states; use crate::{fetch::derive_world_query_impl, set::derive_set}; use bevy_macro_utils::{derive_boxed_label, get_named_struct_fields, BevyManifest}; -use proc_macro::TokenStream; +use proc_macro::{TokenStream, TokenTree}; use proc_macro2::Span; use quote::{format_ident, quote}; use syn::{ @@ -255,9 +255,19 @@ struct SystemParamFieldAttributes { static SYSTEM_PARAM_ATTRIBUTE_NAME: &str = "system_param"; +#[allow(clippy::cmp_owned)] +fn check_for_collision(haystack: TokenStream, value: &Ident) -> bool { + haystack.into_iter().any(|t| match t { + TokenTree::Group(g) => check_for_collision(g.stream(), value), + TokenTree::Ident(ident) => ident.to_string() == value.to_string(), + TokenTree::Punct(_) | TokenTree::Literal(_) => false, + }) +} + /// 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 { + 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 { return syn::Error::new(ast.span(), "Invalid `SystemParam` type: expected a `struct`") @@ -392,6 +402,10 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { let struct_name = &ast.ident; let state_struct_visibility = &ast.vis; + let mut state_struct_name = format_ident!("FetchState"); + while check_for_collision(token_stream.clone(), &state_struct_name) { + state_struct_name = format_ident!("{state_struct_name}A"); + } TokenStream::from(quote! { // We define the FetchState struct in an anonymous scope to avoid polluting the user namespace. @@ -399,7 +413,7 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { // as SystemParam>::State const _: () = { #[doc(hidden)] - #state_struct_visibility struct FetchState <'w, 's, #(#lifetimeless_generics,)*> + #state_struct_visibility struct #state_struct_name <'w, 's, #(#lifetimeless_generics,)*> #where_clause { state: (#(<#tuple_types as #path::system::SystemParam>::State,)*), marker: std::marker::PhantomData<( @@ -409,11 +423,11 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { } unsafe impl<'w, 's, #punctuated_generics> #path::system::SystemParam for #struct_name #ty_generics #where_clause { - type State = FetchState<'static, 'static, #punctuated_generic_idents>; + type State = #state_struct_name<'static, 'static, #punctuated_generic_idents>; type Item<'_w, '_s> = #struct_name <#(#shadowed_lifetimes,)* #punctuated_generic_idents>; fn init_state(world: &mut #path::world::World, system_meta: &mut #path::system::SystemMeta) -> Self::State { - FetchState { + #state_struct_name { state: <(#(#tuple_types,)*) as #path::system::SystemParam>::init_state(world, system_meta), marker: std::marker::PhantomData, } From accbab94153014dd4ffd8c22796d933d9d5016ee Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Thu, 9 Mar 2023 22:25:15 -0500 Subject: [PATCH 04/17] tweak regression test --- crates/bevy_ecs/src/system/system_param.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index e77611831a1c0..b285df5043a6d 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1637,12 +1637,12 @@ mod tests { _q: Query<'w, 's, Q, ()>, } - pub trait FetchState {} + #[derive(Resource)] + pub struct FetchState; - // regression test for https://github.com/bevyengine/bevy/issues/8010. + // regression test for https://github.com/bevyengine/bevy/issues/1727. #[derive(SystemParam)] - pub struct Collide { - #[system_param(ignore)] - _marker: PhantomData, + pub struct Collide<'w> { + _x: Res<'w, FetchState>, } } From df476360450a3b8ad34bd6638eb480a29b8834a8 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Thu, 9 Mar 2023 22:31:44 -0500 Subject: [PATCH 05/17] add a dedicated method for generating identifiers with no collision --- crates/bevy_ecs/macros/src/lib.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index a71a84303956f..557a5a19c1167 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -255,6 +255,7 @@ struct SystemParamFieldAttributes { static SYSTEM_PARAM_ATTRIBUTE_NAME: &str = "system_param"; +/// Checks if the specified identifier occurs in the specified [`TokenStream`]. #[allow(clippy::cmp_owned)] fn check_for_collision(haystack: TokenStream, value: &Ident) -> bool { haystack.into_iter().any(|t| match t { @@ -264,6 +265,13 @@ fn check_for_collision(haystack: TokenStream, value: &Ident) -> bool { }) } +fn ensure_no_collision(haystack: TokenStream, mut value: Ident) -> Ident { + while check_for_collision(haystack.clone(), &value) { + value = format_ident!("{value}X"); + } + value +} + /// 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 { @@ -402,10 +410,7 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { let struct_name = &ast.ident; let state_struct_visibility = &ast.vis; - let mut state_struct_name = format_ident!("FetchState"); - while check_for_collision(token_stream.clone(), &state_struct_name) { - state_struct_name = format_ident!("{state_struct_name}A"); - } + let state_struct_name = ensure_no_collision(token_stream, format_ident!("FetchState")); TokenStream::from(quote! { // We define the FetchState struct in an anonymous scope to avoid polluting the user namespace. From 952781fefc79470ece510d6a0df8dd14bff1a49d Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Thu, 9 Mar 2023 22:40:54 -0500 Subject: [PATCH 06/17] add a regression test for queries --- crates/bevy_ecs/src/query/fetch.rs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 3b4a6a076f143..1c6ccd0f5f01e 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -1388,3 +1388,22 @@ unsafe impl WorldQuery for NopWorldQuery { /// SAFETY: `NopFetch` never accesses any data unsafe impl ReadOnlyWorldQuery for NopWorldQuery {} + +#[cfg(test)] +mod tests { + use super::*; + use crate as bevy_ecs; + + // regression test for https://github.com/bevyengine/bevy/issues/8010. + + #[derive(WorldQuery)] + pub struct Client { + pub state: &'static S, + pub fetch: &'static ClientFetch, + } + + pub trait ClientState: Component {} + + #[derive(Component)] + pub struct ClientFetch; +} From 47b21b93b86da4d6c5e17809ee2760b477b082da Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Thu, 9 Mar 2023 22:50:38 -0500 Subject: [PATCH 07/17] prevent collisions for `WorldQuery` state and fetch types --- crates/bevy_ecs/macros/src/fetch.rs | 16 ++++++++++++---- crates/bevy_ecs/macros/src/lib.rs | 3 +-- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/crates/bevy_ecs/macros/src/fetch.rs b/crates/bevy_ecs/macros/src/fetch.rs index 8bb5597f0d292..4cfdf37946c49 100644 --- a/crates/bevy_ecs/macros/src/fetch.rs +++ b/crates/bevy_ecs/macros/src/fetch.rs @@ -1,9 +1,9 @@ use proc_macro::TokenStream; -use proc_macro2::{Ident, Span}; +use proc_macro2::{Ident, Span, TokenStream as TokenStream2}; use quote::{quote, ToTokens}; use syn::{ parse::{Parse, ParseStream}, - parse_quote, + parse_macro_input, parse_quote, punctuated::Punctuated, Attribute, Data, DataStruct, DeriveInput, Field, Fields, }; @@ -25,7 +25,10 @@ mod field_attr_keywords { pub static WORLD_QUERY_ATTRIBUTE_NAME: &str = "world_query"; -pub fn derive_world_query_impl(ast: DeriveInput) -> TokenStream { +pub fn derive_world_query_impl(input: TokenStream) -> TokenStream { + let tokens = input.clone(); + + let ast = parse_macro_input!(input as DeriveInput); let visibility = ast.vis; let mut fetch_struct_attributes = FetchStructAttributes::default(); @@ -104,13 +107,18 @@ pub fn derive_world_query_impl(ast: DeriveInput) -> TokenStream { }; let fetch_struct_name = Ident::new(&format!("{struct_name}Fetch"), Span::call_site()); + let fetch_struct_name = crate::ensure_no_collision(tokens.clone(), fetch_struct_name); let read_only_fetch_struct_name = if fetch_struct_attributes.is_mutable { - Ident::new(&format!("{struct_name}ReadOnlyFetch"), Span::call_site()) + let new_ident = Ident::new(&format!("{struct_name}ReadOnlyFetch"), Span::call_site()); + crate::ensure_no_collision(tokens.clone(), new_ident) } else { fetch_struct_name.clone() }; + // Generate a name for the state struct that doesn't conflict + // with the struct definition. let state_struct_name = Ident::new(&format!("{struct_name}State"), Span::call_site()); + let state_struct_name = crate::ensure_no_collision(tokens, state_struct_name); let fields = match &ast.data { Data::Struct(DataStruct { diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index 557a5a19c1167..60c5b0c47bfb6 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -471,8 +471,7 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { /// Implement `WorldQuery` to use a struct as a parameter in a query #[proc_macro_derive(WorldQuery, attributes(world_query))] pub fn derive_world_query(input: TokenStream) -> TokenStream { - let ast = parse_macro_input!(input as DeriveInput); - derive_world_query_impl(ast) + derive_world_query_impl(input) } /// Derive macro generating an impl of the trait `ScheduleLabel`. From 81fce59b221e05e46034f1ba8d0e5875b6520fb3 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Thu, 9 Mar 2023 22:57:20 -0500 Subject: [PATCH 08/17] add a comment describing the collision algorithm --- crates/bevy_ecs/macros/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index 60c5b0c47bfb6..911ffe2de928d 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -266,6 +266,8 @@ fn check_for_collision(haystack: TokenStream, value: &Ident) -> bool { } fn ensure_no_collision(haystack: TokenStream, mut value: Ident) -> Ident { + // If there's a collision, add more characters to the identifier + // until it doesn't collide with anything anymore. while check_for_collision(haystack.clone(), &value) { value = format_ident!("{value}X"); } From 9df684625579f01b8869cce82ca116ecdd7d3af7 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Thu, 9 Mar 2023 23:05:34 -0500 Subject: [PATCH 09/17] remove a redundant import --- crates/bevy_ecs/macros/src/fetch.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/macros/src/fetch.rs b/crates/bevy_ecs/macros/src/fetch.rs index 4cfdf37946c49..34b9ff60fe702 100644 --- a/crates/bevy_ecs/macros/src/fetch.rs +++ b/crates/bevy_ecs/macros/src/fetch.rs @@ -1,5 +1,5 @@ use proc_macro::TokenStream; -use proc_macro2::{Ident, Span, TokenStream as TokenStream2}; +use proc_macro2::{Ident, Span}; use quote::{quote, ToTokens}; use syn::{ parse::{Parse, ParseStream}, From 99f1883faa7532b766ec2d7cb24632777610a45b Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 13 Mar 2023 23:05:04 -0400 Subject: [PATCH 10/17] use a more efficient collision checking algorithm --- crates/bevy_ecs/macros/src/lib.rs | 33 +++++++++++++++++++------------ 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index 911ffe2de928d..d96b8b1ef5c44 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -255,23 +255,30 @@ struct SystemParamFieldAttributes { static SYSTEM_PARAM_ATTRIBUTE_NAME: &str = "system_param"; -/// Checks if the specified identifier occurs in the specified [`TokenStream`]. -#[allow(clippy::cmp_owned)] -fn check_for_collision(haystack: TokenStream, value: &Ident) -> bool { - haystack.into_iter().any(|t| match t { - TokenTree::Group(g) => check_for_collision(g.stream(), value), - TokenTree::Ident(ident) => ident.to_string() == value.to_string(), - TokenTree::Punct(_) | TokenTree::Literal(_) => false, - }) -} +fn ensure_no_collision(haystack: TokenStream, value: Ident) -> Ident { + fn visit_tokens(tokens: TokenStream, idents: &mut Vec) { + for t in tokens { + match t { + TokenTree::Group(g) => visit_tokens(g.stream(), idents), + TokenTree::Ident(ident) => idents.push(ident.to_string()), + TokenTree::Punct(_) | TokenTree::Literal(_) => {} + } + } + } + + let mut idents = Vec::new(); + visit_tokens(haystack, &mut idents); + + let span = value.span(); + let mut value = value.to_string(); -fn ensure_no_collision(haystack: TokenStream, mut value: Ident) -> Ident { // If there's a collision, add more characters to the identifier // until it doesn't collide with anything anymore. - while check_for_collision(haystack.clone(), &value) { - value = format_ident!("{value}X"); + while idents.iter().any(|i| *i == value) { + value.push('X'); } - value + + Ident::new(&value, span) } /// Implement `SystemParam` to use a struct as a parameter in a system From c1de12f6018277f55ee6aace5fa4f601d9980bed Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 13 Mar 2023 23:12:42 -0400 Subject: [PATCH 11/17] move `ensure_no_collision` to `bevy_macro_utils` --- crates/bevy_ecs/macros/src/fetch.rs | 7 +++--- crates/bevy_ecs/macros/src/lib.rs | 34 ++++----------------------- crates/bevy_macro_utils/src/lib.rs | 36 +++++++++++++++++++++++++++-- 3 files changed, 43 insertions(+), 34 deletions(-) diff --git a/crates/bevy_ecs/macros/src/fetch.rs b/crates/bevy_ecs/macros/src/fetch.rs index 34b9ff60fe702..3ae328e952074 100644 --- a/crates/bevy_ecs/macros/src/fetch.rs +++ b/crates/bevy_ecs/macros/src/fetch.rs @@ -1,3 +1,4 @@ +use bevy_macro_utils::ensure_no_collision; use proc_macro::TokenStream; use proc_macro2::{Ident, Span}; use quote::{quote, ToTokens}; @@ -107,10 +108,10 @@ pub fn derive_world_query_impl(input: TokenStream) -> TokenStream { }; let fetch_struct_name = Ident::new(&format!("{struct_name}Fetch"), Span::call_site()); - let fetch_struct_name = crate::ensure_no_collision(tokens.clone(), fetch_struct_name); + let fetch_struct_name = ensure_no_collision(fetch_struct_name, tokens.clone()); let read_only_fetch_struct_name = if fetch_struct_attributes.is_mutable { let new_ident = Ident::new(&format!("{struct_name}ReadOnlyFetch"), Span::call_site()); - crate::ensure_no_collision(tokens.clone(), new_ident) + ensure_no_collision(new_ident, tokens.clone()) } else { fetch_struct_name.clone() }; @@ -118,7 +119,7 @@ pub fn derive_world_query_impl(input: TokenStream) -> TokenStream { // Generate a name for the state struct that doesn't conflict // with the struct definition. let state_struct_name = Ident::new(&format!("{struct_name}State"), Span::call_site()); - let state_struct_name = crate::ensure_no_collision(tokens, state_struct_name); + let state_struct_name = ensure_no_collision(state_struct_name, tokens); let fields = match &ast.data { Data::Struct(DataStruct { diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index d96b8b1ef5c44..0055ad5c0b3a6 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -6,8 +6,10 @@ mod set; mod states; use crate::{fetch::derive_world_query_impl, set::derive_set}; -use bevy_macro_utils::{derive_boxed_label, get_named_struct_fields, BevyManifest}; -use proc_macro::{TokenStream, TokenTree}; +use bevy_macro_utils::{ + derive_boxed_label, ensure_no_collision, get_named_struct_fields, BevyManifest, +}; +use proc_macro::TokenStream; use proc_macro2::Span; use quote::{format_ident, quote}; use syn::{ @@ -255,32 +257,6 @@ struct SystemParamFieldAttributes { static SYSTEM_PARAM_ATTRIBUTE_NAME: &str = "system_param"; -fn ensure_no_collision(haystack: TokenStream, value: Ident) -> Ident { - fn visit_tokens(tokens: TokenStream, idents: &mut Vec) { - for t in tokens { - match t { - TokenTree::Group(g) => visit_tokens(g.stream(), idents), - TokenTree::Ident(ident) => idents.push(ident.to_string()), - TokenTree::Punct(_) | TokenTree::Literal(_) => {} - } - } - } - - let mut idents = Vec::new(); - visit_tokens(haystack, &mut idents); - - let span = value.span(); - let mut value = value.to_string(); - - // If there's a collision, add more characters to the identifier - // until it doesn't collide with anything anymore. - while idents.iter().any(|i| *i == value) { - value.push('X'); - } - - Ident::new(&value, span) -} - /// 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 { @@ -419,7 +395,7 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { let struct_name = &ast.ident; let state_struct_visibility = &ast.vis; - let state_struct_name = ensure_no_collision(token_stream, format_ident!("FetchState")); + let state_struct_name = ensure_no_collision(format_ident!("FetchState"), token_stream); TokenStream::from(quote! { // We define the FetchState struct in an anonymous scope to avoid polluting the user namespace. diff --git a/crates/bevy_macro_utils/src/lib.rs b/crates/bevy_macro_utils/src/lib.rs index 3fd275ceb9f48..2d6835f404b7e 100644 --- a/crates/bevy_macro_utils/src/lib.rs +++ b/crates/bevy_macro_utils/src/lib.rs @@ -8,10 +8,10 @@ pub use attrs::*; pub use shape::*; pub use symbol::*; -use proc_macro::TokenStream; +use proc_macro::{TokenStream, TokenTree}; use quote::{quote, quote_spanned}; use std::{env, path::PathBuf}; -use syn::spanned::Spanned; +use syn::{spanned::Spanned, Ident}; use toml_edit::{Document, Item}; pub struct BevyManifest { @@ -108,6 +108,38 @@ impl BevyManifest { } } +/// Finds an identifier that will not conflict with the specified set of tokens. +/// If the identifier is present in `haystack`, extra characters will be added +/// to it until it no longer conflicts with anything. +/// +/// Note that the returned identifier can still conflict in niche cases, +/// such as if an identifier in `haystack` is hidden behind an un-expanded macro. +pub fn ensure_no_collision(value: Ident, haystack: TokenStream) -> Ident { + fn visit_tokens(tokens: TokenStream, idents: &mut Vec) { + for t in tokens { + match t { + TokenTree::Group(g) => visit_tokens(g.stream(), idents), + TokenTree::Ident(ident) => idents.push(ident.to_string()), + TokenTree::Punct(_) | TokenTree::Literal(_) => {} + } + } + } + + let mut idents = Vec::new(); + visit_tokens(haystack, &mut idents); + + let span = value.span(); + let mut value = value.to_string(); + + // If there's a collision, add more characters to the identifier + // until it doesn't collide with anything anymore. + while idents.iter().any(|i| *i == value) { + value.push('X'); + } + + Ident::new(&value, span) +} + /// Derive a label trait /// /// # Args From 53cb5d761f064467edea058ffc726c5744c53580 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 13 Mar 2023 23:14:55 -0400 Subject: [PATCH 12/17] move a struct definition --- crates/bevy_ecs/src/system/system_param.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index b285df5043a6d..6ca4117987439 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1637,12 +1637,12 @@ mod tests { _q: Query<'w, 's, Q, ()>, } - #[derive(Resource)] - pub struct FetchState; - // regression test for https://github.com/bevyengine/bevy/issues/1727. #[derive(SystemParam)] pub struct Collide<'w> { _x: Res<'w, FetchState>, } + + #[derive(Resource)] + pub struct FetchState; } From 7827e7933c6fc566a67a8078f4c3b2ab3c04564e Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 13 Mar 2023 23:19:29 -0400 Subject: [PATCH 13/17] use a hash set --- crates/bevy_macro_utils/src/lib.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/crates/bevy_macro_utils/src/lib.rs b/crates/bevy_macro_utils/src/lib.rs index 2d6835f404b7e..25870df50d0be 100644 --- a/crates/bevy_macro_utils/src/lib.rs +++ b/crates/bevy_macro_utils/src/lib.rs @@ -10,7 +10,7 @@ pub use symbol::*; use proc_macro::{TokenStream, TokenTree}; use quote::{quote, quote_spanned}; -use std::{env, path::PathBuf}; +use std::{collections::HashSet, env, path::PathBuf}; use syn::{spanned::Spanned, Ident}; use toml_edit::{Document, Item}; @@ -115,25 +115,28 @@ impl BevyManifest { /// Note that the returned identifier can still conflict in niche cases, /// such as if an identifier in `haystack` is hidden behind an un-expanded macro. pub fn ensure_no_collision(value: Ident, haystack: TokenStream) -> Ident { - fn visit_tokens(tokens: TokenStream, idents: &mut Vec) { + fn visit_tokens(tokens: TokenStream, idents: &mut HashSet) { for t in tokens { match t { TokenTree::Group(g) => visit_tokens(g.stream(), idents), - TokenTree::Ident(ident) => idents.push(ident.to_string()), + TokenTree::Ident(ident) => { + idents.insert(ident.to_string()); + } TokenTree::Punct(_) | TokenTree::Literal(_) => {} } } } - let mut idents = Vec::new(); + // Collect all the identifiers in `haystack` into a set. + let mut idents = HashSet::new(); visit_tokens(haystack, &mut idents); let span = value.span(); - let mut value = value.to_string(); // If there's a collision, add more characters to the identifier // until it doesn't collide with anything anymore. - while idents.iter().any(|i| *i == value) { + let mut value = value.to_string(); + while idents.contains(&value) { value.push('X'); } From 9289bb2506a0c343f657758fdfbc867e454a4385 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 14 Mar 2023 00:10:05 -0400 Subject: [PATCH 14/17] factor out a regression test into a named test --- crates/bevy_ecs/src/query/fetch.rs | 35 +++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 1c6ccd0f5f01e..17aa0203ed72e 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -1392,18 +1392,33 @@ unsafe impl ReadOnlyWorldQuery for NopWorldQuery {} #[cfg(test)] mod tests { use super::*; - use crate as bevy_ecs; + use crate::{self as bevy_ecs, system::Query}; + + // Ensures that metadata types generated by the WorldQuery macro + // do not conflict with user-defined types. + // Regression test for https://github.com/bevyengine/bevy/issues/8010. + #[test] + fn world_query_metadata_collision() { + // The metadata types generated would be named `ClientState` and `ClientFetch`, + // but they should rename themselves to avoid conflicts. + #[derive(WorldQuery)] + pub struct Client { + pub state: &'static S, + pub fetch: &'static ClientFetch, + } - // regression test for https://github.com/bevyengine/bevy/issues/8010. + pub trait ClientState: Component {} - #[derive(WorldQuery)] - pub struct Client { - pub state: &'static S, - pub fetch: &'static ClientFetch, - } + #[derive(Component)] + pub struct ClientFetch; + + #[derive(Component)] + pub struct C; - pub trait ClientState: Component {} + impl ClientState for C {} - #[derive(Component)] - pub struct ClientFetch; + fn client_system(_: Query>) {} + + crate::system::assert_is_system(client_system); + } } From cd5e92f599d5ac74c534286ff517878f1f929bbe Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 14 Mar 2023 21:07:08 -0400 Subject: [PATCH 15/17] capitalize regression test comments --- crates/bevy_ecs/src/system/system_param.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 6ca4117987439..776accc5f0d7b 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1628,7 +1628,7 @@ mod tests { #[derive(SystemParam)] pub struct EncapsulatedParam<'w>(Res<'w, PrivateResource>); - // regression test for https://github.com/bevyengine/bevy/issues/7103. + // Regression test for https://github.com/bevyengine/bevy/issues/7103. #[derive(SystemParam)] pub struct WhereParam<'w, 's, Q> where @@ -1637,7 +1637,7 @@ mod tests { _q: Query<'w, 's, Q, ()>, } - // regression test for https://github.com/bevyengine/bevy/issues/1727. + // Regression test for https://github.com/bevyengine/bevy/issues/1727. #[derive(SystemParam)] pub struct Collide<'w> { _x: Res<'w, FetchState>, From fbae65f89997baa7cf0fb8089189ef30095e140d Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 14 Mar 2023 21:23:27 -0400 Subject: [PATCH 16/17] don't use recursion to visit token trees --- crates/bevy_macro_utils/src/lib.rs | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/crates/bevy_macro_utils/src/lib.rs b/crates/bevy_macro_utils/src/lib.rs index 25870df50d0be..816e6ac547e14 100644 --- a/crates/bevy_macro_utils/src/lib.rs +++ b/crates/bevy_macro_utils/src/lib.rs @@ -115,21 +115,28 @@ impl BevyManifest { /// Note that the returned identifier can still conflict in niche cases, /// such as if an identifier in `haystack` is hidden behind an un-expanded macro. pub fn ensure_no_collision(value: Ident, haystack: TokenStream) -> Ident { - fn visit_tokens(tokens: TokenStream, idents: &mut HashSet) { - for t in tokens { - match t { - TokenTree::Group(g) => visit_tokens(g.stream(), idents), - TokenTree::Ident(ident) => { - idents.insert(ident.to_string()); + // Collect all the identifiers in `haystack` into a set. + let idents = { + // List of token streams that will be visited in future loop iterations. + let mut unvisited = vec![haystack]; + // Identifiers we have found while searching tokens. + let mut found = HashSet::new(); + while let Some(tokens) = unvisited.pop() { + for t in tokens { + match t { + // Collect any identifiers we encounter. + TokenTree::Ident(ident) => { + found.insert(ident.to_string()); + } + // Queue up nested token streams to be visited in a future loop iteration. + TokenTree::Group(g) => unvisited.push(g.stream()), + TokenTree::Punct(_) | TokenTree::Literal(_) => {} } - TokenTree::Punct(_) | TokenTree::Literal(_) => {} } } - } - // Collect all the identifiers in `haystack` into a set. - let mut idents = HashSet::new(); - visit_tokens(haystack, &mut idents); + found + }; let span = value.span(); From 04ab144918c19a304b4243afb1b20104aa8fd7bd Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Thu, 16 Mar 2023 13:31:50 -0400 Subject: [PATCH 17/17] use FxHash --- crates/bevy_macro_utils/Cargo.toml | 1 + crates/bevy_macro_utils/src/lib.rs | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/bevy_macro_utils/Cargo.toml b/crates/bevy_macro_utils/Cargo.toml index 8b9149864aa67..76cb1d690f295 100644 --- a/crates/bevy_macro_utils/Cargo.toml +++ b/crates/bevy_macro_utils/Cargo.toml @@ -12,3 +12,4 @@ keywords = ["bevy"] toml_edit = "0.19" syn = "1.0" quote = "1.0" +rustc-hash = "1.0" diff --git a/crates/bevy_macro_utils/src/lib.rs b/crates/bevy_macro_utils/src/lib.rs index 816e6ac547e14..3a72994510777 100644 --- a/crates/bevy_macro_utils/src/lib.rs +++ b/crates/bevy_macro_utils/src/lib.rs @@ -10,7 +10,8 @@ pub use symbol::*; use proc_macro::{TokenStream, TokenTree}; use quote::{quote, quote_spanned}; -use std::{collections::HashSet, env, path::PathBuf}; +use rustc_hash::FxHashSet; +use std::{env, path::PathBuf}; use syn::{spanned::Spanned, Ident}; use toml_edit::{Document, Item}; @@ -120,7 +121,7 @@ pub fn ensure_no_collision(value: Ident, haystack: TokenStream) -> Ident { // List of token streams that will be visited in future loop iterations. let mut unvisited = vec![haystack]; // Identifiers we have found while searching tokens. - let mut found = HashSet::new(); + let mut found = FxHashSet::default(); while let Some(tokens) = unvisited.pop() { for t in tokens { match t {