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

[Merged by Bors] - yeet World::components_mut >:( #4092

Closed
wants to merge 1 commit into from

Conversation

BoxyUwU
Copy link
Member

@BoxyUwU BoxyUwU commented Mar 4, 2022

Objective

make bevy ecs a lil bit less unsound

Solution

yeet unsound API World::components_mut:

use bevy_ecs::prelude::*;

#[derive(Component)]
struct Foo(u8);

#[derive(Debug, Component)]
struct Bar([u8; 100]);

fn main() {
    let mut world = World::new();
    let e = world.spawn().insert(Foo(0)).id();
    *world.components_mut() = Default::default();
    let bar = world.entity_mut(e).remove::<Bar>().unwrap();
    // oopsies reading memory copied from outside allocation
    dbg!(bar);
}

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 4, 2022
@BoxyUwU BoxyUwU added the A-ECS Entities, components, systems, and events label Mar 4, 2022
@IceSentry IceSentry removed the S-Needs-Triage This issue needs to be labelled label Mar 4, 2022
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior P-High This is particularly urgent, and deserves immediate attention labels Mar 4, 2022
@bjorn3
Copy link
Contributor

bjorn3 commented Mar 4, 2022

Maybe return a Pin and not have any methods on Components that can remove components?

@BoxyUwU
Copy link
Member Author

BoxyUwU commented Mar 4, 2022

Pin seems sus as we are not actually using any kind of self referential stuff? this does give me an idea to just introduce a ComponentsPtr<'a> with a private field &mut Components and implement a bunch of fns on it that call the Components methods without ever exposing &mut Components to the user

@bjorn3
Copy link
Contributor

bjorn3 commented Mar 4, 2022

Pin seems sus as we are not actually using any kind of self referential stuff?

We require Components doesn't get moved out, which is exactly what Pin does. It just so happens that the main reason to introduce it was self-referential types.

@BoxyUwU
Copy link
Member Author

BoxyUwU commented Mar 4, 2022

Moving out is fine, its mutating the Components in a world (in such a sort of "non appending" way) that is wrong

@bjorn3
Copy link
Contributor

bjorn3 commented Mar 4, 2022

If you move something out of a mutable reference, you have to move something else in which is a non appending mutation.

@BoxyUwU
Copy link
Member Author

BoxyUwU commented Mar 6, 2022

Looking at what methods are on Components there are only three that take &mut self

  • init_resource this method is also on World and App
  • init_non_send this method is also on World and App
  • init_component this method is not callable by users because there is no way to get &mut Storages

additionally all fields are private, so I think there is not any harm in just removing World::components_mut

@BoxyUwU
Copy link
Member Author

BoxyUwU commented Mar 7, 2022

Oh nevermind init_resource on world is different than on Components that is :/

@james7132
Copy link
Member

Looking at what methods are on Components there are only three that take &mut self

* `init_resource` this method is also on `World` and `App`

* `init_non_send` this method is also on `World` and `App`

* `init_component` this method is not callable by users because there is no way to get `&mut Storages`

additionally all fields are private, so I think there is not any harm in just removing World::components_mut

If this is the case, would it be worth privating those APIs too, since there will be no way to reasonably use those from the World API with this removed?

@BoxyUwU BoxyUwU 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 Mar 17, 2022
@cart
Copy link
Member

cart commented Mar 21, 2022

bors r+

bors bot pushed a commit that referenced this pull request Mar 21, 2022
# Objective
make bevy ecs a lil bit less unsound
## Solution
yeet unsound API `World::components_mut`:
```rust
use bevy_ecs::prelude::*;

#[derive(Component)]
struct Foo(u8);

#[derive(Debug, Component)]
struct Bar([u8; 100]);

fn main() {
    let mut world = World::new();
    let e = world.spawn().insert(Foo(0)).id();
    *world.components_mut() = Default::default();
    let bar = world.entity_mut(e).remove::<Bar>().unwrap();
    // oopsies reading memory copied from outside allocation
    dbg!(bar);
}
```
@cart
Copy link
Member

cart commented Mar 21, 2022

This seems like a reasonable fix for this specific instance of the problem, but I am a little concerned about what this "issue" means for Rust api design. Pasting in some of my thoughts from Discord:

On the topic of "you can override the value stored at the pointer returned by World::components_mut() and break everything" (Boxy's 4092 pr fixes this by removing it), this feels like a really fundamental "hole" in my rust api design approach that I wasn't really thinking about. I think Boxy's "just dont expose it" solution is probably a reasonable fix for this specific case, but I'd like to discuss our options for this style of api.

On the one hand, Rust encourages "nested composition" via ownership rules and a lack of inheritance. The current World design (World contains Components, Storages, Entities) is a direct response to these constraints. I personally really like that (prior to this discovered issue), we could build out the functionality of Components, Storages, and Entities (including mutations) without needing to re-define it all at the top of World.

The only way to do "mutable" operations on World::components is to return a mutable reference to Components. But doing that lets people replace Components with whatever, violating the assumptions made by the system as a whole! (the bug Boxy discovered and fixed via removing the mutable accessor).

Is there really no way to embrace this "nested concerns" pattern? So far the "best" solutions I'm aware of are:

  1. Re-expose the relevant Component functionality on top of World and hide the Components collection from users (lots of duplicate code and "globbing together" of concerns).
  2. Strictly worse than (1). Re-expose the relevant Component functionality on top of World and dont hide the Components collection from users (same downsides as (1), but with multiple apis, one top level one for mutation and one lower level one for reads).
  3. Don't implement any methods on Components and instead put all functionality on World. This provides consistency without code duplication, but at the cost of "globbing everything together".

@BoxyUwU then brought up this solution:

we can have a newtyped &mut Compoennts
i.e. ComponentsMut<'a>
and basically put all the methods on that
[with a Deref impl on ComponentsMut to expose read only methods on Components]

Boxy's solution is definitely a "real" solution, but its definitely not "ideal" from an implementation or documentation perspective because it adds real implementation complexity / makes it so you can't talk about Components as a simple "read / write collection" anymore.

@bors bors bot changed the title yeet World::components_mut >:( [Merged by Bors] - yeet World::components_mut >:( Mar 21, 2022
@bors bors bot closed this Mar 21, 2022
aevyrie pushed a commit to aevyrie/bevy that referenced this pull request Jun 7, 2022
# Objective
make bevy ecs a lil bit less unsound
## Solution
yeet unsound API `World::components_mut`:
```rust
use bevy_ecs::prelude::*;

#[derive(Component)]
struct Foo(u8);

#[derive(Debug, Component)]
struct Bar([u8; 100]);

fn main() {
    let mut world = World::new();
    let e = world.spawn().insert(Foo(0)).id();
    *world.components_mut() = Default::default();
    let bar = world.entity_mut(e).remove::<Bar>().unwrap();
    // oopsies reading memory copied from outside allocation
    dbg!(bar);
}
```
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective
make bevy ecs a lil bit less unsound
## Solution
yeet unsound API `World::components_mut`:
```rust
use bevy_ecs::prelude::*;

#[derive(Component)]
struct Foo(u8);

#[derive(Debug, Component)]
struct Bar([u8; 100]);

fn main() {
    let mut world = World::new();
    let e = world.spawn().insert(Foo(0)).id();
    *world.components_mut() = Default::default();
    let bar = world.entity_mut(e).remove::<Bar>().unwrap();
    // oopsies reading memory copied from outside allocation
    dbg!(bar);
}
```
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-Bug An unexpected or incorrect behavior P-High This is particularly urgent, and deserves immediate attention S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants