-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Move allow unsafe_op_in_unsafe_fn to module level in bevy_ecs #22134
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
Move allow unsafe_op_in_unsafe_fn to module level in bevy_ecs #22134
Conversation
|
Ping me when CI is green and I'll approve :) |
| .get_resource_ref::<HotPatchChanges>() | ||
| .map(|r| r.last_changed()) | ||
| .unwrap_or_default(); | ||
| // SAFETY: No system should mutate `HotPatchChanges` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not the best safety, but hotpatching is already pretty unsafe and the resource only exists when hotpatching is active.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really convinced by this safety doc. "Should" seems to imply that HotPatchChanges can be mutated while this reference is held.
The safety doc should prove that the unsafe operation is in-fact safe. For example, if HotPatchChanges cannot be mutated during this, then the safety doc should mention as such. Same with if it's the caller's responsibility to uphold that safety.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not safe. A system could actually try to mutate HotPatchChanges. But I'm don't think it's worth making this safe. We'd have to check if any system that is added tries to mutate HotPatchChanges. But they could only make a system that mutates HotPatchChanges when the hotpatching feature is enabled. Overall hotpatching is probably UB, so if someone ships with hotpatching enabled it's already a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not safe.
Then the safety doc should be removed, as it's not documenting the safety of the unsafe {} block.
And actually, if we know this to be unsafe, then we should document it as such. This ensures a couple things:
- Users perusing the code won't be confused into thinking that thus unsafe operation is somehow safe.
- Future safety-related PRs will know that this code is unsafe, giving them an idea of what they're getting into.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can use an expect annotation here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can use an expect annotation here?
Yeah, that might be more clear.
LikeLakers2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for being pedantic in my previous comments. I think it looks okay now.
alice-i-cecile
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the use of expect here: reading it makes me flinch, as it should lol. This is a good incremental step towards fixing these problems, let's merge it.
Objective
Testing