Check for conflicting system resource parameters (#864)

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<ArchetypeComponent>,
     pub(crate) resource_access: TypeAccess<TypeId>,
+    pub(crate) local_resource_access: TypeAccess<TypeId>,
     pub(crate) query_archetype_component_accesses: Vec<TypeAccess<ArchetypeComponent>>,
     pub(crate) query_accesses: Vec<Vec<QueryAccess>>,
     pub(crate) query_type_names: Vec<&'static str>,
@@ -148,6 +149,7 @@ macro_rules! impl_into_system {
                         name: std::any::type_name::<Self>().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<u8>,
+    }
+
+    fn test_for_conflicting_resources(sys: Box<dyn System>) {
+        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<BufferRes>, _: Res<BufferRes>) {}
+        test_for_conflicting_resources(sys.system())
+    }
+
+    #[test]
+    #[should_panic]
+    fn conflicting_system_resources_reverse_order() {
+        fn sys(_: Res<BufferRes>, _: ResMut<BufferRes>) {}
+        test_for_conflicting_resources(sys.system())
+    }
+
+    #[test]
+    #[should_panic]
+    fn conflicting_system_resources_multiple_mutable() {
+        fn sys(_: ResMut<BufferRes>, _: ResMut<BufferRes>) {}
+        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<BufferRes>, _: ChangedRes<BufferRes>) {}
+        test_for_conflicting_resources(sys.system())
+    }
+
+    #[test]
+    #[should_panic]
+    fn conflicting_system_local_resources() {
+        fn sys(_: Local<BufferRes>, _: Local<BufferRes>) {}
+        test_for_conflicting_resources(sys.system())
+    }
+
+    #[test]
+    fn nonconflicting_system_resources() {
+        fn sys(_: Local<BufferRes>, _: ResMut<BufferRes>, _: Local<A>, _: ResMut<A>) {}
+        test_for_conflicting_resources(sys.system())
+    }
 }

crates/bevy_ecs/src/system/system_param.rs

@@ -113,6 +113,14 @@ impl SystemParam for Arc<Mutex<Commands>> {
 
 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::<T>()) {
+            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::<T>()
+            );
+        }
         system_state.resource_access.add_read(TypeId::of::<T>());
     }
 
@@ -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<Foo>, ResMut<Foo>)` or `fn(ResMut<Foo>, ResMut<Foo>)` must not be allowed.
+        if system_state
+            .resource_access
+            .is_read_or_write(&TypeId::of::<T>())
+        {
+            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::<T>()
+            );
+        }
         system_state.resource_access.add_write(TypeId::of::<T>());
     }
 
@@ -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::<T>()) {
+            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::<T>()
+            );
+        }
         system_state.resource_access.add_read(TypeId::of::<T>());
     }
 
@@ -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::<T>());
+        if system_state
+            .local_resource_access
+            .is_read_or_write(&TypeId::of::<T>())
+        {
+            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::<T>()
+            );
+        }
+
+        // A resource could have been already initialized by another system with
+        // `Commands::insert_local_resource` or `Resources::insert_local`
         if resources.get_local::<T>(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::<T>());
     }
 
     #[inline]