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

Avoid making Fetchs Clone (#5593)

Browse Source 
# Objective

- Do not implement `Copy` or `Clone` for `Fetch` types as this is kind of sus soundness wise (it feels like cloning an `IterMut` in safe code to me). Cloning a fetch seems important to think about soundness wise when doing it so I prefer this over adding a `Clone` bound to the assoc type definition (i.e. `type Fetch: Clone`) even though that would also solve the other listed things here.
- Remove a bunch of `QueryFetch<'w, Q>: Clone` bounds from our API as now all fetches can be "cloned" for use in `iter_combinations`. This should also help avoid the type inference regression ptrification introduced where `for<'a> QueryFetch<'a, Q>: Trait` bounds misbehave since we no longer need any of those kind of higher ranked bounds (although in practice we had none anyway).
- Stop being able to "forget" to implement clone for fetches, we've had a lot of issues where either `derive(Clone)` was used instead of a manual impl (so we ended up with too tight bounds on the impl) or flat out forgot to implement Clone at all. With this change all fetches are able to be cloned for `iter_combinations` so this will no longer be possible to mess up.

On an unrelated note, while making this PR I realised we probably want safety invariants on `archetype/table_fetch` that nothing aliases the table_row/archetype_index according to the access we set.

---

## Changelog

`Clone` and `Copy` were removed from all `Fetch` types.

## Migration Guide

- Call `WorldQuery::clone_fetch` instead of `fetch.clone()`. Make sure to add safety comments :)
This commit is contained in:
Boxy 2022-10-26 13:16:25 +00:00
parent 5622d56be1
commit 54cf45c5b3
5 changed files with 161 additions and 115 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -186,7 +186,6 @@ pub fn derive_world_query_impl(ast: DeriveInput) -> TokenStream {
#(#(#ignored_field_attrs)* #ignored_field_visibilities #ignored_field_idents: #ignored_field_types,)*
}
#[derive(Clone)]
#[doc(hidden)]
#visibility struct #fetch_struct_name #user_impl_generics_with_world #user_where_clauses_with_world {
#(#field_idents: <#field_types as #path::query::WorldQueryGats<'__w>>::Fetch,)*
@ -239,6 +238,19 @@ pub fn derive_world_query_impl(ast: DeriveInput) -> TokenStream {
}
}
unsafe fn clone_fetch<'__w>(
_fetch: &<Self as #path::query::WorldQueryGats<'__w>>::Fetch
) -> <Self as #path::query::WorldQueryGats<'__w>>::Fetch {
#fetch_struct_name {
#(
#field_idents: <#field_types>::clone_fetch(& _fetch. #field_idents),
)*
#(
#ignored_field_idents: Default::default(),
)*
}
}
const IS_DENSE: bool = true #(&& <#field_types>::IS_DENSE)*;
const IS_ARCHETYPAL: bool = true #(&& <#field_types>::IS_ARCHETYPAL)*;

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

