From 0b35eb7423c0eb38cb8768b842db6b403f2276d4 Mon Sep 17 00:00:00 2001 From: SpecificProtagonist Date: Wed, 4 Dec 2024 16:45:05 +0100 Subject: [PATCH 1/5] Descriptive error message --- crates/bevy_ecs/macros/src/component.rs | 17 ++++++-- crates/bevy_ecs/src/bundle.rs | 1 + crates/bevy_ecs/src/component.rs | 55 ++++++++++++++++++++++--- crates/bevy_ecs/src/lib.rs | 14 +++++++ 4 files changed, 77 insertions(+), 10 deletions(-) diff --git a/crates/bevy_ecs/macros/src/component.rs b/crates/bevy_ecs/macros/src/component.rs index 9a205b91e7a5a..0270eb589439a 100644 --- a/crates/bevy_ecs/macros/src/component.rs +++ b/crates/bevy_ecs/macros/src/component.rs @@ -87,7 +87,8 @@ pub fn derive_component(input: TokenStream) -> TokenStream { components, storages, required_components, - inheritance_depth + 1 + inheritance_depth + 1, + recursion_check_stack ); }); match &require.func { @@ -97,7 +98,8 @@ pub fn derive_component(input: TokenStream) -> TokenStream { storages, required_components, || { let x: #ident = #func().into(); x }, - inheritance_depth + inheritance_depth, + recursion_check_stack ); }); } @@ -107,7 +109,8 @@ pub fn derive_component(input: TokenStream) -> TokenStream { storages, required_components, || { let x: #ident = (#func)().into(); x }, - inheritance_depth + inheritance_depth, + recursion_check_stack ); }); } @@ -117,7 +120,8 @@ pub fn derive_component(input: TokenStream) -> TokenStream { storages, required_components, <#ident as Default>::default, - inheritance_depth + inheritance_depth, + recursion_check_stack ); }); } @@ -138,9 +142,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 3da2339c0d501..7faafd4df0d95 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 5bdaeb024a11c..eb607d62cd097 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -390,6 +390,7 @@ pub trait Component: Send + Sync + 'static { _storages: &mut Storages, _required_components: &mut RequiredComponents, _inheritance_depth: u16, + _recursion_check_stack: &mut Vec, ) { } @@ -401,6 +402,28 @@ pub trait Component: Send + Sync + 'static { } } +// 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 check.contains(requiree) { + panic!( + "Recursive required components detected: {}", + recursion_check_stack + .iter() + .map(|id| components.get_name(*id).unwrap()) + .collect::>() + .join(" → ") + ); + } + } +} + /// The storage used for a specific component type. /// /// # Examples @@ -987,7 +1010,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, @@ -1001,13 +1033,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; @@ -1270,6 +1309,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`], @@ -1283,9 +1325,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 { diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index fbf4645a192fc..ccf2a67358671 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -2552,6 +2552,20 @@ mod tests { assert_eq!(to_vec(required_z), vec![(b, 0), (c, 1)]); } + #[test] + #[should_panic = "Recursive required components detected: bevy_ecs::tests::required_components_recursion_errors::A → bevy_ecs::tests::required_components_recursion_errors::B → bevy_ecs::tests::required_components_recursion_errors::A"] + fn required_components_recursion_errors() { + #[derive(Component, Default)] + #[require(B)] + struct A; + + #[derive(Component, Default)] + #[require(A)] + struct B; + + 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)] From f8edccaeffc0e02f1c8583a26d783e8ec9f9ec07 Mon Sep 17 00:00:00 2001 From: SpecificProtagonist Date: Thu, 5 Dec 2024 15:18:54 +0100 Subject: [PATCH 2/5] add help message --- crates/bevy_ecs/src/component.rs | 22 ++++++++++++++++++---- crates/bevy_ecs/src/lib.rs | 6 +++--- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index eb607d62cd097..4be42b3d4a8da 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -28,6 +28,7 @@ use core::{ mem::needs_drop, }; use derive_more::derive::{Display, Error}; +use disqualified::ShortName; pub use bevy_ecs_macros::require; @@ -410,15 +411,28 @@ pub fn enforce_no_required_components_recursion( components: &Components, recursion_check_stack: &[ComponentId], ) { - if let Some((requiree, check)) = recursion_check_stack.split_last() { - if check.contains(requiree) { + if let Some((&requiree, check)) = recursion_check_stack.split_last() { + if let Some(direct_recursion) = check + .iter() + .rev() + .enumerate() + .find_map(|(index, &id)| (id == requiree).then_some(index == 0)) + { panic!( - "Recursive required components detected: {}", + "Recursive required components detected: {}\nhelp: {}", recursion_check_stack .iter() .map(|id| components.get_name(*id).unwrap()) .collect::>() - .join(" → ") + .join(" → "), + if direct_recursion { + format!( + "Remove require({})", + ShortName(components.get_name(requiree).unwrap()) + ) + } else { + "If this is intentional, consider merging the components.".into() + } ); } } diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index ccf2a67358671..a8fddc6af2429 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -2056,7 +2056,7 @@ mod tests { } #[test] - fn remove_component_and_his_runtime_required_components() { + fn remove_component_and_its_runtime_required_components() { #[derive(Component)] struct X; @@ -2101,7 +2101,7 @@ mod tests { } #[test] - fn remove_component_and_his_required_components() { + fn remove_component_and_its_required_components() { #[derive(Component)] #[require(Y)] struct X; @@ -2553,7 +2553,7 @@ mod tests { } #[test] - #[should_panic = "Recursive required components detected: bevy_ecs::tests::required_components_recursion_errors::A → bevy_ecs::tests::required_components_recursion_errors::B → bevy_ecs::tests::required_components_recursion_errors::A"] + #[should_panic = "Recursive required components detected: bevy_ecs::tests::required_components_recursion_errors::A → bevy_ecs::tests::required_components_recursion_errors::B → bevy_ecs::tests::required_components_recursion_errors::A\nhelp: If this is intentional, consider merging the components."] fn required_components_recursion_errors() { #[derive(Component, Default)] #[require(B)] From e800e41735974b56f79920d71ca5e8d76b992364 Mon Sep 17 00:00:00 2001 From: SpecificProtagonist Date: Sat, 7 Dec 2024 03:10:26 +0100 Subject: [PATCH 3/5] use ShortName Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com> --- crates/bevy_ecs/src/component.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 4be42b3d4a8da..10cb82e50c8b4 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -422,7 +422,7 @@ pub fn enforce_no_required_components_recursion( "Recursive required components detected: {}\nhelp: {}", recursion_check_stack .iter() - .map(|id| components.get_name(*id).unwrap()) + .map(|id| ShortName(components.get_name(*id).unwrap())) .collect::>() .join(" → "), if direct_recursion { From bc40d611eae0fa77e67762b67bd8769566b7434a Mon Sep 17 00:00:00 2001 From: SpecificProtagonist Date: Sat, 7 Dec 2024 03:12:57 +0100 Subject: [PATCH 4/5] Use Iter::position --- crates/bevy_ecs/src/component.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 10cb82e50c8b4..a83353375b605 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -414,15 +414,14 @@ pub fn enforce_no_required_components_recursion( if let Some((&requiree, check)) = recursion_check_stack.split_last() { if let Some(direct_recursion) = check .iter() - .rev() - .enumerate() - .find_map(|(index, &id)| (id == requiree).then_some(index == 0)) + .position(|&id| id == requiree) + .map(|index| index == check.len() - 1) { panic!( "Recursive required components detected: {}\nhelp: {}", recursion_check_stack .iter() - .map(|id| ShortName(components.get_name(*id).unwrap())) + .map(|id| format!("{}", ShortName(components.get_name(*id).unwrap()))) .collect::>() .join(" → "), if direct_recursion { From 3e887898c573e854a48e9c39be8ee65d0787d949 Mon Sep 17 00:00:00 2001 From: SpecificProtagonist Date: Sat, 7 Dec 2024 04:26:48 +0100 Subject: [PATCH 5/5] Update test --- crates/bevy_ecs/src/lib.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index a8fddc6af2429..f181fdac11257 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -2553,16 +2553,20 @@ mod tests { } #[test] - #[should_panic = "Recursive required components detected: bevy_ecs::tests::required_components_recursion_errors::A → bevy_ecs::tests::required_components_recursion_errors::B → bevy_ecs::tests::required_components_recursion_errors::A\nhelp: If this is intentional, consider merging the components."] + #[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(A)] + #[require(C)] struct B; + #[derive(Component, Default)] + #[require(B)] + struct C; + World::new().register_component::(); }