Introduce methods on QueryState to obtain a Query

[chescock]

Objective

Simplify and expand the API for QueryState.

QueryState has a lot of methods that mirror those on Query. These are then multiplied by variants that take &World, &mut World, and UnsafeWorldCell. In addition, many of them have _manual variants that take &QueryState and avoid calling update_archetypes(). Not all of the combinations exist, however, so some operations are not possible.

Solution

Introduce methods to get a Query from a QueryState. That will reduce duplication between the types, and ensure that the full Query API is always available for QueryState.

Introduce methods on Query that consume the query to return types with the full 'w lifetime. This avoids issues with borrowing where things like query_state.query(&world).get(entity) don't work because they borrow from the temporary Query.

Finally, implement Copy for read-only Querys. get_inner and iter_inner currently take &self, so changing them to consume self would be a breaking change. By making Query: Copy, they can consume a copy of self and continue to work.

The consuming methods also let us simplify the implementation of methods on Query, by doing fn foo(&self) { self.as_readonly().foo_inner() } and fn foo_mut(&mut self) { self.reborrow().foo_inner() }. That structure makes it more difficult to accidentally extend lifetimes, since the safe as_readonly() and reborrow() methods shrink them appropriately. The optimizer is able to see that they are both identity functions and inline them, so there should be no performance cost.

Note that this change would conflict with #15848. If QueryState is stored as a Cow, then the consuming methods cannot be implemented, and Copy cannot be implemented.

Future Work

The next step is to mark the methods on QueryState as #[deprecated], and move the implementations into Query.

Migration Guide

Query::to_readonly has been renamed to Query::as_readonly.

[Trashtalk217]

I agree that there's a lot of duplication between QueryState's and Query's API and it's good that this is lessened, but just out of curiosity (just to offer a potentially dumb idea), would it perhaps be better if QueryState and Query were just one single struct instead of two?

[chescock]

I agree that there's a lot of duplication between QueryState's and Query's API and it's good that this is lessened, but just out of curiosity (just to offer a potentially dumb idea), would it perhaps be better if QueryState and Query were just one single struct instead of two?

I think the idea is that you need access to the world to actually do querying, but that we want to cache some state that lives longer than that borrow. So a Query is a &QueryState for the state plus UnsafeWorldCell<'w> and Ticks from the world.

I don't see a clean way to get rid of either. If you don't have Query then you'd need to pass a world to every method on QueryState, which is terrible ergonomics for even the simplest system. If you don't have QueryState then you need to look up ComponentIds and match archetypes every time you run a system, which is terrible performance.

[chescock]

I realized I could split this into two independent PRs: One for the methods on QueryState to obtain a Query, and one for the consuming methods on Query. The consuming methods aren't all that useful by themselves, but the methods on QueryState should be, and those shouldn't be as contentious. Would anyone like me to split it up like that?

[BenjaminBrienen]

I say go big! Go ahead and remove the to-be-deprecated methods on QueryState! 😆

You can split it up if you think it would help, but I don't mind bigger PRs.

[BenjaminBrienen]

Why remove this test? Is it no longer applicable?

[BenjaminBrienen]

Why remove this check?

[chescock]

Oh, right, that was subtle and I should have pointed it out!

The reason is that I changed all of the &self and &mut self methods on Query to call self.as_readonly().actual_method() and self.reborrow().actual_method(), and as_readonly() and reborrow() are implemented in terms of Query::new(). I wanted to make sure that change was zero-cost, but checking the assembly with cargo-show-asm showed that it was adding a call to validate_world to every existing method on Query! Since we always had a world that was already validated, I changed it to be a safety requirement, and now the assembly appears to be unchanged.

(Then I removed the test you asked about above because it's no longer true that it panics.)

[BenjaminBrienen]

I think it would be good to add more examples/tests that might help miri find unsafe mistakes, but it seems pretty clean.

[13ros27]

This looks good to me, mostly just nits although I do have one question about that IntoIterator for Query?

[cBournhonesque]

sorry, noob question: how does this work?
The &QueryState inside the Query is simply copied? i.e. we create a new Query that points to the same QueryState?

[chescock]

Yup! Everything in Query is Copy, which means it can be copied by memcpy. That includes references like &QueryState, which just makes a new reference to the same underlying QueryState. So we can make Query be Copy, and then implement Clone in terms of it.

[alice-i-cecile]

Oh my gosh finally.

[cBournhonesque]

Sorry noob question again could you please explain again why we need to re-borrow?

self.reborrow().into_iter() creates a new Query with reborrow() and then borrows from that with into_iter()
self.into_iter() would simply create a QueryIter that borrows from the initital &'a mut Query so the lifetime is constrained by `a; is that the issue?

The lifetimes are elided on reborrow so I don't get exactly what is happening

[chescock]

reborrow() goes from &'a mut Query<'w> to Query<'a>, turning a borrowed Query into an owned one with a shorter lifetime.

into_iter() takes self instead of &mut self, so it actually consumes the Query.

We have an &mut Query, so we first reborrow() to get an owned Query with a shorter lifetime, and then use into_iter() to consume the owned query and create a QueryIter - still with the shorter lifetime.

It's more complex when looking at one method, but across the whole type it means we can re-use the safety proofs in reborrow() and as_readonly(). That lets us implement this method without unsafe code, and means the compiler would catch it if we tried to return QueryIter<'w, ...> instead of QueryIter<'_, ...>.

[Victoronz]

Sorry to block this (the merge queue got there first), but I don't think Query being Copy is sound?
IIUC you can go from a mutable Query to a read-only QueryLens, make a Query out of that lens again, then copy that Query, transmute_lens it again, and get a Query from it for a last time. Then you'd have a read-only query that is no longer bound to the &mut self of the original mutable Query.
Did I overlook something?

[alice-i-cecile]

Discuss in #17693.

[Victoronz]

Something else I'd like to note is that this PR implements IntoIterator for Query, however that impl is not restricted to ReadOnlyQueryData. This means that a simple for loop over a mutable Query will consume it, even if that loop doesn't actually mutate anything!
I'd think this to be quite confusing for users. Especially since there is no "owned" way of iterating Query, we're still just borrowing the data in the end.
Implementing it only for ReadOnlyQueryData would be confusing as well, since that'd mean that writing mutable for loops now diverges from writing immutable ones!
Is that impl necessary?

[chescock]

Something else I'd like to note is that this PR implements IntoIterator for Query, however that impl is not restricted to ReadOnlyQueryData. This means that a simple for loop over a mutable Query will consume it, even if that loop doesn't actually mutate anything!

Yup! That's the generalization of iter_inner() to mutable data, and lets you write query_state.query_mut(world).into_iter() without needing to promote the temporary query to a variable, which may be impossible if you're returning data. That's necessary to completely replace query_state.iter_mut(world).

I'm hoping it will be a nicer experience for users: You can write the obvious for data in query, and in the common case where you only iterate once it will finally just work! If you need to iterate multiple times, you'll need to for data in &mut query, but that's true for other common collection types like Vec, so it should be familiar.

And because read-only queries also impl Copy, you can do multiple for data in query loops over them, and everything will work!

[chescock]

And while I'd been focusing on QueryState when writing this, the IntoIter impl should also make it nicer to work with QueryLens, since you won't always have to put the Query in a variable.

For example:

let mut lens: QueryLens<_> = ...;

// Fails with "error[E0716]: temporary value dropped while borrowed"
let first = lens.query().iter_mut().next();
assert!(first.is_some());

// Works!
let first = lens.query().into_iter().next();
assert!(first.is_some());