Mutable NestedQuery by relaxing ReadOnly::State == State constraint

[chescock]

Objective

Support mutable queries using NestedQuery.

The reason this was not supported in the initial version is that we want NestedQuery<D, F>::ReadOnly == NestedQuery<D::ReadOnly, F>, but that doesn't satisfy the ReadOnly::State == State constraint since QueryState<D::ReadOnly, F> != QueryState<D, F>.

Solution

Remove the ReadOnly::State == State constraint, and replace it with a safety requirement. QueryState<D, F> could always be transmuted to QueryState<D::ReadOnly, F>, so the outer transmute is still valid.

Note that the type constraint was merely a lint here. It was never enough for the states to be the same type, they also had to have compatible values! For example, <&T>::State == ComponentId, but any value other than the ComponentId of T would be unsound. Make this requirement explicit, and update the safety comments of mutable query types to show that they satisfy it.

See also #25642 for an alternative approach.

[tim-blackbird]

This is lovely, much preferred over the state transmute hokey pokey I was doing in #25642 :)

+1 on the removal of the ReadOnly::State == State constraint. This bound prevents some misuse when dealing with D::State directly, but it cannot help prove that e.g. QueryState<D> is valid for QueryState<D::ReadOnly>. As you mentioned, it doesn't help at all with correctness of the value.

I'm liking the changes to the safety comments a lot!

[alice-i-cecile]

Extremely clear and helpful!

[alice-i-cecile]

Nit: I think these safety comments would be clearer if they explained that FilteredEntityRef etc have maximal access, and so any transmutation is necessarily a non-strict subset.

[alice-i-cecile]

Suggested change

	/// and the resulting `Self::ReadOnly` must have a subset of the access of `Self`
	/// and the resulting `Self::ReadOnly` must have a non-strict subset of the access of `Self`

[alice-i-cecile]

Much simpler.

[alice-i-cecile]

This change is a meaningful change to our safety invariants here, and important to any manual implementation, but any impl which met this also meets the new variants.

Is this worth putting in a migration guide? Like, on the one hand this is non-breaking, but on the other hand I would certainly want to be alerted to this so I can update my safety contract.

[Diddykonga]

Rust is working towards Safety Contracts as Types https://internals.rust-lang.org/t/pre-rfc-unsafe-reasons/22093/1
Which means in the future, such a thing would indeed be a breaking change, so perhaps we make it idiom early?

[alice-i-cecile]

I'm basically good to approve this, but I have a couple of nits to discuss :) Very cool to see this lifted.

[alice-i-cecile]

Two other notes caught by AI-augmented review:

1. The QueryData derive fails for mutable nested queries. Important to fix eventually obviously, but not critical to do now.
2. There's stale doc comments saying that this doesn't work for mutable queries (line 2815 and 2960 in fetch.rs).

[chescock]

1. The QueryData derive fails for mutable nested queries. Important to fix eventually obviously, but not critical to do now.

Argh, when looking into how this was different from tuples, I managed to convince myself that the tuple implementations are unsound and we can't take this approach :(.

The problem is that if we have (D1, NestedQuery<D2>), then we're asserting that (D1::State, QueryState<D2>) can be transmuted to (D1::State, QueryState<D2::ReadOnly>). But tuples are #[repr(rust)], so even if the QueryState part is valid the tuples themselves can't be transmuted!

So I think we need to abandon this approach :(. The simplest alternative is the one in #25642, where we store a QueryState<D, F> by transmuting it to QueryState<D::ReadOnly, F> and back.

(The other approach I've come up with is

#[reprt(transparent)]
struct QueryState<D: QueryData, F: QueryFilter>(QueryStateInner<D::State, F::State>);

And then using QueryStateInner in NestedQuery::State. That has the advantage of giving a better layout for QueryState by avoiding #[repr(c)] and permitting field reordering, but the safety justifications are just as complex and we'd need to add .0 everywhere.)

I'll push the doc changes I made in response to the other comments, since we'll want the changes to the NestedQuery docs regardless of the approach, and we may still want a PR to clarify the safety requirement that the State values are valid for ReadOnly::State.

@tim-blackbird Would you like to re-open #25642 and get a bunch of suggestions for documentation changes, or would you prefer that I adopt the PR?

[tim-blackbird]

Sorry to hear this didn't work out. I would like it if you could adopt #25642 for me :)

[chescock]

Closing in favor of #25809