Skip to content

Commit

Permalink
Disallow requesting write resource access in Queries (#17116)
Browse files Browse the repository at this point in the history 
Related to #16843

Since `WorldQuery::Fetch` is `Clone`, it can't store mutable references
to resources, so it doesn't make sense to mutably access resources. In
that sense, it is hard to find usecases of mutably accessing resources
and to clearly define, what mutably accessing resources would mean, so
it's been decided to disallow write resource access.
Also changed documentation of safety requirements of
`WorldQuery::init_fetch` and `WorldQuery::fetch` to clearly state to the
caller, what safety invariants they need to uphold.
  • Loading branch information
@vil-mo
vil-mo authored Jan 6, 2025
1 parent 7dd5686 commit b30ee2d
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 131 deletions.
5 changes: 0 additions & 5 deletions crates/bevy_asset/src/asset_changed.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -233,11 +233,6 @@ unsafe impl<A: AsAssetId> WorldQuery for AssetChanged<A> {
#[inline]
fn update_component_access(state: &Self::State, access: &mut FilteredAccess<ComponentId>) {
<&A>::update_component_access(&state.asset_id, access);
assert!(
!access.access().has_resource_write(state.resource_id),
"AssetChanged<{ty}> requires read-only access to AssetChanges<{ty}>",
ty = ShortName::of::<A>()
);
access.add_resource_read(state.resource_id);
}

Expand Down
115 changes: 1 addition & 114 deletions crates/bevy_ecs/src/query/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -805,9 +805,6 @@ mod tests {
/// `QueryData` that performs read access on R to test that resource access is tracked
struct ReadsRData;

/// `QueryData` that performs write access on R to test that resource access is tracked
struct WritesRData;

/// SAFETY:
/// `update_component_access` adds resource read access for `R`.
/// `update_archetype_component_access` does nothing, as this accesses no components.
Expand Down Expand Up @@ -890,124 +887,20 @@ mod tests {
/// SAFETY: access is read only
unsafe impl ReadOnlyQueryData for ReadsRData {}

/// SAFETY:
/// `update_component_access` adds resource read access for `R`.
/// `update_archetype_component_access` does nothing, as this accesses no components.
unsafe impl WorldQuery for WritesRData {
type Item<'w> = ();
type Fetch<'w> = ();
type State = ComponentId;

fn shrink<'wlong: 'wshort, 'wshort>(_item: Self::Item<'wlong>) -> Self::Item<'wshort> {}

fn shrink_fetch<'wlong: 'wshort, 'wshort>(_: Self::Fetch<'wlong>) -> Self::Fetch<'wshort> {}

unsafe fn init_fetch<'w>(
_world: UnsafeWorldCell<'w>,
_state: &Self::State,
_last_run: Tick,
_this_run: Tick,
) -> Self::Fetch<'w> {
}

const IS_DENSE: bool = true;

#[inline]
unsafe fn set_archetype<'w>(
_fetch: &mut Self::Fetch<'w>,
_state: &Self::State,
_archetype: &'w Archetype,
_table: &Table,
) {
}

#[inline]
unsafe fn set_table<'w>(
_fetch: &mut Self::Fetch<'w>,
_state: &Self::State,
_table: &'w Table,
) {
}

#[inline(always)]
unsafe fn fetch<'w>(
_fetch: &mut Self::Fetch<'w>,
_entity: Entity,
_table_row: TableRow,
) -> Self::Item<'w> {
}

fn update_component_access(
&component_id: &Self::State,
access: &mut FilteredAccess<ComponentId>,
) {
assert!(
!access.access().has_resource_read(component_id),
"WritesRData conflicts with a previous access in this query. Shared access cannot coincide with exclusive access.",
);
access.add_resource_write(component_id);
}

fn init_state(world: &mut World) -> Self::State {
world.components.register_resource::<R>()
}

fn get_state(components: &Components) -> Option<Self::State> {
components.resource_id::<R>()
}

fn matches_component_set(
_state: &Self::State,
_set_contains_id: &impl Fn(ComponentId) -> bool,
) -> bool {
true
}
}

/// SAFETY: `Self` is the same as `Self::ReadOnly`
unsafe impl QueryData for WritesRData {
type ReadOnly = ReadsRData;
}

#[test]
fn read_res_read_res_no_conflict() {
fn system(_q1: Query<ReadsRData, With<A>>, _q2: Query<ReadsRData, Without<A>>) {}
assert_is_system(system);
}

#[test]
#[should_panic]
fn read_res_write_res_conflict() {
fn system(_q1: Query<ReadsRData, With<A>>, _q2: Query<WritesRData, Without<A>>) {}
assert_is_system(system);
}

#[test]
#[should_panic]
fn write_res_read_res_conflict() {
fn system(_q1: Query<WritesRData, With<A>>, _q2: Query<ReadsRData, Without<A>>) {}
assert_is_system(system);
}

