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

bevy_reflect_derive: Clean up attribute logic #11777

Merged
merged 11 commits into from Feb 12, 2024

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented Feb 8, 2024

Objective

The code in bevy_reflect_derive could use some cleanup.

Solution

Took some of the changes in #11659 to create a dedicated PR for cleaning up the field and container attribute logic.

Updated Naming

I renamed ReflectTraits and ReflectFieldAttr to ContainerAttributes and FieldAttributes, respectively. I think these are clearer.

Updated Parsing

Readability

The parsing logic wasn't too bad before, but it was getting difficult to read. There was some duplicated logic between Meta::List and Meta::Path attributes. Additionally, all the logic was kept inside a large method.

To simply things, I replaced the nested meta parsing with ParseStream parsing. In my opinion, this is easier to follow since it breaks up the large match statement into a small set of single-line if statements, where each if-block contains a single call to the appropriate attribute parsing method.

Flexibility

On top of the added simplicity, this also makes our attribute parsing much more flexible. It allows us to more elegantly handle custom where clauses (i.e. #[reflect(where T: Foo)]) and it opens the door for more non-standard attribute syntax (e.g. #11659).

Errors

This also allows us to automatically provide certain errors when parsing. For example, since we can use stream.lookahead1(), we get errors like the following for free:

error: expected one of: `ignore`, `skip_serializing`, `default`
    --> crates/bevy_reflect/src/lib.rs:1988:23
     |
1988 |             #[reflect(foo)]
     |                       ^^^

Changelog

Note

All changes are internal to bevy_reflect_derive and should not affect the public API1.

  • Renamed ReflectTraits to ContainerAttributes
    • Renamed ReflectMeta::traits to ReflectMeta::attrs
  • Renamed ReflectFieldAttr to FieldAttributes
  • Updated parsing logic for field/container attribute parsing
    • Now uses a ParseStream directly instead of nested meta parsing
  • General code cleanup of the field/container attribute modules for bevy_reflect_derive

Footnotes

  1. Does not include errors, which may look slightly different.

Parse ParseStream directly

Custom attributes are not fully implemented yet.
This just permits their definition via the derive macro.
@MrGVSV MrGVSV force-pushed the mrgvsv/reflect/clean-attributes branch from aebf1cd to 1472fc9 Compare February 8, 2024 02:57
@MrGVSV MrGVSV added C-Code-Quality A section of code that is hard to understand or change A-Reflection Runtime information about types labels Feb 8, 2024
@MrGVSV MrGVSV requested a review from soqb February 8, 2024 02:59
@MrGVSV MrGVSV force-pushed the mrgvsv/reflect/clean-attributes branch from 1472fc9 to 1837098 Compare February 8, 2024 03:26
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.

i love this!

@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 8, 2024
auto-merge was automatically disabled February 12, 2024 01:17

Head branch was pushed to by a user without write access

@MrGVSV
Copy link
Member Author

MrGVSV commented Feb 12, 2024

I updated the compile fail tests to account for the new attribution parsing. A side-effect of these changes is that it's now possible to include where attributes inside registration attributes like: #[reflect(Hash, where T: Debug)].

I decided not to remove it since we already allow this behavior for other non-registration attributes (i.e. from_reflect = false, no_field_bound, etc.) and because enforcing this with an error might be worth a separate discussion.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 12, 2024
Merged via the queue into bevyengine:main with commit 9e30aa7 Feb 12, 2024
23 checks passed
@MrGVSV MrGVSV deleted the mrgvsv/reflect/clean-attributes branch February 12, 2024 18:15
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-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
Status: In Progress
Development

Successfully merging this pull request may close these issues.

None yet

3 participants