Fix safety invariants for WorldQuery::fetch and simplify cloning (#…

…8246)

# 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 `WorldQueryItem`s 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 this type does not implement `ReadOnlyWorldQuery`, then the caller
must ensure that it is impossible for more than one `Self::Item` to
exist for the same entity at any given time.

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:

```rust
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:

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

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

---------

Co-authored-by: Alice Cecile <[email protected]>

crates/bevy_ecs/macros/src/fetch.rs

@@ -237,6 +237,16 @@ pub fn derive_world_query_impl(input: TokenStream) -> TokenStream {
                 #marker_name: &'__w (),
             }
 
+            impl #user_impl_generics_with_world Clone for #fetch_struct_name #user_ty_generics_with_world
+            #user_where_clauses_with_world {
+                fn clone(&self) -> Self {
+                    Self {
+                        #(#named_field_idents: self.#named_field_idents.clone(),)*
+                        #marker_name: &(),
+                    }
+                }
+            }
+
             // SAFETY: `update_component_access` and `update_archetype_component_access` are called on every field
             unsafe impl #user_impl_generics #path::query::WorldQuery
                 for #struct_name #user_ty_generics #user_where_clauses {
@@ -275,17 +285,6 @@ pub fn derive_world_query_impl(input: TokenStream) -> TokenStream {
                     }
                 }
 
-                unsafe fn clone_fetch<'__w>(
-                    _fetch: &<Self as #path::query::WorldQuery>::Fetch<'__w>
-                ) -> <Self as #path::query::WorldQuery>::Fetch<'__w> {
-                    #fetch_struct_name {
-                        #(
-                            #named_field_idents: <#field_types>::clone_fetch(& _fetch. #named_field_idents),
-                        )*
-                        #marker_name: &(),
-                    }
-                }
-
                 const IS_DENSE: bool = true #(&& <#field_types>::IS_DENSE)*;
 
                 const IS_ARCHETYPAL: bool = true #(&& <#field_types>::IS_ARCHETYPAL)*;

crates/bevy_ecs/src/query/fetch.rs

@@ -318,7 +318,7 @@ pub unsafe trait WorldQuery {
     type Item<'a>;
 
     /// Per archetype/table state used by this [`WorldQuery`] to fetch [`Self::Item`](crate::query::WorldQuery::Item)
-    type Fetch<'a>;
+    type Fetch<'a>: Clone;
 
     /// The read-only variant of this [`WorldQuery`], which satisfies the [`ReadOnlyWorldQuery`] trait.
     type ReadOnly: ReadOnlyWorldQuery<State = Self::State>;
@@ -345,14 +345,6 @@ pub unsafe trait WorldQuery {
         this_run: Tick,
     ) -> Self::Fetch<'w>;
 
-    /// While this function can be called for any query, it is always safe to call if `Self: ReadOnlyWorldQuery` holds.
-    ///
-    /// # Safety
-    /// 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_row` or `table_row` to be alive at the same time.
-    unsafe fn clone_fetch<'w>(fetch: &Self::Fetch<'w>) -> Self::Fetch<'w>;
-
     /// Returns true if (and only if) every table of every archetype matched by this fetch contains
     /// all of the matched components. This is used to select a more efficient "table iterator"
     /// for "dense" queries. If this returns true, [`WorldQuery::set_table`] must be used before
@@ -404,6 +396,10 @@ pub unsafe trait WorldQuery {
     ///
     /// Must always be called _after_ [`WorldQuery::set_table`] or [`WorldQuery::set_archetype`]. `entity` and
     /// `table_row` must be in the range of the current table and archetype.
+    ///
+    /// 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.
     unsafe fn fetch<'w>(
         fetch: &mut Self::Fetch<'w>,
         entity: Entity,
@@ -483,8 +479,6 @@ unsafe impl WorldQuery for Entity {
     ) -> Self::Fetch<'w> {
     }
 
-    unsafe fn clone_fetch<'w>(_fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {}
-
     #[inline]
     unsafe fn set_archetype<'w>(
         _fetch: &mut Self::Fetch<'w>,
@@ -554,10 +548,6 @@ unsafe impl<'a> WorldQuery for EntityRef<'a> {
         world.world()
     }
 
-    unsafe fn clone_fetch<'w>(world: &Self::Fetch<'w>) -> Self::Fetch<'w> {
-        world
-    }
-
     #[inline]
     unsafe fn set_archetype<'w>(
         _fetch: &mut Self::Fetch<'w>,
@@ -620,6 +610,13 @@ pub struct ReadFetch<'w, T> {
     sparse_set: Option<&'w ComponentSparseSet>,
 }
 
