Add a missing safety invariant to `System::run_unsafe`

[joseph-gio]

Objective

The implementation of System::run_unsafe for FunctionSystem requires that the world is the same one used to initialize the system. However, the System trait has no requirements that the world actually matches, which makes this implementation unsound.

This was previously mentioned in https://github.com/bevyengine/bevy/pull/7605#issuecomment-1426491871

Fixes part of #7833.

Solution

Add the safety invariant that System::update_archetype_component_access must be called prior to System::run_unsafe. Since FunctionSystem::update_archetype_component_access properly validates the world, this ensures that run_unsafe is not called with a mismatched world.

Most exclusive systems are not required to be run on the same world that they are initialized with, so this is not a concern for them. Systems formed by combining an exclusive system with a regular system do require the world to match, however the validation is done inside of System::run when needed.

[joseph-gio]

While I wouldn't consider this a breaking change (the safety invariant already existed, it was just undocumented), it probably would be good to put this in the migration guide.

[joseph-gio]

There is an alternative solution to this issue that I think is cleaner. We could just add the requirement that System::run_unsafe must be called with the same world that the system was initialized with, which more effectively captures the invariant at play. It would make the current default impl of System::run more complicated though, and it would probably be best to just remove that default implementation entirely. That would of course be a truly breaking change though.

[joseph-gio]

I did it in this way because exclusive systems don't actually need to validate the world, and update_archetype_component_access is currently a no-op for them, so calling it is free. I didn't want to introduce extra costs for exclusive systems by requiring them to validate the world.

[joseph-gio]

Actually, that part isn't as much of an issue since exclusive systems don't even use run_unsafe.

However, adding a separate method to validate the world is problematic in that it introduces a new dynamic dispatch call every time a system gets run. Performing the world validation in update_archetype_component_access is free, because that method is getting called anyway.

[github-actions[bot]]

Example alien_cake_addict failed to run, please try running it locally and check the result.

[joseph-gio]

That's been fixed, the bot is just slow.

[joseph-gio]

here is an alternative solution to this issue that I think is cleaner. We could just add the requirement that System::run_unsafe must be called with the same world that the system was initialized with, which more effectively captures the invariant at play. It would make the current default impl of System::run more complicated though, and it would probably be best to just remove that default implementation entirely

I've tried that approach a couple of times since leaving that comment, and I find that it ends up being messier than the approach used in this PR.

[github-actions[bot]]

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy? It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.