Combine Query and QueryLens using a type parameter for state, but don't introduce owned iterators

[chescock]

Objective

Make QueryLens easier to use by allowing query methods to be called on it directly instead of needing to call the query() method first.

Split out from #18162. I thought this might be easier to review separately, but if we do merge #18162 then this PR will no longer be necessary.

Solution

Make Query and the various iterator types generic, where normal queries use borrowed &QueryState<D, F> and QueryLens uses owned Box<QueryState<D, F>>.

Have Query use a default type for the state so that it continues to work without specifying it explicitly.

Note that the 'state lifetime on Query was only used in &'state QueryState, so it is now only used in the default type parameter. It still needs to be part of the Query type in order to be referenced in the default type, so we need a PhantomData so that it's actually used.

Testing

I used cargo-show-asm to verify that the assembly of Query::iter did not change.

This rust code:

use crate::prelude::*;
#[derive(Component)]
pub struct C(usize);

#[unsafe(no_mangle)]
#[inline(never)]
pub fn call_query_iter(query: Query<&C>) -> usize {
    let mut total = 0;
    for c in query.iter() {
        total += c.0;
    }
    total
}

Run with

cargo asm -p bevy_ecs --lib --simplify call_query_iter

Results in the same asm before and after the change

Details

call_query_iter:
	push rsi
	push rdi
	push rbx
	mov rdx, qword ptr [rcx]
	mov rcx, qword ptr [rcx + 8]
	mov r8, qword ptr [rdx + 248]
	mov rax, qword ptr [rdx + 256]
	lea r9, [r8 + 4*rax]
	cmp byte ptr [rdx + 264], 0
	je .LBB1856_2
	xor r11d, r11d
	xor r10d, r10d
	xor esi, esi
	xor eax, eax
	jmp .LBB1856_6
.LBB1856_2:
	xor r11d, r11d
	mov esi, 8
	xor r10d, r10d
	xor edi, edi
	xor eax, eax
	jmp .LBB1856_16
.LBB1856_3:
	xor r11d, r11d
.LBB1856_4:
	xor esi, esi
.LBB1856_5:
	mov edi, esi
	inc esi
	add rax, qword ptr [r11 + 8*rdi]
.LBB1856_6:
	cmp esi, r10d
	jne .LBB1856_5
.LBB1856_7:
	cmp r8, r9
	je .LBB1856_23
	mov r10d, dword ptr [r8]
	add r8, 4
	mov r11, qword ptr [rcx + 416]
	lea rsi, [r10 + 8*r10]
	mov r10, qword ptr [r11 + 8*rsi + 16]
	test r10, r10
	je .LBB1856_7
	lea r11, [r11 + 8*rsi]
	mov rsi, qword ptr [rdx + 272]
	cmp qword ptr [r11 + 64], rsi
	jbe .LBB1856_3
	mov rdi, qword ptr [r11 + 56]
	mov rsi, qword ptr [rdi + 8*rsi]
	test rsi, rsi
	je .LBB1856_3
	mov r11, qword ptr [r11 + 24]
	not rsi
	lea rsi, [rsi + 2*rsi]
	shl rsi, 4
	mov r11, qword ptr [r11 + rsi + 16]
	jmp .LBB1856_4
.LBB1856_13:
	xor r11d, r11d
.LBB1856_14:
	mov rsi, qword ptr [rsi + 80]
	xor edi, edi
.LBB1856_15:
	mov ebx, edi
	shl rbx, 4
	mov ebx, dword ptr [rsi + rbx + 8]
	not ebx
	inc edi
	add rax, qword ptr [r11 + 8*rbx]
.LBB1856_16:
	cmp edi, r10d
	jne .LBB1856_15
.LBB1856_17:
	cmp r8, r9
	je .LBB1856_23
	mov r10d, dword ptr [r8]
	add r8, 4
	mov rsi, qword ptr [rcx + 256]
	lea r11, [r10 + 4*r10]
	shl r11, 5
	mov r10, qword ptr [rsi + r11 + 88]
	test r10, r10
	je .LBB1856_17
	add rsi, r11
	mov r11d, dword ptr [rsi + 148]
	mov rdi, qword ptr [rcx + 416]
	lea rbx, [r11 + 8*r11]
	mov r11, qword ptr [rdx + 272]
	cmp qword ptr [rdi + 8*rbx + 64], r11
	jbe .LBB1856_13
	lea rdi, [rdi + 8*rbx]
	mov rbx, qword ptr [rdi + 56]
	mov r11, qword ptr [rbx + 8*r11]
	test r11, r11
	je .LBB1856_13
	mov rdi, qword ptr [rdi + 24]
	not r11
	lea r11, [r11 + 2*r11]
	shl r11, 4
	mov r11, qword ptr [rdi + r11 + 16]
	jmp .LBB1856_14
