Emphasize no structural changes in SystemParam::get_param

[GlennFolker]

Objective

Many systems like Schedule rely on the fact that every structural ECS changes are deferred until an exclusive system flushes the World itself. This gives us the benefits of being able to run systems in parallel without worrying about dangling references caused by memory (re)allocations, which will in turn lead to Undefined Behavior. However, this isn't explicitly documented in SystemParam; currently it only vaguely hints that in init_state, based on the fact that structural ECS changes require mutable access to the whole World.

Solution

Document this behavior explicitly in SystemParam's type-level documentations.

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[GlennFolker]

Should I document this behavior on System::run_unsafe and SystemState::get_unchecked_manual too? I just realized end-users of those might not stumble upon this documentation on SystemParam...

[GlennFolker]

I've added the note to WorldQuery and SystemState::get_unchecked_manual... I'm now a little confused about what to do with System::run_unsafe. Requiring no structural ECS changes to run_unsafe would be a trait-level safety invariant, thus marking System an unsafe trait, but that'll probably be a breaking change and I'm not sure if that is in the scope of this PR.

[chescock]

The agree that the safety docs here need some clarification, and I do like the idea of giving advice that structural changes like register_component should be made in init_param!

I'm not sure this belongs as a safety requirement on trait SystemParam, though. The only way to perform structural changes from an UnsafeWorldCell is by calling unsafe fn world_mut(), and the safety requirements for that make it impossible to call safely from get_param.

Maybe we could instead improve the safety docs for UnsafeWorldCell?

It might also be less of an issue now that Rust 2024 made the unsafe_op_in_unsafe_fn lint warn by default. Calling unsafe methods on UnsafeWorldCell from within unsafe fn get_param() will now require unsafe blocks, which should help make the safety requirements more obvious.

Also note that if #16622 happens, then &mut World will be a SystemParam and it will be possible to do structural changes! So I don't think we want SystemParam::get_param or SystemState::get_unchecked_manual to promise that there won't be any.

[GlennFolker]

Ah, right! I suppose clarifying this on UnsafeWorldCell::world_mut() is a better idea.

And about #16622, I think the promise would then be implied by whether SystemParam::world_access_level is exclusive or not, which could probably be mentioned in SystemParam::get_param for convenience.

[GlennFolker]

Sorry for the overly long delay, I kinda forgot I made this PR at all 😅 Anyway, only UnsafeWorldCell::world_mut docs is changed now, which explicitly mentions QueryParam and SystemState. That should probably be enough to show the stop sign to users trying to invoke structural ECS changes there!