Add a missing safety invariant to System::run_unsafe

[JoJoJet]

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 #7605 (comment)

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.

[james7132]

Great catch! Though does changing the safety invariants of an unsafe function count as a breaking change? This should probably be documented in the migration guide if possible.

[JoJoJet]

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.

[JoJoJet]

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.

[Carter0]

I suppose this is better than nothing. But why not write another function that validates the world is the correct one and give it its own name rather than using update_archetype_component?

The name update_archetype_component doesn't imply that it checks the world at all. And without the comment explaining why you are calling it, this would be a confusing line of code.

[JoJoJet]

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.

[JoJoJet]

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]

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

[JoJoJet]

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

[JoJoJet]

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.

[hymm]

New safety invariants make sense to me. Unfortunate that we need to make some more of the functions unsafe, but this pr is just exposing an existing problem.

Just had the one question about a TODO and then this looks good to me.

[hymm]

is this meant to stay as a TODO?

[JoJoJet]

Yes. The previous "access is compatible" comment is incorrect, since it's not supported by any invariants in this function. I'd rather be explicit that these invariants aren't fully fleshed out yet than pretend that they are. I have a real fix for this in #8292.

[github-actions]

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.