Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 16 commits into from
Jul 25, 2023
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
ee6be94
require `WorldQuery::fetch` to not alias
JoJoJet Mar 28, 2023
b022837
remove `clone_fetch`
JoJoJet Mar 28, 2023
03a59d8
derive `Clone` for tick filters
JoJoJet Mar 28, 2023
7fef544
remove a space
JoJoJet Mar 28, 2023
7616798
make it safe to clone `QueryIterationCursor`
JoJoJet Mar 28, 2023
0b4ca11
use `Copy` to implement `Clone`
JoJoJet Mar 29, 2023
942c3f2
Update crates/bevy_ecs/src/query/fetch.rs
JoJoJet Mar 29, 2023
6747653
Update crates/bevy_ecs/src/query/fetch.rs
JoJoJet Mar 29, 2023
131b0cc
Merge remote-tracking branch 'upstream/main' into world-query-safety
JoJoJet Mar 30, 2023
2f2c74f
remove an unnecessary marker field
JoJoJet Mar 30, 2023
50c4901
remove an unused import
JoJoJet Mar 30, 2023
4f2446a
Merge remote-tracking branch 'upstream/main' into world-query-safety
JoJoJet Apr 3, 2023
b54ebdf
fix a merge conflict
JoJoJet Apr 3, 2023
4cd4de2
Merge remote-tracking branch 'upstream/main' into world-query-safety
JoJoJet Apr 4, 2023
144bf17
tweak phrasing for the documentation
JoJoJet May 31, 2023
07ad079
Merge remote-tracking branch 'upstream/main' into world-query-safety
JoJoJet Jul 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 10 additions & 13 deletions crates/bevy_ecs/macros/src/fetch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,16 @@ pub fn derive_world_query_impl(input: TokenStream) -> TokenStream {
#(#ignored_field_idents: #ignored_field_types,)*
}

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 {
#(#field_idents: self.#field_idents.clone(),)*
#(#ignored_field_idents: Default::default(),)*
}
}
}

// 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 {
Expand Down Expand Up @@ -249,19 +259,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 {
#(
#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)*;
Expand Down
93 changes: 34 additions & 59 deletions crates/bevy_ecs/src/query/fetch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,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>;
Expand All @@ -333,14 +333,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
Expand Down Expand Up @@ -388,6 +380,9 @@ 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 this type does not implement [`ReadOnlyWorldQuery`], then the caller must ensure that
/// no two `Self::Item`s may exist for the same entity at any given time.
JoJoJet marked this conversation as resolved.
Show resolved Hide resolved
unsafe fn fetch<'w>(
fetch: &mut Self::Fetch<'w>,
entity: Entity,
Expand Down Expand Up @@ -465,8 +460,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>,
Expand Down Expand Up @@ -519,6 +512,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>;
Expand Down Expand Up @@ -557,13 +557,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>,
Expand Down Expand Up @@ -664,6 +657,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>;
Expand Down Expand Up @@ -704,15 +704,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>,
Expand Down Expand Up @@ -825,6 +816,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>;
Expand Down Expand Up @@ -865,15 +863,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>,
Expand Down Expand Up @@ -974,6 +963,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>;
Expand Down Expand Up @@ -1001,13 +999,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>,
Expand Down Expand Up @@ -1099,13 +1090,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)*;
Expand Down Expand Up @@ -1208,13 +1192,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)*;
Expand Down Expand Up @@ -1343,8 +1320,6 @@ unsafe impl<Q: WorldQuery> WorldQuery for NopWorldQuery<Q> {
#[inline(always)]
unsafe fn init_fetch(_world: &World, _state: &Q::State, _last_run: Tick, _this_run: Tick) {}

unsafe fn clone_fetch<'w>(_fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {}

#[inline(always)]
unsafe fn set_archetype(
_fetch: &mut (),
Expand Down
46 changes: 13 additions & 33 deletions crates/bevy_ecs/src/query/filter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,6 @@ unsafe impl<T: Component> WorldQuery for With<T> {

unsafe fn init_fetch(_world: &World, _state: &ComponentId, _last_run: Tick, _this_run: Tick) {}

unsafe fn clone_fetch<'w>(_fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {}

const IS_DENSE: bool = {
match T::Storage::STORAGE_TYPE {
StorageType::Table => true,
Expand Down Expand Up @@ -148,8 +146,6 @@ unsafe impl<T: Component> WorldQuery for Without<T> {

unsafe fn init_fetch(_world: &World, _state: &ComponentId, _last_run: Tick, _this_run: Tick) {}

unsafe fn clone_fetch<'w>(_fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {}

const IS_DENSE: bool = {
match T::Storage::STORAGE_TYPE {
StorageType::Table => true,
Expand Down Expand Up @@ -245,6 +241,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)]
Expand Down Expand Up @@ -273,18 +278,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;
Expand Down Expand Up @@ -400,18 +393,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;
Expand All @@ -430,24 +423,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,
Expand Down
Loading