#[test]
#[should_panic]
fn write_res_write_res_conflict() {
fn system(_q1: Query<WritesRData, With<A>>, _q2: Query<WritesRData, Without<A>>) {}
assert_is_system(system);
}

#[test]
fn read_write_res_sets_archetype_component_access() {
fn read_res_sets_archetype_component_access() {
let mut world = World::new();

fn read_query(_q: Query<ReadsRData, With<A>>) {}
let mut read_query = IntoSystem::into_system(read_query);
read_query.initialize(&mut world);

fn write_query(_q: Query<WritesRData, With<A>>) {}
let mut write_query = IntoSystem::into_system(write_query);
write_query.initialize(&mut world);

fn read_res(_r: Res<R>) {}
let mut read_res = IntoSystem::into_system(read_res);
read_res.initialize(&mut world);
Expand All @@ -1019,14 +912,8 @@ mod tests {
assert!(read_query
.archetype_component_access()
.is_compatible(read_res.archetype_component_access()));
assert!(!write_query
.archetype_component_access()
.is_compatible(read_res.archetype_component_access()));
assert!(!read_query
.archetype_component_access()
.is_compatible(write_res.archetype_component_access()));
assert!(!write_query
.archetype_component_access()
.is_compatible(write_res.archetype_component_access()));
}
}
11 changes: 4 additions & 7 deletions crates/bevy_ecs/src/query/state.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,13 +199,10 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
}
}

if state.component_access.access().has_write_all_resources() {
access.write_all_resources();
} else {
for component_id in state.component_access.access().resource_writes() {
access.add_resource_write(world.initialize_resource_internal(component_id).id());
}
}
debug_assert!(
!state.component_access.access().has_any_resource_write(),
"Mutable resource access in queries is not allowed"
);

state
}
Expand Down
15 changes: 10 additions & 5 deletions crates/bevy_ecs/src/query/world_query.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use variadics_please::all_tuples;
/// # Safety
///
/// Implementor must ensure that
/// [`update_component_access`], [`matches_component_set`], and [`fetch`]
/// [`update_component_access`], [`matches_component_set`], [`fetch`] and [`init_fetch`]
/// obey the following:
///
/// - For each component mutably accessed by [`fetch`], [`update_component_access`] should add write access unless read or write access has already been added, in which case it should panic.
Expand All @@ -26,8 +26,8 @@ use variadics_please::all_tuples;
/// - [`matches_component_set`] must be a disjunction of the element's implementations
/// - [`update_component_access`] must replace the filters with a disjunction of filters
/// - Each filter in that disjunction must be a conjunction of the corresponding element's filter with the previous `access`
/// - For each resource mutably accessed by [`init_fetch`], [`update_component_access`] should add write access unless read or write access has already been added, in which case it should panic.
/// - For each resource readonly accessed by [`init_fetch`], [`update_component_access`] should add read access unless write access has already been added, in which case it should panic.
/// - For each resource readonly accessed by [`init_fetch`], [`update_component_access`] should add read access.
/// - Mutable resource access is not allowed.
///
/// When implementing [`update_component_access`], note that `add_read` and `add_write` both also add a `With` filter, whereas `extend_access` does not change the filters.
///
Expand Down Expand Up @@ -60,11 +60,14 @@ pub unsafe trait WorldQuery {
fn shrink_fetch<'wlong: 'wshort, 'wshort>(fetch: Self::Fetch<'wlong>) -> Self::Fetch<'wshort>;

/// Creates a new instance of this fetch.
/// Readonly accesses resources registered in [`WorldQuery::update_component_access`].
///
/// # Safety
///
/// - `state` must have been initialized (via [`WorldQuery::init_state`]) using the same `world` passed
/// in to this function.
/// - `world` must have the **right** to access any access registered in `update_component_access`.
/// - There must not be simultaneous resource access conflicting with readonly resource access registered in [`WorldQuery::update_component_access`].
unsafe fn init_fetch<'w>(
world: UnsafeWorldCell<'w>,
state: &Self::State,
Expand Down Expand Up @@ -114,11 +117,13 @@ pub unsafe trait WorldQuery {
/// or for the given `entity` in the current [`Archetype`]. This must always be called after
/// [`WorldQuery::set_table`] with a `table_row` in the range of the current [`Table`] or after
/// [`WorldQuery::set_archetype`] with a `entity` in the current archetype.
/// Accesses components registered in [`WorldQuery::update_component_access`].
///
/// # Safety
///
/// 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.
/// - 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.
/// - There must not be simultaneous conflicting component access registered in `update_component_access`.
unsafe fn fetch<'w>(
fetch: &mut Self::Fetch<'w>,
entity: Entity,
Expand Down

0 comments on commit b30ee2d

Please sign in to comment.