.LBB1856_23:
	pop rbx
	pop rdi
	pop rsi
	ret

[ElliottjPierce]

I'm broadly on board with this. I do something similar for my ecs too.

It's really hard to see the changes to all the*_inner functions that return 'w types, but I'm assuming those were just copied over. And the asm didn't change, so the impl is still correct.

The only thought I have is to change S: Deref<Target = QueryState<D, F>> to S: Borrow<QueryState<D, F>>. That would let this work with an owned state directly. In fact, you might even be able to get rid of the Box in the query lens.

[chescock]

It's really hard to see the changes to all the*_inner functions that return 'w types, but I'm assuming those were just copied over. And the asm didn't change, so the impl is still correct.

Yeah, I did put the moves in their own commit in case that helps, but it's still hard to confirm that that commit really is just a move. I wish we had better tooling for reviewing things like that!

The only thought I have is to change S: Deref<Target = QueryState<D, F>> to S: Borrow<QueryState<D, F>>. That would let this work with an owned state directly. In fact, you might even be able to get rid of the Box in the query lens.

Hah, yeah, that's what I did first :). @Victoronz convinced me that Box and Deref are better here: #18162 (comment).

[ElliottjPierce]

Hah, yeah, that's what I did first :). @Victoronz convinced me that Box and Deref are better here: #18162 (comment).

That makes a lot of since. I still hate to loose all the flexibility that Borrow would afford us, but I get the argument for deref.

[Victoronz]

While there is some follow-up to be done, I think this is a good next step!
Hope realigning the changes with main isn't too tedious.

[alice-i-cecile]

Marked as X-Controversial as it adds another generic to Query. I think that this is probably the right call, but I'm nervous about making query more complex to ease things for QueryLens.

[Victoronz]

To clarify some of the benefits this direction has:
In the past, Query-related code was constrained by the need to work with a &'state QueryState lifetime both because of the lifetime, and the lack of ownership.

This led to various cases where we could not directly create a usable Query even if we wanted to, leading to workarounds like QueryLens, and limitations to transmutes.
The inability to directly work with Query is also part of why a lot of functionality was implemented on QueryState, instead of the more appropriate Query.

Further, this direction allows us to more closely match the 'w and 's lifetimes by letting code create and immediately use owned QueryState, which will alleviate some of our double lifetime pains.
This is not a guarantee, but I suspect that we may be able to remove the get_inner methods (they serve to remove the 'self lifetime constraint), ReleaseStateQueryData (serves to declare query items as independent of 's) , and maybe even 's entirely.

I believe that such improvements surrounding 's will make it easier to mend some of the edge cases surrounding query items that depend on it. The resulting incompatibilities between query features are a headache, and a design risk for upcoming work!

Additionally, this should ease the design space for Queries-as-Entities (QueryState can now live behind various pointer types), Uncached Queries (its functionality could be folded into the generic), Relation Queries (Query is now easier to create on the fly), etc.

Overall, I think the simplifications it will allow are worth the surface complexity it introduces!

The Query implementation lore is quite a scattered puzzle, I hope these are enough (sensible) pieces to develop a feeling for the current situation.

[cBournhonesque]

As Victoronz stated above, some other features like Uncached Queries or MultiSource Queries (Relation Queries) will also need that third generic on Query, so I think it's a step in the right direction!

Currently the third generic is Deref<Target = QueryState<D, F>> = &'state QueryState<D, F>,
Ultimately it would be a trait for QueryState which would have different implementations based on whether or not the state is owned/borrowed, cached/uncached, single-source/multi-source.

It's easier to introduce this third generic for the owned/borrowed case because the code changes are small and the use-case is pretty clear.
The only reason for QueryLens to exist was that to transmute the Query you had to put the new state somewhere, and Query doesn't own the state, only borrows it. It's clearer to indicate directly in the type system that a QueryLens is simply a Query that owns its state.

[chescock]

Marked as X-Controversial

