[Merged by Bors] - Lock down access to Entities #6740

james7132 · 2022-11-24T10:12:37Z

Objective

The soundness of the ECS World partially relies on the correctness of the state of Entities stored within it. We're currently allowing users to (unsafely) mutate it, as well as readily construct it without using a World. While this is not strictly unsound so long as users (including bevy_render) safely use the APIs, it's a fairly easy path to unsoundness without much of a guard rail.

Addresses #3362 for bevy_ecs::entity. Incorporates the changes from #3985.

Solution

Remove Entities's Default implementation and force access to the type to only be through a properly constructed World.

Additional cleanup for other parts of bevy_ecs::entity:

Entity::index and Entity::generation are no longer pub(crate), opting to force the rest of bevy_ecs to use the public interface to access these values.
EntityMeta is no longer pub and also not pub(crate) to attempt to cut down on updating generation without going through an Entities API. It's currently inaccessible except via the pub(crate) Vec on Entities, there was no way for an outside user to use it.
Added Entities::set, an unsafe pub(crate) API for setting the location of an Entity (parallel to Entities::get) that replaces the internal case where we need to set the location of an entity when it's been spawned, moved, or despawned.
Entities::alloc_at_without_replacement is only used in World::get_or_spawn within the first party crates, and I cannot find a public use of this API in any ecosystem crate that I've checked (via GitHub search).
Attempted to document the few remaining undocumented public APIs in the module.

Changelog

Removed: Entities's Default implementation.
Removed: EntityMeta
Removed: Entities::alloc_at_without_replacement and AllocAtWithoutReplacement.

Migration Guide

Entities's Default implementation has been removed. You can fetch a reference to a World's Entities via World::entities and World::entities_mut.

Entities::alloc_at_without_replacement and AllocAtWithoutReplacement has been made private due to difficulty in using it properly outside of bevy_ecs. If you still need use of this API, please file an issue.

crates/bevy_ecs/src/entity/map_entities.rs

crates/bevy_ecs/src/entity/mod.rs

alice-i-cecile

I like this change a lot, and the docs are good. IMO this is essential for #6733, as it restricts users to the happy path.

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

crates/bevy_ecs/src/entity/mod.rs

Co-authored-by: Jakob Hellermann <jakob.hellermann@protonmail.com>

…tity

james7132 · 2022-11-25T02:11:18Z

Incorporated the changes from #3985 and reworded it to address the unresolved comments from that PR.

jakobhellermann

One thought I had is that Entity: Reflect which usually means that the fields are considered public and exposed. But we do impl_reflect_value

bevy/crates/bevy_ecs/src/reflect.rs

Line 404 in 3827316

impl_reflect_value!(Entity(Hash, PartialEq, Serialize, Deserialize));

so it is treated as an opaque identifier.

jakobhellermann · 2022-11-25T08:09:08Z

