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.