Yup, that label seems appropriate!

as it adds another generic to Query

I'll note that defaulted generic parameters are usually pretty invisible to users. I often forget that Vec<T> is secretly short for Vec<T, A = Global>!

to ease things for QueryLens

One of my goals here is to make world.query::<D>().iter() work. We could almost make that work today by having query() return a QueryLens, but without these changes plus the rest of #18162 it needs to be bound to a local variable to be usable.

So, it's not so much about the current uses of QueryLens, as about making it nicer to use QueryLens or uncached queries in new places.

I suspect that we may be able to remove the get_inner methods (they serve to remove the 'self lifetime constraint), ReleaseStateQueryData (serves to declare query items as independent of 's) , and maybe even 's entirely.

I don't think I see how this change would help with that.

The get_inner methods let you create a Query within a method and return data from it, and that's useful with ordinary state-borrowing queries even if we add new versions.

It might be possible to remove the 's lifetime entirely! We could set it equal to 'w everywhere by having SystemState::get and get_mut take &'w mut self instead of &'s mut self, and most uses would still work. But I don't think this change would make that any easier (or harder).

[alice-i-cecile]

With a bit more docs this LGTM. These arguments have convinced me!

[Victoronz]

I suspect that we may be able to remove the get_inner methods (they serve to remove the 'self lifetime constraint), ReleaseStateQueryData (serves to declare query items as independent of 's) , and maybe even 's entirely.

I don't think I see how this change would help with that.

The get_inner methods let you create a Query within a method and return data from it, and that's useful with ordinary state-borrowing queries even if we add new versions.

It might be possible to remove the 's lifetime entirely! We could set it equal to 'w everywhere by having SystemState::get and get_mut take &'w mut self instead of &'s mut self, and most uses would still work. But I don't think this change would make that any easier (or harder).

How I see it, this change gives us a new lever manipulate 's with:
Every &'state QueryState has to refer to a QueryState that was created and stored somewhere else at some point.
If this original location has more options for how it can store/give out its QueryState, we can prevent some situations where 's is unnaturally shortened.
I'd see query iterators that want to create a transient QueryState for their items as an example of that.

But for now, this is still a suspicion! It might also be that the situations I've seen were hindered by the tech debt that can be addressed as follow-ups to this PR if it goes through, or that there'd be other issues to run into.

[hymm]

Is this going to mess up people who want to take a query as a parameter into a function? i.e. are they going to have to be generic over the cache type now?

[chescock]

Is this going to mess up people who want to take a query as a parameter into a function? i.e. are they going to have to be generic over the cache type now?

No, the new type parameter has a default, so it should all be backwards compatible. Functions will only need to be generic over the cache type if they want to work with both Query and QueryLens.

[hymm]

That's what I was trying to say. At the very least this needs to be mentioned in the migration guide as that's one of the main uses of QueryLens.

[chescock]

Oh, I thought the intended usage was the one in the transmute_lens docs, where the reusable function takes a QueryLens. That case will work unchanged. (Edit: And by "unchanged" I mean other than removing the call to .query().)

If a user instead has a function that takes a Query and are calling it like foo(query_lens.query()), then they'll get a warning that query() is deprecated. The deprecation message and the migration guide both mention that the reborrow() method can be used instead.

I suppose we could note that they could also make the function generic, but reborrow() is going to be the cleaner solution in most cases, and it covers any existing code, so I'd rather stick with recommending that.

[ItsDoot]

I have a thought about cleaning up the sometimes-unused 'state lifetime, which I've PR'd here: chescock#9

[ItsDoot]

Low hanging fruit for a follow-up PR: QueryLens's definition and impl still use Q: QueryData where they should be D: QueryData to match Query.

[chescock]

I have a thought about cleaning up the sometimes-unused 'state lifetime, which I've PR'd here: chescock#9

Yeah, in some ways that would be cleaner! The trouble is that a type alias has worse ergonomics than a defaulted type parameter, and Query is a really common type. And the cost of the extra lifetime parameter is pretty low, since it can mostly be ignored. So I think a defaulted type parameter is a better tradeoff overall.

[cart]

This is definitely a win for QueryLens UX, which was just clunky enough that I didn't want to use it.

The only real alternative I can see is a Query trait, which would likely also benefit from the ability to share internal impls like this.

The implementation is straightforward / I don't have any concerns there.

I do have some concerns!

