Skip to content

Component requirements shall need either Default or to be a unit struct. #18911

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
Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Component requirements shall need either Default or to be a unit struct. #18911

wants to merge 12 commits into from
+105 −162

Conversation

coolcatcoder
Copy link

@coolcatcoder coolcatcoder commented Apr 24, 2025 •
edited
Loading

Objective

Solution

  • Auto deref specialisation. Both const and non-const.

Testing

  • We must check that requirements with default are allowed, and ones without default are denied unless they so happen to be a unit struct.

@coolcatcoder
A few failed attempts.
4646ae2 
Not certain how small you want each commit to be? I'll just commit every now and again I suppose.
@coolcatcoder
rock and a hard place
19dca3f 
 Continuous integration complains that my let binding has unit value, but if I remove it, then it claims that my code is a path statement with no effect.
@coolcatcoder
Switch constant to const fn!
7560c69 
This gets rid of the continuous integration errors.
@coolcatcoder
Nothing needs default! Ah!
9f5f8dd 
Because this is inside a trait, I need to use a slightly different approach. Whoops. Should be fixed next commit.
@github-actions GitHub Actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events X-Contentious There are nontrivial implications that should be thought through S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged D-Macros Code that generates Rust code labels Apr 24, 2025
@coolcatcoder
Correctly deny invalid requirements.
6a61f32 
Had to switch to const deref specialisation, but it caused the error message to improve, which is quite nice.
@coolcatcoder
extra assurance
62cfa52 
Added a check to make 100% sure that only 0 sized types don't use default.
@coolcatcoder
Remove random hidden space.
bced340
@coolcatcoder
Switch to a compile fail test!
71f9c29 
It doesn't work yet, but it is along the right track.
Also why are there so many clippy issues? I didn't even touch the code they are complaining about!
@coolcatcoder
Fixed the compile fail!
7af39dd
@coolcatcoder
clean up
ab5a07b
@coolcatcoder coolcatcoder marked this pull request as ready for review April 25, 2025 00:30
@andriyDev andriyDev added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Apr 25, 2025
benfrankel
benfrankel suggested changes Apr 25, 2025
View reviewed changes
Copy link
Contributor

@benfrankel benfrankel 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.

I'm curious about the motivation for this feature, as opposed to deriving Default on the required component. Is this meant for components that are defined in external crates?

EDIT: I see there's a linked issue but this seems to be an open question there as well.

.cargo/config_fast_builds.toml Outdated
Copy link
Contributor

@benfrankel benfrankel Apr 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This file was probably not meant to be deleted.

Copy link
Author

Choose a reason for hiding this comment

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

I wasn't exactly sure how to handle the fast build toml. I don't yet know enough about git to know how to exclude my changes to that directory. I suppose I'll just revert those files back to normal at the end.

crates/bevy_ecs/src/component.rs
}

/// Checks if a component's requirement is valid.
/// It is valid if it has Default, or if it has a size of 0 and is therefore a unit struct.
Copy link
Contributor

@benfrankel benfrankel Apr 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Technically not all ZSTs are unit structs. For example, struct Foo { x: () } is a ZST but not a unit struct. Also empty types (like ! and enum Bar {}) and functions are ZSTs.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah. I am worried about that, but at the same time, it seems unlikely for such structs to be marked as a component. Are there any foot guns I haven't noticed?

Copy link
Author

Choose a reason for hiding this comment

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

If a struct has a single private field that is a zst, so that people cannot construct it, then this bypasses it. Yikes. Unlikely to be a problem, but still not great.
We could solve this by making the component derive implement a UnitStruct trait when it encounters a unit struct.

coolcatcoder and others added 2 commits April 25, 2025 15:50
@coolcatcoder @benfrankel
Update crates/bevy_ecs/compile_fail/tests/ui/non_unit_without_default…
6b2931b 
…_required_components.rs


Thanks benfrankel! That was left over from when I accidentally put the error checker in the wrong place...

Co-authored-by: Ben Frankel <ben.frankel7@gmail.com>
@coolcatcoder @benfrankel
Update crates/bevy_ecs/compile_fail/tests/ui/non_unit_without_default…
c3c05de 
…_required_components.rs


Thanks benfrankel!

Co-authored-by: Ben Frankel <ben.frankel7@gmail.com>
@coolcatcoder
Copy link
Author

I'm curious about the motivation for this feature, as opposed to deriving Default on the required component. Is this meant for components that are defined in external crates?

EDIT: I see there's a linked issue but this seems to be an open question there as well.

I have no idea why this would be wanted. I simply saw an issue that it was claimed was not possible to solve, and as such I just had to solve it. (It was fun! Although, I did spend 90% of my time fighting with continuous integration, because I had never used it before!)

@alice-i-cecile alice-i-cecile added S-Nominated-To-Close A triage team member thinks this PR or issue should be closed out. and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 5, 2025
@alice-i-cecile
Copy link
Member

alice-i-cecile commented May 5, 2025 •
edited
Loading

Technically not all ZSTs are unit structs. For example, struct Foo { x: () } is a ZST but not a unit struct. Also empty types (like ! and enum Bar {}) and functions are ZSTs.

This makes me very nervous. I don't think that the value to risk / complexity is worth it here. I've marked the upstream issue Nominated-to-Close, and think we should do the same here.

Super cool that you could do this though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benfrankel benfrankel benfrankel requested changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible D-Macros Code that generates Rust code S-Nominated-To-Close A triage team member thinks this PR or issue should be closed out. X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ignore Default requirements when using unit structs in required components
4 participants
@coolcatcoder @alice-i-cecile @benfrankel @andriyDev