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

[Merged by Bors] - implement TypeUuid for primitives and fix multiple-parameter generics having the same TypeUuid #6633

Closed
wants to merge 20 commits into from

Conversation

dis-da-moe
Copy link
Contributor

@dis-da-moe dis-da-moe commented Nov 15, 2022

Objective

Solution

  • move code responsible for generating the impl TypeUuid from type_uuid_derive into a new function, gen_impl_type_uuid.
  • this allows the new proc macro, impl_type_uuid, to call the code for generation.
  • added struct TypeUuidDef and implemented syn::Parse to allow parsing of the input for the new macro.
  • finally, used the new macro impl_type_uuid to implement TypeUuid for the standard library (in crates/bevy_reflect/src/type_uuid_impl.rs).
  • fixes Generic types with multiple parameters can have two different types with the same TypeUuid #6680 by doing a wrapping add of the param's index to its TYPE_UUID

@nicopap nicopap added A-Reflection Runtime information about types C-Bug An unexpected or incorrect behavior labels Nov 15, 2022
@jakobhellermann
Copy link
Contributor

Generic types should not have a single static UUID.

@dis-da-moe
Copy link
Contributor Author

Generic types should not have a single static UUID.

I'm pretty sure they don't, this macro generates the same code as the derive, where a generic type's UUID will be a composite of their own uuid and their parameter's uuid.

@dis-da-moe
Copy link
Contributor Author

does anyone know why compilation sometimes fails due to the smallvec import?

@MrGVSV
Copy link
Member

MrGVSV commented Nov 16, 2022

does anyone know why compilation sometimes fails due to the smallvec import?

It's because smallvec is gated behind a feature.

@harudagondi
Copy link
Member

harudagondi commented Nov 17, 2022

Can you implement TypeUuid for arrays? Ideally [T; 1] and [T; 2] should have different TYPE_UUID.

@dis-da-moe
Copy link
Contributor Author

Can you implement TypeUuid for arrays? Ideally [T; 1] and [T; 2] should have different TYPE_UUID.

I think it can be implemented for [T; N], by converting N to a u128 and then to a Uuid from that, and then compositing T::TYPE_UUID, a static uuid for all arrays, and N::TYPE_UUID. Does that make sense?

@MrGVSV
Copy link
Member

MrGVSV commented Nov 17, 2022

On the topic of handling arrays, do we handle tuples as well? If not, that might need to be added for completeness (up to twelve fields to match Reflect and other std impls).

@dis-da-moe dis-da-moe changed the title implement TypeUuid for primitives implement TypeUuid for primitives and fix multiple-parameter generics having the TypeUuid Nov 18, 2022
@dis-da-moe dis-da-moe changed the title implement TypeUuid for primitives and fix multiple-parameter generics having the TypeUuid implement TypeUuid for primitives and fix multiple-parameter generics having the same TypeUuid Nov 18, 2022
crates/bevy_reflect/bevy_reflect_derive/src/type_uuid.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/type_uuid_impl.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/type_uuid_impl.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/bevy_reflect_derive/src/type_uuid.rs Outdated Show resolved Hide resolved
@dis-da-moe
Copy link
Contributor Author

I've had to move the all_tuples macro outside of bevy_ecs in order to avoid a circular dependency. I've moved it into a new proc macro crate in bevy_utils called bevy_utils_macros. This seemed the most suitable place to me but lmk if there is a better place.

@soqb
Copy link
Contributor

soqb commented Dec 22, 2022

the name bevy_utils_macros is very confusing when we already have a crate named bevy_macro_utils. i agree that bevy_ecs is not the best place for the macro but i'm not settled on the name and perhaps this should be part of a separate pr.

@dis-da-moe
Copy link
Contributor Author

well the only reason it needs to be in a separate crate is that it's a proc macro. I could move it as a sub-crate of bevy_macro_utils called bevy_macro_utils_proc. I've already done all the refactoring work and if it's just the name that's the issue that could change.

@soqb
Copy link
Contributor

soqb commented Dec 22, 2022

bevy_macro_utils is not the correct place for the macro imo. the macro utils crate is for crates like bevy_reflect_derive to use, not for a crate like bevy_reflect. i'm not really sure where the correct place for the macro is. bevy_utils seems like a sound fit but our current naming scheme of bevy_xxx_{macros, derive} doesn't fit very well here.

