#[derive(WorldQuery)] on Foo produces FooState which docs don't really mention

[Xion]

Bevy version

bevy v0.8.1

What you did

I was trying to make a WorldQuery item (and the SystemParam) for big_brain's common query item:

use big_brain::prelude::*;

#[derive(WorldQuery)]
#[world_query(mutable)]
pub struct Action {
    actor: &'static Actor,
    state: &'static mut ActionState,
}

Both Actor and ActionState are types from big_brain.

What went wrong

Got very confusing compilation errors about ActionState not implementing Component, even though big_brain::ActionState does implement it.

It was doubly-confusing because when I tried to see the definition of ActionState to check, rust-analyzer would point me back to my own Action struct. It basically looked like rust-analyzer was confused and/or broke.

Additional information

I have since traced the problem to the fact that WorldQuery derive produces a -State type in addition to -Item and -ReadOnly. This is touched upon very lightly in WorldQuery docs that say:

The implementation also generates two other structs that implement Fetch and FetchState and are used as WorldQuery::Fetch and WorldQuery::State associated types respectively.

but it doesn't explicitly mention their names (which, in addition to FooState, I presume also includes FooFetch).

Suggestion

Any derive macro that introduces new symbols to the module namespace should have all those symbols clearly outlined in the docs, preferably close to the beginning, and possibly as a prominent note/warning.

[DJMcNab]

This appears to be a rehash of #1727 (which incidentally I think was solved by #4100), but for WorldQuery.

[Xion]

This is a bit tougher, I think, because some of the types generated by WorldQuery are useful to reference in custom code (e.g. adding accessors or other helper methods to Foo(ReadOnly)Items). Putting these in an anonymous scope would make it impossible.

An API solution for this (as opposed to just updating docs) would probably require adding a capability to customize the names of all those generated types.e.g.:

#[derive(WorldQuery)]
#[world_query(mutable, state_type_name = "ActionWorldQueryState")]  // item_type_name=, ro_item_type_name="" etc.
struct Action { ... }

I've seen this done by other proc-macro crates, such as derive_builder with its #[builder(name = "...")]. I suspect it's a non-trivial amount of work, though.

[DJMcNab]

There isn't any reason that you should need access to the State types, since every part of their design is implementation detail. Those types should be in an unnameable namespace.

The case of adding accessors to the FooReadOnlyItem/FooItem structs does however seem valid. I'd be tempted to say that if you want to support custom readonly methods, you should have to manually create your own Item $\lor$ ReadOnlyItem structs, so that you have complete control over the struct.

That is, we should avoid creating any new structs in the local namespace under any circumstances, but allow the user's own struct which matches the correct layout to be used if desired.

[BoxyUwU]

What's the point in making users create their own Item type thats exactly identical to the auto generated one, just so that they can do impl MyItem when impl FooItem would work just fine

[DJMcNab]

Broadly, to mitigate the issue that tooling does not like it when item names collide, and fails to provide adequate diagnostics/suggestions when that case does occur. I thought it was therefore unidiomatic to create new items in a derive macro, although the docs I can find don't suggest that.

In addition, a number of derives which may be useful need the specific fields to be marked (e.g. fields on a Serialize for the ReadOnly versions may need the #[serde(rename="...")]). This requires a complexity explosion to handle all the permutations of this.

My point is more broadly that we should not be in the business of choosing names to pollute the local namespace with. My updated position would be that we can have three cases:

1. If nothing is specified, use an item in the implemenation anonymous namespace
2. If specified, create the item in the current scope with the given name
3. Optionally, support using an item with the given name which matches the correct field types at a given path

I would predict that ~75% of cases should use the first form, so polluting the local scope is not a good default.

[BoxyUwU]

I think im on board with those 3 points you listed, ive definitely wanted something like 3 in the past. when gats land Q::Item ought to Just Work instead of QueryItem<Q> so hiding all the item names by default seems alright to me

[james7132]

This seems to be resolved as of #6877. Is there something else that this issue still requires addressing?

[BoxyUwU]

no, that PR doesn't do anything about the State for the worldquery, just the readonly version of the worldquery