crates/bevy_ecs/src/entity/mod.rs

    Exists(EntityLocation),
    DidNotExist,
    ExistsWithWrongGeneration,
 }

 impl Entity {
+    #[cfg(test)]
+    pub(crate) const fn new(index: u32, generation: u32) -> Entity {


I looked at the other methods on Entity.

After this PR we still have

Entity::from_bits(Entity::to_bits(entity)) is still a perfect roundtrip

Entity::from_raw(index) still exists and gives an entity with index and generation 0

Entity::index() and Entity::generation()

The use case from the docs of from_raw could be replace with Entity::INVALID. Do we know of other use cases, otherwise maybe replace it?

This actually isn't a departure from the current status quo. Both fields were pub(crate) before this so you couldn't create a raw entity outside of Entity::from_raw before this PR.

The only time I can think of needing to construct a raw entity that isn't covered by serialization or reflection is for testing, for which there is no real need to round-trip it.

alice-i-cecile · 2022-11-28T13:20:19Z

That last thread may be nice to address elsewhere, but shouldn't block this work :)

bors r+

# Objective The soundness of the ECS `World` partially relies on the correctness of the state of `Entities` stored within it. We're currently allowing users to (unsafely) mutate it, as well as readily construct it without using a `World`. While this is not strictly unsound so long as users (including `bevy_render`) safely use the APIs, it's a fairly easy path to unsoundness without much of a guard rail. Addresses #3362 for `bevy_ecs::entity`. Incorporates the changes from #3985. ## Solution Remove `Entities`'s `Default` implementation and force access to the type to only be through a properly constructed `World`. Additional cleanup for other parts of `bevy_ecs::entity`: - `Entity::index` and `Entity::generation` are no longer `pub(crate)`, opting to force the rest of bevy_ecs to use the public interface to access these values. - `EntityMeta` is no longer `pub` and also not `pub(crate)` to attempt to cut down on updating `generation` without going through an `Entities` API. It's currently inaccessible except via the `pub(crate)` Vec on `Entities`, there was no way for an outside user to use it. - Added `Entities::set`, an unsafe `pub(crate)` API for setting the location of an Entity (parallel to `Entities::get`) that replaces the internal case where we need to set the location of an entity when it's been spawned, moved, or despawned. - `Entities::alloc_at_without_replacement` is only used in `World::get_or_spawn` within the first party crates, and I cannot find a public use of this API in any ecosystem crate that I've checked (via GitHub search). - Attempted to document the few remaining undocumented public APIs in the module. --- ## Changelog Removed: `Entities`'s `Default` implementation. Removed: `EntityMeta` Removed: `Entities::alloc_at_without_replacement` and `AllocAtWithoutReplacement`. Co-authored-by: james7132 <contact@jamessliu.com> Co-authored-by: James Liu <contact@jamessliu.com>

bors · 2022-11-28T13:38:53Z

Build failed (retrying...):

check-compiles

bors · 2022-11-28T14:13:58Z

Merge conflict.

james7132 · 2022-11-28T20:38:43Z

bors retry

# Objective The soundness of the ECS `World` partially relies on the correctness of the state of `Entities` stored within it. We're currently allowing users to (unsafely) mutate it, as well as readily construct it without using a `World`. While this is not strictly unsound so long as users (including `bevy_render`) safely use the APIs, it's a fairly easy path to unsoundness without much of a guard rail. Addresses #3362 for `bevy_ecs::entity`. Incorporates the changes from #3985. ## Solution Remove `Entities`'s `Default` implementation and force access to the type to only be through a properly constructed `World`. Additional cleanup for other parts of `bevy_ecs::entity`: - `Entity::index` and `Entity::generation` are no longer `pub(crate)`, opting to force the rest of bevy_ecs to use the public interface to access these values. - `EntityMeta` is no longer `pub` and also not `pub(crate)` to attempt to cut down on updating `generation` without going through an `Entities` API. It's currently inaccessible except via the `pub(crate)` Vec on `Entities`, there was no way for an outside user to use it. - Added `Entities::set`, an unsafe `pub(crate)` API for setting the location of an Entity (parallel to `Entities::get`) that replaces the internal case where we need to set the location of an entity when it's been spawned, moved, or despawned. - `Entities::alloc_at_without_replacement` is only used in `World::get_or_spawn` within the first party crates, and I cannot find a public use of this API in any ecosystem crate that I've checked (via GitHub search). - Attempted to document the few remaining undocumented public APIs in the module. --- ## Changelog Removed: `Entities`'s `Default` implementation. Removed: `EntityMeta` Removed: `Entities::alloc_at_without_replacement` and `AllocAtWithoutReplacement`. Co-authored-by: james7132 <contact@jamessliu.com> Co-authored-by: James Liu <contact@jamessliu.com>

bors · 2022-11-28T20:52:03Z

Pull request successfully merged into main.

Build succeeded:

# Objective The soundness of the ECS `World` partially relies on the correctness of the state of `Entities` stored within it. We're currently allowing users to (unsafely) mutate it, as well as readily construct it without using a `World`. While this is not strictly unsound so long as users (including `bevy_render`) safely use the APIs, it's a fairly easy path to unsoundness without much of a guard rail. Addresses bevyengine#3362 for `bevy_ecs::entity`. Incorporates the changes from bevyengine#3985. ## Solution Remove `Entities`'s `Default` implementation and force access to the type to only be through a properly constructed `World`. Additional cleanup for other parts of `bevy_ecs::entity`: - `Entity::index` and `Entity::generation` are no longer `pub(crate)`, opting to force the rest of bevy_ecs to use the public interface to access these values. - `EntityMeta` is no longer `pub` and also not `pub(crate)` to attempt to cut down on updating `generation` without going through an `Entities` API. It's currently inaccessible except via the `pub(crate)` Vec on `Entities`, there was no way for an outside user to use it. - Added `Entities::set`, an unsafe `pub(crate)` API for setting the location of an Entity (parallel to `Entities::get`) that replaces the internal case where we need to set the location of an entity when it's been spawned, moved, or despawned. - `Entities::alloc_at_without_replacement` is only used in `World::get_or_spawn` within the first party crates, and I cannot find a public use of this API in any ecosystem crate that I've checked (via GitHub search). - Attempted to document the few remaining undocumented public APIs in the module. --- ## Changelog Removed: `Entities`'s `Default` implementation. Removed: `EntityMeta` Removed: `Entities::alloc_at_without_replacement` and `AllocAtWithoutReplacement`. Co-authored-by: james7132 <contact@jamessliu.com> Co-authored-by: James Liu <contact@jamessliu.com>

nakedible and others added 4 commits February 19, 2022 01:54

Document MapEntities trait.

fbde14e

Make EntityMeta non-pub and non-pub(crate)

eac9a79

Make index and generation not pub(crate)

86c9f54

Force Entities to only be constructed in the context of a World

595415b

james7132 requested a review from alice-i-cecile November 24, 2022 10:13

james7132 added 5 commits November 24, 2022 02:26

Add basic type-level docs to Entities

6cad075

Fix CI

fa8eaf1

meta_len -> total_count

d83fc4c

Fix doc link

f0d3d46

Document MapEntities and MapEntitiesError

a3fd80f

alice-i-cecile reviewed Nov 24, 2022

View reviewed changes

crates/bevy_ecs/src/entity/map_entities.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Nov 24, 2022

View reviewed changes

crates/bevy_ecs/src/entity/mod.rs Outdated Show resolved Hide resolved

alice-i-cecile approved these changes Nov 24, 2022

View reviewed changes

Add missing "internal"

4ac6307

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

jakobhellermann approved these changes Nov 24, 2022

View reviewed changes

crates/bevy_ecs/src/entity/mod.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/src/entity/mod.rs Outdated Show resolved Hide resolved

james7132 and others added 3 commits November 24, 2022 07:35

Wording on Entities' type level docs

9cf5fee

Co-authored-by: Jakob Hellermann <jakob.hellermann@protonmail.com>

Fix docs on total_count

693dd03

Update Enities::set's safety docs

17ee537

alice-i-cecile 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 Nov 24, 2022

Weibye approved these changes Nov 24, 2022

View reviewed changes

james7132 mentioned this pull request Nov 24, 2022

Document MapEntities trait. #3985

Closed

james7132 added this to the 0.10 milestone Nov 24, 2022

james7132 added 2 commits November 24, 2022 17:35

Merge remote-tracking branch 'nakedible/patch-1' into document-ecs-en…

7769620

…tity

Apply review comments from bevyengine#3985.

ded316f

jakobhellermann reviewed Nov 25, 2022

View reviewed changes

Merge branch 'main' into document-ecs-entity

8ddce06

bors bot changed the title ~~Lock down access to Entities~~ [Merged by Bors] - Lock down access to Entities Nov 28, 2022

bors bot closed this Nov 28, 2022

james7132 deleted the document-ecs-entity branch December 12, 2022 04:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Lock down access to Entities #6740

[Merged by Bors] - Lock down access to Entities #6740

james7132 commented Nov 24, 2022 •

edited

alice-i-cecile left a comment

james7132 commented Nov 25, 2022

jakobhellermann left a comment

jakobhellermann Nov 25, 2022

james7132 Nov 25, 2022

alice-i-cecile commented Nov 28, 2022

bors bot commented Nov 28, 2022

bors bot commented Nov 28, 2022

james7132 commented Nov 28, 2022

bors bot commented Nov 28, 2022

[Merged by Bors] - Lock down access to Entities #6740

[Merged by Bors] - Lock down access to Entities #6740

Conversation

james7132 commented Nov 24, 2022 • edited

Objective

Solution

Changelog

Migration Guide

alice-i-cecile left a comment

Choose a reason for hiding this comment

james7132 commented Nov 25, 2022

jakobhellermann left a comment

Choose a reason for hiding this comment

jakobhellermann Nov 25, 2022

Choose a reason for hiding this comment

james7132 Nov 25, 2022

Choose a reason for hiding this comment

alice-i-cecile commented Nov 28, 2022

bors bot commented Nov 28, 2022

bors bot commented Nov 28, 2022

james7132 commented Nov 28, 2022

bors bot commented Nov 28, 2022

james7132 commented Nov 24, 2022 •

edited