[cart]

This does mean that being generic on any query type is arcane to the point of being nonviable for normal use cases. Going with this approach means we're committing to writing state-type-specific APIs (ex: Query defaulting to QueryState, QueryLens, UncachedQuery, etc).

The proposal to use QueryLens as the "common" API for reusable functions makes some sense. But that currently relies on doing things like transmute_filtered, which is an "expensive" API that copies a lot of data, recomputes access, recomputes state, and allocates. I don't find that a satisfying answer to the general problem in its current form, and its not currently something I think we should be training people to use as if it were as simple as a "cast" operation. Not really an alternative to being generic on the query type. Are there plans to make this cheaper?

I'm also uncertain about how UncachedQuery would relate to QueryLens, as QueryLens is currently a "cached" API. Converting UncachedQuery to a cached QueryLens just to use it for a "general purpose function" is another case of "suboptimal and unsatisfying".

[Victoronz]

This does mean that being generic on any query type is arcane to the point of being nonviable for normal use cases. Going with this approach means we're committing to writing state-type-specific APIs (ex: Query defaulting to QueryState, QueryLens, UncachedQuery, etc).

The closest comparison here that I can think of is HashMap. It similarly has generics everyone uses, plus one that most do not care about, but when they do, it really matters. The generics situation of HashMap is complex compared to other std types, but not the point of being arcane imo.
In comparison, I believe what makes our current Query-related logic arcane: The various undesired edge cases/intricacies (mostly lifetime related) strewn about various parts of the Query implementation are entangled with one another to the point of obscuring the bigger picture.

We already have multiple types that Query behavior is split amongst, Query, QueryLens and QueryState. What caused this split is the lifetime behaviors we desired of Query and its respective items.
What Deref<Target = QueryState<D, F>> abstracts over is not our reliance on QueryState, but ultimately the need to reference fetch_state/filter_state via differing kinds of ownership.
Once we unify this behavior, we are then freed to ask what we want to do with QueryState itself. We can split it apart, or change up the fetching infrastructure as a whole. We can also address the lifetime questions in a more disjoint fashion.

I think by focusing these behaviors, and then addressing their complexities individually, we can ultimately dispel a good part of the confusion around Query!
(Anecdotally, I've recently seen an increased amount of confusion around Query lifetimes in the discord the more we've built on top of them.)

The proposal to use QueryLens as the "common" API for reusable functions makes some sense. But that currently relies on doing things like transmute_filtered, which is an "expensive" API that copies a lot of data, recomputes access, recomputes state, and allocates. I don't find that a satisfying answer to the general problem in its current form, and its not currently something I think we should be training people to use as if it were as simple as a "cast" operation. Not really an alternative to being generic on the query type. Are there plans to make this cheaper?

I do think similarly, and figure that we can address this in a manner similar to string-interning:
We compute the QueryState for each individual <D, F> combination when first needed, and then have a global structure retain it for subsequent uses/transmutes to access. This would of course be best-effort, as mutation or type/access mismatches may prevent reuse. However, I do think it be a good improvement over the status quo.

I'm also uncertain about how UncachedQuery would relate to QueryLens, as QueryLens is currently a "cached" API. Converting UncachedQuery to a cached QueryLens just to use it for a "general purpose function" is another case of "suboptimal and unsatisfying".

Either way I think that uncached queries will need their own type, be that via a generic on Query or some separate UncachedQuery.
Folding that in the same S generic with a different trait doesn't mean that such types need to follow the "Referenceable Cache" meaning of S.
This is somewhat unsatisfying, but what I'd personally deem as a workable intermediate solution.
Like this, we can not step onto each others toes while developing Query-related logic, and more easily isolate and reuse shared behaviors surrounding fetching-like logic.
As you've said, it might turn out that some Query trait is the better option for uncached queries, but I think we'll have a better picture after further work in our respective areas.

(A Query trait does not work for the "state-referencing" behavior this PR captures, because this behavior is internal to Query itself.)

[chescock]

This does mean that being generic on any query type is arcane to the point of being nonviable for normal use cases.

Oh, that's a good point. I hadn't really been focusing on that as a use case, but I agree it will be important, especially if we introduce other query variants.

We might be able to make that easier with a type alias. Suppose we add a trait for QueryState with associated types for the Data and Filter. Then we could do:

trait QueryStateTrait {
    type Data: QueryData;
    type Filter: QueryFilter;
}

