Fix EntityMeta.spawned_or_despawned unsoundness

[SpecificProtagonist]

Objective

#19047 added an MaybeUninit field to EntityMeta, but did not guarantee that it will be initialized before access:

let mut world = World::new();
let id = world.entities().reserve_entity();
world.flush();
world.entity(id);

Miri Error

error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory
    --> /home/vj/workspace/rust/bevy/crates/bevy_ecs/src/entity/mod.rs:1121:26
     |
1121 |                 unsafe { meta.spawned_or_despawned.assume_init() }
     |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ using uninitialized data, but this operation requires initialized memory
     |
     = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
     = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
     = note: BACKTRACE:
     = note: inside closure at /home/vj/workspace/rust/bevy/crates/bevy_ecs/src/entity/mod.rs:1121:26: 1121:65
     = note: inside `std::option::Option::<&bevy_ecs::entity::EntityMeta>::map::<bevy_ecs::entity::SpawnedOrDespawned, {closure@bevy_ecs::entity::Entities::entity_get_spawned_or_despawned::{closure#1}}>` at /home/vj/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/option.rs:1144:29: 1144:33
     = note: inside `bevy_ecs::entity::Entities::entity_get_spawned_or_despawned` at /home/vj/workspace/rust/bevy/crates/bevy_ecs/src/entity/mod.rs:1112:9: 1122:15
     = note: inside closure at /home/vj/workspace/rust/bevy/crates/bevy_ecs/src/entity/mod.rs:1094:13: 1094:57
     = note: inside `bevy_ecs::change_detection::MaybeLocation::<std::option::Option<&std::panic::Location<'_>>>::new_with_flattened::<{closure@bevy_ecs::entity::Entities::entity_get_spawned_or_despawned_by::{closure#0}}>` at /home/vj/workspace/rust/bevy/crates/bevy_ecs/src/change_detection.rs:1371:20: 1371:24
     = note: inside `bevy_ecs::entity::Entities::entity_get_spawned_or_despawned_by` at /home/vj/workspace/rust/bevy/crates/bevy_ecs/src/entity/mod.rs:1093:9: 1096:11
     = note: inside `bevy_ecs::entity::Entities::entity_does_not_exist_error_details` at /home/vj/workspace/rust/bevy/crates/bevy_ecs/src/entity/mod.rs:1163:23: 1163:70
     = note: inside `bevy_ecs::entity::EntityDoesNotExistError::new` at /home/vj/workspace/rust/bevy/crates/bevy_ecs/src/entity/mod.rs:1182:22: 1182:74
     = note: inside `bevy_ecs::world::unsafe_world_cell::UnsafeWorldCell::<'_>::get_entity` at /home/vj/workspace/rust/bevy/crates/bevy_ecs/src/world/unsafe_world_cell.rs:368:20: 368:73
     = note: inside `<bevy_ecs::entity::Entity as bevy_ecs::world::WorldEntityFetch>::fetch_ref` at /home/vj/workspace/rust/bevy/crates/bevy_ecs/src/world/entity_fetch.rs:207:21: 207:42
     = note: inside `bevy_ecs::world::World::get_entity::<bevy_ecs::entity::Entity>` at /home/vj/workspace/rust/bevy/crates/bevy_ecs/src/world/mod.rs:911:18: 911:42
note: inside `main`
    --> src/main.rs:12:15
     |
12   |     world.entity(id);
     |

Solution

• remove the existing MaybeUninit in EntityMeta.spawned_or_despawned
• initialize during flush. This is not needed for soundness, but not doing this means we can't return a sensible location/tick for flushed entities.

Testing

Test via the snippet above (also added equivalent test).

[urben1680]

I think this is a sensible solution, with two things to clear up.

I also wonder if the internal method names and SpawnedOrDespawned should be renamed to reflect that flushing also updating these values, not only spawn/despawn. "spawn_or_despawn_or_flush" is quite long though, maybe updated_at/updated_by is fine in EntityMeta. After all without MaybeUninit these two fields dont need to be in a separate type anymore.

I think this PR and your nickname belongs to the feature's release note by the way. 😉

[urben1680]

What is the idea behind separating this and set? This here still gets called next to set, adding another indexing into the same EntityMeta. You already added S-Needs-Benchmarking so I am interested if this has bad effects or just gets optimized away.

[SpecificProtagonist]

There shouldn't be a perf difference between those given that they should get inlined, but I'll check anyways. I think it's a bit cleaner, but I don't have a strong opinion – if you prefer, I'll happily revert that part :)

[urben1680]

If it makes no difference it is fine either way for me. Originally these calls were more apart so I doubt it was inlining.

[SpecificProtagonist]

Ah, during spawning via Commands, it's only mark_spawn_despawn without a combined archetype location update.

[urben1680]

I don't see it, could you link to that?

[SpecificProtagonist]

Sorry I was unclear: #19351

[SpecificProtagonist]

I also wonder if the internal method names and SpawnedOrDespawned should be renamed to reflect that flushing also updating these values, not only spawn/despawn. "spawn_or_despawn_or_flush" is quite long though, maybe updated_at/updated_by is fine in EntityMeta.

Agree about the name, but updated_at is misleading as archetype moves also update the entity metadata. I'll leave it as is for now as dealing with flushed but not spawned entities is a unusual (no strong opinion though).

After all without MaybeUninit these two fields dont need to be in a separate type anymore.

Yes. I've still kept the type because of entity_get_spawned_or_despawned.

I think this PR and your nickname belongs to the feature's release note by the way. 😉

noted

[urben1680]

If you don't mind, in the release note in the Spawned example code I named one query q and one query. Would you make the naming consistent?

[alice-i-cecile]

@SpecificProtagonist can you take a look at the CI logs?

[github-actions]

Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke!
You can review it at https://pixel-eagle.com/project/B04F67C0-C054-4A6F-92EC-F599FEC2FD1D?filter=PR-19350

If it's expected, please add the M-Deliberate-Rendering-Change label.

If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it.