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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - re-enable #[derive(TypeUuid)] for generics #4118

Closed
wants to merge 7 commits into from
Closed

[Merged by Bors] - re-enable #[derive(TypeUuid)] for generics #4118

wants to merge 7 commits into from

Conversation

jakobhellermann
Copy link
Contributor

Support for deriving TypeUuid for types with generics was initially added in #2044 but later reverted #2204 because it lead to MyStruct<A> and MyStruct<B> having the same type uuid.

This PR fixes this by generating code like

#[derive(TypeUuid)]
#[uuid = "69b09733-a21a-4dab-a444-d472986bd672"]
struct Type<T>(T);

impl<T: TypeUuid> TypeUuid for Type<T> {
  const TYPE_UUID: TypeUuid = generate_compound_uuid(Uuid::from_bytes([/* 69b0 uuid */]), T::TYPE_UUID);
}

where generate_compound_uuid will XOR the non-metadata bits of the two UUIDs.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 5, 2022
@XBagon XBagon mentioned this pull request Mar 5, 2022
Closed
@jakobhellermann jakobhellermann added A-Assets Load files from disk to use for things like images, models, and sounds C-Feature A new feature, making something new possible and removed S-Needs-Triage This issue needs to be labelled labels Mar 5, 2022
@bjorn3
Copy link
Contributor

bjorn3 commented Mar 6, 2022

Xoring would give Foo<Bar<T>> and Bar<Foo<T>> the same UUID.

@jakobhellermann
Copy link
Contributor Author

Xoring would give Foo<Bar<T>> and Bar<Foo<T>> the same UUID.

Good point.

Maybe we need struct Generic<#[uuid = "..."] T>;?

A XOR B XOR C XOR UNIQUE1 should be distinct from
A XOR B XOR C XOR UNIQUE2

@jakobhellermann
Copy link
Contributor Author

Hm, so

#[uuid = "11b483e0-594f-42b2-b004-8aea624cb2c1"]
struct TestDeriveStruct<#[uuid = "dadbf4fb-55b4-4e96-a3b2-b178bad890af"] T>(T);

works, you can parse the attribute correctly with syn, however you still get the error message

error: cannot find attribute `uuid` in this scope

I assume this is because derives are purely additive, so you can't remove the uuid bound? Or maybe this should be allowed because of #[proc_macro_derive(TypeUuid, attributes(uuid))]?

Alternatively you could have

#[uuid = "11b483e0-594f-42b2-b004-8aea624cb2c1"]
#[uuid(T = "dadbf4fb-55b4-4e96-a3b2-b178bad890af")] 
struct TestDeriveStruct<T>(T);

@XBagon
Copy link
Contributor

XBagon commented Mar 7, 2022

Xoring would give Foo<Bar<T>> and Bar<Foo<T>> the same UUID.

XBagon@8fc04b0
I tried a different approach, and chose to do some rotating to make it a non-commutative operation.

@jakobhellermann
Copy link
Contributor Author

I merged the version done by @XBagon, where the UUID is generated by a bitwise a XOR b.rotate_right(1). This should fix the concern about the conflicting UUIDs with generics.

XBagon
XBagon approved these changes Mar 16, 2022
View reviewed changes
Copy link
Contributor

@XBagon XBagon left a comment •
edited
Loading

Choose a reason for hiding this comment

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

As I also have a commit in this and had to work with the code, I'm approving of these changes 😄!

@jakobhellermann jakobhellermann 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 31, 2022
@alice-i-cecile alice-i-cecile added the P-Regression Functionality that used to work but no longer does. Add a test for this! label Apr 26, 2022
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Apr 26, 2022
@jakobhellermann @XBagon
re-enable #[derive(TypeUuid)] for generics (#4118)
3d36ec4 
Support for deriving `TypeUuid` for types with generics was initially added in #2044 but later reverted #2204 because it lead to `MyStruct<A>` and `MyStruct<B>` having the same type uuid.

This PR fixes this by generating code like
```rust
#[derive(TypeUuid)]
#[uuid = "69b09733-a21a-4dab-a444-d472986bd672"]
struct Type<T>(T);

impl<T: TypeUuid> TypeUuid for Type<T> {
  const TYPE_UUID: TypeUuid = generate_compound_uuid(Uuid::from_bytes([/* 69b0 uuid */]), T::TYPE_UUID);
}
```

where `generate_compound_uuid` will XOR the non-metadata bits of the two UUIDs.

Co-authored-by: XBagon <[email protected]>
Co-authored-by: Jakob Hellermann <[email protected]>
@bors
Copy link
Contributor

bors bot commented Apr 26, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title re-enable #[derive(TypeUuid)] for generics [Merged by Bors] - re-enable #[derive(TypeUuid)] for generics Apr 26, 2022
@bors bors bot closed this Apr 26, 2022
@jakobhellermann jakobhellermann deleted the type-uuid-generics branch May 4, 2022 10:58
exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022
@jakobhellermann @XBagon @exjam
re-enable #[derive(TypeUuid)] for generics (bevyengine#4118)
6ba61e0 
Support for deriving `TypeUuid` for types with generics was initially added in bevyengine#2044 but later reverted bevyengine#2204 because it lead to `MyStruct<A>` and `MyStruct<B>` having the same type uuid.

This PR fixes this by generating code like
```rust
#[derive(TypeUuid)]
#[uuid = "69b09733-a21a-4dab-a444-d472986bd672"]
struct Type<T>(T);

impl<T: TypeUuid> TypeUuid for Type<T> {
  const TYPE_UUID: TypeUuid = generate_compound_uuid(Uuid::from_bytes([/* 69b0 uuid */]), T::TYPE_UUID);
}
```

where `generate_compound_uuid` will XOR the non-metadata bits of the two UUIDs.

Co-authored-by: XBagon <[email protected]>
Co-authored-by: Jakob Hellermann <[email protected]>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@jakobhellermann @XBagon @ItsDoot
re-enable #[derive(TypeUuid)] for generics (bevyengine#4118)
c621a0b 
Support for deriving `TypeUuid` for types with generics was initially added in bevyengine#2044 but later reverted bevyengine#2204 because it lead to `MyStruct<A>` and `MyStruct<B>` having the same type uuid.

This PR fixes this by generating code like
```rust
#[derive(TypeUuid)]
#[uuid = "69b09733-a21a-4dab-a444-d472986bd672"]
struct Type<T>(T);

impl<T: TypeUuid> TypeUuid for Type<T> {
  const TYPE_UUID: TypeUuid = generate_compound_uuid(Uuid::from_bytes([/* 69b0 uuid */]), T::TYPE_UUID);
}
```

where `generate_compound_uuid` will XOR the non-metadata bits of the two UUIDs.

Co-authored-by: XBagon <[email protected]>
Co-authored-by: Jakob Hellermann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrGVSV MrGVSV MrGVSV left review comments

@XBagon XBagon XBagon approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

Assignees
No one assigned
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Feature A new feature, making something new possible P-Regression Functionality that used to work but no longer does. Add a test for this! 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jakobhellermann @bjorn3 @XBagon @alice-i-cecile @MrGVSV