Descriptive error message for circular required components recursion (#16648)

# Objective

Fixes #16645

## Solution

Keep track of components in callstack when registering required
components.

## Testing

Added a test checking that the error fires.

---

## Showcase

```rust
#[derive(Component, Default)]
#[require(B)]
struct A;

#[derive(Component, Default)]
#[require(A)]
struct B;
World::new().spawn(A);
```

```
thread 'main' panicked at /home/vj/workspace/rust/bevy/crates/bevy_ecs/src/component.rs:415:13:
Recursive required components detected: A → B → A
```

---------

Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>

crates/bevy_ecs/macros/src/component.rs

@@ -88,7 +88,8 @@ pub fn derive_component(input: TokenStream) -> TokenStream {
                     components,
                     storages,
                     required_components,
-                    inheritance_depth + 1
+                    inheritance_depth + 1,
+                    recursion_check_stack
                 );
             });
             match &require.func {
@@ -98,7 +99,8 @@ pub fn derive_component(input: TokenStream) -> TokenStream {
                             storages,
                             required_components,
                             || { let x: #ident = #func().into(); x },
-                            inheritance_depth
+                            inheritance_depth,
+                            recursion_check_stack
                         );
                     });
                 }
@@ -108,7 +110,8 @@ pub fn derive_component(input: TokenStream) -> TokenStream {
                             storages,
                             required_components,
                             || { let x: #ident = (#func)().into(); x },
-                            inheritance_depth
+                            inheritance_depth,
+                            recursion_check_stack
                         );
                     });
                 }
@@ -118,7 +121,8 @@ pub fn derive_component(input: TokenStream) -> TokenStream {
                             storages,
                             required_components,
                             <#ident as Default>::default,
-                            inheritance_depth
+                            inheritance_depth,
+                            recursion_check_stack
                         );
                     });
                 }
@@ -145,9 +149,14 @@ pub fn derive_component(input: TokenStream) -> TokenStream {
                 storages: &mut #bevy_ecs_path::storage::Storages,
                 required_components: &mut #bevy_ecs_path::component::RequiredComponents,
                 inheritance_depth: u16,
+                recursion_check_stack: &mut Vec<#bevy_ecs_path::component::ComponentId>
             ) {
+                #bevy_ecs_path::component::enforce_no_required_components_recursion(components, recursion_check_stack);
+                let self_id = components.register_component::<Self>(storages);
+                recursion_check_stack.push(self_id);
                 #(#register_required)*
                 #(#register_recursive_requires)*
+                recursion_check_stack.pop();
             }
 
             #[allow(unused_variables)]

crates/bevy_ecs/src/bundle.rs

@@ -227,6 +227,7 @@ unsafe impl<C: Component> Bundle for C {
             storages,
             required_components,
             0,
+            &mut Vec::new(),
         );
     }
 

crates/bevy_ecs/src/component.rs

@@ -27,6 +27,7 @@ use core::{
     marker::PhantomData,
     mem::needs_drop,
 };
+use disqualified::ShortName;
 use thiserror::Error;
 
 pub use bevy_ecs_macros::require;
@@ -404,6 +405,7 @@ pub trait Component: Send + Sync + 'static {
         _storages: &mut Storages,
         _required_components: &mut RequiredComponents,
         _inheritance_depth: u16,
+        _recursion_check_stack: &mut Vec<ComponentId>,
     ) {
     }
 