@ -327,6 +327,16 @@ pub unsafe trait WorldQuery: for<'w> WorldQueryGats<'w> {
change_tick: u32,
) -> <Self as WorldQueryGats<'w>>::Fetch;
/// 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_index` or `table_row` to be alive at the same time.
unsafe fn clone_fetch<'w>(
fetch: &<Self as WorldQueryGats<'w>>::Fetch,
) -> <Self as WorldQueryGats<'w>>::Fetch;
/// 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`] and [`WorldQuery::table_fetch`]
@ -461,7 +471,6 @@ pub type ROQueryFetch<'w, Q> = QueryFetch<'w, <Q as WorldQuery>::ReadOnly>;
pub type ROQueryItem<'w, Q> = QueryItem<'w, <Q as WorldQuery>::ReadOnly>;
#[doc(hidden)]
#[derive(Clone)]
pub struct EntityFetch<'w> {
entities: Option<ThinSlicePtr<'w, Entity>>,
}
@ -488,6 +497,14 @@ unsafe impl WorldQuery for Entity {
EntityFetch { entities: None }
}
unsafe fn clone_fetch<'w>(
fetch: &<Self as WorldQueryGats<'w>>::Fetch,
) -> <Self as WorldQueryGats<'w>>::Fetch {
EntityFetch {
entities: fetch.entities,
}
}
#[inline]
unsafe fn set_archetype<'w>(
fetch: &mut EntityFetch<'w>,
@ -595,6 +612,17 @@ unsafe impl<T: Component> WorldQuery for &T {
}
}
unsafe fn clone_fetch<'w>(
fetch: &<Self as WorldQueryGats<'w>>::Fetch,
) -> <Self as WorldQueryGats<'w>>::Fetch {
ReadFetch {
table_components: fetch.table_components,
entity_table_rows: fetch.entity_table_rows,
entities: fetch.entities,
sparse_set: fetch.sparse_set,
}
}
#[inline]
unsafe fn set_archetype<'w>(
fetch: &mut ReadFetch<'w, T>,
@ -692,17 +720,6 @@ unsafe impl<T: Component> WorldQuery for &T {
}
}
impl<T> Clone for ReadFetch<'_, T> {
fn clone(&self) -> Self {
Self {
table_components: self.table_components,
entity_table_rows: self.entity_table_rows,
entities: self.entities,
sparse_set: self.sparse_set,
}
}
}
/// SAFETY: access is read only
unsafe impl<T: Component> ReadOnlyWorldQuery for &T {}
@ -761,6 +778,20 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T {
}
}
unsafe fn clone_fetch<'w>(
fetch: &<Self as WorldQueryGats<'w>>::Fetch,
) -> <Self as WorldQueryGats<'w>>::Fetch {
WriteFetch {
table_components: fetch.table_components,
table_ticks: fetch.table_ticks,
entities: fetch.entities,
entity_table_rows: fetch.entity_table_rows,
sparse_set: fetch.sparse_set,
last_change_tick: fetch.last_change_tick,
change_tick: fetch.change_tick,
}
}
#[inline]
unsafe fn set_archetype<'w>(
fetch: &mut WriteFetch<'w, T>,
@ -887,20 +918,6 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T {
}
}
impl<T> Clone for WriteFetch<'_, T> {
fn clone(&self) -> Self {
Self {
table_components: self.table_components,
table_ticks: self.table_ticks,
entities: self.entities,
entity_table_rows: self.entity_table_rows,
sparse_set: self.sparse_set,
last_change_tick: self.last_change_tick,
change_tick: self.change_tick,
}
}
}
impl<'w, T: Component> WorldQueryGats<'w> for &mut T {
type Fetch = WriteFetch<'w, T>;
type Item = Mut<'w, T>;
@ -911,17 +928,6 @@ pub struct OptionFetch<'w, T: WorldQuery> {
fetch: <T as WorldQueryGats<'w>>::Fetch,
matches: bool,
}
impl<'w, T: WorldQuery> Clone for OptionFetch<'w, T>
where
<T as WorldQueryGats<'w>>::Fetch: Clone,
{
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> {
@ -948,6 +954,15 @@ unsafe impl<T: WorldQuery> WorldQuery for Option<T> {
}
}
unsafe fn clone_fetch<'w>(
fetch: &<Self as WorldQueryGats<'w>>::Fetch,
) -> <Self as WorldQueryGats<'w>>::Fetch {
OptionFetch {
fetch: T::clone_fetch(&fetch.fetch),
matches: fetch.matches,
}
}
#[inline]
unsafe fn set_archetype<'w>(
fetch: &mut OptionFetch<'w, T>,
@ -1065,7 +1080,6 @@ impl<'w, T: WorldQuery> WorldQueryGats<'w> for Option<T> {
/// }
/// # bevy_ecs::system::assert_is_system(print_moving_objects_system);
/// ```
#[derive(Clone)]
pub struct ChangeTrackers<T: Component> {
pub(crate) component_ticks: ComponentTicks,
pub(crate) last_change_tick: u32,
@ -1073,6 +1087,18 @@ pub struct ChangeTrackers<T: Component> {
marker: PhantomData<T>,
}
impl<T: Component> Clone for ChangeTrackers<T> {
fn clone(&self) -> Self {
Self {
component_ticks: self.component_ticks,
last_change_tick: self.last_change_tick,
change_tick: self.change_tick,
marker: PhantomData,
}
}
}
impl<T: Component> Copy for ChangeTrackers<T> {}
impl<T: Component> std::fmt::Debug for ChangeTrackers<T> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("ChangeTrackers")
@ -1111,20 +1137,6 @@ pub struct ChangeTrackersFetch<'w, T> {
change_tick: u32,
}
impl<T> Clone for ChangeTrackersFetch<'_, T> {
fn clone(&self) -> Self {
Self {
table_ticks: self.table_ticks,
entity_table_rows: self.entity_table_rows,
entities: self.entities,
sparse_set: self.sparse_set,
marker: self.marker,
last_change_tick: self.last_change_tick,
change_tick: self.change_tick,
}
}
}
// SAFETY: `ROQueryFetch<Self>` is the same as `QueryFetch<Self>`
unsafe impl<T: Component> WorldQuery for ChangeTrackers<T> {
type ReadOnly = Self;
@ -1161,6 +1173,20 @@ unsafe impl<T: Component> WorldQuery for ChangeTrackers<T> {
}
}
unsafe fn clone_fetch<'w>(
fetch: &<Self as WorldQueryGats<'w>>::Fetch,
) -> <Self as WorldQueryGats<'w>>::Fetch {
ChangeTrackersFetch {
table_ticks: fetch.table_ticks,
entity_table_rows: fetch.entity_table_rows,
entities: fetch.entities,
sparse_set: fetch.sparse_set,
marker: fetch.marker,
last_change_tick: fetch.last_change_tick,
change_tick: fetch.change_tick,
}
}
#[inline]
unsafe fn set_archetype<'w>(
fetch: &mut ChangeTrackersFetch<'w, T>,
@ -1317,6 +1343,13 @@ macro_rules! impl_tuple_fetch {
($($name::init_fetch(_world, $name, _last_change_tick, _change_tick),)*)
}
unsafe fn clone_fetch<'w>(
fetch: &<Self as WorldQueryGats<'w>>::Fetch,
) -> <Self as WorldQueryGats<'w>>::Fetch {
let ($($name,)*) = &fetch;
($($name::clone_fetch($name),)*)
}
const IS_DENSE: bool = true $(&& $name::IS_DENSE)*;
const IS_ARCHETYPAL: bool = true $(&& $name::IS_ARCHETYPAL)*;
@ -1395,7 +1428,6 @@ macro_rules! impl_tuple_fetch {
/// `Query<AnyOf<(&A, &B, &mut C)>>` is equivalent to `Query<(Option<&A>, Option<&B>, Option<&mut C>), Or<(With<A>, With<B>, With<C>)>>`.
/// Each of the components in `T` is returned as an `Option`, as with `Option<A>` queries.
/// Entities are guaranteed to have at least one of the components in `T`.
#[derive(Clone)]
pub struct AnyOf<T>(PhantomData<T>);
macro_rules! impl_anytuple_fetch {
@ -1427,6 +1459,13 @@ macro_rules! impl_anytuple_fetch {
($(($name::init_fetch(_world, $name, _last_change_tick, _change_tick), false),)*)
}
unsafe fn clone_fetch<'w>(
fetch: &<Self as WorldQueryGats<'w>>::Fetch,
) -> <Self as WorldQueryGats<'w>>::Fetch {
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)*;
@ -1559,6 +1598,11 @@ unsafe impl<Q: WorldQuery> WorldQuery for NopWorldQuery<Q> {
) {
}
unsafe fn clone_fetch<'w>(
_fetch: &<Self as WorldQueryGats<'w>>::Fetch,
) -> <Self as WorldQueryGats<'w>>::Fetch {
}
#[inline(always)]
unsafe fn set_archetype(
_fetch: &mut (),

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

@ -66,6 +66,11 @@ unsafe impl<T: Component> WorldQuery for With<T> {
) {
}
unsafe fn clone_fetch<'w>(
_fetch: &<Self as WorldQueryGats<'w>>::Fetch,
) -> <Self as WorldQueryGats<'w>>::Fetch {
}
const IS_DENSE: bool = {
match T::Storage::STORAGE_TYPE {
StorageType::Table => true,
@ -173,6 +178,11 @@ unsafe impl<T: Component> WorldQuery for Without<T> {
) {
}
unsafe fn clone_fetch<'w>(
_fetch: &<Self as WorldQueryGats<'w>>::Fetch,
) -> <Self as WorldQueryGats<'w>>::Fetch {
}
const IS_DENSE: bool = {
match T::Storage::STORAGE_TYPE {
StorageType::Table => true,
@ -271,7 +281,6 @@ unsafe impl<T: Component> ReadOnlyWorldQuery for Without<T> {}
/// }
/// # bevy_ecs::system::assert_is_system(print_cool_entity_system);
/// ```
#[derive(Clone, Copy)]
pub struct Or<T>(pub T);
#[doc(hidden)]
@ -279,18 +288,6 @@ pub struct OrFetch<'w, T: WorldQuery> {
fetch: QueryFetch<'w, T>,
matches: bool,
}
impl<'w, T: WorldQuery> Copy for OrFetch<'w, T> where QueryFetch<'w, T>: Copy {}
impl<'w, T: WorldQuery> Clone for OrFetch<'w, T>
where
QueryFetch<'w, T>: Clone,
{
fn clone(&self) -> Self {
Self {
fetch: self.fetch.clone(),
matches: self.matches,
}
}
}
macro_rules! impl_query_filter_tuple {
($(($filter: ident, $state: ident)),*) => {
@ -326,6 +323,18 @@ macro_rules! impl_query_filter_tuple {
},)*)
}
unsafe fn clone_fetch<'w>(
fetch: &<Self as WorldQueryGats<'w>>::Fetch,
) -> <Self as WorldQueryGats<'w>>::Fetch {
let ($($filter,)*) = &fetch;
($(
OrFetch {
fetch: $filter::clone_fetch(&$filter.fetch),
matches: $filter.matches,
},
)*)
}
#[inline]
unsafe fn set_table<'w>(fetch: &mut <Self as WorldQueryGats<'w>>::Fetch, state: &Self::State, table: &'w Table) {
let ($($filter,)*) = fetch;
@ -471,6 +480,20 @@ macro_rules! impl_tick_filter {
}
}
unsafe fn clone_fetch<'w>(
fetch: &<Self as WorldQueryGats<'w>>::Fetch,
) -> <Self as WorldQueryGats<'w>>::Fetch {
$fetch_name {
table_ticks: fetch.table_ticks,
entity_table_rows: fetch.entity_table_rows,
entities: fetch.entities,
sparse_set: fetch.sparse_set,
last_change_tick: fetch.last_change_tick,
change_tick: fetch.change_tick,
marker: PhantomData,
}
}
const IS_DENSE: bool = {
match T::Storage::STORAGE_TYPE {
StorageType::Table => true,
@ -565,22 +588,6 @@ macro_rules! impl_tick_filter {
/// SAFETY: read-only access
unsafe impl<T: Component> ReadOnlyWorldQuery for $name<T> {}
impl<T> Clone for $fetch_name<'_, T> {
fn clone(&self) -> Self {
Self {
table_ticks: self.table_ticks.clone(),
entity_table_rows: self.entity_table_rows.clone(),
marker: self.marker.clone(),
entities: self.entities.clone(),
sparse_set: self.sparse_set.clone(),
last_change_tick: self.last_change_tick.clone(),
change_tick: self.change_tick.clone(),
}
}
}
impl<T> Copy for $fetch_name<'_, T> {}
};
}

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