+impl<T> Clone for ReadFetch<'_, T> {
+    fn clone(&self) -> Self {
+        *self
+    }
+}
+impl<T> Copy for ReadFetch<'_, T> {}
+
 /// SAFETY: `Self` is the same as `Self::ReadOnly`
 unsafe impl<T: Component> WorldQuery for &T {
     type Fetch<'w> = ReadFetch<'w, T>;
@@ -663,13 +660,6 @@ unsafe impl<T: Component> WorldQuery for &T {
         }
     }
 
-    unsafe fn clone_fetch<'w>(fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {
-        ReadFetch {
-            table_components: fetch.table_components,
-            sparse_set: fetch.sparse_set,
-        }
-    }
-
     #[inline]
     unsafe fn set_archetype<'w>(
         fetch: &mut ReadFetch<'w, T>,
@@ -770,6 +760,13 @@ pub struct RefFetch<'w, T> {
     this_run: Tick,
 }
 
+impl<T> Clone for RefFetch<'_, T> {
+    fn clone(&self) -> Self {
+        *self
+    }
+}
+impl<T> Copy for RefFetch<'_, T> {}
+
 /// SAFETY: `Self` is the same as `Self::ReadOnly`
 unsafe impl<'__w, T: Component> WorldQuery for Ref<'__w, T> {
     type Fetch<'w> = RefFetch<'w, T>;
@@ -812,15 +809,6 @@ unsafe impl<'__w, T: Component> WorldQuery for Ref<'__w, T> {
         }
     }
 
-    unsafe fn clone_fetch<'w>(fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {
-        RefFetch {
-            table_data: fetch.table_data,
-            sparse_set: fetch.sparse_set,
-            last_run: fetch.last_run,
-            this_run: fetch.this_run,
-        }
-    }
-
     #[inline]
     unsafe fn set_archetype<'w>(
         fetch: &mut RefFetch<'w, T>,
@@ -933,6 +921,13 @@ pub struct WriteFetch<'w, T> {
     this_run: Tick,
 }
 
+impl<T> Clone for WriteFetch<'_, T> {
+    fn clone(&self) -> Self {
+        *self
+    }
+}
+impl<T> Copy for WriteFetch<'_, T> {}
+
 /// SAFETY: access of `&T` is a subset of `&mut T`
 unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T {
     type Fetch<'w> = WriteFetch<'w, T>;
@@ -975,15 +970,6 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T {
         }
     }
 
-    unsafe fn clone_fetch<'w>(fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {
-        WriteFetch {
-            table_data: fetch.table_data,
-            sparse_set: fetch.sparse_set,
-            last_run: fetch.last_run,
-            this_run: fetch.this_run,
-        }
-    }
-
     #[inline]
     unsafe fn set_archetype<'w>(
         fetch: &mut WriteFetch<'w, T>,
@@ -1084,6 +1070,15 @@ pub struct OptionFetch<'w, T: WorldQuery> {
     matches: bool,
 }
 
+impl<T: WorldQuery> Clone for OptionFetch<'_, T> {
+    fn clone(&self) -> Self {
+        Self {
+            fetch: self.fetch.clone(),
+            matches: self.matches,
+        }
+    }
+}
+
 // SAFETY: defers to soundness of `T: WorldQuery` impl
 unsafe impl<T: WorldQuery> WorldQuery for Option<T> {
     type Fetch<'w> = OptionFetch<'w, T>;
@@ -1112,13 +1107,6 @@ unsafe impl<T: WorldQuery> WorldQuery for Option<T> {
         }
     }
 
-    unsafe fn clone_fetch<'w>(fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {
-        OptionFetch {
-            fetch: T::clone_fetch(&fetch.fetch),
-            matches: fetch.matches,
-        }
-    }
-
     #[inline]
     unsafe fn set_archetype<'w>(
         fetch: &mut OptionFetch<'w, T>,
@@ -1271,10 +1259,6 @@ unsafe impl<T: Component> WorldQuery for Has<T> {
         false
     }
 
-    unsafe fn clone_fetch<'w>(fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {
-        *fetch
-    }
-
     #[inline]
     unsafe fn set_archetype<'w>(
         fetch: &mut Self::Fetch<'w>,
@@ -1351,13 +1335,6 @@ macro_rules! impl_tuple_fetch {
                 ($($name::init_fetch(_world, $name, _last_run, _this_run),)*)
             }
 
-            unsafe fn clone_fetch<'w>(
-                fetch: &Self::Fetch<'w>,
-            ) -> Self::Fetch<'w> {
-                let ($($name,)*) = &fetch;
-                ($($name::clone_fetch($name),)*)
-            }
-
             const IS_DENSE: bool = true $(&& $name::IS_DENSE)*;
 
             const IS_ARCHETYPAL: bool = true $(&& $name::IS_ARCHETYPAL)*;
@@ -1461,13 +1438,6 @@ macro_rules! impl_anytuple_fetch {
                 ($(($name::init_fetch(_world, $name, _last_run, _this_run), false),)*)
             }
 
