From b0a253ba5fc7c1e34feaa932280224bdd18f8422 Mon Sep 17 00:00:00 2001 From: GlennFolker Date: Sun, 23 Feb 2025 16:46:01 +0700 Subject: [PATCH 1/4] Emphasize no structural changes in SystemParam::get_param --- crates/bevy_ecs/src/system/system_param.rs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 8ef804a926be5..e2c8c7e60af19 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -180,6 +180,9 @@ use variadics_please::{all_tuples, all_tuples_enumerated}; /// The implementor must ensure the following is true. /// - [`SystemParam::init_state`] correctly registers all [`World`] accesses used /// by [`SystemParam::get_param`] with the provided [`system_meta`](SystemMeta). +/// - [`SystemParam::get_param`] does **not** make any structural ECS changes in the +/// [`World`], e.g. [`World::spawn`], [`World::register_component`], and other similar +/// methods. In case that's necessary, do that in [`SystemParam::init_state`] instead. /// - None of the world accesses may conflict with any prior accesses registered /// on `system_meta`. pub unsafe trait SystemParam: Sized { @@ -274,12 +277,17 @@ pub unsafe trait SystemParam: Sized { /// Creates a parameter to be passed into a [`SystemParamFunction`](super::SystemParamFunction). /// + /// This method guarantees no structural ECS changes to the `world`, which effectively acts like + /// [`DeferredWorld`] with unsafe accesses to everything requested in [`SystemParam::init_state`]. + /// This allows, for example (but not limited to), invoking `get_param` in multiple systems to + /// the same world concurrently, provided all mutable accesses are disjoint. + /// /// # Safety /// - /// - The passed [`UnsafeWorldCell`] must have access to any world data - /// registered in [`init_state`](SystemParam::init_state). + /// - The passed [`UnsafeWorldCell`] must have access to any world data registered + /// in [`init_state`](SystemParam::init_state). /// - `world` must be the same [`World`] that was used to initialize [`state`](SystemParam::init_state). - /// - all `world`'s archetypes have been processed by [`new_archetype`](SystemParam::new_archetype). + /// - All `world`'s archetypes have been processed by [`new_archetype`](SystemParam::new_archetype). unsafe fn get_param<'world, 'state>( state: &'state mut Self::State, system_meta: &SystemMeta, From a37bc17df9300bfa76948b24583389ccf6a42341 Mon Sep 17 00:00:00 2001 From: GlennFolker Date: Mon, 24 Feb 2025 13:48:40 +0700 Subject: [PATCH 2/4] Add more docs --- crates/bevy_ecs/src/query/world_query.rs | 1 + crates/bevy_ecs/src/system/function_system.rs | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/crates/bevy_ecs/src/query/world_query.rs b/crates/bevy_ecs/src/query/world_query.rs index da147770e0fcf..6e88c03ce4c75 100644 --- a/crates/bevy_ecs/src/query/world_query.rs +++ b/crates/bevy_ecs/src/query/world_query.rs @@ -27,6 +27,7 @@ use variadics_please::all_tuples; /// - Each filter in that disjunction must be a conjunction of the corresponding element's filter with the previous `access` /// - For each resource readonly accessed by [`init_fetch`], [`update_component_access`] should add read access. /// - Mutable resource access is not allowed. +/// - No structural ECS changes, such as [`World::register_component`] etc., may occur. /// /// When implementing [`update_component_access`], note that `add_read` and `add_write` both also add a `With` filter, whereas `extend_access` does not change the filters. /// diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 0f3950d1d4ba6..9dd29c8ceeb1a 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -619,6 +619,10 @@ impl SystemState { } /// Retrieve the [`SystemParam`] values. This will not update archetypes automatically. + /// + /// Notably, this method guarantees no structural ECS changes to the `world`, as part of the safety contracts required by + /// [`SystemParam`]. This allows calling `get_unchecked_manual` in multiple `SystemState`s to the same world concurrently, + /// provided all mutable accesses are disjoint. /// /// # Safety /// This call might access any of the input parameters in a way that violates Rust's mutability rules. Make sure the data From f842d912b4696bd3827fc2eb56d8e68e75c78f2c Mon Sep 17 00:00:00 2001 From: GlennFolker Date: Mon, 24 Feb 2025 13:50:23 +0700 Subject: [PATCH 3/4] `cargo fmt --all` --- crates/bevy_ecs/src/system/function_system.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 9dd29c8ceeb1a..1f078906a7c2d 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -619,7 +619,7 @@ impl SystemState { } /// Retrieve the [`SystemParam`] values. This will not update archetypes automatically. - /// + /// /// Notably, this method guarantees no structural ECS changes to the `world`, as part of the safety contracts required by /// [`SystemParam`]. This allows calling `get_unchecked_manual` in multiple `SystemState`s to the same world concurrently, /// provided all mutable accesses are disjoint. From 801cf281504eb00628b8af3ab73571b93bd24c9f Mon Sep 17 00:00:00 2001 From: GlennFolker Date: Tue, 4 Mar 2025 12:27:45 +0700 Subject: [PATCH 4/4] Just document UnsafeWorldCell --- crates/bevy_ecs/src/query/world_query.rs | 1 - crates/bevy_ecs/src/system/function_system.rs | 4 ---- crates/bevy_ecs/src/system/system_param.rs | 8 -------- crates/bevy_ecs/src/world/unsafe_world_cell.rs | 6 ++++++ 4 files changed, 6 insertions(+), 13 deletions(-) diff --git a/crates/bevy_ecs/src/query/world_query.rs b/crates/bevy_ecs/src/query/world_query.rs index 6e88c03ce4c75..da147770e0fcf 100644 --- a/crates/bevy_ecs/src/query/world_query.rs +++ b/crates/bevy_ecs/src/query/world_query.rs @@ -27,7 +27,6 @@ use variadics_please::all_tuples; /// - Each filter in that disjunction must be a conjunction of the corresponding element's filter with the previous `access` /// - For each resource readonly accessed by [`init_fetch`], [`update_component_access`] should add read access. /// - Mutable resource access is not allowed. -/// - No structural ECS changes, such as [`World::register_component`] etc., may occur. /// /// When implementing [`update_component_access`], note that `add_read` and `add_write` both also add a `With` filter, whereas `extend_access` does not change the filters. /// diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 1f078906a7c2d..0f3950d1d4ba6 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -620,10 +620,6 @@ impl SystemState { /// Retrieve the [`SystemParam`] values. This will not update archetypes automatically. /// - /// Notably, this method guarantees no structural ECS changes to the `world`, as part of the safety contracts required by - /// [`SystemParam`]. This allows calling `get_unchecked_manual` in multiple `SystemState`s to the same world concurrently, - /// provided all mutable accesses are disjoint. - /// /// # Safety /// This call might access any of the input parameters in a way that violates Rust's mutability rules. Make sure the data /// access is safe in the context of global [`World`] access. The passed-in [`World`] _must_ be the [`World`] the [`SystemState`] was diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 596704bcf7fa3..c30cc191dd4b0 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -180,9 +180,6 @@ use variadics_please::{all_tuples, all_tuples_enumerated}; /// The implementor must ensure the following is true. /// - [`SystemParam::init_state`] correctly registers all [`World`] accesses used /// by [`SystemParam::get_param`] with the provided [`system_meta`](SystemMeta). -/// - [`SystemParam::get_param`] does **not** make any structural ECS changes in the -/// [`World`], e.g. [`World::spawn`], [`World::register_component`], and other similar -/// methods. In case that's necessary, do that in [`SystemParam::init_state`] instead. /// - None of the world accesses may conflict with any prior accesses registered /// on `system_meta`. pub unsafe trait SystemParam: Sized { @@ -277,11 +274,6 @@ pub unsafe trait SystemParam: Sized { /// Creates a parameter to be passed into a [`SystemParamFunction`](super::SystemParamFunction). /// - /// This method guarantees no structural ECS changes to the `world`, which effectively acts like - /// [`DeferredWorld`] with unsafe accesses to everything requested in [`SystemParam::init_state`]. - /// This allows, for example (but not limited to), invoking `get_param` in multiple systems to - /// the same world concurrently, provided all mutable accesses are disjoint. - /// /// # Safety /// /// - The passed [`UnsafeWorldCell`] must have access to any world data registered diff --git a/crates/bevy_ecs/src/world/unsafe_world_cell.rs b/crates/bevy_ecs/src/world/unsafe_world_cell.rs index c6e02d943bf21..f094906b1267b 100644 --- a/crates/bevy_ecs/src/world/unsafe_world_cell.rs +++ b/crates/bevy_ecs/src/world/unsafe_world_cell.rs @@ -139,6 +139,12 @@ impl<'w> UnsafeWorldCell<'w> { /// Gets a mutable reference to the [`World`] this [`UnsafeWorldCell`] belongs to. /// This is an incredibly error-prone operation and is only valid in a small number of circumstances. /// + /// Calling this method implies mutable access to the *whole* world (see first point on safety section + /// below), which includes all entities, components, and resources. Notably, calling this on + /// [`WorldQuery::init_fetch`](crate::query::WorldQuery::init_fetch) and + /// [`SystemParam::get_param`](crate::system::SystemParam::get_param) are most likely *unsound* unless + /// you can prove that the underlying [`World`] is exclusive, which in normal circumstances is not. + /// /// # Safety /// - `self` must have been obtained from a call to [`World::as_unsafe_world_cell`] /// (*not* `as_unsafe_world_cell_readonly` or any other method of construction that