Add basic debug checks for read-only UnsafeWorldCell

[JoJoJet]

Objective

The method World::as_unsafe_world_cell_readonly is used to create an UnsafeWorldCell which is only allowed to access world data immutably. This can be tricky to use, as the data that an UnsafeWorldCell is allowed to access exists only in documentation (you could think of it as a "doc-time abstraction" rather than a "compile-time" abstraction). It's quite easy to forget where a particular instance came from and attempt to use it for mutable access, leading to instant, silent undefined behavior.

Solution

Add a debug-mode only flag to UnsafeWorldCell which tracks whether or not the instance can be used to access world data mutably. This should catch basic improper usages of as_unsafe_world_cell_readonly.

Future work

There are a few ways that you can bypass the runtime checks introduced by this PR:

• Any world accesses done via UnsafeWorldCell::storages are completely invisible to these runtime checks. Unfortunately, storages constitutes most of the world accesses used in the engine itself, so this PR will mostly benefit downstream users of bevy.
• It's possible to call get_resource_by_id, and then convert the returned Ptr to a PtrMut by calling assert_unique. In the future we'll probably want to add a debug-mode only flag to Ptr which tracks whether or not it can be upgraded to a PtrMut. I didn't include this change in this PR as those types are currently defined using macros which makes it a bit tricky to modify their definitions.
• Any data accesses done through a mutable UnsafeWorldCell are completely unchecked, meaning it's possible to unsoundly create multiple mutable references to a single component, for example. In the future we may want to store an Access<> set inside of the world's Storages to add granular debug-mode runtime checks.

That said, I'd consider this PR to be a good first step towards adding full runtime checks to UnsafeWorldCell.

Testing

Added a few tests that basic invalid mutable world access result in a panic.

[alice-i-cecile]

Good compromise. I like this direction.

[chescock]

This is a good idea!

I bet we could expand on it in the future by adding checks in SystemState::get_unchecked_manual() and the various QueryState::something_unchecked() methods.

[chescock]

inline(always) and inline(never) are pretty strong. Are you sure we can't just leave them out and trust the optimizer? For the debug version, I'd expect it to inline the self.allows_mutable_access check to avoid the call overhead but leave the panic branch in a cold function.

[JoJoJet]

I don't have a very strong opinion on what we do here, but I feel like in cases like this where a given inlining strategy is "obviously right" there's not much reason not to include the inline attributes. In debug mode there's pretty much never a reason to inline a panicking function, while in release mode this function is empty, so inline(always) is just saving the compiler from having to use a heuristic here to eliminate the dead code.

[chescock]

Yup, it's not a big deal either way! I have just been wrong often enough when I thought something was "obviously right" that I prefer to trust the compiler instead of myself :).

The reason I would want to inline the function in debug mode is to avoid the function call overhead for the if check.
That is, I want it to wind up with

pub unsafe fn world_mut(self) -> &'w mut World {
    if !self.allows_mutable_access {
        call_some_cold_function_to_panic();
    }

and I think that requires inlining assert_allows_mutable_access.

... And I checked in godbolt and it does not do that. It inlines the whole thing, including the panic. So I have no idea which way is better!

[JoJoJet]

In my view, the release-mode behavior does seem obviously correct here, while the performance in debug mode isn't really worth worrying about unless it majorly impacts dev experience, but that seems unlikely for something this small.

[chescock]

You don't technically need this one (or the one in get_non_send_resource_mut), since it calls get_resource_mut_by_id which already does the check.

Or, if you want to do it consistently this way, you're missing a few like UnsafeEntityCell::get_mut.

[JoJoJet]

Ah good point, this call is unnecessary

[JoJoJet]

I bet we could expand on it in the future by adding checks in SystemState::get_unchecked_manual() and the various QueryState::something_unchecked() methods.

In the future I'm hoping to add checks into Storages, which would provide runtime checks to Query and friends for 'free'

[alice-i-cecile]

@JoJoJet it looks like you missed some feature-flagged code on web. Ping me when that's fixed and I'll try merging this in again.