Shorten the 'world lifetime returned from QueryLens::query().

[chescock]

Objective

Fix unsoundness introduced by #15858. QueryLens::query() would hand out a Query with the full 'w lifetime, and the new _inner methods would let the results outlive the Query. This could be used to create aliasing mutable references, like

fn bad<'w>(mut lens: QueryLens<'w, EntityMut>, entity: Entity) {
    let one: EntityMut<'w> = lens.query().get_inner(entity).unwrap();
    let two: EntityMut<'w> = lens.query().get_inner(entity).unwrap();
    assert!(one.entity() == two.entity());
}

Fixes #17693

Solution

Restrict the 'world lifetime in the Query returned by QueryLens::query() to '_, the lifetime of the borrow of the QueryLens.

The model here is that Query<'w, 's, D, F> and QueryLens<'w, D, F> have permission to access their components for the lifetime 'w. So going from &'a mut QueryLens<'w> to Query<'w, 'a> would borrow the permission only for the 'a lifetime, but incorrectly give it out for the full 'w lifetime.

To handle any cases where users were calling get_inner() or iter_inner() on the Query and expecting the full 'w lifetime, we introduce a new QueryLens::query_inner() method. This is only valid for ReadOnlyQueryData, so it may safely hand out a copy of the permission for the full 'w lifetime. Since get_inner() and iter_inner() were only valid on ReadOnlyQueryData prior to #15858, that should cover any uses that relied on the longer lifetime.

Migration Guide

Users of QueryLens::query() who were calling get_inner() or iter_inner() will need to replace the call with QueryLens::query_inner().

[alice-i-cecile]

@Victoronz @chescock @13ros27 take a look at this please :)

[13ros27]

I'll be honest the way Query uses lifetimes goes slightly over my head but this seems reasonable from what was discussed in #17693. Is it worth a compile fail test to ensure the lifetimes aren't changed at some point?

[chescock]

Is it worth a compile fail test to ensure the lifetimes aren't changed at some point?

Yes, definitely!

I have never written a compile-fail test, though. Can anyone point me in the right direction to get started?

[alice-i-cecile]

https://github.com/bevyengine/bevy/tree/main/crates/bevy_ecs/compile_fail :)

[13ros27]

As I mentioned I can't fully review the lifetime changes but the compile-fail test seems good and compiles successfully on main (where it shouldn't of course).

[13ros27]

Did you mean to leave these in?

[chescock]

I copied the skeleton from https://github.com/bevyengine/bevy/blob/f2a65c2dd3a3ef75964a8aaed67e1f24618bf19f/crates/bevy_ecs/compile_fail/tests/ui/query_lifetime_safety.rs . So, I did mean to leave them there, but not for any particularly good reason :). I'm inclined to leave it like this for consistency with the other file, but I'm happy to change it if there are strong objections.

[13ros27]

Huh, I must have got my search filtering wrong because I thought none of the other compile-fail tests had dbg in them, feel free to leave it then.

[cBournhonesque]

So the new flow is

1. query -> Query<'w, s'>
2. lens = &'a query.as_query_lens() -> QueryLens<'a>
3. lens.query() -> Query<'a, 'a>
4. get_inner() -> QueryItem<'a>

and the old one is

3. lens.query() -> Query<'w, 'a>
4. get_inner() -> QueryItem<'w>

So in the old case, the item we get only depends on the 'w lifetime, so the compiler doesn't stop us from creating a second object with the 'a lifetime via lens.query().

I think the change makes sense; out of curoisity, would this PR #15396 also work to fix your compile-fail test?
Since the QueryItem we would get would have been QueryItem<'w, 'a>, we might not be able to call lens.query() a second time because the 'a lifetime is present in the return value

[chescock]

I did't completely follow your notation, but your conclusion sounds right! We had an &'a mut QueryLens<'w>, and currently that gives a Query<'w, 'a> which we can use with get_inner() to get a QueryItem<'w>, but with this change it gives a Query<'a, 'a> that we can only use to get a QueryItem<'a>.

I believe the compile-fail test would still compile with #15396, because the compiler would see that the concrete type of QueryItem<'w, 'a> is Mut<'w, Foo>, which doesn't involve the 'a lifetime.