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] - Bevy derives handling generics in impl definitions. #2044

Closed
wants to merge 3 commits into from
Closed

[Merged by Bors] - Bevy derives handling generics in impl definitions. #2044

wants to merge 3 commits into from

Conversation

NathanSWard
Copy link
Contributor

@NathanSWard NathanSWard commented Apr 29, 2021 •
edited
Loading

Fixes #2037 (and then some)

Problem:

  • TypeUuid, RenderResource, and Bytes derive macros did not properly handle generic structs.

Solution:

  • Rework the derive macro implementations to handle the generics.

@NathanSWard
Copy link
Contributor Author

A few comments to take into account!

  1. There are a few more derive marcos that do not handle generics (e.g. RenderResources (plural)), which should probably get included in this PR.

  2. There are currently no tests for the specific scenarios that would have prevented Derives ignore const generics #2037 . I would propose adding some integration testing inside tests/ for the derive macros to ensure the use cases are tested.

@NathanSWard NathanSWard force-pushed the nward/bevy-derive-generics branch from f534f8d to 778d00f Compare April 29, 2021 06:57
@MinerSebas
Copy link
Contributor

Could you edit the PR description to change Addresses #2037 Fixes #2037, so that the issue is linked and automatically closed once this is merged.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change labels Apr 29, 2021
@alice-i-cecile
Copy link
Member

There are currently no tests for the specific scenarios that would have prevented #2037 . I would propose adding some integration testing inside tests/ for the derive macros to ensure the use cases are tested.

I would like to see this; I think it's a sensible place to put them.

@cart
Copy link
Member

cart commented Apr 29, 2021 •
edited
Loading

I dig it!

I personally think the tests should live in the crates where the macros are actually consumed/exported (ex: RenderResources should be tested in bevy_render) and be tested wherever their trait is defined.

bevy_derive really only exists because Rust forces a separate crate for proc macros and I don't want duplicate crates for everything that needs a proc macro (ex: bevy_render_derive, bevy_asset_derive, etc). In general we should just pretend that proc macros originate from their "real" crates (and discourage users from pulling in bevy_derive directly).

@NathanSWard NathanSWard force-pushed the nward/bevy-derive-generics branch from 778d00f to b80fc26 Compare April 30, 2021 20:50
@NathanSWard
Copy link
Contributor Author

Tests are added for RenderResource, RenderResources, and TypeUuid 😸

@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 Apr 30, 2021
@cart
Copy link
Member

cart commented May 1, 2021

bors r+

bors bot pushed a commit that referenced this pull request May 1, 2021
@NathanSWard
Bevy derives handling generics in impl definitions. (#2044)
b07db84 
Fixes #2037 (and then some)

Problem:
- `TypeUuid`, `RenderResource`, and `Bytes` derive macros did not properly handle generic structs. 

Solution:
- Rework the derive macro implementations to handle the generics.
@bors
Copy link
Contributor

bors bot commented May 1, 2021

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Bevy derives handling generics in impl definitions. [Merged by Bors] - Bevy derives handling generics in impl definitions. May 1, 2021
@bors bors bot closed this May 1, 2021
@SafariMonkey
Copy link

I might be missing something here, but doesn't this mean that different instantiations of a generic type deriving TypeUuid will have the same UUID? Is that not a problem if each TypeUuid is supposed to uniquely identify a type?

I'd love tools for making generics and reflection work better together, of course. Maybe add a generic bound to the impl that the child is TypeUuid and merge them? That could hopefully be const (PRNG merging UUIDs), but worst case it could be at runtime with TypeUuidDynamic. I'm currently making a different newtype struct for each collection that has to be TypeUuid, though I might be doing something wrong - I hope there's a better way.

@cart
Copy link
Member

cart commented May 17, 2021

Hmm yeah supporting generics on TypeUuid derives does seem wrong. I think we should revert that change.

@NathanSWard NathanSWard mentioned this pull request May 17, 2021
Closed
bors bot pushed a commit that referenced this pull request May 17, 2021
@NathanSWard
revert supporting generics for deriving TypeUuid (#2204)
0719659 
This reverts some of the changes made in #2044 as supporting generics for a `#[derive(TypeUuid)]` should not work as each generic instantiation would have the same uuid.

Stems from [this conversation](#2044 (comment))
@NathanSWard
Copy link
Contributor Author

I might be missing something here, but doesn't this mean that different instantiations of a generic type deriving TypeUuid will have the same UUID? Is that not a problem if each TypeUuid is supposed to uniquely identify a type?

@SafariMonkey this was fixed in #2204, that you for finding this bug!

ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
@NathanSWard @ostwilkens
Bevy derives handling generics in impl definitions. (bevyengine#2044)
402f73d 
Fixes bevyengine#2037 (and then some)

Problem:
- `TypeUuid`, `RenderResource`, and `Bytes` derive macros did not properly handle generic structs. 

Solution:
- Rework the derive macro implementations to handle the generics.
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
@NathanSWard @ostwilkens
revert supporting generics for deriving TypeUuid (bevyengine#2204)
95f8944 
This reverts some of the changes made in bevyengine#2044 as supporting generics for a `#[derive(TypeUuid)]` should not work as each generic instantiation would have the same uuid.

Stems from [this conversation](bevyengine#2044 (comment))
@jakobhellermann jakobhellermann mentioned this pull request Mar 5, 2022
Closed
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]>
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

@mockersf mockersf mockersf approved these changes

Assignees
No one assigned
Labels
C-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change 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.

Derives ignore const generics
6 participants
@NathanSWard @MinerSebas @alice-i-cecile @cart @SafariMonkey @mockersf