bevy/crates/bevy_ecs/macros
Joseph 02688a99b8
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 <alice.i.cecile@gmail.com>
2023-07-25 21:16:22 +00:00
..
src Fix safety invariants for WorldQuery::fetch and simplify cloning (#8246) 2023-07-25 21:16:22 +00:00
Cargo.toml Bump Version after Release (#9106) 2023-07-10 21:19:27 +00:00