Skip to content

Commit

Permalink
Query filter types must be ReadOnlyWorldQuery (#6008)
Browse files Browse the repository at this point in the history
# Objective

Fixes Issue #6005.

## Solution

Replaced WorldQuery with ReadOnlyWorldQuery on F generic in Query filters and QueryState to restrict its trait bound.

## Migration Guide

Query filter (`F`) generics are now bound by `ReadOnlyWorldQuery`, rather than `WorldQuery`. If for some reason you were requesting `Query<&A, &mut B>`, please use `Query<&A, With<B>>` instead.
  • Loading branch information
targrub committed Sep 18, 2022
1 parent 1f0fa59 commit d0e294c
Show file tree
Hide file tree
Showing 10 changed files with 51 additions and 42 deletions.
6 changes: 4 additions & 2 deletions crates/bevy_ecs/src/lib.rs
Expand Up @@ -57,7 +57,9 @@ mod tests {
bundle::Bundle,
component::{Component, ComponentId},
entity::Entity,
query::{Added, ChangeTrackers, Changed, FilteredAccess, With, Without, WorldQuery},
query::{
Added, ChangeTrackers, Changed, FilteredAccess, ReadOnlyWorldQuery, With, Without,
},
system::Resource,
world::{Mut, World},
};
Expand Down Expand Up @@ -902,7 +904,7 @@ mod tests {
}
}

