[Merged by Bors] - bevy_reflect_derive: Tidying up the code #4712
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nicopap
suggested changes
May 10, 2022
crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs
Outdated
Show resolved
Hide resolved
crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs
Outdated
Show resolved
Hide resolved
crates/bevy_reflect/bevy_reflect_derive/src/field_attributes.rs
Outdated
Show resolved
Hide resolved
crates/bevy_reflect/bevy_reflect_derive/src/field_attributes.rs
Outdated
Show resolved
Hide resolved
IceSentry
added
A-Reflection
Co-authored-by: Nicola Papale <[email protected]>
|
I'll probably move some of the constants into their own module as well. This would include making things like |
crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs
Outdated
Show resolved
Hide resolved
crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs
Outdated
Show resolved
Hide resolved
|
Generally much cleaner. More docs will help; it's still pretty rough to follow as a first-time reader. |
Yeah I left out a lot of docs so that they could be more of a focus in a separate PR. I'll try to fill in what I can, though, at least for internal items. |
Also made certain attributes into constants rather than hardcoded strings.
nicopap
approved these changes
May 11, 2022
MrGVSV
commented
May 11, 2022
|
Further cleaned things up and added a lot more comments (I chose to err on the side of over-commenting in some places). Some doc comments are still missing or in need of improvement. Again, those can be improved in a dedicated PR. |
alice-i-cecile
approved these changes
May 11, 2022
PROMETHIA-27
approved these changes
May 11, 2022
| /// | ||
| /// #[derive(Reflect)] | ||
| /// // Register the custom `Hash` function | ||
| /// #[reflect(Hash(get_hash))] |
There was a problem hiding this comment.
I'm wondering if it would make sense to add Default as a special trait- it definitely has special behavior, and allowing a special custom function for it like this would be nice.
Outside of the scope of this PR, but occurred to me nonetheless.
There was a problem hiding this comment.
That makes sense to me. It would involve adding a Reflect::reflect_default() method or something though. "Special traits" aren't stored as type data like other traits (can't access it via the registry). So we either add that method to Reflect and remove ReflectDefault or we make all "special traits" accessible from the registry as well.
But yeah that's probably best for a follow up PR (maybe even in #4140).
There was a problem hiding this comment.
I like this in #4140 at first glance.
PROMETHIA-27
suggested changes
May 11, 2022
There was a problem hiding this comment.
Oh no! I just noticed the bug I originally added the path attribute to fix is back. These changes seem to fix it; I don't know if they'd break something else, but these lines don't seem necessary.
EDIT: To elaborate, the bug is caused by use-ing anything that starts with #bevy_reflect_path, because that path could be ambiguous during doc-tests... somehow. To fix this I remove any superfluous uses, and if I actually need the Struct trait in scope, instead I switch to UFCS and do #bevy_reflect_path::Struct::{method name}(self, parameters), which does not cause the bug.
Thank you! I was scratching my head trying figure out what regressed from your PR to this one. I'll implement the fix tonight. |
Now the order is: derive macros, attribute macros, then standard proc macros.
Removed all usages of `use #bevy_reflect_path`
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
May 12, 2022
# Objective The `bevy_reflect_derive` crate is not the cleanest or easiest to follow/maintain. The `lib.rs` file is especially difficult with over 1000 lines of code written in a confusing order. This is just a result of growth within the crate and it would be nice to clean it up for future work. ## Solution Split `bevy_reflect_derive` into many more submodules. The submodules include: * `container_attributes` - Code relating to container attributes * `derive_data` - Code relating to reflection-based derive metadata * `field_attributes` - Code relating to field attributes * `impls` - Code containing actual reflection implementations * `reflect_value` - Code relating to reflection-based value metadata * `registration` - Code relating to type registration * `utility` - General-purpose utility functions This leaves the `lib.rs` file to contain only the public macros, making it much easier to digest (and fewer than 200 lines). By breaking up the code into smaller modules, we make it easier for future contributors to find the code they're looking for or identify which module best fits their own additions. ### Metadata Structs This cleanup also adds two big metadata structs: `ReflectFieldAttr` and `ReflectDeriveData`. The former is used to store all attributes for a struct field (if any). The latter is used to store all metadata for struct-based derive inputs. Both significantly reduce code duplication and make editing these macros much simpler. The tradeoff is that we may collect more metadata than needed. However, this is usually a small thing (such as checking for attributes when they're not really needed or creating a `ReflectFieldAttr` for every field regardless of whether they actually have an attribute). We could try to remove these tradeoffs and squeeze some more performance out, but doing so might come at the cost of developer experience. Personally, I think it's much nicer to create a `ReflectFieldAttr` for every field since it means I don't have to do two `Option` checks. Others may disagree, though, and so we can discuss changing this either in this PR or in a future one. ### Out of Scope _Some_ documentation has been added or improved, but ultimately good docs are probably best saved for a dedicated PR. ## 🔍 Focus Points (for reviewers) I know it's a lot to sift through, so here is a list of **key points for reviewers**: - The following files contain code that was mostly just relocated: - `reflect_value.rs` - `registration.rs` - `container_attributes.rs` was also mostly moved but features some general cleanup (reducing nesting, removing hardcoded strings, etc.) and lots of doc comments - Most impl logic was moved from `lib.rs` to `impls.rs`, but they have been significantly modified to use the new `ReflectDeriveData` metadata struct in order to reduce duplication. - `derive_data.rs` and `field_attributes.rs` contain almost entirely new code and should probably be given the most attention. - Likewise, `from_reflect.rs` saw major changes using `ReflectDeriveData` so it should also be given focus. - There was no change to the `lib.rs` exports so the end-user API should be the same. ## Prior Work This task was initially tackled by @NathanSWard in #2377 (which was closed in favor of this PR), so hats off to them for beating me to the punch by nearly a year! --- ## Changelog * **[INTERNAL]** Split `bevy_reflect_derive` into smaller submodules * **[INTERNAL]** Add `ReflectFieldAttr` * **[INTERNAL]** Add `ReflectDeriveData` * Add `BevyManifest::get_path_direct()` method (`bevy_macro_utils`) Co-authored-by: MrGVSV <[email protected]>
bors
bot
changed the title
This was referenced May 12, 2022
Closed
robtfm
pushed a commit
to robtfm/bevy
that referenced
this pull request
May 17, 2022
# Objective The `bevy_reflect_derive` crate is not the cleanest or easiest to follow/maintain. The `lib.rs` file is especially difficult with over 1000 lines of code written in a confusing order. This is just a result of growth within the crate and it would be nice to clean it up for future work. ## Solution Split `bevy_reflect_derive` into many more submodules. The submodules include: * `container_attributes` - Code relating to container attributes * `derive_data` - Code relating to reflection-based derive metadata * `field_attributes` - Code relating to field attributes * `impls` - Code containing actual reflection implementations * `reflect_value` - Code relating to reflection-based value metadata * `registration` - Code relating to type registration * `utility` - General-purpose utility functions This leaves the `lib.rs` file to contain only the public macros, making it much easier to digest (and fewer than 200 lines). By breaking up the code into smaller modules, we make it easier for future contributors to find the code they're looking for or identify which module best fits their own additions. ### Metadata Structs This cleanup also adds two big metadata structs: `ReflectFieldAttr` and `ReflectDeriveData`. The former is used to store all attributes for a struct field (if any). The latter is used to store all metadata for struct-based derive inputs. Both significantly reduce code duplication and make editing these macros much simpler. The tradeoff is that we may collect more metadata than needed. However, this is usually a small thing (such as checking for attributes when they're not really needed or creating a `ReflectFieldAttr` for every field regardless of whether they actually have an attribute). We could try to remove these tradeoffs and squeeze some more performance out, but doing so might come at the cost of developer experience. Personally, I think it's much nicer to create a `ReflectFieldAttr` for every field since it means I don't have to do two `Option` checks. Others may disagree, though, and so we can discuss changing this either in this PR or in a future one. ### Out of Scope _Some_ documentation has been added or improved, but ultimately good docs are probably best saved for a dedicated PR. ## 🔍 Focus Points (for reviewers) I know it's a lot to sift through, so here is a list of **key points for reviewers**: - The following files contain code that was mostly just relocated: - `reflect_value.rs` - `registration.rs` - `container_attributes.rs` was also mostly moved but features some general cleanup (reducing nesting, removing hardcoded strings, etc.) and lots of doc comments - Most impl logic was moved from `lib.rs` to `impls.rs`, but they have been significantly modified to use the new `ReflectDeriveData` metadata struct in order to reduce duplication. - `derive_data.rs` and `field_attributes.rs` contain almost entirely new code and should probably be given the most attention. - Likewise, `from_reflect.rs` saw major changes using `ReflectDeriveData` so it should also be given focus. - There was no change to the `lib.rs` exports so the end-user API should be the same. ## Prior Work This task was initially tackled by @NathanSWard in bevyengine#2377 (which was closed in favor of this PR), so hats off to them for beating me to the punch by nearly a year! --- ## Changelog * **[INTERNAL]** Split `bevy_reflect_derive` into smaller submodules * **[INTERNAL]** Add `ReflectFieldAttr` * **[INTERNAL]** Add `ReflectDeriveData` * Add `BevyManifest::get_path_direct()` method (`bevy_macro_utils`) Co-authored-by: MrGVSV <[email protected]>
bors bot
pushed a commit
that referenced
this pull request
May 18, 2022
# Objective Quick followup to #4712. While updating some [other PRs](#4218), I realized the `ReflectTraits` struct could be improved. The issue with the current implementation is that `ReflectTraits::get_xxx_impl(...)` returns just the _logic_ to the corresponding `Reflect` trait method, rather than the entire function. This makes it slightly more annoying to manage since the variable names need to be consistent across files. For example, `get_partial_eq_impl` uses a `value` variable. But the name "value" isn't defined in the `get_partial_eq_impl` method, it's defined in three other methods in a completely separate file. It's not likely to cause any bugs if we keep it as it is since differing variable names will probably just result in a compile error (except in very particular cases). But it would be useful to someone who wanted to edit/add/remove a method. ## Solution Made `get_hash_impl`, `get_partial_eq_impl` and `get_serialize_impl` return the entire method implementation for `reflect_hash`, `reflect_partial_eq`, and `serializable`, respectively. As a result of this, those three `Reflect` methods were also given default implementations. This was fairly simple to do since all three could just be made to return `None`. --- ## Changelog * Small cleanup/refactor to `ReflectTraits` in `bevy_reflect_derive` * Gave `Reflect::reflect_hash`, `Reflect::reflect_partial_eq`, and `Reflect::serializable` default implementations
exjam
pushed a commit
to exjam/bevy
that referenced
this pull request
May 22, 2022
# Objective The `bevy_reflect_derive` crate is not the cleanest or easiest to follow/maintain. The `lib.rs` file is especially difficult with over 1000 lines of code written in a confusing order. This is just a result of growth within the crate and it would be nice to clean it up for future work. ## Solution Split `bevy_reflect_derive` into many more submodules. The submodules include: * `container_attributes` - Code relating to container attributes * `derive_data` - Code relating to reflection-based derive metadata * `field_attributes` - Code relating to field attributes * `impls` - Code containing actual reflection implementations * `reflect_value` - Code relating to reflection-based value metadata * `registration` - Code relating to type registration * `utility` - General-purpose utility functions This leaves the `lib.rs` file to contain only the public macros, making it much easier to digest (and fewer than 200 lines). By breaking up the code into smaller modules, we make it easier for future contributors to find the code they're looking for or identify which module best fits their own additions. ### Metadata Structs This cleanup also adds two big metadata structs: `ReflectFieldAttr` and `ReflectDeriveData`. The former is used to store all attributes for a struct field (if any). The latter is used to store all metadata for struct-based derive inputs. Both significantly reduce code duplication and make editing these macros much simpler. The tradeoff is that we may collect more metadata than needed. However, this is usually a small thing (such as checking for attributes when they're not really needed or creating a `ReflectFieldAttr` for every field regardless of whether they actually have an attribute). We could try to remove these tradeoffs and squeeze some more performance out, but doing so might come at the cost of developer experience. Personally, I think it's much nicer to create a `ReflectFieldAttr` for every field since it means I don't have to do two `Option` checks. Others may disagree, though, and so we can discuss changing this either in this PR or in a future one. ### Out of Scope _Some_ documentation has been added or improved, but ultimately good docs are probably best saved for a dedicated PR. ## 🔍 Focus Points (for reviewers) I know it's a lot to sift through, so here is a list of **key points for reviewers**: - The following files contain code that was mostly just relocated: - `reflect_value.rs` - `registration.rs` - `container_attributes.rs` was also mostly moved but features some general cleanup (reducing nesting, removing hardcoded strings, etc.) and lots of doc comments - Most impl logic was moved from `lib.rs` to `impls.rs`, but they have been significantly modified to use the new `ReflectDeriveData` metadata struct in order to reduce duplication. - `derive_data.rs` and `field_attributes.rs` contain almost entirely new code and should probably be given the most attention. - Likewise, `from_reflect.rs` saw major changes using `ReflectDeriveData` so it should also be given focus. - There was no change to the `lib.rs` exports so the end-user API should be the same. ## Prior Work This task was initially tackled by @NathanSWard in bevyengine#2377 (which was closed in favor of this PR), so hats off to them for beating me to the punch by nearly a year! --- ## Changelog * **[INTERNAL]** Split `bevy_reflect_derive` into smaller submodules * **[INTERNAL]** Add `ReflectFieldAttr` * **[INTERNAL]** Add `ReflectDeriveData` * Add `BevyManifest::get_path_direct()` method (`bevy_macro_utils`) Co-authored-by: MrGVSV <[email protected]>
exjam
pushed a commit
to exjam/bevy
that referenced
this pull request
May 22, 2022
…e#4739) # Objective Quick followup to bevyengine#4712. While updating some [other PRs](bevyengine#4218), I realized the `ReflectTraits` struct could be improved. The issue with the current implementation is that `ReflectTraits::get_xxx_impl(...)` returns just the _logic_ to the corresponding `Reflect` trait method, rather than the entire function. This makes it slightly more annoying to manage since the variable names need to be consistent across files. For example, `get_partial_eq_impl` uses a `value` variable. But the name "value" isn't defined in the `get_partial_eq_impl` method, it's defined in three other methods in a completely separate file. It's not likely to cause any bugs if we keep it as it is since differing variable names will probably just result in a compile error (except in very particular cases). But it would be useful to someone who wanted to edit/add/remove a method. ## Solution Made `get_hash_impl`, `get_partial_eq_impl` and `get_serialize_impl` return the entire method implementation for `reflect_hash`, `reflect_partial_eq`, and `serializable`, respectively. As a result of this, those three `Reflect` methods were also given default implementations. This was fairly simple to do since all three could just be made to return `None`. --- ## Changelog * Small cleanup/refactor to `ReflectTraits` in `bevy_reflect_derive` * Gave `Reflect::reflect_hash`, `Reflect::reflect_partial_eq`, and `Reflect::serializable` default implementations
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
# Objective The `bevy_reflect_derive` crate is not the cleanest or easiest to follow/maintain. The `lib.rs` file is especially difficult with over 1000 lines of code written in a confusing order. This is just a result of growth within the crate and it would be nice to clean it up for future work. ## Solution Split `bevy_reflect_derive` into many more submodules. The submodules include: * `container_attributes` - Code relating to container attributes * `derive_data` - Code relating to reflection-based derive metadata * `field_attributes` - Code relating to field attributes * `impls` - Code containing actual reflection implementations * `reflect_value` - Code relating to reflection-based value metadata * `registration` - Code relating to type registration * `utility` - General-purpose utility functions This leaves the `lib.rs` file to contain only the public macros, making it much easier to digest (and fewer than 200 lines). By breaking up the code into smaller modules, we make it easier for future contributors to find the code they're looking for or identify which module best fits their own additions. ### Metadata Structs This cleanup also adds two big metadata structs: `ReflectFieldAttr` and `ReflectDeriveData`. The former is used to store all attributes for a struct field (if any). The latter is used to store all metadata for struct-based derive inputs. Both significantly reduce code duplication and make editing these macros much simpler. The tradeoff is that we may collect more metadata than needed. However, this is usually a small thing (such as checking for attributes when they're not really needed or creating a `ReflectFieldAttr` for every field regardless of whether they actually have an attribute). We could try to remove these tradeoffs and squeeze some more performance out, but doing so might come at the cost of developer experience. Personally, I think it's much nicer to create a `ReflectFieldAttr` for every field since it means I don't have to do two `Option` checks. Others may disagree, though, and so we can discuss changing this either in this PR or in a future one. ### Out of Scope _Some_ documentation has been added or improved, but ultimately good docs are probably best saved for a dedicated PR. ## 🔍 Focus Points (for reviewers) I know it's a lot to sift through, so here is a list of **key points for reviewers**: - The following files contain code that was mostly just relocated: - `reflect_value.rs` - `registration.rs` - `container_attributes.rs` was also mostly moved but features some general cleanup (reducing nesting, removing hardcoded strings, etc.) and lots of doc comments - Most impl logic was moved from `lib.rs` to `impls.rs`, but they have been significantly modified to use the new `ReflectDeriveData` metadata struct in order to reduce duplication. - `derive_data.rs` and `field_attributes.rs` contain almost entirely new code and should probably be given the most attention. - Likewise, `from_reflect.rs` saw major changes using `ReflectDeriveData` so it should also be given focus. - There was no change to the `lib.rs` exports so the end-user API should be the same. ## Prior Work This task was initially tackled by @NathanSWard in bevyengine#2377 (which was closed in favor of this PR), so hats off to them for beating me to the punch by nearly a year! --- ## Changelog * **[INTERNAL]** Split `bevy_reflect_derive` into smaller submodules * **[INTERNAL]** Add `ReflectFieldAttr` * **[INTERNAL]** Add `ReflectDeriveData` * Add `BevyManifest::get_path_direct()` method (`bevy_macro_utils`) Co-authored-by: MrGVSV <[email protected]>
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
…e#4739) # Objective Quick followup to bevyengine#4712. While updating some [other PRs](bevyengine#4218), I realized the `ReflectTraits` struct could be improved. The issue with the current implementation is that `ReflectTraits::get_xxx_impl(...)` returns just the _logic_ to the corresponding `Reflect` trait method, rather than the entire function. This makes it slightly more annoying to manage since the variable names need to be consistent across files. For example, `get_partial_eq_impl` uses a `value` variable. But the name "value" isn't defined in the `get_partial_eq_impl` method, it's defined in three other methods in a completely separate file. It's not likely to cause any bugs if we keep it as it is since differing variable names will probably just result in a compile error (except in very particular cases). But it would be useful to someone who wanted to edit/add/remove a method. ## Solution Made `get_hash_impl`, `get_partial_eq_impl` and `get_serialize_impl` return the entire method implementation for `reflect_hash`, `reflect_partial_eq`, and `serializable`, respectively. As a result of this, those three `Reflect` methods were also given default implementations. This was fairly simple to do since all three could just be made to return `None`. --- ## Changelog * Small cleanup/refactor to `ReflectTraits` in `bevy_reflect_derive` * Gave `Reflect::reflect_hash`, `Reflect::reflect_partial_eq`, and `Reflect::serializable` default implementations
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
The
bevy_reflect_derivecrate is not the cleanest or easiest to follow/maintain. Thelib.rsfile is especially difficult with over 1000 lines of code written in a confusing order. This is just a result of growth within the crate and it would be nice to clean it up for future work.Solution
Split
bevy_reflect_deriveinto many more submodules. The submodules include:container_attributes- Code relating to container attributesderive_data- Code relating to reflection-based derive metadatafield_attributes- Code relating to field attributesimpls- Code containing actual reflection implementationsreflect_value- Code relating to reflection-based value metadataregistration- Code relating to type registrationutility- General-purpose utility functionsThis leaves the
lib.rsfile to contain only the public macros, making it much easier to digest (and fewer than 200 lines).By breaking up the code into smaller modules, we make it easier for future contributors to find the code they're looking for or identify which module best fits their own additions.
Metadata Structs
This cleanup also adds two big metadata structs:
ReflectFieldAttrandReflectDeriveData. The former is used to store all attributes for a struct field (if any). The latter is used to store all metadata for struct-based derive inputs.Both significantly reduce code duplication and make editing these macros much simpler. The tradeoff is that we may collect more metadata than needed. However, this is usually a small thing (such as checking for attributes when they're not really needed or creating a
ReflectFieldAttrfor every field regardless of whether they actually have an attribute).We could try to remove these tradeoffs and squeeze some more performance out, but doing so might come at the cost of developer experience. Personally, I think it's much nicer to create a
ReflectFieldAttrfor every field since it means I don't have to do twoOptionchecks. Others may disagree, though, and so we can discuss changing this either in this PR or in a future one.Out of Scope
Some documentation has been added or improved, but ultimately good docs are probably best saved for a dedicated PR.
🔍 Focus Points (for reviewers)
I know it's a lot to sift through, so here is a list of key points for reviewers:
reflect_value.rsregistration.rscontainer_attributes.rswas also mostly moved but features some general cleanup (reducing nesting, removing hardcoded strings, etc.) and lots of doc commentslib.rstoimpls.rs, but they have been significantly modified to use the newReflectDeriveDatametadata struct in order to reduce duplication.derive_data.rsandfield_attributes.rscontain almost entirely new code and should probably be given the most attention.from_reflect.rssaw major changes usingReflectDeriveDataso it should also be given focus.lib.rsexports so the end-user API should be the same.Prior Work
This task was initially tackled by @NathanSWard in #2377 (which was closed in favor of this PR), so hats off to them for beating me to the punch by nearly a year!
Changelog
bevy_reflect_deriveinto smaller submodulesReflectFieldAttrReflectDeriveDataBevyManifest::get_path_direct()method (bevy_macro_utils)