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

Fix unsoundness with Or/AnyOf/Option component access' (#4659)

Browse Source 
# Objective

Fixes #4657

Example code that wasnt panic'ing before this PR (and so was unsound):
```rust
    #[test]
    #[should_panic = "error[B0001]"]
    fn option_has_no_filter_with() {
        fn sys(_1: Query<(Option<&A>, &mut B)>, _2: Query<&mut B, Without<A>>) {}
        let mut world = World::default();
        run_system(&mut world, sys);
    }

    #[test]
    #[should_panic = "error[B0001]"]
    fn any_of_has_no_filter_with() {
        fn sys(_1: Query<(AnyOf<(&A, ())>, &mut B)>, _2: Query<&mut B, Without<A>>) {}
        let mut world = World::default();
        run_system(&mut world, sys);
    }

    #[test]
    #[should_panic = "error[B0001]"]
    fn or_has_no_filter_with() {
        fn sys(_1: Query<&mut B, Or<(With<A>, With<B>)>>, _2: Query<&mut B, Without<A>>) {}
        let mut world = World::default();
        run_system(&mut world, sys);
    }
```
## Solution

- Only add the intersection of `with`/`without` accesses of all the elements in `Or/AnyOf` to the world query's `FilteredAccess<ComponentId>` instead of the union.
- `Option`'s fix can be thought of the same way since its basically `AnyOf<T, ()>` but its impl is just simpler as `()` has no `with`/`without` accesses
---

## Changelog

- `Or`/`AnyOf`/`Option` will now report more query conflicts in order to fix unsoundness

## Migration Guide

- If you are now getting query conflicts from `Or`/`AnyOf`/`Option` rip to you and ur welcome for it now being caught
This commit is contained in:
Boxy 2022-05-18 20:57:24 +00:00
parent 2c93b5cf73
commit 1320818f96
4 changed files with 149 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
crates/bevy_ecs/src/query/access.rs
View File