@alice-i-cecile - can you provide some guidance here?

@alice-i-cecile
Copy link
Member

Hmm, I agree with your analysis @soqb.

bevy_macro_utils_proc

This seems fine for now. @mockersf may have better ideas: he's very good at this sort of organizational thinking.

@soqb
Copy link
Contributor

soqb commented Dec 23, 2022

i don't think it's correct to put all_tuples in bevy_macro_utils so perhaps bevy_utils_proc_macros?

Copy link
Contributor

@soqb soqb left a comment

Choose a reason for hiding this comment

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

LGTM!

considering the all_tuples! move this should have C-Breaking-Change.

@harudagondi
Copy link
Member

LGTM!

considering the all_tuples! move this should have C-Breaking-Change.

Is all_tuples! public API?

@alice-i-cecile alice-i-cecile added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Jan 8, 2023
@alice-i-cecile
Copy link
Member

https://docs.rs/bevy/latest/bevy/ecs/macro.all_tuples.html

Apparently yes, although it has no docs.

@alice-i-cecile alice-i-cecile self-requested a review January 8, 2023 05:27
@alice-i-cecile alice-i-cecile added this to the 0.10 milestone Jan 8, 2023
@alice-i-cecile
Copy link
Member

@dis-da-moe can you please rebase? This is worth including in the release.

@alice-i-cecile alice-i-cecile 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 Feb 14, 2023
Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

Looks good! I think we should follow up by adding this trait on the types in the glam and rect modules as well

crates/bevy_reflect/bevy_reflect_derive/src/type_uuid.rs Outdated Show resolved Hide resolved
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Feb 16, 2023
…cs having the same `TypeUuid` (#6633)

# Objective

- Fixes #5432 
- Fixes #6680

## Solution

- move code responsible for generating the `impl TypeUuid` from `type_uuid_derive` into a new function, `gen_impl_type_uuid`.
- this allows the new proc macro, `impl_type_uuid`, to call the code for generation.
- added struct `TypeUuidDef` and implemented `syn::Parse` to allow parsing of the input for the new macro.
- finally, used the new macro `impl_type_uuid` to implement `TypeUuid` for the standard library (in `crates/bevy_reflect/src/type_uuid_impl.rs`).
- fixes #6680 by doing a wrapping add of the param's index to its `TYPE_UUID`

Co-authored-by: dis-da-moe <84386186+dis-da-moe@users.noreply.github.com>
@bors bors bot changed the title implement TypeUuid for primitives and fix multiple-parameter generics having the same TypeUuid [Merged by Bors] - implement TypeUuid for primitives and fix multiple-parameter generics having the same TypeUuid Feb 16, 2023
@bors bors bot closed this Feb 16, 2023
myreprise1 pushed a commit to myreprise1/bevy that referenced this pull request Feb 16, 2023
…cs having the same `TypeUuid` (bevyengine#6633)

# Objective

- Fixes bevyengine#5432 
- Fixes bevyengine#6680

## Solution

- move code responsible for generating the `impl TypeUuid` from `type_uuid_derive` into a new function, `gen_impl_type_uuid`.
- this allows the new proc macro, `impl_type_uuid`, to call the code for generation.
- added struct `TypeUuidDef` and implemented `syn::Parse` to allow parsing of the input for the new macro.
- finally, used the new macro `impl_type_uuid` to implement `TypeUuid` for the standard library (in `crates/bevy_reflect/src/type_uuid_impl.rs`).
- fixes bevyengine#6680 by doing a wrapping add of the param's index to its `TYPE_UUID`

Co-authored-by: dis-da-moe <84386186+dis-da-moe@users.noreply.github.com>
bors bot pushed a commit that referenced this pull request Feb 20, 2023
# Objective

`cargo run -p ci` is currently failing locally for me.

```
error: variables can be used directly in the `format!` string
   --> crates/bevy_reflect/bevy_reflect_derive/src/type_uuid.rs:106:69
    |
106 |         let uuid = Uuid::parse_str(&uuid).map_err(|err| input.error(format!("{}", err)))?;
```

It's not clear to me why CI/clippy didn't pick this up in #6633.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: Done
7 participants