From 613cbd5437f1d859e24ac4b445023c914f2a17a1 Mon Sep 17 00:00:00 2001 From: memoryruins Date: Sat, 14 Nov 2020 09:15:35 -0500 Subject: [PATCH] Check for conflicting system resource parameters --- crates/bevy_ecs/src/system/into_system.rs | 64 +++++++++++++++++++++- crates/bevy_ecs/src/system/system_param.rs | 48 +++++++++++++++- 2 files changed, 108 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/system/into_system.rs b/crates/bevy_ecs/src/system/into_system.rs index 548f25a7e7ca5..9745c3fe3746c 100644 --- a/crates/bevy_ecs/src/system/into_system.rs +++ b/crates/bevy_ecs/src/system/into_system.rs @@ -9,6 +9,7 @@ pub struct SystemState { pub(crate) is_initialized: bool, pub(crate) archetype_component_access: TypeAccess, pub(crate) resource_access: TypeAccess, + pub(crate) local_resource_access: TypeAccess, pub(crate) query_archetype_component_accesses: Vec>, pub(crate) query_accesses: Vec>, pub(crate) query_type_names: Vec<&'static str>, @@ -148,6 +149,7 @@ macro_rules! impl_into_system { name: std::any::type_name::().into(), archetype_component_access: TypeAccess::default(), resource_access: TypeAccess::default(), + local_resource_access: TypeAccess::default(), is_initialized: false, id: SystemId::new(), commands: Commands::default(), @@ -204,13 +206,13 @@ impl_into_system!(A, B, C, D, E, F, G, H, I, J, K, L, M, N, O, P); mod tests { use super::IntoSystem; use crate::{ - resource::{ResMut, Resources}, + resource::{Res, ResMut, Resources}, schedule::Schedule, - ChangedRes, Query, QuerySet, System, + ChangedRes, Local, Query, QuerySet, System, }; use bevy_hecs::{Entity, Or, With, World}; - #[derive(Debug, Eq, PartialEq)] + #[derive(Debug, Eq, PartialEq, Default)] struct A; struct B; struct C; @@ -441,4 +443,60 @@ mod tests { schedule.initialize(world, resources); schedule.run(world, resources); } + + #[derive(Default)] + struct BufferRes { + _buffer: Vec, + } + + fn test_for_conflicting_resources(sys: Box) { + let mut world = World::default(); + let mut resources = Resources::default(); + resources.insert(BufferRes::default()); + resources.insert(A); + resources.insert(B); + run_system(&mut world, &mut resources, sys); + } + + #[test] + #[should_panic] + fn conflicting_system_resources() { + fn sys(_: ResMut, _: Res) {} + test_for_conflicting_resources(sys.system()) + } + + #[test] + #[should_panic] + fn conflicting_system_resources_reverse_order() { + fn sys(_: Res, _: ResMut) {} + test_for_conflicting_resources(sys.system()) + } + + #[test] + #[should_panic] + fn conflicting_system_resources_multiple_mutable() { + fn sys(_: ResMut, _: ResMut) {} + test_for_conflicting_resources(sys.system()) + } + + #[test] + #[should_panic] + fn conflicting_changed_and_mutable_resource() { + // A tempting pattern, but unsound if allowed. + fn sys(_: ResMut, _: ChangedRes) {} + test_for_conflicting_resources(sys.system()) + } + + #[test] + #[should_panic] + fn conflicting_system_local_resources() { + fn sys(_: Local, _: Local) {} + test_for_conflicting_resources(sys.system()) + } + + #[test] + fn nonconflicting_system_resources() { + fn sys(_: Local, _: ResMut, _: Local, _: ResMut) {} + test_for_conflicting_resources(sys.system()) + } } diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 047778d56e127..eee3ac1af94a6 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -113,6 +113,14 @@ impl SystemParam for Arc> { impl<'a, T: Resource> SystemParam for Res<'a, T> { fn init(system_state: &mut SystemState, _world: &World, _resources: &mut Resources) { + if system_state.resource_access.is_write(&TypeId::of::()) { + panic!( + "System `{}` has a `Res<{res}>` parameter that conflicts with \ + another parameter with mutable access to the same `{res}` resource.", + system_state.name, + res = std::any::type_name::() + ); + } system_state.resource_access.add_read(TypeId::of::()); } @@ -130,6 +138,19 @@ impl<'a, T: Resource> SystemParam for Res<'a, T> { impl<'a, T: Resource> SystemParam for ResMut<'a, T> { fn init(system_state: &mut SystemState, _world: &World, _resources: &mut Resources) { + // If a system already has access to the resource in another parameter, then we fail early. + // e.g. `fn(Res, ResMut)` or `fn(ResMut, ResMut)` must not be allowed. + if system_state + .resource_access + .is_read_or_write(&TypeId::of::()) + { + panic!( + "System `{}` has a `ResMut<{res}>` parameter that conflicts with \ + another parameter to the same `{res}` resource. `ResMut` must have unique access.", + system_state.name, + res = std::any::type_name::() + ); + } system_state.resource_access.add_write(TypeId::of::()); } @@ -147,6 +168,14 @@ impl<'a, T: Resource> SystemParam for ResMut<'a, T> { impl<'a, T: Resource> SystemParam for ChangedRes<'a, T> { fn init(system_state: &mut SystemState, _world: &World, _resources: &mut Resources) { + if system_state.resource_access.is_write(&TypeId::of::()) { + panic!( + "System `{}` has a `ChangedRes<{res}>` parameter that conflicts with \ + another parameter with mutable access to the same `{res}` resource.", + system_state.name, + res = std::any::type_name::() + ); + } system_state.resource_access.add_read(TypeId::of::()); } @@ -169,11 +198,28 @@ impl<'a, T: Resource> SystemParam for ChangedRes<'a, T> { impl<'a, T: Resource + FromResources> SystemParam for Local<'a, T> { fn init(system_state: &mut SystemState, _world: &World, resources: &mut Resources) { - system_state.resource_access.add_write(TypeId::of::()); + if system_state + .local_resource_access + .is_read_or_write(&TypeId::of::()) + { + panic!( + "System `{}` has multiple parameters requesting access to a local resource of type `{}`. \ + There may be at most one `Local` parameter per resource type.", + system_state.name, + std::any::type_name::() + ); + } + + // A resource could have been already initialized by another system with + // `Commands::insert_local_resource` or `Resources::insert_local` if resources.get_local::(system_state.id).is_none() { let value = T::from_resources(resources); resources.insert_local(system_state.id, value); } + + system_state + .local_resource_access + .add_write(TypeId::of::()); } #[inline]