Remove Query::to_readonly

[BoxyUwU]

Query::to_readonly is unsound:

use bevy::prelude::*;

#[derive(Component)]
struct Foo(u8);

fn main() {
    App::new()
        .add_startup_system(|mut cmds: Commands| {
            let id = cmds.spawn().insert(Foo(10)).id();
            cmds.insert_resource(id);
        })
        .add_system(my_sys)
        .run();
}

fn my_sys(q: Query<&mut Foo>, res: Res<Entity>) {
    let mut q1 = q.to_readonly();
    let mut q2 = q.to_readonly();
    let mut_t1 = &mut *q1.get_component_mut::<Foo>(*res).unwrap();
    let mut_t2 = &mut *q2.get_component_mut::<Foo>(*res).unwrap();
    assert_eq!(mut_t1 as *mut Foo as usize, mut_t2 as *mut Foo as usize); //aliasing go brr
}

output of cargo miri run

error: Undefined Behavior: trying to reborrow from <681381> for SharedReadWrite permission at alloc225967[0x0], but that tag does not exist in the borrow stack for this location
  --> src/main.rs:22:16
   |
22 |     assert_eq!(mut_t1 as *mut Foo as usize, mut_t2 as *mut Foo as usize); //aliasing go brr
   |                ^^^^^^
   |                |
   |                trying to reborrow from <681381> for SharedReadWrite permission at alloc225967[0x0], but that tag does not exist in the borrow stack for this location
   |                this error occurs as part of a reborrow at alloc225967[0x0..0x1]
   |

---

The "logic" for why this happens is because get_component (and get_component_mut) rely on QueryState's access exactly reflecting what the query is allowed to access. But Query::to_readonly() invalidates that by having a Query<&T> exist with write access for T.

Alternative solutions:

• Make to_readonly() take &mut self, I don't like this because implementing Copy for Query<Q: ReadOnlyWorldQuery> would then be unsound which is something we should do in the future
• remove get_component(_mut) since they seem like they have limited utility and are now causing problems. This lifts the requirement by removing the only methods that actually rely on it. I'm opposed to this since it seems reasonable to me that the access of querystate does accurately reflect what you're allowed to access. Alternatively find some way to rewrite get_component(_mut) to not use the QueryState access and instead have a method on WorldQuery to dynamically check if a component is part of our access (and at the same time remove the ability to access the access from querystate)
• Create a second QueryState for the readonly variant and change to it in Query::to_readonly, I imagine this has some overlap with query joining since I expect that will want to be able to create "new" QueryState's for that. I think I prefer this solution but it seems like a lot of work so doing this PR first to remove unsoundness seems best :)

[mockersf]

added in #5376, pinging @harudagondi in case you have an opinion on keeping this api

[cart]

Make to_readonly() take &mut self, I don't like this because implementing Copy for Query<Q: ReadOnlyWorldQuery> would then be unsound which seems like something we may want to do in the future

Can we do into_readonly(self) instead? That seems sound. Less flexible, but it at least leaves the doors open to building abstractions. I think as_readonly is a really helpful feature (I used it in my last jam game) because it lets you write reuseable immutable logic (ex: fn print_players(players: Query<&Player>) {}), then use it in systems that only have Query<&mut Player>. Without it, we need to go back to defining a mutable and immutable version of the query and putting them in a ParamSet. Bleh :)

[BoxyUwU]

same issue as &mut self just less flexible- it'd still be wrong to implement copy for Query<Q: ReadOnlyWorldQuery>. There's definitely ways to implement to_readonly soundly so itd be possible (though whether time is found to impl + review + merge etc...) to have it readded for 0.9 even if we remove it here (Although its probably best to assume that doesnt happen if we remove it since I really don't know how long itll be til something gets implemented and I suspect it wouldn't be simple, unless i'm missing something which I really hope I am!).

I definitely agree with this being a super useful feature :( A lot of work went into making it possible to implement and its disappointing that it can't be done "properly" yet..

[alice-i-cecile]

Create a second QueryState for the readonly variant and change to it in Query::to_readonly, I imagine this has some overlap with query joining since I expect that will want to be able to create "new" QueryState's for that. I think I prefer this solution but it seems like a lot of work so doing this PR first to remove unsoundness seems best :)

Salvaging this feature in some way is important to me; I definitely agree that we should prioritize a simple solution to start though rather than wait on a full fix.

@BoxyUwU what if we made the API unsafe for now?

[BoxyUwU]

making the API unsafe basically renders it useless imo, the safety comment would be "dont call get_component(_mut) on the returned query" which to satisfy would require not passing the query to random safe code which is basically the whole point of this API. (and regardless I would much rather us point people to ParamSet with a bit of boilerplate over such an invariant 😬)

[BoxyUwU]

An alt fix could be to in get_component(_mut) stop checking access of QueryState and instead call <(Q, F)>::update_archetype_component_access on an empty access and then check if it populated it with a read/write for the T we're trying to get. (This assumes that there's no other code in Query the assumes QueryState's access exactly reflacts that of (Q, F)).

Almost definitely slower than just checking QueryState's access but it would resolve the unsoundness, and in a future where we get the ability to do a "real" fix of creating more QueryState and changing which one we're referencing we can remove the temp fix and change the query state in to_readonly. (This would probably be a lot more efficient if it didn't require creating bitsets that were empty until the ArchetypeComponentId we care about lol)

[alice-i-cecile]

cart: Yeah we should definitely fix this before 0.9. If nothing better shows up then we'll merge the removal PR

[cart]

Riffing on @BoxyUwU's variant idea: can we just have a Query:force_readonly_component_access flag (defaults to false, manually set to true in to_readonly) that we check on get_component_mut? Seems reasonably cheap and would only be a few lines of code.

[BoxyUwU]

Yeah that'd probably work too, its definitely not a "long term" fix but it would make this sound.

[cart]

Cool I think that has my preference as a short term fix. I'll put that together.

A compile-time hungry option that might open some other design doors:

Impl: ReadComponentQuery<T> and WriteComponentQuery<T> for every relevant WorldQuery (including tuples), then just constrain get_component_mut<C> where Q: WriteComponentQuery<C>. The type change in to_readonly would automatically change the constraints to stop working.

Probably gratuitously expensive?

[cart]

Closing in favor of #6401