-            unsafe fn clone_fetch<'w>(
-                fetch: &Self::Fetch<'w>,
-            ) -> Self::Fetch<'w> {
-                let ($($name,)*) = &fetch;
-                ($(($name::clone_fetch(& $name.0), $name.1),)*)
-            }
-
             const IS_DENSE: bool = true $(&& $name::IS_DENSE)*;
 
             const IS_ARCHETYPAL: bool = true $(&& $name::IS_ARCHETYPAL)*;
@@ -1588,8 +1558,6 @@ unsafe impl<Q: WorldQuery> WorldQuery for NopWorldQuery<Q> {
     ) {
     }
 
-    unsafe fn clone_fetch<'w>(_fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {}
-
     #[inline(always)]
     unsafe fn set_archetype(
         _fetch: &mut (),
@@ -1651,8 +1619,6 @@ unsafe impl<T: ?Sized> WorldQuery for PhantomData<T> {
     ) -> Self::Fetch<'w> {
     }
 
-    unsafe fn clone_fetch<'w>(_fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {}
-
     // `PhantomData` does not match any components, so all components it matches
     // are stored in a Table (vacuous truth).
     const IS_DENSE: bool = true;

crates/bevy_ecs/src/query/filter.rs

@@ -59,8 +59,6 @@ unsafe impl<T: Component> WorldQuery for With<T> {
     ) {
     }
 
-    unsafe fn clone_fetch<'w>(_fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {}
-
     const IS_DENSE: bool = {
         match T::Storage::STORAGE_TYPE {
             StorageType::Table => true,
@@ -162,8 +160,6 @@ unsafe impl<T: Component> WorldQuery for Without<T> {
     ) {
     }
 
-    unsafe fn clone_fetch<'w>(_fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {}
-
     const IS_DENSE: bool = {
         match T::Storage::STORAGE_TYPE {
             StorageType::Table => true,
@@ -259,6 +255,15 @@ pub struct OrFetch<'w, T: WorldQuery> {
     matches: bool,
 }
 
+impl<T: WorldQuery> Clone for OrFetch<'_, T> {
+    fn clone(&self) -> Self {
+        Self {
+            fetch: self.fetch.clone(),
+            matches: self.matches,
+        }
+    }
+}
+
 macro_rules! impl_query_filter_tuple {
     ($(($filter: ident, $state: ident)),*) => {
         #[allow(unused_variables)]
@@ -288,18 +293,6 @@ macro_rules! impl_query_filter_tuple {
                 },)*)
             }
 
-            unsafe fn clone_fetch<'w>(
-                fetch: &Self::Fetch<'w>,
-            ) -> Self::Fetch<'w> {
-                let ($($filter,)*) = &fetch;
-                ($(
-                    OrFetch {
-                        fetch: $filter::clone_fetch(&$filter.fetch),
-                        matches: $filter.matches,
-                    },
-                )*)
-            }
-
             #[inline]
             unsafe fn set_table<'w>(fetch: &mut Self::Fetch<'w>, state: &Self::State, table: &'w Table) {
                 let ($($filter,)*) = fetch;
@@ -403,18 +396,18 @@ macro_rules! impl_tick_filter {
         pub struct $name<T>(PhantomData<T>);
 
         #[doc(hidden)]
+        #[derive(Clone)]
         $(#[$fetch_meta])*
-        pub struct $fetch_name<'w, T> {
-            table_ticks: Option< ThinSlicePtr<'w, UnsafeCell<Tick>>>,
-            marker: PhantomData<T>,
+        pub struct $fetch_name<'w> {
+            table_ticks: Option<ThinSlicePtr<'w, UnsafeCell<Tick>>>,
             sparse_set: Option<&'w ComponentSparseSet>,
             last_run: Tick,
             this_run: Tick,
         }
 
         // SAFETY: `Self::ReadOnly` is the same as `Self`
         unsafe impl<T: Component> WorldQuery for $name<T> {
-            type Fetch<'w> = $fetch_name<'w, T>;
+            type Fetch<'w> = $fetch_name<'w>;
             type Item<'w> = bool;
             type ReadOnly = Self;
             type State = ComponentId;
@@ -440,24 +433,11 @@ macro_rules! impl_tick_filter {
                                  .get(id)
                                  .debug_checked_unwrap()
                         }),
-                    marker: PhantomData,
                     last_run,
                     this_run,
                 }
             }
 
-            unsafe fn clone_fetch<'w>(
-                fetch: &Self::Fetch<'w>,
-            ) -> Self::Fetch<'w> {
-                $fetch_name {
-                    table_ticks: fetch.table_ticks,
-                    sparse_set: fetch.sparse_set,
-                    last_run: fetch.last_run,
-                    this_run: fetch.this_run,
-                    marker: PhantomData,
-                }
-            }
-
             const IS_DENSE: bool = {
                 match T::Storage::STORAGE_TYPE {
                     StorageType::Table => true,