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

Fix name conflicts caused by the SystemParam and WorldQuery macros #8012

Merged
merged 21 commits into from
Mar 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
7a40865
add a regression test
JoJoJet Mar 10, 2023
0bf2c7f
fix the regression test
JoJoJet Mar 10, 2023
7ad905c
ensure state types don't collide for `SystemParam`
JoJoJet Mar 10, 2023
accbab9
tweak regression test
JoJoJet Mar 10, 2023
df47636
add a dedicated method for generating identifiers with no collision
JoJoJet Mar 10, 2023
952781f
add a regression test for queries
JoJoJet Mar 10, 2023
47b21b9
prevent collisions for `WorldQuery` state and fetch types
JoJoJet Mar 10, 2023
81fce59
add a comment describing the collision algorithm
JoJoJet Mar 10, 2023
9df6846
remove a redundant import
JoJoJet Mar 10, 2023
99f1883
use a more efficient collision checking algorithm
JoJoJet Mar 14, 2023
c1de12f
move `ensure_no_collision` to `bevy_macro_utils`
JoJoJet Mar 14, 2023
53cb5d7
move a struct definition
JoJoJet Mar 14, 2023
7827e79
use a hash set
JoJoJet Mar 14, 2023
9289bb2
factor out a regression test into a named test
JoJoJet Mar 14, 2023
cd5e92f
capitalize regression test comments
JoJoJet Mar 15, 2023
fbae65f
don't use recursion to visit token trees
JoJoJet Mar 15, 2023
04ab144
use FxHash
JoJoJet Mar 16, 2023
96a1d61
Merge remote-tracking branch 'upstream/main' into state-name-conflict
JoJoJet Mar 16, 2023
3801ddc
Merge remote-tracking branch 'upstream/main' into state-name-conflict
JoJoJet Mar 18, 2023
dca0f0c
Merge remote-tracking branch 'upstream/main' into state-name-conflict
JoJoJet Mar 19, 2023
cf8e525
Merge remote-tracking branch 'upstream/main' into state-name-conflict
JoJoJet Mar 21, 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
15 changes: 12 additions & 3 deletions crates/bevy_ecs/macros/src/fetch.rs
Original file line number Diff line number Diff line change
@@ -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,
};
Expand All @@ -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();
Expand Down Expand Up @@ -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 {
Expand Down
15 changes: 9 additions & 6 deletions crates/bevy_ecs/macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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`")
Expand Down Expand Up @@ -394,14 +397,15 @@ 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.
// The struct can still be accessed via SystemParam::State, e.g. EventReaderState can be accessed via
// <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<(
Expand All @@ -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,
}
Expand Down Expand Up @@ -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`.
Expand Down
34 changes: 34 additions & 0 deletions crates/bevy_ecs/src/query/fetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
11 changes: 10 additions & 1 deletion crates/bevy_ecs/src/system/system_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1625,12 +1625,21 @@ 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
Q: 'static + WorldQuery,
{
_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;
}
1 change: 1 addition & 0 deletions crates/bevy_macro_utils/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,4 @@ keywords = ["bevy"]
toml_edit = "0.19"
syn = "1.0"
quote = "1.0"
rustc-hash = "1.0"
47 changes: 45 additions & 2 deletions crates/bevy_macro_utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love that this is reusable :D

// 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
Expand Down