type QueryAlias<'w, 's, S> =
    Query<'w, 's, <S as QueryStateTrait>::Data, <S as QueryStateTrait>::Filter, S>;

fn generic_in_query_state<S: QueryStateTrait>(query: QueryAlias<S>) {}

That winds up very similar to the suggestion of having Query be a type alias for a QueryBase<&'s QueryState>. The difference is just in which type is the "real" type and which is the alias. This version is a little more complex overall, but puts the complexity on generic functions instead of on ordinary systems.

So, users would need to know that the alias exists, but it's straightforward to write once they do. Is that good enough to be viable?

And does anyone have good suggestions for what to name those items?

The proposal to use QueryLens as the "common" API for reusable functions makes some sense. But that currently relies on doing things like transmute_filtered, which is an "expensive" API ... Are there plans to make this cheaper?

Yeah, @hymm is correct that Query is a better common API than QueryLens.

But I bet we could also make transmutes cheaper! Most of the allocations there are cloning the matched_storage_ids, but it should be possible to borrow those from the source query. I think we could introduce yet another query variant that owns the fetch_state (which is normally a set of ComponentIds with no allocations) but borrows the access and storage ids.

That should be a separate PR, though.

[cart]

@Victoronz

The generics situation of HashMap is complex compared to other std types, but not the point of being arcane imo.

Yeah my point about arcane-ness was not the presence of the third parameter, but rather the spells that you must cast if you want to write a function that is actually generic on it in practice. Being generic on a "simple" Query like Query<(&Transform, &Mesh3d)> requires this to make it actually usable:

fn mesh_transform<'a, 'b, S: Deref<Target = QueryState<(&'a Transform, &'b Mesh3d), ()>>>(
    query: Query<(&'a Transform, &'b Mesh3d), (), S>,
) {
}

This is noisy and complicated to the point of being unusable in practice, in a way that HashMap's third generic is not.

I have a slight suspicion that @chescock's proposal will break down in practice for lifetime reasons. I'd be curious to see a working implementation (and a port of the example above to that system). I don't love the added type complexity here. In some ways it just serves to make this more arcane.

What Deref<Target = QueryState<D, F>> abstracts over is not our reliance on QueryState, but ultimately the need to reference fetch_state/filter_state via differing kinds of ownership.

It seems like supporting differing kinds of ownership is only really necessitated by the fact that we're trying to make QueryLens as a type more "passable" by using Box<QueryState>. This whole problem seems inside-out, and we're taking on a ton of extra complexity, which will incur overhead (ex: cloning Query state just to coerce it into QueryLens), maintainability challenges, and user-facing UX concerns going forward. We already have a good cheap way to pass a reference to some X into a function ... borrowing it via &X. Converting between reference and ownership semantics reallllly shouldn't require recompiling our entire suite of Query functionality, or at least, that feels like an indicator that we have somehow strayed from the happy path.

