(I suspect we'll eventually want to make this a safety condition and only debug_assert! it, but since this PR already improves performance I don't think we need to micro-optimize too much :).)

It wasn't super rigorous, but I did run benchmarks before and after adding the assert!s and didn't see any difference.

I'm having a little trouble following the control flow here. I think this is supposed to affect each source column exactly once, and either copy it or drop/forget it. Is that right? Could we rework it to use a for loop over the source columns so that it's more clear that it touches each one once?

I think that would look something like this:

let mut dst_iter = dst_table.columns.iter_mut();
let mut dst_next = dst_iter.next();
for (src_component_id, src_column) in src_table.columns.iter_mut() {
    // Skip past any destination columns that don't exist in the source table.
    // The caller is responsible for initializing those columns.
    while dst_next
        .as_ref()
        .is_some_and(|(dst_component_id, _dst_column)| *dst_component_id < src_component_id)
    {
        dst_next = dst_iter.next();
    }
    // Then move the value in the source column if it exists in the destination table,
    // or remove it if it does not.
    if let Some((dst_component_id, dst_column)) = dst_next.as_mut()
        && *dst_component_id == src_component_id
    {
        unsafe {
            dst_column.initialize_from_unchecked(src_column, last_index, row, dst_row);
        }
        dst_next = dst_iter.next();
    } else {
        unsafe {
            src_column.swap_remove_unchecked::<DROP>(last_index, row);
        }
    }
}

If that's equivalent, then I think it will be easier to follow.

Definitely an improvement, and even a bit faster! (being compared to the same main as before):

Actually, it might be unsound for this to panic...

It looks like this is called after moving the entity between archetypes and stuff. If it unwinds, it leaves the entity in the wrong archetype, which could potentially cause UB. So this might need to be a safety requirement.

Shouldn't panicking always be safer than continuing with violated safety requirements?

They are both the same here IIUC. If the safety requirement is not fulfilled, it's UB. If it panics, it unwinds with the world in an invalid state. The entity thinks it's in the new archetype, even though it isn't initialize fully yet. If something catches the unwind, like bevy's error handler, then the world will continue as is, so a query could return references to uninitialized memory which is UB. It would also probably try to drop uninitialized memory which is UB. So either way, if the caller is wrong, it's UB, so it should be unsafe to reflect that.

(All of that is from memory from where bevy catches unwinds. But I think it's right.) You could also make the argument that documenting the panic makes it implicitly unsafe when called in these contexts, but I don't think that's a patter that bevy addopts.

If it panics, it unwinds with the world in an invalid state.

Yeah, see #20368 for another example of it being unsound to unwind during component insertion.

Hmm, we also run arbitrary user code in the drop impls within this. Does that mean it's already unsound to remove a component with a panicking drop? ... Well, it's definitely buggy, since it will swap components between entities while doing swap_remove, but I haven't actually been able to trigger UB since it doesn't update the EntityLocation until afterwards. I might try to put together a clean example and make an issue.

This is one of those things that's really hard to get right. Especially with drop logic, allocator panics, hook panics (which could leave some hooks not run), etc. All of these leave the world in an invalid state, which could be totally fine or cause UB depending on how the world is used next.

From my research in building my own ecs, I find the simplest thing to do is to declare a variable that panics when it's dropped and forget it afterward.

let panic_bomb = PanicOnDrop("Unacceptable panic in structural world change");

// Structural change here (ex: spawn, insert, etc.)

core::mem::forget(panic_bomb);

If something does panic inside, it will cause a non-unwinding panic when it hits the panic_bomb, aborting the process. IMO, this is the best balance of minimal safety requirements that doesn't require catching unwinds in component drops, hooks, and such.

There are other things you can do, too. For example, you can make sure the hook and drop function pointers include a catch_unwind or something themselves, which rust can make 0 cost if it can verify that a panic is impossible. But this gets messy IMO.

This is one of those things that's really hard to get right.

Agreed!

I find the simplest thing to do is to declare a variable that panics when it's dropped and forget it afterward.

Yeah, this is a good pattern to ensure safety!

But we also want to reduce the number of ways that users can cause Bevy to abort the process, of course :).

I think it should be possible to at least make sure we aren't running arbitrary user code while the panic_bomb is armed. If we move the drop calls to happen last, after all the EntityLocation bookkeeping is finished, then any panics there will only cause other values to leak. Conversely, if we initialize required components after reserving capacity in their tables, but before doing anything else, then any panics there will only cause other required component values to leak.

(I also wonder whether we could move the drop calls out far enough that they can happen where we have static types. That could eliminate the indirect calls and let the optimizer inline them.)

Very interesting. I definitely think this is worth splitting out into its own issue to discuss. There are lots of paths forward here. I think the simplest thing for this PR is to just make it unsafe and call it good for now.

But we should try to solidify a "Bevy way" for these "unwinding in unsafe code" questions and patch all the existing soundness holes, ideally before any users start running into aborts.

If it panics, it unwinds with the world in an invalid state.

Yeah, see #20368 for another example of it being unsound to unwind during component insertion.

Hmm, we also run arbitrary user code in the drop impls within this. Does that mean it's already unsound to remove a component with a panicking drop? ... Well, it's definitely buggy, since it will swap components between entities while doing swap_remove, but I haven't actually been able to trigger UB since it doesn't update the EntityLocation until afterwards. I might try to put together a clean example and make an issue.