Tracking: bevy-trait-query breakage after changes to WorldQuery API

[RobWalt]

The issue

At some point (#13442, #13343) the WorldQuery API was changed from this ... to this. Most notably, the function signature of init_state and get_state changed which didn't cause any direct problems in the bevy repo itself.

However, it seems like at least one downstream crate, bevy-trait-query, depends on the previous version of this API to access a vital building block: The TraitImplRegistry resource. This was previously possible through the &mut World argument and now isn't possible anymore. It also doesn't appear to be easily fixable with the current API.

Alice also mentioned that things like indexed queries wouldn't be possible anymore without access to resources.

[alice-i-cecile]

@JoJoJet @hymm @james7132 @Victoronz @james-j-obrien: bringing this to your attention.

We should figure out how to address this without reintroducing the UB (or potential for similar UB).

[hymm]

The simplest fix would be to revert the init_state change, but keep the get_state change. I think that makes sense since one is immutable, but the other is mutable. So we can be more permissive in init_state.

[Victoronz]

That won't fix this issue. The breakage here is that retrieving TraitImplRegistry inside of get_state is no longer possible, as this would create a reference to World without an access check.

[hymm]

Can we pass an UnsafeWorldCell to get_state? I feel like there was some blocker to that, but I can't remember for sure.

edit: This would allow bevy_trait_query to register a read on the resource it needs. So that the system param does have access to get the resource from the UnsafeWorldCell. Maybe? Can QueryData register a read on a resource?

[alice-i-cecile]

[9:49 PM]WrongShoe: I don't think this is safe. What if you transmute a trait query and have a ResMut in the system?
[9:50 PM]WrongShoe: That would have a mutable reference and a shared referece to the reource live at the same time.
[1:41 AM]Giooschi: Well, from the point of view of bevy-trait-query it gets a &World so it's perfectly safe to access that resource.
[1:53 AM]WrongShoe: It's ub, so our api's are incorrect.

[Victoronz]

Passing an UnsafeWorldCell to get_state would simply allow this UB again, not solve it either :/
I'd suggest seeing if there is a workaround that doesn't use the init_state/get_state machinery, or goes via ids only

[hymm]

It wouldn't be UB as long as you use the unsafe methods on UnsafeWorldCell to access the resource and the read on the resource is correctly registered.

[alice-i-cecile]

Is it possible to bubble up the read on the resource to the containing system properly? If so, I think passing in an additional UnsafeWorldCell with a doc test demonstrating the pattern is a good solution.

[Victoronz]

While technically possible, how does a user or library author ensure the safety here? If a different system wishes to access a parameter mutably, then it is now up to the user to schedule this out of each others way.

This would mean we say, "If you wish to transmute_lens, join, or sort a query iterator, make sure not to use these components mutably in other systems with the potential to run at the same time"

[Victoronz]

I wonder, could a third *_state method be added that is explicitly unsafe? This would make more sense than changing the safe methods for this IMO

[hymm]

get_state can be safe with passing UnsafeWorldCell. It's just that the implementor has to prove that any unsafe calls are safe inside the body of the implementation.

[Victoronz]

There currently is no API to retrieve World data without references, so passing UnsafeWorldCell to get_state does not resolve the aliasing problem, it'd be up to the user.

[hymm]

Is it possible to bubble up the read on the resource to the containing system properly?

Hmm. So I tried to change to get_state(world_cell: UnsafeWorldCell, but it wasn't possible to make this work without other changes. What we seem to need is a side way to register the resource access in QueryState::new_unitialized that doesn't effect the fetch_state. We could add a register_resource_access(world: &mut World, &mut FilteredAccess<ComponentId>) to WorldQuery that is called in new_unitialized. This would allow the registry resource to register it's read access.

The other option is to just revert the init_state change and have bevy_trait_query return none when get_state is called. This would make it panic when transmute_lens is called.

I think someone should probably make the pr to revert the init_state change, since that would be needed for either approach.

[alice-i-cecile]

This has been reverted: I'm going to close this and continue the discussion in #13358

[Azorlogh]

@alice-i-cecile Wasn't this revert only one part of the issue? The revert provided a workaround for WorldQuery::init_state, but it didn't address the part about WorldQuery::get_state not being able to read a resource. At the moment the best we can do in bevy-trait-query is return None in WorldQuery::get_state which breaks transmute_lens