constrain WorldQuery::get_state to only use &Components

[Victoronz]

Objective

Passing &World in the WorldQuery::get_state method is unnecessary, as all implementations of this method in the engine either only access Components in &World, or do nothing with it.
It can introduce UB by necessitating the creation of a &World from a UnsafeWorldCell.
This currently happens in Query::transmute_lens, which obtains a &World from the internal UnsafeWorldCell solely to pass to get_state. Query::join suffers from the same issue.
Other cases of UB come from allowing implementors of WorldQuery to freely access &World, like in the bevy-trait-query crate, where a reference to a resource is obtained inside of get_state, potentially aliasing with a ResMut parameter in the same system.

WorldQuery::init_state currently requires &mut World, which doesn't suffer from these issues.
But that too can be changed to receive a wrapper around &mut Components and &mut Storages for consistency in a follow-up PR.

Solution

Replace the &World parameter in get_state with &Components.

Changelog

WorldQuery::get_state now takes &Components instead of &World.
The transmute, transmute_filtered, join and join_filtered methods on QueryState now similarly take &Components instead of &World.

Migration Guide

Users of WorldQuery::get_state or transmute, transmute_filtered, join and join_filtered methods on QueryState now need to pass &Components instead of &World.
&Components can be trivially obtained from either components method on &World or UnsafeWorldCell.
For implementors of WorldQuery::get_state that were accessing more than the Components inside &World and its methods, this is no longer allowed.

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[james7132]

In terms of trait design, I don't think this is appropriate, given that init_state, the infallible version of get_state requires a &mut World, there may be WorldQuery implementations that may read other state in the world (i.e. resources, other metadata, etc).

If we were to seal WorldQuery so that it cannot be externally implemented, this change may make more sense, and it does seem like something that @maniwani was looking into for ABI compatibility. However, without those changes, I don't think this is a limitation we should be imposing without also limiting the scope of init_state.

[Victoronz]

That is true, init_state should mirror get_state. Since the only use the engine ever has for the &mut World argument is to call init_component on it, the &mut World argument is replaceable by &mut Components and &mut Storages together (the arguments for the Components::init_component method that World::init_components wraps).

I added this change in the second commit.

[Victoronz]

I realized that because Storages and Components aren't public fields on World, and there are no mutable accessor functions, doing the same change likely doesn't work for init_state.
Adding mutable accessor function does not make sense, because this would allow users to readily cause UB.
Instead we could have a function on &mut World that returns an opaque struct on which init_component, ... can be called.

[Victoronz]

Since the get_state change is a UB fix and init_state is for consistency, I'll remove the init_state change again and leave that for a follow-up PR.

[alice-i-cecile]

I agree with this as an immediately actionable step to address UB. The restriction on get_state fundamentally makes sense to me: queries should generally not be accessing non-component data. I can see a world where additional data (like indexes) are requested, but I would rather lock this down now and then add more careful access.

init_state should be changed to match, but that doesn't have to happen right now, and frankly probably shouldn't, given that it will require more careful design thought.

[james7132]

OK, deferring to a later PR SGTM, so long as both make it into 0.14.