Remove unsound cast in thread local resources

[memoryruins]

Noticed in #671 that it acquires an unique reference from a shared reference, which is always unsound regardless of runtime tracking. miri confirms this concern:

error: Undefined Behavior: not granting access to tag <untagged> because incompatible item is protected: [SharedReadOnly for <682186> (call 245045)]
   --> crates\bevy_ecs\src\resource\resources.rs:56:37
    |
56  |                 ResourceRefMut::new(&mut *value, &stored.atomic_borrow)
    |                                     ^^^^^^^^^^^ not granting access to tag <untagged> because incompatible item is protected: [SharedReadOnly for <682186> (call 245045)]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information

    = note: inside closure at crates\bevy_ecs\src\resource\resources.rs:56:37
    = note: inside `std::option::Option::<&resource::resources::StoredResource<i64>>::map::<resource::resources::ResourceRefMut<i64>, [closure@crates\bevy_ecs\src\resource\resources.rs:52:36: 57:14]>` at C:\Users\Michael\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\option.rs:453:29
note: inside `resource::resources::VecResourceStorage::<i64>::get_mut` at crates\bevy_ecs\src\resource\resources.rs:52:9
   --> crates\bevy_ecs\src\resource\resources.rs:52:9
    |
52  | /         self.stored.get(index).map(|stored|
53  | |             // SAFE: ResourceRefMut ensures that this borrow is unique
54  | |             unsafe {
55  | |                 let value = &stored.value as *const T as *mut T;
56  | |                 ResourceRefMut::new(&mut *value, &stored.atomic_borrow)
57  | |             })
    | |______________^

This PR changes get_mut(&self) to get_mut(&mut self) and removes the problematic code.

gilrs_event_system needed to be tweaked slightly (making a temporary buffer for events). If desired, we can add a new resource that only this system can access by type to re-use a buffer. I have tested gamepad_input and gamepad_input_events examples with a xbox 360 controller and the behavior was the same before and after.

thread_local_resource_mut_ref_aliasing test is commented out for now since the issue it aimed to catch is now caught at compile time. Does this make ResourceRefMut no longer needed? To keep the diff small at first, I have left it there for now.

[DJMcNab]

I think the problem there is might be ResourceRefMut::new, in that it should take a raw pointer rather than a reference.

[memoryruins]

I think the problem there is might be ResourceRefMut::new, in that it should take a raw pointer rather than a reference.

There would still be the issue of acquiring a mut pointer from a shared reference to a type that doesn't have interior mutability. While I prefer having the issues caught at compile-time with get_mut(&mut self), I might be missing some key context for why that wasn't already the case here.

[DJMcNab]

It's possible that we need to make above change and also need an UnsafeCell as a higher level.
I think the idea of this API is that you can access multiple of the resources in parallel, with run-time borrow checking to make sure it's safe.
I think the old API is sound, but the implementation is not safe.

[cart]

Yeah I think disallowing multiple mutable resource borrows of different types significantly decreases the utility of the api / makes writing "thread local systems" very hard. If the implementation isn't sound, lets fix that.

Do we have a real example of this api producing unsafe behavior?

It seems like our implementation is sound, but miri doesn't like the type of unsafe code we're writing? Is there a better way to write it?

[BoxyUwU]

It seems like our implementation is sound, but miri doesn't like the type of unsafe code we're writing?

Even if you never produce aliasing &mut and never produce aliasing & and &mut this code is still unsound. The nomicon explicitly calls out transmuting/pointercasting/etc & to &mut as always UB
A sound way of doing this is to store the data in an UnsafeCell like in this commit

[cart]

Gotcha gotcha. This reddit comment helped my understand why this is problematic. I wish the nomicon link was a bit more like the reddit comment. It currently reads as hyperbole to me.

Sounds like UnsafeCell is the way to go here if we want to continue allowing multiple mutable borrows of different resources (which i personally do).

[DJMcNab]

@EllenNyan that's basically exactly what I meant, but I think that's still UB in the small period where you call get_mut(0), then get_mut(0). Specifically at BoxyUwU@e680c45#diff-80c9e47db9e48efb9e1d9c90fa47aa6a059b0ed4cee0019a0a8030bfc589e156R54, there is a second &mut to the same data which the outer scope already has access to, so ResourceRefMut has to use raw pointers and do the conversion to an &mut after we know that it is unique.

Disclaimer: I don't really know what I'm talking about, just have taken in stuff that (I think) RalfJung has written.

[memoryruins]

I appreciate everyone's input and the motivations for the API. I'm currently unavailable to look into and test a proper fix that would also keep the current interface's ergonomics. If anyone would like to take up the torch, please do ^^

[BoxyUwU]

I can throw together a proper solution and PR it ^^