From 5f1e114209731975814cfd275e1e8f1fb29618d4 Mon Sep 17 00:00:00 2001 From: SpecificProtagonist Date: Wed, 11 Dec 2024 02:26:35 +0100 Subject: [PATCH] Descriptive error message for circular required components recursion (#16648) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # 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 | 17 +++++-- crates/bevy_ecs/src/bundle.rs | 1 + crates/bevy_ecs/src/component.rs | 68 ++++++++++++++++++++++--- crates/bevy_ecs/src/lib.rs | 22 +++++++- 4 files changed, 96 insertions(+), 12 deletions(-) diff --git a/crates/bevy_ecs/macros/src/component.rs b/crates/bevy_ecs/macros/src/component.rs index 82949e8a92..84c69467a7 100644 --- a/crates/bevy_ecs/macros/src/component.rs +++ b/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::(storages); + recursion_check_stack.push(self_id); #(#register_required)* #(#register_recursive_requires)* + recursion_check_stack.pop(); } #[allow(unused_variables)] diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 97833a1d7d..8c552b3763 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -227,6 +227,7 @@ unsafe impl Bundle for C { storages, required_components, 0, + &mut Vec::new(), ); } diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 56e06315e5..e6eaafb900 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/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, ) { } @@ -1075,7 +1077,16 @@ impl Components { /// * [`Components::register_component_with_descriptor()`] #[inline] pub fn register_component(&mut self, storages: &mut Storages) -> ComponentId { - let mut registered = false; + self.register_component_internal::(storages, &mut Vec::new()) + } + + #[inline] + fn register_component_internal( + &mut self, + storages: &mut Storages, + recursion_check_stack: &mut Vec, + ) -> ComponentId { + let mut is_new_registration = false; let id = { let Components { indices, @@ -1089,13 +1100,20 @@ impl Components { storages, ComponentDescriptor::new::(), ); - 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, ) { - let requiree = self.register_component::(storages); - let required = self.register_component::(storages); + let requiree = self.register_component_internal::(storages, recursion_check_stack); + let required = self.register_component_internal::(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::>() + .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. diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index c7def12de3..a4eba6880e 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/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::(); + } + // 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)]