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

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Mar 10, 2023

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.

@JoJoJet JoJoJet added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use labels Mar 10, 2023
///
/// 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

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Test needs some cleanup, then LGTM.

crates/bevy_macro_utils/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_macro_utils/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I'm reluctant to complicate this further in the name of compile time performance. I don't think that's a great use of our complexity budget on our macros TBH.

@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 22, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 22, 2023
@alice-i-cecile alice-i-cecile merged commit 27f2265 into bevyengine:main Mar 22, 2023
@JoJoJet JoJoJet deleted the state-name-conflict branch March 22, 2023 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior C-Usability A simple quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
3 participants