Use distinct types for query "data" generics and query "filter" generics

[alice-i-cecile]

What problem does this solve or what need does it fill?

It's easy for users to accidentally mix up their "data" (Q) and "filter" (F) generics on Query.

For example:

• Query<(&A, With<B>): forces ugly unpacking.
• Query<(&A, Without<B>): likely seriously wrong.
• Query<Changed<T>): probably not what the user wants.
• Query<(), Entity>: nonsensical
• Query<&A, &B>: very weird, probably broken

See #7679 for an example of this footgun in the wild.

What solution would you like?

1. Add WorldQueryData: WorldQuery and WorldQueryFilter: WorldQuery.
2. Split derive(WorldQuery) macro into two parts.
3. Only implement WorldQueryData / WorldQueryFilter for the correct WorldQuery types.

What alternative(s) have you considered?

We could use a custom linter #1602, but a) this catches errors later / less elegantly and b) we don't have one yet.

[wainwrightmark]

Hi, I've had a quick go at this and gotten all the tests and examples working - there's no logic changes, just changing some types and type parameters. I haven't done any of the "fun" bits like fixing documentation and writing tests yet.

I've added WorldQueryData: WorldQuery and WorldQueryFilter: WorldQuery, implemented them for the appropriate types, and forced you to use them in the left and right hand sides of the query respectively.

The thing I'm unsure of is how the derive macros ought to work.
In bevy 0.10 you do one of these

#[derive(WorldQuery)]
pub struct DataQuery {
    a: &'static A,
}

#[derive(WorldQuery)]
#[world_query(mutable)]
pub struct MutDataQuery {
    pub a: &'static mut A,
}

#[derive(WorldQuery)]
struct FilterQuery{
    a: With<A>,
}

and it generates implementations of WorldQuery and ReadOnlyWorldQuery (on the same struct, unless you specify mutable). But with the split, we need a way of specifying whether to generate WorldQueryFilter or WorldQueryData or, in principle, both.

Should we use two different macros like this:

#[derive(WorldQueryData)]
pub struct DataQuery {
    a: &'static A,
}

#[derive(WorldQueryData)]
#[world_query(mutable)]
pub struct MutDataQuery {
    pub a: &'static mut A,
}

#[derive(WorldQueryFilter)]
struct FilterQuery{
    a: With<A>,
}

That seems good but I worry that that makes it impossible to derive both for the same struct. (Both macros would generate a WorldQuery impl and they would clash). Is that a use case we want to support? I think it's probably only possible for trivial or generic types anyway but I worry that doing it this way would take away that possibility.

The other options would be:

#[derive(WorldQuery)]
#[world_query(data)]
pub struct DataQuery {
    a: &'static A,
}

#[derive(WorldQuery)]
#[world_query(mutable)] //data is implied by mutable
pub struct MutDataQuery {
    pub a: &'static mut A,
}

#[derive(WorldQuery)]
#[world_query(filter)]
struct FilterQuery{
    a: With<A>,
}

or

#[derive(WorldQuery, WorldQueryData)]
pub struct DataQuery {
    a: &'static A,
}

#[derive(WorldQuery, WorldQueryData)]
#[world_query(mutable)]
pub struct MutDataQuery {
    pub a: &'static mut A,
}

#[derive(WorldQuery, WorldQueryFilter)]
struct FilterQuery{
    a: With<A>,
}

or possibly one I haven't thought of.

[urben1680]

That seems good but I worry that that makes it impossible to derive both for the same struct.

I would want to keep having this use case possible. I want to implement WorldQuery for my type that has a &mut Foo and a Without<Bar> in the same struct. Only in this combination I want to offer methods on the Item of this WorldQuery.

However, I want to point out I am still in a design phase and I might end up not needing this. Others might though.

[alice-i-cecile]

I like the two distinct WorldQueryData versus WorldQueryFilter macros here: I think it's much clearer.

I would want to keep having this use case possible. I want to implement WorldQuery for my type that has a &mut Foo and a Without in the same struct. Only in this combination I want to offer methods on the Item of this WorldQuery.

Yeah, this is an important pattern to consider how to support. We could implement WorldQueryData for the filter types as well, but have them fetch () instead, and be omitted from the query item completely?

My personal experience has been that these types are only really used as the only type in Query: we could instead encourage people to use the SystemParam derive 🤔 But that won't give us a nice way to implement methods on the Item associated type, which is really what you tend to need.

[wainwrightmark]

I like the two distinct WorldQueryData versus WorldQueryFilter macros here: I think it's much clearer.

Agreed. After trying the different options, I think this is definitely the best one.

I would want to keep having this use case possible. I want to implement WorldQuery for my type that has a &mut Foo and a Without in the same struct. Only in this combination I want to offer methods on the Item of this WorldQuery.

The changes I've tried so far do prevent that pattern from working so it would be good to find a way to keep it.

Yeah, this is an important pattern to consider how to support. We could implement WorldQueryData for the filter types as well, but have them fetch () instead, and be omitted from the query item completely?

We could have something like FilterDataAdapter<F: WorldQueryFilter> : WorldQueryData which could then be used by the macro if you mark filters with an attribute.

#[derive(WorldQuery)]
#[world_query(mutable)]
pub struct FooWithoutBar{
    foo: &'static mut Foo,
    #[filter]
    filter: Without<Bar>
}

The macro could then exclude the filter items from the returned Item.

The problem with that approach is that it might be difficult to get to work for Changed<T> and Added<T> (which current do not work correctly if used in the Data position). It also relies on the implementations of WorldQueryData and WorldQueryFilter being similar and this restriction could block hypothetical future enhancements or optimizations that diverge the implementations.

Another option might be to have some sort of FilteredItem<Q, F> type which contained the data plus the type information of the filter. Queries would have a method as_filtered that returned items of that type and you could implement methods on it.

impl FilteredItem<&'static mut Foo, Without<Bar>>
{
    fn baz(self){
        todo!()
    }
}

fn baz_all_foos_without_bar(mut query: Query<&'static mut Foo, Without<Bar>>)
{
   for item in query.as_filtered()
   {
    item.baz();
   }
}

This way keeps the separation of data and filter and it's quite nice in that you don't need to use the derive macro to take advantage of it. That said, it is a bit less ergonomic and I think it would require major refactoring of Query (moving all of the iter* methods onto a trait.