From a5c392180b0c1df42865221b0338bd34644c5e54 Mon Sep 17 00:00:00 2001 From: Giacomo Stevanato Date: Thu, 17 Jul 2025 18:56:12 +0200 Subject: [PATCH] Fix soundness issue and add more migration docs --- crates/bevy_ecs/macros/src/component.rs | 8 ++--- crates/bevy_ecs/src/component/mod.rs | 5 ++-- crates/bevy_ecs/src/component/register.rs | 9 +++--- crates/bevy_ecs/src/component/required.rs | 30 +++++++++++++++++++ .../required_components_rework.md | 2 ++ 5 files changed, 42 insertions(+), 12 deletions(-) diff --git a/crates/bevy_ecs/macros/src/component.rs b/crates/bevy_ecs/macros/src/component.rs index c6d530dc95..743c16ca3e 100644 --- a/crates/bevy_ecs/macros/src/component.rs +++ b/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)* } diff --git a/crates/bevy_ecs/src/component/mod.rs b/crates/bevy_ecs/src/component/mod.rs index eeb4b2fdf8..1d808f0e1d 100644 --- a/crates/bevy_ecs/src/component/mod.rs +++ b/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, ) { } diff --git a/crates/bevy_ecs/src/component/register.rs b/crates/bevy_ecs/src/component/register.rs index 6c4efae0c2..4fde7a6390 100644 --- a/crates/bevy_ecs/src/component/register.rs +++ b/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(); diff --git a/crates/bevy_ecs/src/component/required.rs b/crates/bevy_ecs/src/component/required.rs index 3e0ba5b7d1..14d295cb26 100644 --- a/crates/bevy_ecs/src/component/required.rs +++ b/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(&mut self, constructor: fn() -> C) { + // SAFETY: + unsafe { + self.required_components + .register(self.components, constructor); + } + } +} + #[cfg(test)] mod tests { use alloc::string::{String, ToString}; diff --git a/release-content/migration-guides/required_components_rework.md b/release-content/migration-guides/required_components_rework.md index db5c55abb4..c023143e4d 100644 --- a/release-content/migration-guides/required_components_rework.md +++ b/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.