fn get_filtered<F: WorldQuery>(world: &mut World) -> Vec<Entity> {
fn get_filtered<F: ReadOnlyWorldQuery>(world: &mut World) -> Vec<Entity> {
world
.query_filtered::<Entity, F>()
.iter(world)
Expand Down
26 changes: 14 additions & 12 deletions crates/bevy_ecs/src/query/iter.rs
Expand Up @@ -13,14 +13,14 @@ use super::{QueryFetch, QueryItem, ReadOnlyWorldQuery};
///
/// This struct is created by the [`Query::iter`](crate::system::Query::iter) and
/// [`Query::iter_mut`](crate::system::Query::iter_mut) methods.
pub struct QueryIter<'w, 's, Q: WorldQuery, F: WorldQuery> {
pub struct QueryIter<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> {
tables: &'w Tables,
archetypes: &'w Archetypes,
query_state: &'s QueryState<Q, F>,
cursor: QueryIterationCursor<'w, 's, Q, F>,
}

impl<'w, 's, Q: WorldQuery, F: WorldQuery> QueryIter<'w, 's, Q, F> {
impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIter<'w, 's, Q, F> {
/// # Safety
/// This does not check for mutable query correctness. To be safe, make sure mutable queries
/// have unique access to the components they query.
Expand All @@ -41,7 +41,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> QueryIter<'w, 's, Q, F> {
}
}

impl<'w, 's, Q: WorldQuery, F: WorldQuery> Iterator for QueryIter<'w, 's, Q, F> {
impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Iterator for QueryIter<'w, 's, Q, F> {
type Item = QueryItem<'w, Q>;

#[inline(always)]
Expand Down Expand Up @@ -70,12 +70,12 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Iterator for QueryIter<'w, 's, Q, F>
}

// This is correct as [`QueryIter`] always returns `None` once exhausted.
impl<'w, 's, Q: WorldQuery, F: WorldQuery> FusedIterator for QueryIter<'w, 's, Q, F> {}
impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> FusedIterator for QueryIter<'w, 's, Q, F> {}

/// An [`Iterator`] over [`Query`](crate::system::Query) results of a list of [`Entity`]s.
///
/// This struct is created by the [`Query::iter_many`](crate::system::Query::iter_many) and [`Query::iter_many_mut`](crate::system::Query::iter_many_mut) methods.
pub struct QueryManyIter<'w, 's, Q: WorldQuery, F: WorldQuery, I: Iterator>
pub struct QueryManyIter<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery, I: Iterator>
where
I::Item: Borrow<Entity>,
{
Expand All @@ -88,7 +88,7 @@ where
query_state: &'s QueryState<Q, F>,
}

impl<'w, 's, Q: WorldQuery, F: WorldQuery, I: Iterator> QueryManyIter<'w, 's, Q, F, I>
impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery, I: Iterator> QueryManyIter<'w, 's, Q, F, I>
where
I::Item: Borrow<Entity>,
{
Expand Down Expand Up @@ -267,14 +267,16 @@ where
/// [`Query`]: crate::system::Query
/// [`Query::iter_combinations`]: crate::system::Query::iter_combinations
/// [`Query::iter_combinations_mut`]: crate::system::Query::iter_combinations_mut
pub struct QueryCombinationIter<'w, 's, Q: WorldQuery, F: WorldQuery, const K: usize> {
pub struct QueryCombinationIter<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery, const K: usize> {
tables: &'w Tables,
archetypes: &'w Archetypes,
query_state: &'s QueryState<Q, F>,
cursors: [QueryIterationCursor<'w, 's, Q, F>; K],
}

impl<'w, 's, Q: WorldQuery, F: WorldQuery, const K: usize> QueryCombinationIter<'w, 's, Q, F, K> {
impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery, const K: usize>
QueryCombinationIter<'w, 's, Q, F, K>
{
/// # Safety
/// This does not check for mutable query correctness. To be safe, make sure mutable queries
/// have unique access to the components they query.
Expand Down Expand Up @@ -436,7 +438,7 @@ where
}
}

impl<'w, 's, Q: WorldQuery, F: WorldQuery> ExactSizeIterator for QueryIter<'w, 's, Q, F>
impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> ExactSizeIterator for QueryIter<'w, 's, Q, F>
where
F: ArchetypeFilter,
{
Expand Down Expand Up @@ -473,7 +475,7 @@ where
{
}

struct QueryIterationCursor<'w, 's, Q: WorldQuery, F: WorldQuery> {
struct QueryIterationCursor<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> {
table_id_iter: std::slice::Iter<'s, TableId>,
archetype_id_iter: std::slice::Iter<'s, ArchetypeId>,
fetch: QueryFetch<'w, Q>,
Expand All @@ -485,7 +487,7 @@ struct QueryIterationCursor<'w, 's, Q: WorldQuery, F: WorldQuery> {
phantom: PhantomData<Q>,
}

impl<'w, 's, Q: WorldQuery, F: WorldQuery> Clone for QueryIterationCursor<'w, 's, Q, F>
impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Clone for QueryIterationCursor<'w, 's, Q, F>
where
QueryFetch<'w, Q>: Clone,
QueryFetch<'w, F>: Clone,
Expand All @@ -503,7 +505,7 @@ where
}
}

impl<'w, 's, Q: WorldQuery, F: WorldQuery> QueryIterationCursor<'w, 's, Q, F> {
impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIterationCursor<'w, 's, Q, F> {
const IS_DENSE: bool = Q::IS_DENSE && F::IS_DENSE;

unsafe fn init_empty(
Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_ecs/src/query/mod.rs
Expand Up @@ -19,7 +19,7 @@ pub(crate) unsafe fn debug_checked_unreachable() -> ! {

#[cfg(test)]
mod tests {
use super::WorldQuery;
use super::{ReadOnlyWorldQuery, WorldQuery};
use crate::prelude::{AnyOf, Entity, Or, QueryState, With, Without};
use crate::query::{ArchetypeFilter, QueryCombinationIter, QueryFetch};
use crate::system::{IntoSystem, Query, System, SystemState};
Expand Down Expand Up @@ -68,7 +68,7 @@ mod tests {
fn assert_combination<Q, F, const K: usize>(world: &mut World, expected_size: usize)
where
Q: WorldQuery,
F: WorldQuery,
F: ReadOnlyWorldQuery,
F::ReadOnly: ArchetypeFilter,
for<'w> QueryFetch<'w, Q::ReadOnly>: Clone,
for<'w> QueryFetch<'w, F::ReadOnly>: Clone,
Expand All @@ -81,7 +81,7 @@ mod tests {
fn assert_all_sizes_equal<Q, F>(world: &mut World, expected_size: usize)
where
Q: WorldQuery,
F: WorldQuery,
F: ReadOnlyWorldQuery,
F::ReadOnly: ArchetypeFilter,
for<'w> QueryFetch<'w, Q::ReadOnly>: Clone,
for<'w> QueryFetch<'w, F::ReadOnly>: Clone,
Expand Down
12 changes: 6 additions & 6 deletions crates/bevy_ecs/src/query/state.rs
Expand Up @@ -13,14 +13,14 @@ use bevy_utils::tracing::Instrument;
use fixedbitset::FixedBitSet;
use std::{borrow::Borrow, fmt};

use super::{NopWorldQuery, QueryItem, QueryManyIter, ROQueryItem};
use super::{NopWorldQuery, QueryItem, QueryManyIter, ROQueryItem, ReadOnlyWorldQuery};

/// Provides scoped access to a [`World`] state according to a given [`WorldQuery`] and query filter.
#[repr(C)]
// SAFETY NOTE:
// Do not add any new fields that use the `Q` or `F` generic parameters as this may
// make `QueryState::as_transmuted_state` unsound if not done with care.
pub struct QueryState<Q: WorldQuery, F: WorldQuery = ()> {
pub struct QueryState<Q: WorldQuery, F: ReadOnlyWorldQuery = ()> {
world_id: WorldId,
pub(crate) archetype_generation: ArchetypeGeneration,
pub(crate) matched_tables: FixedBitSet,
Expand All @@ -35,13 +35,13 @@ pub struct QueryState<Q: WorldQuery, F: WorldQuery = ()> {
pub(crate) filter_state: F::State,
}

impl<Q: WorldQuery, F: WorldQuery> FromWorld for QueryState<Q, F> {
impl<Q: WorldQuery, F: ReadOnlyWorldQuery> FromWorld for QueryState<Q, F> {
fn from_world(world: &mut World) -> Self {
world.query_filtered()
}
}

impl<Q: WorldQuery, F: WorldQuery> QueryState<Q, F> {
impl<Q: WorldQuery, F: ReadOnlyWorldQuery> QueryState<Q, F> {
/// Converts this `QueryState` reference to a `QueryState` that does not access anything mutably.
pub fn as_readonly(&self) -> &QueryState<Q::ReadOnly, F::ReadOnly> {
// SAFETY: invariant on `WorldQuery` trait upholds that `Q::ReadOnly` and `F::ReadOnly`
Expand Down Expand Up @@ -71,15 +71,15 @@ impl<Q: WorldQuery, F: WorldQuery> QueryState<Q, F> {
/// `NewF` must have a subset of the access that `F` does and match the exact same archetypes/tables
pub(crate) unsafe fn as_transmuted_state<
NewQ: WorldQuery<State = Q::State>,
NewF: WorldQuery<State = F::State>,
NewF: ReadOnlyWorldQuery<State = F::State>,
>(
&self,
) -> &QueryState<NewQ, NewF> {
&*(self as *const QueryState<Q, F> as *const QueryState<NewQ, NewF>)
}
}

impl<Q: WorldQuery, F: WorldQuery> QueryState<Q, F> {
impl<Q: WorldQuery, F: ReadOnlyWorldQuery> QueryState<Q, F> {
/// Creates a new [`QueryState`] from a given [`World`] and inherits the result of `world.id()`.
pub fn new(world: &mut World) -> Self {
let fetch_state = Q::init_state(world);
Expand Down
10 changes: 5 additions & 5 deletions crates/bevy_ecs/src/system/query.rs
Expand Up @@ -273,14 +273,14 @@ use std::{any::TypeId, borrow::Borrow, fmt::Debug};
/// [`Table`]: crate::storage::Table
/// [`With`]: crate::query::With
/// [`Without`]: crate::query::Without
pub struct Query<'world, 'state, Q: WorldQuery, F: WorldQuery = ()> {
pub struct Query<'world, 'state, Q: WorldQuery, F: ReadOnlyWorldQuery = ()> {
pub(crate) world: &'world World,
pub(crate) state: &'state QueryState<Q, F>,
pub(crate) last_change_tick: u32,
pub(crate) change_tick: u32,
}

impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> {
impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> {
/// Creates a new query.
///
/// # Safety
Expand Down Expand Up @@ -1380,7 +1380,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> {
}
}

impl<'w, 's, Q: WorldQuery, F: WorldQuery> IntoIterator for &'w Query<'_, 's, Q, F> {
impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> IntoIterator for &'w Query<'_, 's, Q, F> {
type Item = ROQueryItem<'w, Q>;
type IntoIter = QueryIter<'w, 's, Q::ReadOnly, F::ReadOnly>;

Expand All @@ -1389,7 +1389,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> IntoIterator for &'w Query<'_, 's, Q,
}
}

impl<'w, 's, Q: WorldQuery, F: WorldQuery> IntoIterator for &'w mut Query<'_, 's, Q, F> {
impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> IntoIterator for &'w mut Query<'_, 's, Q, F> {
type Item = QueryItem<'w, Q>;
type IntoIter = QueryIter<'w, 's, Q, F>;

Expand Down Expand Up @@ -1434,7 +1434,7 @@ impl std::fmt::Display for QueryComponentError {
}
}

impl<'w, 's, Q: ReadOnlyWorldQuery, F: WorldQuery> Query<'w, 's, Q, F> {
impl<'w, 's, Q: ReadOnlyWorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> {
/// Returns the query item for the given [`Entity`], with the actual "inner" world lifetime.
///
/// In case of a nonexisting entity or mismatched component, a [`QueryEntityError`] is
Expand Down
17 changes: 11 additions & 6 deletions crates/bevy_ecs/src/system/system_param.rs
Expand Up @@ -135,16 +135,21 @@ pub trait SystemParamFetch<'world, 'state>: SystemParamState {
) -> Self::Item;
}

impl<'w, 's, Q: WorldQuery + 'static, F: WorldQuery + 'static> SystemParam for Query<'w, 's, Q, F> {
impl<'w, 's, Q: WorldQuery + 'static, F: ReadOnlyWorldQuery + 'static> SystemParam
for Query<'w, 's, Q, F>
{
type Fetch = QueryState<Q, F>;
}

// SAFETY: QueryState is constrained to read-only fetches, so it only reads World.
unsafe impl<Q: ReadOnlyWorldQuery, F: WorldQuery> ReadOnlySystemParamFetch for QueryState<Q, F> {}
unsafe impl<Q: ReadOnlyWorldQuery, F: ReadOnlyWorldQuery> ReadOnlySystemParamFetch
for QueryState<Q, F>
{
}

// SAFETY: Relevant query ComponentId and ArchetypeComponentId access is applied to SystemMeta. If
// this QueryState conflicts with any prior access, a panic will occur.
unsafe impl<Q: WorldQuery + 'static, F: WorldQuery + 'static> SystemParamState
unsafe impl<Q: WorldQuery + 'static, F: ReadOnlyWorldQuery + 'static> SystemParamState
for QueryState<Q, F>
{
fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self {
Expand Down Expand Up @@ -174,7 +179,7 @@ unsafe impl<Q: WorldQuery + 'static, F: WorldQuery + 'static> SystemParamState
}
}

impl<'w, 's, Q: WorldQuery + 'static, F: WorldQuery + 'static> SystemParamFetch<'w, 's>
impl<'w, 's, Q: WorldQuery + 'static, F: ReadOnlyWorldQuery + 'static> SystemParamFetch<'w, 's>
for QueryState<Q, F>
{
type Item = Query<'w, 's, Q, F>;
Expand Down Expand Up @@ -1595,7 +1600,7 @@ mod tests {
use super::SystemParam;
use crate::{
self as bevy_ecs, // Necessary for the `SystemParam` Derive when used inside `bevy_ecs`.
query::WorldQuery,
query::{ReadOnlyWorldQuery, WorldQuery},
system::Query,
};

Expand All @@ -1605,7 +1610,7 @@ mod tests {
'w,
's,
Q: WorldQuery + Send + Sync + 'static,
F: WorldQuery + Send + Sync + 'static = (),
F: ReadOnlyWorldQuery + Send + Sync + 'static = (),
> {
_query: Query<'w, 's, Q, F>,
}
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_ecs/src/world/mod.rs
Expand Up @@ -16,7 +16,7 @@ use crate::{
StorageType,
},
entity::{AllocAtWithoutReplacement, Entities, Entity},
query::{QueryState, WorldQuery},
query::{QueryState, ReadOnlyWorldQuery, WorldQuery},
storage::{Column, SparseSet, Storages},
system::Resource,
};
Expand Down Expand Up @@ -610,7 +610,7 @@ impl World {
/// assert_eq!(matching_entities, vec![e2]);
/// ```
#[inline]
pub fn query_filtered<Q: WorldQuery, F: WorldQuery>(&mut self) -> QueryState<Q, F> {
pub fn query_filtered<Q: WorldQuery, F: ReadOnlyWorldQuery>(&mut self) -> QueryState<Q, F> {
QueryState::new(self)
}

Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_reflect/src/lib.rs
Expand Up @@ -93,7 +93,7 @@ pub mod __macro_exports {
}

#[cfg(test)]
#[allow(clippy::blacklisted_name, clippy::approx_constant)]
#[allow(clippy::disallowed_types, clippy::approx_constant)]
mod tests {
#[cfg(feature = "glam")]
use ::glam::{vec3, Vec3};
Expand Down Expand Up @@ -188,7 +188,7 @@ mod tests {
}

#[test]
#[allow(clippy::blacklisted_name)]
#[allow(clippy::disallowed_types)]
fn reflect_unit_struct() {
#[derive(Reflect)]
struct Foo(u32, u64);
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_tasks/src/task_pool.rs
Expand Up @@ -150,7 +150,7 @@ impl TaskPool {
// before this function returns. However, rust has no way of knowing
// this so we must convert to 'static here to appease the compiler as it is unable to
// validate safety.
let executor: &async_executor::Executor = &*self.executor;
let executor: &async_executor::Executor = &self.executor;
let executor: &'scope async_executor::Executor = unsafe { mem::transmute(executor) };
let local_executor: &'scope async_executor::LocalExecutor =
unsafe { mem::transmute(local_executor) };
Expand Down Expand Up @@ -287,7 +287,7 @@ impl<'scope, T: Send + 'scope> Scope<'scope, T> {
}

#[cfg(test)]
#[allow(clippy::blacklisted_name)]
#[allow(clippy::disallowed_types)]
mod tests {
use super::*;
use std::sync::{
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_ui/src/flex/mod.rs
Expand Up @@ -4,7 +4,7 @@ use crate::{CalculatedSize, Node, Style, UiScale};
use bevy_ecs::{
entity::Entity,
event::EventReader,
query::{Changed, With, Without, WorldQuery},
query::{Changed, ReadOnlyWorldQuery, With, Without},
system::{Query, RemovedComponents, Res, ResMut, Resource},
};
use bevy_hierarchy::{Children, Parent};
Expand Down Expand Up @@ -234,7 +234,7 @@ pub fn flex_node_system(
update_changed(&mut *flex_surface, scale_factor, node_query);
}

fn update_changed<F: WorldQuery>(
fn update_changed<F: ReadOnlyWorldQuery>(
flex_surface: &mut FlexSurface,
scaling_factor: f64,
query: Query<(Entity, &Style, Option<&CalculatedSize>), F>,
Expand Down

0 comments on commit d0e294c

Please sign in to comment.