@ -148,6 +148,23 @@ impl<T: SparseSetIndex> Access<T> {
/// An [`Access`] that has been filtered to include and exclude certain combinations of elements.
///
/// Used internally to statically check if queries are disjoint.
///
/// Subtle: a `read` or `write` in `access` should not be considered to imply a
/// `with` access.
///
/// For example consider `Query<Option<&T>>` this only has a `read` of `T` as doing
/// otherwise would allow for queriess to be considered disjoint that actually arent:
/// - `Query<(&mut T, Option<&U>)>` read/write `T`, read `U`, with `U`
/// - `Query<&mut T, Without<U>>` read/write `T`, without `U`
/// from this we could reasonably conclude that the queries are disjoint but they aren't.
///
/// In order to solve this the actual access that `Query<(&mut T, Option<&U>)>` has
/// is read/write `T`, read `U`. It must still have a read `U` access otherwise the following
/// queries would be incorrectly considered disjoint:
/// - `Query<&mut T>` read/write `T`
/// - `Query<Option<&T>` accesses nothing
///
/// See comments the `WorldQuery` impls of `AnyOf`/`Option`/`Or` for more information.
#[derive(Debug, Clone, Eq, PartialEq)]
pub struct FilteredAccess<T: SparseSetIndex> {
access: Access<T>,
@ -210,6 +227,15 @@ impl<T: SparseSetIndex> FilteredAccess<T> {
self.without.insert(index.sparse_set_index());
}
pub fn extend_intersect_filter(&mut self, other: &FilteredAccess<T>) {
self.without.intersect_with(&other.without);
self.with.intersect_with(&other.with);
}
pub fn extend_access(&mut self, other: &FilteredAccess<T>) {
self.access.extend(&other.access);
}
/// Returns `true` if this and `other` can be active at the same time.
pub fn is_compatible(&self, other: &FilteredAccess<T>) -> bool {
if self.access.is_compatible(&other.access) {

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

@ -1091,7 +1091,13 @@ unsafe impl<T: FetchState> FetchState for OptionState<T> {
}
fn update_component_access(&self, access: &mut FilteredAccess<ComponentId>) {
self.state.update_component_access(access);
// We don't want to add the `with`/`without` of `T` as `Option<T>` will match things regardless of
// `T`'s filters. for example `Query<(Option<&U>, &mut V)>` will match every entity with a `V` component
// regardless of whether it has a `U` component. If we dont do this the query will not conflict with
// `Query<&mut V, Without<U>>` which would be unsound.
let mut intermediate = access.clone();
self.state.update_component_access(&mut intermediate);
access.extend_access(&intermediate);
}
fn update_archetype_component_access(
@ -1660,7 +1666,34 @@ macro_rules! impl_anytuple_fetch {
fn update_component_access(&self, _access: &mut FilteredAccess<ComponentId>) {
let ($($name,)*) = &self.0;
$($name.update_component_access(_access);)*
// We do not unconditionally add `$name`'s `with`/`without` accesses to `_access`
// as this would be unsound. For example the following two queries should conflict:
// - Query<(AnyOf<(&A, ())>, &mut B)>
// - Query<&mut B, Without<A>>
//
// If we were to unconditionally add `$name`'s `with`/`without` accesses then `AnyOf<(&A, ())>`
// would have a `With<A>` access which is incorrect as this `WorldQuery` will match entities that
// do not have the `A` component. This is the same logic as the `Or<...>: WorldQuery` impl.
//
// The correct thing to do here is to only add a `with`/`without` access to `_access` if all
// `$name` params have that `with`/`without` access. More jargony put- we add the intersection
// of all `with`/`without` accesses of the `$name` params to `_access`.
let mut _intersected_access = _access.clone();
let mut _not_first = false;
$(
if _not_first {
let mut intermediate = _access.clone();
$name.update_component_access(&mut intermediate);
_intersected_access.extend_intersect_filter(&intermediate);
_intersected_access.extend_access(&intermediate);
} else {
$name.update_component_access(&mut _intersected_access);
_not_first = true;
}
)*
*_access = _intersected_access;
}
fn update_archetype_component_access(&self, _archetype: &Archetype, _access: &mut Access<ArchetypeComponentId>) {

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

@ -442,7 +442,34 @@ macro_rules! impl_query_filter_tuple {
fn update_component_access(&self, access: &mut FilteredAccess<ComponentId>) {
let ($($filter,)*) = &self.0;
$($filter.update_component_access(access);)*
// We do not unconditionally add `$filter`'s `with`/`without` accesses to `access`
// as this would be unsound. For example the following two queries should conflict:
// - Query<&mut B, Or<(With<A>, ())>>
// - Query<&mut B, Without<A>>
//
// If we were to unconditionally add `$name`'s `with`/`without` accesses then `Or<(With<A>, ())>`
// would have a `With<A>` access which is incorrect as this `WorldQuery` will match entities that
// do not have the `A` component. This is the same logic as the `AnyOf<...>: WorldQuery` impl.
//
// The correct thing to do here is to only add a `with`/`without` access to `_access` if all
// `$filter` params have that `with`/`without` access. More jargony put- we add the intersection
// of all `with`/`without` accesses of the `$filter` params to `access`.
let mut _intersected_access = access.clone();
let mut _not_first = false;
$(
if _not_first {
let mut intermediate = access.clone();
$filter.update_component_access(&mut intermediate);
_intersected_access.extend_intersect_filter(&intermediate);
_intersected_access.extend_access(&intermediate);
} else {
$filter.update_component_access(&mut _intersected_access);
_not_first = true;
}
)*
*access = _intersected_access;
}
fn update_archetype_component_access(&self, archetype: &Archetype, access: &mut Access<ArchetypeComponentId>) {

60
crates/bevy_ecs/src/system/mod.rs
View File

@ -100,6 +100,7 @@ mod tests {
bundle::Bundles,
component::{Component, Components},
entity::{Entities, Entity},
prelude::AnyOf,
query::{Added, Changed, Or, With, Without},
schedule::{Schedule, Stage, SystemStage},
system::{
@ -281,6 +282,65 @@ mod tests {
assert_eq!(world.resource::<Changed>().0, 2);
}
#[test]
#[should_panic = "error[B0001]"]
fn option_has_no_filter_with() {
fn sys(_: Query<(Option<&A>, &mut B)>, _: Query<&mut B, Without<A>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}
#[test]
fn option_doesnt_remove_unrelated_filter_with() {
fn sys(_: Query<(Option<&A>, &mut B, &A)>, _: Query<&mut B, Without<A>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}
#[test]
#[should_panic = "error[B0001]"]
fn any_of_has_no_filter_with() {
fn sys(_: Query<(AnyOf<(&A, ())>, &mut B)>, _: Query<&mut B, Without<A>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}
#[test]
fn any_of_has_filter_with_when_both_have_it() {
fn sys(_: Query<(AnyOf<(&A, &A)>, &mut B)>, _: Query<&mut B, Without<A>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}
#[test]
fn any_of_doesnt_remove_unrelated_filter_with() {
fn sys(_: Query<(AnyOf<(&A, ())>, &mut B, &A)>, _: Query<&mut B, Without<A>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}
#[test]
#[should_panic = "error[B0001]"]
fn or_has_no_filter_with() {
fn sys(_: Query<&mut B, Or<(With<A>, With<B>)>>, _: Query<&mut B, Without<A>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}
#[test]
fn or_has_filter_with_when_both_have_it() {
fn sys(_: Query<&mut B, Or<(With<A>, With<A>)>>, _: Query<&mut B, Without<A>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}
#[test]
fn or_doesnt_remove_unrelated_filter_with() {
fn sys(_: Query<&mut B, (Or<(With<A>, With<B>)>, With<A>)>, _: Query<&mut B, Without<A>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}
#[test]
#[should_panic]
fn conflicting_query_mut_system() {