@@ -1075,7 +1077,16 @@ impl Components {
     /// * [`Components::register_component_with_descriptor()`]
     #[inline]
     pub fn register_component<T: Component>(&mut self, storages: &mut Storages) -> ComponentId {
-        let mut registered = false;
+        self.register_component_internal::<T>(storages, &mut Vec::new())
+    }
+
+    #[inline]
+    fn register_component_internal<T: Component>(
+        &mut self,
+        storages: &mut Storages,
+        recursion_check_stack: &mut Vec<ComponentId>,
+    ) -> ComponentId {
+        let mut is_new_registration = false;
         let id = {
             let Components {
                 indices,
@@ -1089,13 +1100,20 @@ impl Components {
                     storages,
                     ComponentDescriptor::new::<T>(),
                 );
-                registered = true;
+                is_new_registration = true;
                 id
             })
         };
-        if registered {
+        if is_new_registration {
             let mut required_components = RequiredComponents::default();
-            T::register_required_components(id, self, storages, &mut required_components, 0);
+            T::register_required_components(
+                id,
+                self,
+                storages,
+                &mut required_components,
+                0,
+                recursion_check_stack,
+            );
             let info = &mut self.components[id.index()];
             T::register_component_hooks(&mut info.hooks);
             info.required_components = required_components;
@@ -1358,6 +1376,9 @@ impl Components {
     /// A direct requirement has a depth of `0`, and each level of inheritance increases the depth by `1`.
     /// Lower depths are more specific requirements, and can override existing less specific registrations.
     ///
+    /// The `recursion_check_stack` allows checking whether this component tried to register itself as its
+    /// own (indirect) required component.
+    ///
     /// This method does *not* register any components as required by components that require `T`.
     ///
     /// Only use this method if you know what you are doing. In most cases, you should instead use [`World::register_required_components`],
@@ -1371,9 +1392,10 @@ impl Components {
         required_components: &mut RequiredComponents,
         constructor: fn() -> R,
         inheritance_depth: u16,
+        recursion_check_stack: &mut Vec<ComponentId>,
     ) {
-        let requiree = self.register_component::<T>(storages);
+        let requiree = self.register_component_internal::<T>(storages, recursion_check_stack);
-        let required = self.register_component::<R>(storages);
+        let required = self.register_component_internal::<R>(storages, recursion_check_stack);
 
         // SAFETY: We just created the components.
         unsafe {
@@ -2044,6 +2066,40 @@ impl RequiredComponents {
     }
 }
 
+// NOTE: This should maybe be private, but it is currently public so that `bevy_ecs_macros` can use it.
+// This exists as a standalone function instead of being inlined into the component derive macro so as
+// to reduce the amount of generated code.
+#[doc(hidden)]
+pub fn enforce_no_required_components_recursion(
+    components: &Components,
+    recursion_check_stack: &[ComponentId],
+) {
+    if let Some((&requiree, check)) = recursion_check_stack.split_last() {
+        if let Some(direct_recursion) = check
+            .iter()
+            .position(|&id| id == requiree)
+            .map(|index| index == check.len() - 1)
+        {
+            panic!(
+                "Recursive required components detected: {}\nhelp: {}",
+                recursion_check_stack
+                    .iter()
+                    .map(|id| format!("{}", ShortName(components.get_name(*id).unwrap())))
+                    .collect::<Vec<_>>()
+                    .join(" → "),
+                if direct_recursion {
+                    format!(
+                        "Remove require({})",
+                        ShortName(components.get_name(requiree).unwrap())
+                    )
+                } else {
+                    "If this is intentional, consider merging the components.".into()
+                }
+            );
+        }
+    }
+}
+
 /// Component [clone handler function](ComponentCloneFn) implemented using the [`Clone`] trait.
 /// Can be [set](ComponentCloneHandlers::set_component_handler) as clone handler for the specific component it is implemented for.
 /// It will panic if set as handler for any other component.

crates/bevy_ecs/src/lib.rs

@@ -2096,7 +2096,7 @@ mod tests {
     }
 
     #[test]
-    fn remove_component_and_his_runtime_required_components() {
+    fn remove_component_and_its_runtime_required_components() {
         #[derive(Component)]
         struct X;
 
@@ -2141,7 +2141,7 @@ mod tests {
     }
 
     #[test]
-    fn remove_component_and_his_required_components() {
+    fn remove_component_and_its_required_components() {
         #[derive(Component)]
         #[require(Y)]
         struct X;
@@ -2592,6 +2592,24 @@ mod tests {
         assert_eq!(to_vec(required_z), vec![(b, 0), (c, 1)]);
     }
 
+    #[test]
+    #[should_panic = "Recursive required components detected: A → B → C → B\nhelp: If this is intentional, consider merging the components."]
+    fn required_components_recursion_errors() {
+        #[derive(Component, Default)]
+        #[require(B)]
+        struct A;
+
+        #[derive(Component, Default)]
+        #[require(C)]
+        struct B;
+
+        #[derive(Component, Default)]
+        #[require(B)]
+        struct C;
+
+        World::new().register_component::<A>();
+    }
+
     // These structs are primarily compilation tests to test the derive macros. Because they are
     // never constructed, we have to manually silence the `dead_code` lint.
     #[allow(dead_code)]