Fix unsound query transmutes on queries obtained from Query::as_readonly()

[chescock]

Objective

Fix unsound query transmutes on queries obtained from Query::as_readonly().

The following compiles, and the call to transmute_lens() should panic, but does not:

fn bad_system(query: Query<&mut A>) {
    let mut readonly = query.as_readonly();
    let mut lens: QueryLens<&mut A> = readonly.transmute_lens();
    let other_readonly: Query<&A> = query.as_readonly();
    // `lens` and `other_readonly` alias, and are both alive here!
}

To make Query::as_readonly() zero-cost, we pointer-cast &QueryState<D, F> to &QueryState<D::ReadOnly, F>. This means that the component_access for a read-only query's state may include accesses for the original mutable version, but the Query does not have exclusive access to those components! transmute and join use that access to ensure that a join is valid, and will incorrectly allow a transmute that includes mutable access.

As a bonus, allow Query::joins that output FilteredEntityRef or FilteredEntityMut to receive access from the other query. Currently they only receive access from self.

Solution

When transmuting or joining from a read-only query, remove any writes before performing checking that the transmute is valid. For joins, be sure to handle the case where one input query was the result of as_readonly() but the other has valid mutable access.

This requires identifying read-only queries, so add a QueryData::IS_READ_ONLY associated constant. Note that we only call QueryState::as_transmuted_state() with NewD: ReadOnlyQueryData, so checking for read-only queries is sufficient to check for as_transmuted_state().

Removing writes requires allocating a new FilteredAccess, so only do so if the query is read-only and the state has writes. Otherwise, the existing access is correct and we can continue using a reference to it.

Use the new read-only state to call NewD::set_access, so that transmuting to a FilteredAccessMut results in a read-only FilteredAccessMut. Otherwise, it would take the original write access, and then the transmute would panic because it had too much access.

Note that join was previously passing self.component_access to NewD::set_access. Switching it to joined_component_access also allows a join that outputs FilteredEntity(Ref|Mut) to receive access from other. The fact that it didn't do that before seems like an oversight, so I didn't try to prevent that change.

Testing

Added unit tests with the unsound transmute and join.

[andriyDev]

Why do we restrict this to ReadOnlyQueryData?

[chescock]

After I wrote a safety comment that said "The current state was transmuted from a mutable QueryData to a read-only one", I realized that as_transmuted_state() could technically transmute to mutable queries, as well. So I added the bound to prevent that.

The restriction isn't necessary for soundness, since we have to check that when constructing a new Query with the transmuted state. But it would be hard to use this method safely with mutable QueryData, and it didn't seem costly to restrict the API of a pub(crate) method.

I'm happy to revert this line if anyone feels strongly that it shouldn't change!

[andriyDev]

I suppose if this is a problem in the future, we can deal with it then! So I'm fine with this.

[andriyDev]

Unnecessary line?

[chescock]

That's the local variable that holds the clone() if we need to make one. It's only initialized in the if, but it needs to be declared outside the if so that it lives long enough to be borrowed from. And Rust will handle conditionally dropping it!

[andriyDev]

I think we can do this with a little weirdness. I drafted it up in the playground: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=369649ab4f7a305b17055a0fae83fa5a

I personally think this is preferable since it means we get rid of this rather weird interaction. Generally you need to initialize the variable in both branches - not doing so makes me deeply uncomfortable haha. Also the shadowing seems like a mistake. At least with the internal block it's clear that it's "localized".

[chescock]

Oh, using lifetime extension like that is really clever! The &{ part is a little odd, but I can extract the body into a local function to avoid that. I'll make that change!

[andriyDev]

Ah even better!

[alice-i-cecile]

Previously: #5866