I spent a chunk of time yesterday playing around with renaming QueryLens to OwnedQuery, moving all Query functionality to OwnedQuery, and making Query just a type alias for &mut OwnedQuery. It kiiiiiind of worked, lets us cut down on a TON of infrastructure, trivially solved the QueryLens / Query compatibility and conversion issue, and had some neat behaviors like removing the need to add the mut in mut query: Query<&mut T> (because that was implied in the type alias). But it also broke down in a variety of fundamental ways (SystemParam ownership issues, QueryState functionality relying on conversion to Query breaking down, SystemParam's lifetimes not lining up because it essentially forced the param state lifetime to encapsulate the world lifetime, which is a nono). I don't think my attempt in its current form is a viable path.

That being said, when I see big deletes of code, arcane nests of types and traits collapsing, system understandability increasing, performance increasing, and excess monomorphization decreasing, it makes me feel like I'm on roughly the right track. Has anyone explored a similar path?

Reliance on QueryState / abstracting over S

From my perspective, abstracting over S feels like we're just kicking the can down the road from a "type complexity" perspective. Any new S behavior we replace QueryState with to provide generalized functionality would probably still look something like:

fn mesh_transform<'a, 'b, S: StateBehavior<(&'a Transform, &'b Mesh3d)>>(
    query: Query<(&'a Transform, &'b Mesh3d), (), S>,
) {
}

Or perhaps somehow:

fn mesh_transform<S: StateBehavior<(Read<Transform, Read<Mesh3d>)>>(
    query: Query<(&Transform, &Mesh3d), (), S>,
) {
}

The benefit we'd get from shared Query infrastructure is likely to be minimal when most of the details of the implementation will live in S. And adopting the "punt the abstraction to S" strategy means we're also punting that to users in our public interface.

This whole scenario would be better served with a trait like:

fn mesh_transform(query: impl QueryLens<(&Transform, &Mesh3d)>) {
}

Note that this relies on anonymous lifetimes in impl trait, otherwise manually specifying each lifetime is necessary. However I'm reasonably certain manual lifetimes will be required for the abstract over S approach too.

I do think similarly, and figure that we can address this in a manner similar to string-interning:

While that could potentially help the situation (depending on how the "interning" is implemented), I think leaning on that to solve the type level "how do I pass a query to a reusable function" problem can only get us so far performance wise (checking the cache takes time, and it forces things like uncached queries to go through the cache). This feels like a solve-able problem type-system-wise.

[chescock]

making Query just a type alias for &mut OwnedQuery. ... [I] feel like I'm on roughly the right track. Has anyone explored a similar path?

Oh, that's an interesting idea! That would also let us get rid of the _inner methods and the reborrow() method, since we'd get the language-level reborrowing from &mut.

I don't see how to put the world in the query, though. I assume that's one of fundmental ways it broke down. A Query needs an UnsafeWorldCell, so if we use &mut OwnedQuery, then there must be an OwnedQuery somewhere that has the UnsafeWorldCell. But UnsafeWorldCell has a 'w lifetime, so it can't live in the SystemParam::State. And we don't have anywhere else to store the OwnedQuery value!

I have a slight suspicion that @chescock's proposal will break down in practice for lifetime reasons. I'd be curious to see a working implementation (and a port of the example above to that system). I don't love the added type complexity here. In some ways it just serves to make this more arcane.

I gave it a try and got something working, but it wasn't pretty. The "state" is only valid for one lifetime, so you wind up needing a named 's parameter in order to ensure the query lifetime matches the state lifetime. I'm guessing that's the lifetime issue you were expecting.

Note that this relies on anonymous lifetimes in impl trait, otherwise manually specifying each lifetime is necessary.

Brainstorming some more... that lifetime issue means the only pleasant solution for generic functions today is for D and F to be parameters to a type and not a trait. So maybe we could take the parameters out of the trait using a "type family" pattern and get it down to

fn mesh_transform(
    query: Query<(&Transform, &Mesh3d), (), impl QueryStateFamily>,
) {
}

By doing something like

pub trait QueryStateFamily {
    type QueryState<'s, D: QueryData + 's, F: QueryFilter + 's>: Deref<Target = QueryState<D, F>>;
}

pub struct BorrowedQueryState;
impl QueryStateFamily for BorrowedQueryState {
    type QueryState<'s, D: QueryData + 's, F: QueryFilter + 's> = &'s QueryState<D, F>;
}

pub struct BoxedQueryState;
impl QueryStateFamily for BoxedQueryState {
    type QueryState<'s, D: QueryData + 's, F: QueryFilter + 's> = Box<QueryState<D, F>>;
}

That lets users write generic functions without any explicit lifetimes and without repeating the query data type. And the default type on Query is F = BorrowedQueryState, which is a nice-looking simple name instead of the scary generic we have now.

Would that be an acceptable API?

[cart]

Oh, that's an interesting idea! That would also let us get rid of the _inner methods and the reborrow() method, since we'd get the language-level reborrowing from &mut.

Yeah I got to remove all of those. Felt super cathartic :)

I don't see how to put the world in the query

Yup that is one of the fundamental ways it broke down.

I'm guessing that's the lifetime issue you were expecting.

I expected it to be worse than adding a single lifetime. I expected one for each &Component in the query.

Would that be an acceptable API?

Thats close to best-case scenario I think, given the current lack of "anonymous lifetimes in impl trait". However it does seem like all fingers point to that being stabilized in the near future (not that it helps us if we want to make progress here in the short term).

I'm also a bit dubious that it would work in practice, as there is nothing in Query<(&Transform, &Mesh3d), (), impl QueryStateFamily> that constrains QueryStateFamily::QueryState<D, F> to D = (&Transform, &Mesh3d) and F = (). Given that the impl for Query will rely on those matching, I suspect those functions will not be callable on that query.

