System::type_id Consistency (#11728)

# Objective - Fixes #11679 ## Solution - Added `IntoSystem::system_type_id` which returns the equivalent of `system.into_system().type_id()` without construction. This allows for getting the `TypeId` of functions (a function is an unnamed type and therefore you cannot call `TypeId::of::<apply_deferred::System>()`) - Added default implementation of `System::type_id` to ensure consistency between implementations. Some returned `Self`, while others were returning an inner value instead. This ensures consistency with `IntoSystem::system_type_id`. ## Migration Guide If you use `System::type_id()` on function systems (exclusive or not), ensure you are comparing its value to other `System::type_id()` calls, or `IntoSystem::system_type_id()`. This code wont require any changes, because `IntoSystem`'s are directly compared to each other. ```rust fn test_system() {} let type_id = test_system.type_id(); // ... // No change required assert_eq!(test_system.type_id(), type_id); ``` Likewise, this code wont, because `System`'s are directly compared. ```rust fn test_system() {} let type_id = IntoSystem::into_system(test_system).type_id(); // ... // No change required assert_eq!(IntoSystem::into_system(test_system).type_id(), type_id); ``` The below _does_ require a change, since you're comparing a `System` type to a `IntoSystem` type. ```rust fn test_system() {} // Before assert_eq!(test_system.type_id(), IntoSystem::into_system(test_system).type_id()); // After assert_eq!(test_system.system_type_id(), IntoSystem::into_system(test_system).type_id()); ```
bevyengine · Feb 6, 2024 · 950bd22 · 950bd22
1 parent f2cb155
commit 950bd22
Show file tree

Hide file tree

Showing 8 changed files with 102 additions and 32 deletions.
diff --git a/crates/bevy_ecs/src/schedule/executor/mod.rs b/crates/bevy_ecs/src/schedule/executor/mod.rs
@@ -104,7 +104,7 @@ pub fn apply_deferred(world: &mut World) {}
 
 /// Returns `true` if the [`System`](crate::system::System) is an instance of [`apply_deferred`].
 pub(super) fn is_apply_deferred(system: &BoxedSystem) -> bool {
-    use std::any::Any;
+    use crate::system::IntoSystem;
     // deref to use `System::type_id` instead of `Any::type_id`
-    system.as_ref().type_id() == apply_deferred.type_id()
+    system.as_ref().type_id() == apply_deferred.system_type_id()
 }
diff --git a/crates/bevy_ecs/src/schedule/stepping.rs b/crates/bevy_ecs/src/schedule/stepping.rs
@@ -4,7 +4,7 @@ use std::collections::HashMap;
 
 use crate::{
     schedule::{InternedScheduleLabel, NodeId, Schedule, ScheduleLabel},
-    system::{IntoSystem, ResMut, Resource, System},
+    system::{IntoSystem, ResMut, Resource},
 };
 use bevy_utils::{
     thiserror::Error,
@@ -252,10 +252,7 @@ impl Stepping {
         schedule: impl ScheduleLabel,
         system: impl IntoSystem<(), (), Marker>,
     ) -> &mut Self {
-        // PERF: ideally we don't actually need to construct the system to retrieve the TypeId.
-        // Unfortunately currently IntoSystem::into_system(system).type_id() != TypeId::of::<I::System>()
-        // If these are aligned, we can use TypeId::of::<I::System>() here
-        let type_id = IntoSystem::into_system(system).type_id();
+        let type_id = system.system_type_id();
         self.updates.push(Update::SetBehavior(
             schedule.intern(),
             SystemIdentifier::Type(type_id),
@@ -281,7 +278,7 @@ impl Stepping {
         schedule: impl ScheduleLabel,
         system: impl IntoSystem<(), (), Marker>,
     ) -> &mut Self {
-        let type_id = IntoSystem::into_system(system).type_id();
+        let type_id = system.system_type_id();
         self.updates.push(Update::SetBehavior(
             schedule.intern(),
             SystemIdentifier::Type(type_id),
@@ -307,7 +304,7 @@ impl Stepping {
         schedule: impl ScheduleLabel,
         system: impl IntoSystem<(), (), Marker>,
     ) -> &mut Self {
-        let type_id = IntoSystem::into_system(system).type_id();
+        let type_id = system.system_type_id();
         self.updates.push(Update::SetBehavior(
             schedule.intern(),
             SystemIdentifier::Type(type_id),
@@ -354,7 +351,7 @@ impl Stepping {
         schedule: impl ScheduleLabel,
         system: impl IntoSystem<(), (), Marker>,
     ) -> &mut Self {
-        let type_id = IntoSystem::into_system(system).type_id();
+        let type_id = system.system_type_id();
         self.updates.push(Update::ClearBehavior(
             schedule.intern(),
             SystemIdentifier::Type(type_id),

diff --git a/crates/bevy_ecs/src/system/adapter_system.rs b/crates/bevy_ecs/src/system/adapter_system.rs
@@ -81,10 +81,6 @@ where
         self.name.clone()
     }
 
-    fn type_id(&self) -> std::any::TypeId {
-        std::any::TypeId::of::<Self>()
-    }
-
     fn component_access(&self) -> &crate::query::Access<crate::component::ComponentId> {
         self.system.component_access()
     }

diff --git a/crates/bevy_ecs/src/system/combinator.rs b/crates/bevy_ecs/src/system/combinator.rs
@@ -142,10 +142,6 @@ where
         self.name.clone()
     }
 
-    fn type_id(&self) -> std::any::TypeId {
-        std::any::TypeId::of::<Self>()
-    }
-
     fn component_access(&self) -> &Access<ComponentId> {
         &self.component_access
     }

diff --git a/crates/bevy_ecs/src/system/exclusive_function_system.rs b/crates/bevy_ecs/src/system/exclusive_function_system.rs
@@ -11,7 +11,7 @@ use crate::{
 };
 
 use bevy_utils::all_tuples;
-use std::{any::TypeId, borrow::Cow, marker::PhantomData};
+use std::{borrow::Cow, marker::PhantomData};
 
 /// A function system that runs with exclusive [`World`] access.
 ///
@@ -65,11 +65,6 @@ where
         self.system_meta.name.clone()
     }
 
-    #[inline]
-    fn type_id(&self) -> TypeId {
-        TypeId::of::<F>()
-    }
-
     #[inline]
     fn component_access(&self) -> &Access<ComponentId> {
         self.system_meta.component_access_set.combined_access()
@@ -250,3 +245,44 @@ macro_rules! impl_exclusive_system_function {
 // Note that we rely on the highest impl to be <= the highest order of the tuple impls
 // of `SystemParam` created.
 all_tuples!(impl_exclusive_system_function, 0, 16, F);
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn into_system_type_id_consistency() {
+        fn test<T, In, Out, Marker>(function: T)
+        where
+            T: IntoSystem<In, Out, Marker> + Copy,
+        {
+            fn reference_system(_world: &mut World) {}
+
+            use std::any::TypeId;
+
+            let system = IntoSystem::into_system(function);
+
+            assert_eq!(
+                system.type_id(),
+                function.system_type_id(),
+                "System::type_id should be consistent with IntoSystem::system_type_id"
+            );
+
+            assert_eq!(
+                system.type_id(),
+                TypeId::of::<T::System>(),
+                "System::type_id should be consistent with TypeId::of::<T::System>()"
+            );
+
+            assert_ne!(
+                system.type_id(),
+                IntoSystem::into_system(reference_system).type_id(),
+                "Different systems should have different TypeIds"
+            );
+        }
+
+        fn exclusive_function_system(_world: &mut World) {}
+
+        test(exclusive_function_system);
+    }
+}
diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs
@@ -9,7 +9,7 @@ use crate::{
 };
 
 use bevy_utils::all_tuples;
-use std::{any::TypeId, borrow::Cow, marker::PhantomData};
+use std::{borrow::Cow, marker::PhantomData};
 
 #[cfg(feature = "trace")]
 use bevy_utils::tracing::{info_span, Span};
@@ -453,11 +453,6 @@ where
         self.system_meta.name.clone()
     }
 
-    #[inline]
-    fn type_id(&self) -> TypeId {
-        TypeId::of::<F>()
-    }
-
     #[inline]
     fn component_access(&self) -> &Access<ComponentId> {
         self.system_meta.component_access_set.combined_access()
@@ -695,3 +690,44 @@ macro_rules! impl_system_function {
 // Note that we rely on the highest impl to be <= the highest order of the tuple impls
 // of `SystemParam` created.
 all_tuples!(impl_system_function, 0, 16, F);
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn into_system_type_id_consistency() {
+        fn test<T, In, Out, Marker>(function: T)
+        where
+            T: IntoSystem<In, Out, Marker> + Copy,
+        {
+            fn reference_system() {}
+
+            use std::any::TypeId;
+
+            let system = IntoSystem::into_system(function);
+
+            assert_eq!(
+                system.type_id(),
+                function.system_type_id(),
+                "System::type_id should be consistent with IntoSystem::system_type_id"
+            );
+
+            assert_eq!(
+                system.type_id(),
+                TypeId::of::<T::System>(),
+                "System::type_id should be consistent with TypeId::of::<T::System>()"
+            );
+
+            assert_ne!(
+                system.type_id(),
+                IntoSystem::into_system(reference_system).type_id(),
+                "Different systems should have different TypeIds"
+            );
+        }
+
+        fn function_system() {}
+
+        test(function_system);
+    }
+}
diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs
@@ -114,7 +114,7 @@ mod system_name;
 mod system_param;
 mod system_registry;
 
-use std::borrow::Cow;
+use std::{any::TypeId, borrow::Cow};
 
 pub use adapter_system::*;
 pub use combinator::*;
@@ -195,6 +195,12 @@ pub trait IntoSystem<In, Out, Marker>: Sized {
         let name = system.name();
         AdapterSystem::new(f, system, name)
     }
+
+    /// Get the [`TypeId`] of the [`System`] produced after calling [`into_system`](`IntoSystem::into_system`).
+    #[inline]
+    fn system_type_id(&self) -> TypeId {
+        TypeId::of::<Self::System>()
+    }
 }
 
 // All systems implicitly implement IntoSystem.

diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs
@@ -31,7 +31,10 @@ pub trait System: Send + Sync + 'static {
     /// Returns the system's name.
     fn name(&self) -> Cow<'static, str>;
     /// Returns the [`TypeId`] of the underlying system type.
-    fn type_id(&self) -> TypeId;
+    #[inline]
+    fn type_id(&self) -> TypeId {
+        TypeId::of::<Self>()
+    }
     /// Returns the system's component [`Access`].
     fn component_access(&self) -> &Access<ComponentId>;
     /// Returns the system's archetype component [`Access`].