Fix access checks for DeferredWorld as SystemParam.

[chescock]

Objective

Prevent unsound uses of DeferredWorld as a SystemParam. It is currently unsound because it does not check for existing access, and because it incorrectly registers filtered access.

Solution

Have DeferredWorld panic if a previous parameter has conflicting access.

Have DeferredWorld update archetype_component_access so that the multi-threaded executor sees the access.

Fix FilteredAccessSet::read_all() and write_all() to correctly add a FilteredAccess with no filter so that Query is able to detect the conflicts.

Remove redundant read_all() call, since write_all() already declares read access.

Remove unnecessary set_has_deferred() call, since <DeferredWorld as SystemParam>::apply_deferred() does nothing. Previously we were inserting unnecessary apply_deferred systems in the schedule.

Testing

Added unit tests for systems where DeferredWorld conflicts with a Query in the same system.

[alice-i-cecile]

Remove unnecessary set_has_deferred() call, since ::apply_deferred() does nothing

Are you sure this is the right fix? My understanding was that DeferredWorld was command-like, and apply_deferred should be applying it.

[hymm]

Are you sure this is the right fix? My understanding was that DeferredWorld was command-like, and apply_deferred should be applying it.

This pr won't work as DeferredWorld pushed into the the World's command queue and so requires exclusive access to the world. https://docs.rs/bevy/latest/bevy/ecs/world/struct.DeferredWorld.html#method.commands. This would only work if Deferred World somehow pushed to the system's command queue.

[chescock]

This pr won't work as DeferredWorld pushed into the the World's command queue and so requires exclusive access to the world. https://docs.rs/bevy/latest/bevy/ecs/world/struct.DeferredWorld.html#method.commands. This would only work if Deferred World somehow pushed to the system's command queue.

Sorry, I'm a little behind on understanding command queues. Are you worried that multiple systems will access the command queue concurrently and have UB from aliased &mut? Or are you worried that the command queue will never get flushed?

I hadn't thought about flushing the queue at all! Would failing to flush be UB, or just horribly confusing? Would it work correctly if we restore set_has_deferred() and do

fn apply(state: &mut Self::State, system_meta: &SystemMeta, world: &mut World) {
    world.flush_commands();
}

?

[hymm]

Sorry, I'm a little behind on understanding command queues. Are you worried that multiple systems will access the command queue concurrently and have UB from aliased &mut? Or are you worried that the command queue will never get flushed?

I'm a little worried that there might be UB there, but I suspect it should be fine, since you only have one DeferredWorld at a time.
Having to use write_all though sort of defeats the purpose of using DeferredWorld though. It would be better in most instances to just use &World and Command, since those systems can be run in parallel.

Would it work correctly if we restore set_has_deferred() and do

This would be better, but not quite right still. Consider if we have a (deferred_world_system_1, system_with_commands, deferred_world_system_2).chain_ignore_deferred(). You'd expect the commands to be applied in the same order, but the second deferred world system's commands will get applied at the same time as the first's, since they push to the same command queue.

[chescock]

It sounds like you're arguing that the existing impl SystemParam for DeferredWorld wasn't even useful? I read the original message as saying that it was useful but currently unsound, and was trying to fix the unsoundness so that we didn't need to get rid of something useful. I don't think I'm the right person to make it useful, though!

If we wind up just removing it, then I should split off the FilteredAccessSet::read_all() and write_all() changes into a new PR, since I think we'll want to fix those regardless.

[hymm]

Realized I didn't really understand what DeferredWorld is used for in systems, so did some research and found out it's because observers aren't allowed to use &mut World. So this is the closest equivalent for those use cases and so we need to keep the impl around for that.

Fix FilteredAccessSet::read_all() and write_all() to correctly add a FilteredAccess with no filter so that Query is able to detect the conflicts.

Am I understanding correctly that this makes the following system error where it wouldn't before this pr?

fn my_system(_: Query<EntityRefExcept<T>>, _: &World, _:Query<&mut T>) {}

Could we add a test for that if that's the case?

[chescock]

Am I understanding correctly that this makes the following system error where it wouldn't before this pr?

fn my_system(_: Query<EntityRefExcept<T>>, _: &World, _:Query<&mut T>) {}

Could we add a test for that if that's the case?

Oh, I hadn't checked &World... okay, &World is working correctly, because it's creating a FilteredAccess itself and add()ing it. It looks like DeferredWorld was the only place actually using the FilteredAccessSet::(read|write)_all convenience methods outside of tests. We should still fix them, but it doesn't look like there is any other in-engine code affected by the bug.

There is a test for fn system(_query: &World, _q2: Query<EntityMut>) {}. I'll add one for the other order while I'm here, to be safe.

[chescock]

This pr won't work as DeferredWorld pushed into the the World's command queue and so requires exclusive access to the world. https://docs.rs/bevy/latest/bevy/ecs/world/struct.DeferredWorld.html#method.commands. This would only work if Deferred World somehow pushed to the system's command queue.

Also, to clarify: Do you want me to make any changes to queue flushing in this PR? I removed the set_has_deferred() call because it doesn't do anything useful when the apply() method is empty, so I believe I'm preserving the current behavior.

[hymm]

I think it's fine to keep the current behavior for now. It's not really meant to be used with scheduled systems. (you should just use exclusive systems.) So probably not the biggest deal that the behavior if you use it the is a little weird. It's probably more trouble than it's worth to fix it properly.