@ -339,11 +339,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery, const K: usize>
/// references to the same component, leading to unique reference aliasing.
///.
/// It is always safe for shared access.
unsafe fn fetch_next_aliased_unchecked(&mut self) -> Option<[QueryItem<'w, Q>; K]>
where
QueryFetch<'w, Q>: Clone,
QueryFetch<'w, F>: Clone,
{
unsafe fn fetch_next_aliased_unchecked(&mut self) -> Option<[QueryItem<'w, Q>; K]> {
if K == 0 {
return None;
}
@ -354,7 +350,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery, const K: usize>
Some(_) => {
// walk forward up to last element, propagating cursor state forward
for j in (i + 1)..K {
self.cursors[j] = self.cursors[j - 1].clone();
self.cursors[j] = self.cursors[j - 1].clone_cursor();
match self.cursors[j].next(self.tables, self.archetypes, self.query_state) {
Some(_) => {}
None if i > 0 => continue 'outer,
@ -380,11 +376,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery, const K: usize>
/// Get next combination of queried components
#[inline]
pub fn fetch_next(&mut self) -> Option<[QueryItem<'_, Q>; K]>
where
for<'a> QueryFetch<'a, Q>: Clone,
for<'a> QueryFetch<'a, F>: Clone,
{
pub fn fetch_next(&mut self) -> Option<[QueryItem<'_, Q>; K]> {
// SAFETY: we are limiting the returned reference to self,
// making sure this method cannot be called multiple times without getting rid
// of any previously returned unique references first, thus preventing aliasing.
@ -400,9 +392,6 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery, const K: usize>
// multiple times would allow multiple owned references to the same data to exist.
impl<'w, 's, Q: ReadOnlyWorldQuery, F: ReadOnlyWorldQuery, const K: usize> Iterator
for QueryCombinationIter<'w, 's, Q, F, K>
where
QueryFetch<'w, Q>: Clone,
QueryFetch<'w, F>: Clone,
{
type Item = [QueryItem<'w, Q>; K];
@ -467,9 +456,6 @@ where
// This is correct as [`QueryCombinationIter`] always returns `None` once exhausted.
impl<'w, 's, Q: ReadOnlyWorldQuery, F: ReadOnlyWorldQuery, const K: usize> FusedIterator
for QueryCombinationIter<'w, 's, Q, F, K>
where
QueryFetch<'w, Q>: Clone,
QueryFetch<'w, F>: Clone,
{
}
@ -485,17 +471,20 @@ struct QueryIterationCursor<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> {
phantom: PhantomData<Q>,
}
impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Clone for QueryIterationCursor<'w, 's, Q, F>
where
QueryFetch<'w, Q>: Clone,
QueryFetch<'w, F>: Clone,
{
fn clone(&self) -> Self {
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_index` or `table_row` to be alive at the same time.
unsafe fn clone_cursor(&self) -> Self {
Self {
table_id_iter: self.table_id_iter.clone(),
archetype_id_iter: self.archetype_id_iter.clone(),
fetch: self.fetch.clone(),
filter: self.filter.clone(),
// SAFETY: upheld by caller invariants
fetch: Q::clone_fetch(&self.fetch),
filter: F::clone_fetch(&self.filter),
current_len: self.current_len,
current_index: self.current_index,
phantom: PhantomData,

8
crates/bevy_ecs/src/query/mod.rs
View File

@ -21,7 +21,7 @@ pub(crate) unsafe fn debug_checked_unreachable() -> ! {
mod tests {
use super::{ReadOnlyWorldQuery, WorldQuery};
use crate::prelude::{AnyOf, Entity, Or, QueryState, With, Without};
use crate::query::{ArchetypeFilter, QueryCombinationIter, QueryFetch};
use crate::query::{ArchetypeFilter, QueryCombinationIter};
use crate::system::{IntoSystem, Query, System, SystemState};
use crate::{self as bevy_ecs, component::Component, world::World};
use std::any::type_name;
@ -88,8 +88,6 @@ mod tests {
Q: ReadOnlyWorldQuery,
F: ReadOnlyWorldQuery,
F::ReadOnly: ArchetypeFilter,
for<'w> QueryFetch<'w, Q::ReadOnly>: Clone,
for<'w> QueryFetch<'w, F::ReadOnly>: Clone,
{
let mut query = world.query_filtered::<Q, F>();
let iter = query.iter(world);
@ -166,8 +164,6 @@ mod tests {
Q: WorldQuery,
F: ReadOnlyWorldQuery,
F::ReadOnly: ArchetypeFilter,
for<'w> QueryFetch<'w, Q::ReadOnly>: Clone,
for<'w> QueryFetch<'w, F::ReadOnly>: Clone,
{
let mut query = world.query_filtered::<Q, F>();
let iter = query.iter_combinations::<K>(world);
@ -179,8 +175,6 @@ mod tests {
Q: WorldQuery,
F: ReadOnlyWorldQuery,
F::ReadOnly: ArchetypeFilter,
for<'w> QueryFetch<'w, Q::ReadOnly>: Clone,
for<'w> QueryFetch<'w, F::ReadOnly>: Clone,
{
let mut query = world.query_filtered::<Q, F>();
let iter = query.iter(world);