Skip to content

Commit

Permalink
AnyOf soundness fix (bevyengine#14013)
Browse files Browse the repository at this point in the history
# Objective
Fixes bevyengine#13993 
PR inspired by bevyengine#14007 to
accomplish the same thing, but maybe in a clearer fashion.

@Gingeh feel free to take my changes and add them to your PR, I don't
want to steal any credit

---------

Co-authored-by: Gingeh <[email protected]>
Co-authored-by: Bob Gardner <[email protected]>
Co-authored-by: Martín Maita <[email protected]>
  • Loading branch information
4 people authored Jun 25, 2024
1 parent 336fddb commit 8308ad0
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 5 deletions.
14 changes: 9 additions & 5 deletions crates/bevy_ecs/src/query/fetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1862,24 +1862,28 @@ macro_rules! impl_anytuple_fetch {
}

fn update_component_access(state: &Self::State, _access: &mut FilteredAccess<ComponentId>) {
let ($($name,)*) = state;

let mut _new_access = _access.clone();

// update the access (add the read/writes)
<($(Option<$name>,)*)>::update_component_access(state, _access);

// update the filters (Or<(With<$name>,)>)
let ($($name,)*) = state;
let mut _not_first = false;
$(
if _not_first {
let mut intermediate = _access.clone();
// we use an intermediate access because we only want to update the filters, not the access
let mut intermediate = FilteredAccess::default();
$name::update_component_access($name, &mut intermediate);
_new_access.append_or(&intermediate);
_new_access.extend_access(&intermediate);
} else {
$name::update_component_access($name, &mut _new_access);
_new_access.required = _access.required.clone();
_not_first = true;
}
)*

*_access = _new_access;
_access.filter_sets = _new_access.filter_sets;
}
#[allow(unused_variables)]
fn init_state(world: &mut World) -> Self::State {
Expand Down
53 changes: 53 additions & 0 deletions crates/bevy_ecs/src/system/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -556,6 +556,51 @@ mod tests {
run_system(&mut world, sys);
}

#[test]
fn any_of_working() {
fn sys(_: Query<AnyOf<(&mut A, &B)>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}

#[test]
#[should_panic = "&bevy_ecs::system::tests::A conflicts with a previous access in this query."]
fn any_of_with_mut_and_ref() {
fn sys(_: Query<AnyOf<(&mut A, &A)>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}

#[test]
#[should_panic = "&mut bevy_ecs::system::tests::A conflicts with a previous access in this query."]
fn any_of_with_ref_and_mut() {
fn sys(_: Query<AnyOf<(&A, &mut A)>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}

#[test]
#[should_panic = "&bevy_ecs::system::tests::A conflicts with a previous access in this query."]
fn any_of_with_mut_and_option() {
fn sys(_: Query<AnyOf<(&mut A, Option<&A>)>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}

#[test]
fn any_of_with_entity_and_mut() {
fn sys(_: Query<AnyOf<(Entity, &mut A)>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}

#[test]
fn any_of_with_empty_and_mut() {
fn sys(_: Query<AnyOf<((), &mut A)>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}

#[test]
#[should_panic = "error[B0001]"]
fn any_of_has_no_filter_with() {
Expand All @@ -564,6 +609,14 @@ mod tests {
run_system(&mut world, sys);
}

#[test]
#[should_panic = "&mut bevy_ecs::system::tests::A conflicts with a previous access in this query."]
fn any_of_with_conflicting() {
fn sys(_: Query<AnyOf<(&mut A, &mut 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>>) {}
Expand Down

0 comments on commit 8308ad0

Please sign in to comment.