forestia/bevy
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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

Browse Source 
# 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>
This commit is contained in:
Joseph 2023-07-25 14:16:22 -07:00 committed by GitHub
parent 774fb56a67
commit 02688a99b8
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 77 additions and 134 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
crates/bevy_ecs/macros/src/fetch.rs
View File

@ -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)*;

104
crates/bevy_ecs/src/query/fetch.rs
View File

@ -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;

46
crates/bevy_ecs/src/query/filter.rs
View File

@ -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,10 +396,10 @@ 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,
@ -414,7 +407,7 @@ macro_rules! impl_tick_filter {
// 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,

40
crates/bevy_ecs/src/query/iter.rs
View File

@ -6,7 +6,7 @@ use crate::{
storage::{TableId, TableRow, Tables},
world::unsafe_world_cell::UnsafeWorldCell,
};
use std::{borrow::Borrow, iter::FusedIterator, marker::PhantomData, mem::MaybeUninit};
use std::{borrow::Borrow, iter::FusedIterator, mem::MaybeUninit};
use super::ReadOnlyWorldQuery;
@ -163,7 +163,9 @@ where
// SAFETY: set_archetype was called prior.
// `location.archetype_row` is an archetype index row in range of the current archetype, because if it was not, the match above would have `continue`d
if F::filter_fetch(&mut self.filter, entity, location.table_row) {
// SAFETY: set_archetype was called prior, `location.archetype_row` is an archetype index in range of the current archetype
// SAFETY:
// - set_archetype was called prior, `location.archetype_row` is an archetype index in range of the current archetype
// - fetch is only called once for each entity.
return Some(Q::fetch(&mut self.fetch, entity, location.table_row));
}
}
@ -344,7 +346,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery, const K: usize>
match self.cursors[i].next(self.tables, self.archetypes, self.query_state) {
Some(_) => {
for j in (i + 1)..K {
self.cursors[j] = self.cursors[j - 1].clone_cursor();
self.cursors[j] = self.cursors[j - 1].clone();
match self.cursors[j].next(self.tables, self.archetypes, self.query_state) {
Some(_) => {}
None if i > 0 => continue 'outer,
@ -453,28 +455,19 @@ struct QueryIterationCursor<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> {
current_len: usize,
// either table row or archetype index, depending on whether both `Q`'s and `F`'s fetches are dense
current_row: usize,
phantom: PhantomData<Q>,
}
impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIterationCursor<'w, 's, Q, F> {
/// This function is safe to call if `(Q, F): 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<Q>` for the same
/// `archetype_row` or `table_row` to be alive at the same time.
unsafe fn clone_cursor(&self) -> Self {
impl<Q: WorldQuery, F: ReadOnlyWorldQuery> Clone for QueryIterationCursor<'_, '_, Q, F> {
fn clone(&self) -> Self {
Self {
table_id_iter: self.table_id_iter.clone(),
archetype_id_iter: self.archetype_id_iter.clone(),
table_entities: self.table_entities,
archetype_entities: self.archetype_entities,
// SAFETY: upheld by caller invariants
fetch: Q::clone_fetch(&self.fetch),
filter: F::clone_fetch(&self.filter),
fetch: self.fetch.clone(),
filter: self.filter.clone(),
current_len: self.current_len,
current_row: self.current_row,
phantom: PhantomData,
}
}
}
@ -515,7 +508,6 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIterationCursor<'w, 's,
archetype_id_iter: query_state.matched_archetype_ids.iter(),
current_len: 0,
current_row: 0,
phantom: PhantomData,
}
}
@ -593,8 +585,11 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIterationCursor<'w, 's,
continue;
}
// SAFETY: set_table was called prior.
// `current_row` is a table row in range of the current table, because if it was not, then the if above would have been executed.
// SAFETY:
// - set_table was called prior.
// - `current_row` must be a table row in range of the current table,
// because if it was not, then the if above would have been executed.
// - fetch is only called once for each `entity`.
let item = Q::fetch(&mut self.fetch, *entity, row);
self.current_row += 1;
@ -633,8 +628,11 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIterationCursor<'w, 's,
continue;
}
// SAFETY: set_archetype was called prior, `current_row` is an archetype index in range of the current archetype
// `current_row` is an archetype index row in range of the current archetype, because if it was not, then the if above would have been executed.
// SAFETY:
// - set_archetype was called prior.
// - `current_row` must be an archetype index row in range of the current archetype,
// because if it was not, then the if above would have been executed.
// - fetch is only called once for each `archetype_entity`.
let item = Q::fetch(
&mut self.fetch,
archetype_entity.entity(),