From 27f2265e114dd4b35160e0f14224f3752080442b Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Wed, 22 Mar 2023 11:45:25 -0400 Subject: [PATCH] Fix name conflicts caused by the `SystemParam` and `WorldQuery` macros (#8012) # Objective Fix #1727 Fix #8010 Meta types generated by the `SystemParam` and `WorldQuery` derive macros can conflict with user-defined types if they happen to have the same name. ## Solution In order to check if an identifier would conflict with user-defined types, we can just search the original `TokenStream` passed to the macro to see if it contains the identifier (since the meta types are defined in an anonymous scope, it's only possible for them to conflict with the struct definition itself). When generating an identifier for meta types, we can simply check if it would conflict, and then add additional characters to the name until it no longer conflicts with anything. The `WorldQuery` "Item" and read-only structs are a part of a module's public API, and thus it is intended for them to conflict with user-defined types. --- crates/bevy_ecs/macros/src/fetch.rs | 15 +++++-- crates/bevy_ecs/macros/src/lib.rs | 15 ++++--- crates/bevy_ecs/src/query/fetch.rs | 34 ++++++++++++++++ crates/bevy_ecs/src/system/system_param.rs | 11 ++++- crates/bevy_macro_utils/Cargo.toml | 1 + crates/bevy_macro_utils/src/lib.rs | 47 +++++++++++++++++++++- 6 files changed, 111 insertions(+), 12 deletions(-) diff --git a/crates/bevy_ecs/macros/src/fetch.rs b/crates/bevy_ecs/macros/src/fetch.rs index 8bb5597f0d..3ae328e952 100644 --- a/crates/bevy_ecs/macros/src/fetch.rs +++ b/crates/bevy_ecs/macros/src/fetch.rs @@ -1,9 +1,10 @@ +use bevy_macro_utils::ensure_no_collision; use proc_macro::TokenStream; use proc_macro2::{Ident, Span}; 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 +26,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 +108,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 = ensure_no_collision(fetch_struct_name, tokens.clone()); 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()); + ensure_no_collision(new_ident, tokens.clone()) } 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 = 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 a59c54db8d..3866cea315 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -6,7 +6,9 @@ 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 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}; @@ -260,6 +262,7 @@ static SYSTEM_PARAM_ATTRIBUTE_NAME: &str = "system_param"; /// 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`") @@ -394,6 +397,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(format_ident!("FetchState"), token_stream); TokenStream::from(quote! { // We define the FetchState struct in an anonymous scope to avoid polluting the user namespace. @@ -401,7 +405,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<( @@ -411,11 +415,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, } @@ -454,8 +458,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`. diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 3b4a6a076f..17aa0203ed 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -1388,3 +1388,37 @@ unsafe impl WorldQuery for NopWorldQuery { /// SAFETY: `NopFetch` never accesses any data unsafe impl ReadOnlyWorldQuery for NopWorldQuery {} + +#[cfg(test)] +mod tests { + use super::*; + 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, + } + + pub trait ClientState: Component {} + + #[derive(Component)] + pub struct ClientFetch; + + #[derive(Component)] + pub struct C; + + impl ClientState for C {} + + fn client_system(_: Query>) {} + + crate::system::assert_is_system(client_system); + } +} diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 47f668d8ae..d0668025c7 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1625,7 +1625,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 @@ -1633,4 +1633,13 @@ mod tests { { _q: Query<'w, 's, Q, ()>, } + + // 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; } diff --git a/crates/bevy_macro_utils/Cargo.toml b/crates/bevy_macro_utils/Cargo.toml index 8b9149864a..76cb1d690f 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 3fd275ceb9..3a72994510 100644 --- a/crates/bevy_macro_utils/src/lib.rs +++ b/crates/bevy_macro_utils/src/lib.rs @@ -8,10 +8,11 @@ pub use attrs::*; pub use shape::*; pub use symbol::*; -use proc_macro::TokenStream; +use proc_macro::{TokenStream, TokenTree}; use quote::{quote, quote_spanned}; +use rustc_hash::FxHashSet; use std::{env, path::PathBuf}; -use syn::spanned::Spanned; +use syn::{spanned::Spanned, Ident}; use toml_edit::{Document, Item}; pub struct BevyManifest { @@ -108,6 +109,48 @@ 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 { + // 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 = FxHashSet::default(); + 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(_) => {} + } + } + } + + found + }; + + let span = value.span(); + + // If there's a collision, add more characters to the identifier + // until it doesn't collide with anything anymore. + let mut value = value.to_string(); + while idents.contains(&value) { + value.push('X'); + } + + Ident::new(&value, span) +} + /// Derive a label trait /// /// # Args