Reflexive world queries

[JoJoJet]

RENDERED.

Currently, the #[derive(WorldQuery)] macro produces types with a sub-par API. For each user-defined type, the macro generates an additional "Item" type associated with it, and the API is spread between these two types in a way that increases complexity and makes custom world query types more difficult to use and create. I propose we amend this by requiring derived world queries to be reflexive, which removes the need for "Item" types.

[CGMossa]

I'd welcome an improvement to this honestly.
An example from my stuff: I have an enum and marker types related to this enum;
Since we don't have enum-variants-something-feature, I wanted to roll my own setup;
I could succinctly write it as

/// Query for figuring out if a given animal is [`Piglet`], [`Juvenile`] or [`Adult`].
/// See [`AgeGroup`].
#[derive(Debug, WorldQuery)]
pub struct ReadAgeGroup {
    piglet: Option<&'static Piglet>,
    juvenile: Option<&'static Juvenile>,
    adult: Option<&'static Adult>,
    //FIXME: this should probably have `With<WildBoar>` on it.
}

impl ReadAgeGroupItem<'_> {
    /// Returns the [`AgeGroup`] of the given entity.
    pub fn get(&self) -> AgeGroup {
        match (self.piglet, self.juvenile, self.adult) {
            // piglet
            (Some(_), None, None) => AgeGroup::Piglet,
            // juvenile
            (None, Some(_), None) => AgeGroup::Juvenile,
            // adult
            (None, None, Some(_)) => AgeGroup::Adult,
            _ => panic!("`WildBoar`-entity has the wrong age-groups component."),
        }
    }
}

Notice the *Item I need to write. Now the documentation for this is in three places: Module with marker types; the enum;
and now this.

[alice-i-cecile]

Well-motivated and a good change. I'm in favor!

This is particularly nasty when trying to write impl blocks for the QueryItem type. It's a great pattern, but hard to discover and hard to document.

[james7132]

This is my biggest gripe with this approach. This sacrifices the simpler mental model for normal users and happy path and forces any mutative query to use Mut<T> over &mut T, and that mapping from &T to &mut T is now lost.

We can address this by implementing &mut T: World Query, and just forcibly mark everything matched as mutated, but that would make the distinction between &mut T and Mut<T> extremely nuanced.

[alice-i-cecile]

Yeah, I'm nervous about that, especially because of how subtly it breaks existing code.

[JoJoJet]

This is my biggest gripe with this approach. This sacrifices the simpler mental model for normal users and happy path and forces any mutative query to use Mut<T> over &mut T, and that mapping from &T to &mut T is now lost.

Not sure if we're on the same page, but I want to clarify that you can still write Query<&mut T> and have it return Mut<T> -- the reflexive constraint is only for named WorldQuery structs made using the derive macro. Very little user code will be affected in practice. I think this constraint will surprise users at first, but I also think it will make sense when they think about it, or read the docs and have it explained to them (we'll have to think of a good way of explaining this that would make it click more easily). Also, this behavior is consistent with how the SystemParam derive macro works, which might make it easier to understand for some users.

[JoJoJet]

Another point I want to add: using &mut T in a derived WorldQuery already fails with a confusing type error, until you read the docs and see that you need the special attribute #[world_query(mutable)]. Since we're already relying on the user to read the docs for this macro to be usable, I don't think there's much of a regression caused by requiring world queries to be reflexive.