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

Disallow requesting write resource access in Queries (#17116)

Browse Source 
Related to https://github.com/bevyengine/bevy/pull/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.
This commit is contained in:
vil'mo 2025-01-07 02:04:26 +07:00 committed by GitHub
parent 7dd56862a4
commit b30ee2d051
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 15 additions and 131 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
crates/bevy_asset/src/asset_changed.rs
View File

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

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

@ -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.
@ -890,85 +887,6 @@ 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>>) {}
@ -976,38 +894,13 @@ mod tests {
}
#[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);
@ -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
crates/bevy_ecs/src/query/state.rs
View File

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

15
crates/bevy_ecs/src/query/world_query.rs
View File

@ -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.
@ -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.
///
@ -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,
@ -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,