[chescock]

I expected it to be worse than adding a single lifetime. I expected one for each &Component in the query.

Oh, yeah, checking again I see I used (&'static Transform, &'static Mesh3d) there because of the "anonymous lifetimes in impl trait" issue. And it still needed a named 's lifetime :).

Thats close to best-case scenario I think, given the current lack of "anonymous lifetimes in impl trait".

Alright, I'll try to get that working so we can evaluate it properly, then!

I'm also a bit dubious that it would work in practice, as there is nothing in Query<(&Transform, &Mesh3d), (), impl QueryStateFamily> that constrains QueryStateFamily::QueryState<D, F> to D = (&Transform, &Mesh3d) and F = (). Given that the impl for Query will rely on those matching, I suspect those functions will not be callable on that query.

I think the Deref<Target = QueryState<D, F>> bound on the definition of QueryStateFamily::QueryState ensures that? I definitely have something where

fn mesh_transform(query: GenericQuery<(&Transform, &Mesh3d), (), impl QueryStateType>) {
    for (transform, mesh) in query.iter() {
        let _transform: &Transform = &*transform;
        let _mesh: &Mesh3d = &*mesh;
    }
}

fn call_mesh_transform_query(query: Query<(&Transform, &Mesh3d)>) {
    mesh_transform(query);
}

fn call_mesh_transform_lens(query: QueryLens<(&Transform, &Mesh3d)>) {
    mesh_transform(query);
}

compiles, but I have a bunch of experiments on my branch and GenericQuery is currently a type alias, so there might be confounding factors.

[cart]

I can see how adding S positions us better to support "uncached queries", but I don't have an easy answer to how we get from S: Deref<Target = QueryState<D, F>> to a type constraint that could accommodate uncached queries (which certainly won't be able to deref to QueryState).

What is the vision there?

[chescock]

I can see how adding S positions us better to support "uncached queries", but I don't have an easy answer to how we get from S: Deref<Target = QueryState<D, F>> to a type constraint that could accommodate uncached queries (which certainly won't be able to deref to QueryState).

What is the vision there?

I'm also uncertain about how UncachedQuery would relate to QueryLens, as QueryLens is currently a "cached" API. Converting UncachedQuery to a cached QueryLens just to use it for a "general purpose function" is another case of "suboptimal and unsatisfying".

The vision is to replace the Deref bound with a new trait. We'd have separate implementations for cached queries, uncached queries, and query lenses.

My original larger PR, #18162, had such a trait, but this smaller part just didn't need it yet. That wasn't written to handle uncached queries as in #21607, though, so the trait might look a little different, but the general approach should still work.

Brainstorming a bit more: There might even be multiple kinds of uncached queries that we want to work with.

One is a "get-only" query parameter. This comes up when following relations, where we query by Entity. If you only need to get and not iterate, then you don't need the cached storage ids. But the rest of the QueryState can still be borrowed from the system state.

Another is "immediate" queries, where you write for data in world.query::<D>(). These don't need cached storage ids, but will need to own the fetch_state, since there is nowhere to borrow it from.

Those have different needs, so if we want the best solution for each case then we'll probably need separate QueryState types for them. But they should still be able to share the same Query API!

[cBournhonesque]

That trait would be useful for Uncached queries but also for 'relation' queries.
Flecs iterators returned by queries allow you to iterate through data on any of the query 'terms' (entities that are being considered by a query: https://www.flecs.dev/flecs/structflecs_1_1iter.html)

Currently bevy queries have only one term, but a trait

trait QueryState {
   type Iter;
   type Cache;
}

would enable various impls for Uncached/Cached, Borrowed/Owned, Single-Term/Multiple-Terms.

This seems the direction with your QueryType trait, where the associated type QueryState handles the Borrowed/Owned split

[chescock]

I updated the PR with the "type family" approach. Instead of the new type parameter on Query being the QueryState itself, it's now an impl QueryType with a generic associated type that defines the QueryState. That type is BorrowedQuery for Query, OwnedQuery for QueryLens, and can be impl QueryType for a function that wants to be generic.

One consequence of using a GAT there is that Query is no longer covariant in 's! I didn't expect this to matter at all, but it turned out there were a few in-engine uses of &'a Query<'a, 'a, D, F>. I added a migration guide for it and fixed the in-engine code, but I have no idea whether that is a common pattern in downstream code.