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

ECS metadata stores should operate on Pin<&mut T> #12272

Open
james7132 opened this issue Mar 3, 2024 · 1 comment
Open

ECS metadata stores should operate on Pin<&mut T> #12272

james7132 opened this issue Mar 3, 2024 · 1 comment
Labels
A-ECS Entities, components, systems, and events P-Unsound A bug that results in undefined compiler behavior

Comments

@james7132
Copy link
Member

james7132 commented Mar 3, 2024

What problem does this solve or what need does it fill?

#10756 highlighted std::mem::swap and std::mem::take as potential venues for unsoundness: moving the metadata out of the World invalidates the metadata stores, which the entire rest of the ECS implementation relies on.

What solution would you like?

Use the tools that Rust gives us to prevent trivial moves: std::pin::Pin. Core metadata stores like Components, Archetypes, Bundles, etc. should minimize mutable access, and only return mutable access in the form of Pin<&mut T> instead of &mut T.

Note this likely means the Index/IndexMut implementations for these various metadata stores will need to go away, as they require &mut T to be returned.

bevy_ecs already follows the practices of not exposing any mutable access to metadata stores in it's public interface, but I'm of the opinion that the internal APIs should be no different either. There are quite a few cases of pub(crate) access that should be minimized where possible.

What alternative(s) have you considered?

Returning wrappers around &mut ComponentInfo and the other metadata. Requires much more code to implement.

@james7132 james7132 added A-ECS Entities, components, systems, and events P-Unsound A bug that results in undefined compiler behavior labels Mar 3, 2024
@SkiFire13
Copy link
Contributor

moving the metadata out of the World invalidates the metadata stores, which the entire rest of the ECS implementation relies on.

Note that this can happen with Pin too, since Pin::set is safe. You would have to disallow creating new instances of the metadata structs too (e.g. disallow Default for Components).

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 P-Unsound A bug that results in undefined compiler behavior
Projects
None yet
Development

No branches or pull requests

2 participants