Yes please, I'd like a small Migration Guide note here.

Ah, wait. Since EntityWorldMut can be converted into the other kinds of entity references, we may need to add the invalid location check to them as well (or make the conversion fallible).

Some of the UnsafeEntityCell methods those references use probably assume its location is valid.

warn or warn_once?

Hello! 👋

I lack context about the changes in this PR, but by petition of @alice-i-cecile , I checked out this branch to see if I can still reproduce the crash of flush() needs to be called before this operation is legal that I was hitting in my code

The answer is that no, I cannot.

The thing is, I can't repro it in bevy main neither, so this bug must have been fixed at some point between Bevy 0.14.2 (the version where I encountered it) and current Bevy main atow

Here, for reference:

this is my minimal reproduceable example, on top of Bevy 0.14.2:

https://github.com/Maximetinu/bevy/commits/minimal-reproduceable-bug-on-remove-hook/

see changes in latest commit

running that modified example does reproduce the bug

notice that the cause is just spawning an empty entity during a on_remove hook

This is my minimal reproduceable example cherry-picked to this branch:

https://github.com/Maximetinu/bevy/commits/world-entity-mut-flushes-checking-if-panic-keeps-happening/

see changes in latest commit, they are the same as in the branch above

running that modified example does not reproduce the bug

so, this branch works 🥳

However, bevy main also works:

https://github.com/Maximetinu/bevy/commits/reproducing-on-remove-hook-panic-on-bevy-main/

which means that, well, this branch does not fix per se this bug, because it is already fixed on Bevy main

but at least it means that it's not re-introducing it neither 🙂

Maybe my minimal reproduceable example can help to write a test covering from this specific scenario

I hope this helps!

I'd love to see that turned into a test, either in this PR or separately :)

I'd love to see that turned into a test, either in this PR or separately :)

I have made a test for this, that fails at 0.14.2 but passes in current bevy main, so it should cover from this regression

#[test]
fn test_spawn_entities_during_on_remove_hook() {
    #[derive(Component)]
    struct MyComponent;

    App::new()
        .add_plugins(MinimalPlugins)
        .add_systems(Startup, |world: &mut World| {
            world.register_component_hooks::<MyComponent>().on_remove(
                |mut world, _entity, _component_id| {
                    world.commands().spawn_empty();
                },
            );
        })
        .add_systems(
            Update,
            |mut commands: Commands,
                my_component_q: Query<Entity, With<MyComponent>>,
                mut exit_event: EventWriter<AppExit>,
                mut after_first_frame: Local<bool>| {
                for entity in &my_component_q {
                    commands.entity(entity).despawn();
                }

                commands.spawn(MyComponent);

                if *after_first_frame {
                    exit_event.send(AppExit::Success);
                }

                *after_first_frame = true;
            },
        )
        .run();
}

It's a bit of an unusual test (I'm not even sure if MinimalPlugins can be used in a test like this). It's more an integration test rather than a unit test. I don't know if there is some place inside Bevy repo where more of these tests exist. If so, I can open a PR to add it.

Let's spin this out into a new issue :)

@maniwani Updated design (just quick notes for now):

• Handle also UnsafeEntityCell
• Instead of setting location entirely to invalid, instead set both archetype_id and table_id to "empty", but keep archetype_row and table_row as invalid. That allows us to behave more like an empty entity for most APIs.

I changed this pull to instead panic rather than warn and try to behave if nothing is wrong. The reason for this is that if we only try to warn, there's a quite wide ranging knock-off effect into UnsafeEntityCell and a lot of other places which store EntityLocation. If we instead panic, the effects (and the extra runtime check) are limited to just EntityWorldMut. Also, Bevy already opts to panicing if using commands to modify an entity that has been despawned (https://bevyengine.org/learn/errors/b0003/) so this is similar behaviour. There should be nobody depending on this behaviour currently as calling any of the methods after the entity has been despawned by hooks would have already been unsound in the past – so if it happens, then the panics will reveal places where there has been already been a bug.

@alice-i-cecile @maniwani I'd love to get feedback if the new panicing approach is good or bad. Also, is it reasonable to add a panics section to all the methods which might panic for this. It should truly be a corner case when this happens, so I'm not sure if it's sensible to spam it everywhere (and it cannot be put into some of the non-fallible From conversions).

Implementation of panic is still placeholderish, just to note.

Also, Bevy already opts to panicing if using commands to modify an entity that has been despawned

For what it's worth, this behavior is widely hated :)

I think I'm broadly on board with panicking here though, especially as a first pass. This really is an edge case, and I think that trying to figure out better ways to prevent this bad state from ever arising is more useful than bubbling the error up.

If #16499 gets merged before this one, one of the test cases will break, as it should when the ordering of hooks and observers is changed.

@alice-i-cecile -S-Waiting-on-Author +S-Needs-Review 🙇

An alternative to checking in every method if the entity has been despawned would be to panic after calling world.flush instead of checking in each method. This would disallow despawning the entity in hooks, but I think that makes sense that you wouldn't be able to despawn the EntityWorldMut while holding it.

I don't entirely understand what world.flush() does, but I do believe this PR calls assert_not_despawned(); before every read or write of the entity or its components and calls world.flush() and update_location() after every write, so self.location should always be correct.

The one exception is that the world_scope() method calls update_location() but not world.flush(). Should that call world.flush() as well?

World flush runs any Commands that have been queued. Commands are queued by hooks and observers if they want to do archetype changes, as they only get a DeferredWorld.

The world_scope function just allows a function to run with the world, but it doesn't necessarily queue any commands. If it does, it's the responsibility of the function to flush if necessary.

There has been talk of having commands apply immediately when holding a &mut World, like in this world_scope function, which would solve the whole issue of asking the user to flush.

An alternative to checking in every method if the entity has been despawned would be to panic after calling world.flush instead of checking in each method. This would disallow despawning the entity in hooks, but I think that makes sense that you wouldn't be able to despawn the EntityWorldMut while holding it.

It would be an alternative, but instead the aim is to go to another direction, to make the methods fallible instead.

Also, there are two other reasons.

First, there aren't any other ways to make archetype changes to entities but to take WorldEntityMut via world.entity_mut(ent). So if despawning the current entity would be totally disallowed, then there would not be a way to safely trigger such a change as you can't not hold a reference while doing it.

Second, you can't really prevent hooks and observers from making these types of changes in some special cases. So there isn't a workaround for that. Where as just dropping a WorldEntityMut after an archetype mutation operation is a trivial thing to do by the user.

FWIW i think panicking is fine here; there's similar stuff in the engine that already works this way. We can reconsider it in a broader review of bevy's error handling. But especially for guarding invariant going into unsafe code, panicking is probably the way to go.