Fix safety invariants for WorldQuery::fetch and simplify cloning

[JoJoJet]

Objective

Cloning a WorldQuery type's "fetch" struct was made unsafe in #5593, by adding the unsafe fn clone_fetch to WorldQuery. However, as that method's documentation explains, it is not the right place to put the safety invariant:

While calling this method on its own cannot cause UB it is marked unsafe as the caller must ensure that the returned value is not used in any way that would cause two QueryItem<Self> for the same archetype_index or table_row to be alive at the same time.

You can clone a fetch struct all you want and it will never cause undefined behavior -- in order for something to go wrong, you need to improperly call WorldQuery::fetch with it (which is marked unsafe). Additionally, making it unsafe to clone a fetch struct does not even prevent undefined behavior, since there are other ways to incorrectly use a fetch struct. For example, you could just call fetch more than once for the same entity, which is not currently forbidden by any documented invariants.

Solution

Document a safety invariant on WorldQuery::fetch that requires the caller to not create aliased WorldQueryItems for mutable types. Remove the clone_fetch function, and add the bound Fetch: Clone instead.

---

Changelog

• Removed the associated function WorldQuery::clone_fetch, and added a Clone bound to WorldQuery::Fetch.

Migration Guide

fetch invariants

The function WorldQuery::fetch has had the following safety invariant added:

If update_component_access includes any mutable accesses, then the caller must ensure that fetch is called no more than once for each entity/table_row in each archetype.
If Self implements ReadOnlyWorldQuery, then this can safely be called multiple times.

This invariant was always required for soundness, but was previously undocumented. If you called this function manually anywhere, you should check to make sure that this invariant is not violated.

Removed clone_fetch

The function WorldQuery::clone_fetch has been removed. The associated type WorldQuery::Fetch now has the bound Clone.

Before:

struct MyFetch<'w> { ... }

unsafe impl WorldQuery for MyQuery {
    ...
    type Fetch<'w> = MyFetch<'w>
    unsafe fn clone_fetch<'w>(fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {
        MyFetch {
            field1: fetch.field1,
            field2: fetch.field2.clone(),
            ...
        }
    }
}

After:

#[derive(Clone)]
struct MyFetch<'w> { ... }

unsafe impl WorldQuery for MyQuery {
    ...
    type Fetch<'w> = MyFetch<'w>;
}

[alice-i-cecile]

Good change: these safety invariants are clearer and more correct. Code simplification is a nice win too.

[jakobhellermann]

The code is a lot cleaner, and the safety requirement seems better placed as well.

I'd like to tweak the wording a bit, but otherwise this looks good to me.

[jakobhellermann]

I'm not a fan of the wording here, because it sounds like only the Entity needs to be unique.

Maybe something like "the caller must ensure that fetch isn't called with the same entity or table_row, which could lead to unsound aliasing of the Item"?
I would like something shorter but I can't think of anything right now.

[JoJoJet]

I updated the wording, do you prefer it now?

[cart]

Just did a review. I'm on board, but something weird is going on with this branch. I can't do a merge locally because the branches have "unrelated histories", nor can I rebase without getting a ton of conflicts from files untouched by this PR.

Git has a way of overriding this for merges (--allow-unrelated-histories) but thats the kind of thing that makes me uncomfortable.

A couple of questions for @JoJoJet :

1. Can you think of any reasons for why that would happen?
2. Can you re-apply these changes somehow on a valid branch from main?

[cart]

Moving to 0.12 as we've run out of time and this branch needs fixing. This is good to fix, but its not a pressing issue.

[JoJoJet]

Not sure what the problem was, but it seems to be resolved now.