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

Relax documented safety requirements for *_assume_mut #17140

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vil-mo
Copy link
Contributor

@vil-mo vil-mo commented Jan 4, 2025

Objective

Often you want to lock end users from mutating component, while still having the ability to mutate it in the library internals. For example there is a Child and Parent relationship in the bevy_hierarchy #16662

For that to be possible, the safety requirements of *_assume_mutable methods should be relaxed, since they are currently too restrictive for no reason, and do not allow mutation of an immutable component under any circumstances.

Solution

Current safety requirements:

- T must be a mutable component

Replaced with:

- One of the following should be true:
   - Either `T` is mutable, or
   - `OnReplace` hooks and observers for that component on that entity should trigger immediately before the mutation
     and `OnInsert` should trigger immediately after the mutation, or
   - The user should uphold documented invariants of `T`. If no such documentation provided, it is impossible for the end user to uphold this.

Note

The pr linked above already breaks current safety requirements, and the suggested alternative also breaks it.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Unsafe Touches with unsafe code in some way S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 5, 2025
Copy link
Contributor

@chescock chescock left a comment

Choose a reason for hiding this comment

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

Loosening the requirement like this seems like a good idea, because people will expect this to work, as shown in the Parent/Child PR.

(If we don't, then we might want to debug_assert! that the component is actually mutable to help catch misuses.)

On the other hand, it seems hard to have a case where firing the triggers manually is simpler than calling modify_component(). When would this method actually be called on an immutable component?

Allowing this will prohibit at least one possible optimization: With the current rules, the changed_tick is always equal to the added_tick for an immutable component, and it would be valid to store only a single copy, with the column storing two pointers to the same allocation. It would then be legal to create a Ref, but UB to create a Mut, since the ticks pointers would alias. (That optimization might be a bad idea anyway! I just want to illustrate that changing the safety requirement here isn't completely free.)

/// - Either `T` is mutable, or
/// - `OnReplace` hooks and observers for that component on that entity should trigger immediately before the mutation
/// and `OnInsert` should trigger immediately after the mutation, or
/// - The user should uphold documented invariants of `T`. If no such documentation provided, it is impossible for the end user to uphold this.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this requirement necessary? If it is, then I think fn modify_component() would need to be unsafe and include a similar requirement.

But it seems like this could be handled by privacy: Anything that would violate the invariants of T just shouldn't be pub. That just leaves things like mem::swap, but as long as you're firing triggers that should be indistinguishable from overwriting the component with insert.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Unsafe Touches with unsafe code in some way S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants