Fix soundness issue and add more migration docs

crates/bevy_ecs/macros/src/component.rs

@@ -245,8 +245,7 @@ pub fn derive_component(input: TokenStream) -> TokenStream {
                 None => quote! { <#ident as Default>::default },
             };
             register_required.push(quote! {
-                // SAFETY: we registered all components with the same instance of components.
-                unsafe { required_components.register::<#ident>(components, #constructor) };
+                required_components.register_required::<#ident>(#constructor);
             });
         }
     }
@@ -287,10 +286,9 @@ pub fn derive_component(input: TokenStream) -> TokenStream {
         impl #impl_generics #bevy_ecs_path::component::Component for #struct_name #type_generics #where_clause {
             const STORAGE_TYPE: #bevy_ecs_path::component::StorageType = #storage;
             type Mutability = #mutable_type;
-            unsafe fn register_required_components(
+            fn register_required_components(
                 _requiree: #bevy_ecs_path::component::ComponentId,
-                components: &mut #bevy_ecs_path::component::ComponentsRegistrator,
-                required_components: &mut #bevy_ecs_path::component::RequiredComponents,
+                required_components: &mut #bevy_ecs_path::component::RequiredComponentsRegistrator,
             ) {
                 #(#register_required)*
             }

crates/bevy_ecs/src/component/mod.rs

@@ -526,10 +526,9 @@ pub trait Component: Send + Sync + 'static {
     /// # Safety
     ///
     /// - `_required_components` must only contain components valid in `_components`.
-    unsafe fn register_required_components(
+    fn register_required_components(
         _component_id: ComponentId,
-        _components: &mut ComponentsRegistrator,
-        _required_components: &mut RequiredComponents,
+        _required_components: &mut RequiredComponentsRegistrator,
     ) {
     }
 

crates/bevy_ecs/src/component/register.rs

@@ -5,7 +5,7 @@ use core::any::Any;
 use core::ops::DerefMut;
 use core::{any::TypeId, fmt::Debug, ops::Deref};
 
-use crate::component::enforce_no_required_components_recursion;
+use crate::component::{enforce_no_required_components_recursion, RequiredComponentsRegistrator};
 use crate::query::DebugCheckedUnwrap as _;
 use crate::{
     component::{
@@ -232,11 +232,12 @@ impl<'w> ComponentsRegistrator<'w> {
         self.recursion_check_stack.push(id);
         let mut required_components = RequiredComponents::default();
         // SAFETY: `required_components` is empty
-        unsafe { T::register_required_components(id, self, &mut required_components) };
+        let mut required_components_registrator =
+            unsafe { RequiredComponentsRegistrator::new(self, &mut required_components) };
+        T::register_required_components(id, &mut required_components_registrator);
         // SAFETY:
         // - `id` was just registered in `self`
-        // - `register_required_components` have been given `self` to register components in
-        //   (TODO: this is not really true... but the alternative would be making `Component` `unsafe`...)
+        // - RequiredComponentsRegistrator guarantees that only components from `self` are included in `required_components`.
         unsafe { self.register_required_by(id, &required_components) };
         self.recursion_check_stack.pop();
 

crates/bevy_ecs/src/component/required.rs

@@ -536,6 +536,36 @@ pub(super) fn enforce_no_required_components_recursion(
     }
 }
 
+/// This is a safe handle around `ComponentsRegistrator` and `RequiredComponents` to register required components.
+pub struct RequiredComponentsRegistrator<'a, 'w> {
+    components: &'a mut ComponentsRegistrator<'w>,
+    required_components: &'a mut RequiredComponents,
+}
+
+impl<'a, 'w> RequiredComponentsRegistrator<'a, 'w> {
+    /// # Safety
+    ///
+    /// All components in `required_components` must have been registered in `components`
+    pub(super) unsafe fn new(
+        components: &'a mut ComponentsRegistrator<'w>,
+        required_components: &'a mut RequiredComponents,
+    ) -> Self {
+        Self {
+            components,
+            required_components,
+        }
+    }
+
+    /// Register `C` as a required component.
+    pub fn register_required<C: Component>(&mut self, constructor: fn() -> C) {
+        // SAFETY:
+        unsafe {
+            self.required_components
+                .register(self.components, constructor);
+        }
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use alloc::string::{String, ToString};

release-content/migration-guides/required_components_rework.md

@@ -11,3 +11,5 @@ The required components feature has been reworked to be more consistent around t
 - uses of the inheritance depth were removed from the `RequiredComponent` struct and from the methods for registering runtime required components, as it's not unused for the depth-first ordering;
 - `Component::register_required_components`, `RequiredComponents::register` and `RequiredComponents::register_by_id` are now `unsafe`;
 - `RequiredComponentConstructor`'s only field is now private for safety reasons.
+
+The `Component::register_required_components` method has also changed signature. It now takes the `ComponentId` of the component currently being registered and a single other parameter `RequiredComponentsRegistrator` which combines the old `components` and `required_components` parameters, since exposing both of them was unsound. As previously discussed the `inheritance_depth` is now useless and has been removed, while the `recursion_check_stack` has been moved into `ComponentsRegistrator` and will be handled automatically.