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

@@ -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 {

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 {
         // <EventReader<'static, 'static, T> 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`.

crates/bevy_ecs/src/query/fetch.rs

@@ -1388,3 +1388,37 @@ unsafe impl<Q: WorldQuery> WorldQuery for NopWorldQuery<Q> {
 
 /// SAFETY: `NopFetch` never accesses any data
 unsafe impl<Q: WorldQuery> ReadOnlyWorldQuery for NopWorldQuery<Q> {}
+
+#[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<S: ClientState> {
+            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<Client<C>>) {}
+
+        crate::system::assert_is_system(client_system);
+    }
+}

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;
 }

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"

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