Skip to content

Commit 777c782

Browse files
MiniaczQMiniaczQ
committed
lots
1 parent b20c7af commit 777c782

17 files changed

+283
-117
lines changed

‎crates/bevy_ecs/macros/src/lib.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -272,10 +272,10 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream {
272272
}
273273

274274
#[inline]
275-
fn validate_param<'w, 's>(
275+
unsafe fn validate_param<'w, 's>(
276276
state: &'s Self::State,
277277
system_meta: &SystemMeta,
278-
world: &World,
278+
world: UnsafeWorldCell,
279279
) -> bool {
280280
<(#(#param,)*) as SystemParam>::validate_param(state, system_meta, world)
281281
}
@@ -522,10 +522,10 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
522522
}
523523

524524
#[inline]
525-
fn validate_param<'w, 's>(
525+
unsafe fn validate_param<'w, 's>(
526526
state: &'s Self::State,
527527
system_meta: &#path::system::SystemMeta,
528-
world: &#path::world::World,
528+
world: #path::world::unsafe_world_cell::UnsafeWorldCell<'w>,
529529
) -> bool {
530530
<(#(#tuple_types,)*) as #path::system::SystemParam>::validate_param(&state.state, system_meta, world)
531531
}

‎crates/bevy_ecs/src/removal_detection.rs

+5-1
Original file line numberDiff line numberDiff line change
@@ -259,7 +259,11 @@ unsafe impl<'a> SystemParam for &'a RemovedComponentEvents {
259259
fn init_state(_world: &mut World, _system_meta: &mut SystemMeta) -> Self::State {}
260260

261261
#[inline]
262-
fn validate_param(_state: &Self::State, _system_meta: &SystemMeta, _world: &World) -> bool {
262+
unsafe fn validate_param(
263+
_state: &Self::State,
264+
_system_meta: &SystemMeta,
265+
_world: UnsafeWorldCell,
266+
) -> bool {
263267
true
264268
}
265269

‎crates/bevy_ecs/src/schedule/executor/mod.rs

+12
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,18 @@ mod __rust_begin_short_backtrace {
177177
}
178178
}
179179

180+
#[macro_export]
181+
/// Emits a warning about system being skipped.
182+
macro_rules! warn_system_skipped {
183+
($ty:literal, $sys:expr) => {
184+
bevy_utils::tracing::warn!(
185+
"{} {} was skipped due to inaccessible system parameters.",
186+
$ty,
187+
$sys
188+
)
189+
};
190+
}
191+
180192
#[cfg(test)]
181193
mod tests {
182194
use crate::{

‎crates/bevy_ecs/src/schedule/executor/multi_threaded.rs

+25-20
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ use crate::{
1919
query::Access,
2020
schedule::{is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule},
2121
system::BoxedSystem,
22+
warn_system_skipped,
2223
world::{unsafe_world_cell::UnsafeWorldCell, World},
2324
};
2425

@@ -527,13 +528,6 @@ impl ExecutorState {
527528
conditions: &mut Conditions,
528529
world: UnsafeWorldCell,
529530
) -> bool {
530-
// Short circuit when system cannot be executed
531-
// TODO: move validation before checking system access
532-
if !system.validate_param(world.world()) {
533-
// TODO: Warn about system skipping.
534-
return false;
535-
}
536-
537531
let mut should_run = !self.skipped_systems.contains(system_index);
538532

539533
for set_idx in conditions.sets_with_conditions_of_systems[system_index].ones() {
@@ -559,8 +553,6 @@ impl ExecutorState {
559553
self.evaluated_sets.insert(set_idx);
560554
}
561555

562-
// TODO: short circuit if system is skipped after system set conditions.
563-
564556
// Evaluate the system's conditions.
565557
// SAFETY:
566558
// - The caller ensures that `world` has permission to read any data
@@ -576,6 +568,19 @@ impl ExecutorState {
576568

577569
should_run &= system_conditions_met;
578570

571+
// SAFETY:
572+
// - The caller ensures that `world` has permission to read any data
573+
// required by the system.
574+
// - `update_archetype_component_access` has been called for system.
575+
let valid_params = unsafe { system.validate_param_unsafe(world) };
576+
577+
if !valid_params {
578+
warn_system_skipped!("System", system.name());
579+
self.skipped_systems.insert(system_index);
580+
}
581+
582+
should_run &= valid_params;
583+
579584
should_run
580585
}
581586

@@ -733,22 +738,22 @@ unsafe fn evaluate_and_fold_conditions(
733738
conditions: &mut [BoxedCondition],
734739
world: UnsafeWorldCell,
735740
) -> bool {
736-
// TODO: move validation before checking system access
737-
if !conditions
738-
.iter()
739-
.all(|condition| condition.validate_param(world.world()))
740-
{
741-
// TODO: Warn about system skipping.
742-
return false;
743-
}
744-
745741
// not short-circuiting is intentional
746742
#[allow(clippy::unnecessary_fold)]
747743
conditions
748744
.iter_mut()
749745
.map(|condition| {
750-
// SAFETY: The caller ensures that `world` has permission to
751-
// access any data required by the condition.
746+
// SAFETY:
747+
// - The caller ensures that `world` has permission to read any data
748+
// required by the condition.
749+
// - `update_archetype_component_access` has been called for condition.
750+
if !unsafe { condition.validate_param_unsafe(world) } {
751+
warn_system_skipped!("Condition", condition.name());
752+
return false;
753+
}
754+
// - The caller ensures that `world` has permission to read any data
755+
// required by the condition.
756+
// - `update_archetype_component_access` has been called for condition.
752757
unsafe { __rust_begin_short_backtrace::readonly_run_unsafe(&mut **condition, world) }
753758
})
754759
.fold(true, |acc, res| acc && res)

‎crates/bevy_ecs/src/schedule/executor/simple.rs

+27-17
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ use crate::{
77
schedule::{
88
executor::is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule,
99
},
10+
warn_system_skipped,
1011
world::World,
1112
};
1213

@@ -54,13 +55,6 @@ impl SystemExecutor for SimpleExecutor {
5455
#[cfg(feature = "trace")]
5556
let should_run_span = info_span!("check_conditions", name = &*name).entered();
5657

57-
let system = &mut schedule.systems[system_index];
58-
if !system.validate_param(world) {
59-
// TODO: Warn about system skipping.
60-
self.completed_systems.insert(system_index);
61-
continue;
62-
}
63-
6458
let mut should_run = !self.completed_systems.contains(system_index);
6559
for set_idx in schedule.sets_with_conditions_of_systems[system_index].ones() {
6660
if self.evaluated_sets.contains(set_idx) {
@@ -86,6 +80,21 @@ impl SystemExecutor for SimpleExecutor {
8680

8781
should_run &= system_conditions_met;
8882

83+
let system = &mut schedule.systems[system_index];
84+
85+
// SAFETY:
86+
// - The caller ensures that `world` has permission to read any data
87+
// required by the system.
88+
// - `update_archetype_component_access` has been called for system.
89+
let valid_params =
90+
unsafe { system.validate_param_unsafe(world.as_unsafe_world_cell_readonly()) };
91+
92+
if !valid_params {
93+
warn_system_skipped!("System", system.name());
94+
}
95+
96+
should_run &= valid_params;
97+
8998
#[cfg(feature = "trace")]
9099
should_run_span.exit();
91100

@@ -130,20 +139,21 @@ impl SimpleExecutor {
130139
}
131140

132141
fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut World) -> bool {
133-
// TODO: move validation before checking system access
134-
if !conditions
135-
.iter()
136-
.all(|condition| condition.validate_param(world))
137-
{
138-
// TODO: Warn about system skipping.
139-
return false;
140-
}
141-
142142
// not short-circuiting is intentional
143143
#[allow(clippy::unnecessary_fold)]
144144
conditions
145145
.iter_mut()
146-
.map(|condition| __rust_begin_short_backtrace::readonly_run(&mut **condition, world))
146+
.map(|condition| {
147+
// SAFETY:
148+
// - The caller ensures that `world` has permission to read any data
149+
// required by the condition.
150+
// - `update_archetype_component_access` has been called for condition.
151+
if !unsafe { condition.validate_param_unsafe(world.as_unsafe_world_cell_readonly()) } {
152+
warn_system_skipped!("Condition", condition.name());
153+
return false;
154+
}
155+
__rust_begin_short_backtrace::readonly_run(&mut **condition, world)
156+
})
147157
.fold(true, |acc, res| acc && res)
148158
}
149159

‎crates/bevy_ecs/src/schedule/executor/single_threaded.rs

+27-19
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ use std::panic::AssertUnwindSafe;
55

66
use crate::{
77
schedule::{is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule},
8+
warn_system_skipped,
89
world::World,
910
};
1011

@@ -60,13 +61,6 @@ impl SystemExecutor for SingleThreadedExecutor {
6061
#[cfg(feature = "trace")]
6162
let should_run_span = info_span!("check_conditions", name = &*name).entered();
6263

63-
let system = &mut schedule.systems[system_index];
64-
if !system.validate_param(world) {
65-
// TODO: Warn about system skipping.
66-
self.completed_systems.insert(system_index);
67-
continue;
68-
}
69-
7064
let mut should_run = !self.completed_systems.contains(system_index);
7165
for set_idx in schedule.sets_with_conditions_of_systems[system_index].ones() {
7266
if self.evaluated_sets.contains(set_idx) {
@@ -86,14 +80,27 @@ impl SystemExecutor for SingleThreadedExecutor {
8680
self.evaluated_sets.insert(set_idx);
8781
}
8882

89-
// TODO: short circuit if system is skipped after system set conditions.
90-
9183
// evaluate system's conditions
9284
let system_conditions_met =
9385
evaluate_and_fold_conditions(&mut schedule.system_conditions[system_index], world);
9486

9587
should_run &= system_conditions_met;
9688

89+
let system = &mut schedule.systems[system_index];
90+
91+
// SAFETY:
92+
// - The caller ensures that `world` has permission to read any data
93+
// required by the system.
94+
// - `update_archetype_component_access` has been called for system.
95+
let valid_params =
96+
unsafe { system.validate_param_unsafe(world.as_unsafe_world_cell_readonly()) };
97+
98+
if !valid_params {
99+
warn_system_skipped!("System", system.name());
100+
}
101+
102+
should_run &= valid_params;
103+
97104
#[cfg(feature = "trace")]
98105
should_run_span.exit();
99106

@@ -164,19 +171,20 @@ impl SingleThreadedExecutor {
164171
}
165172

166173
fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut World) -> bool {
167-
// TODO: move validation before checking system access
168-
if !conditions
169-
.iter()
170-
.all(|condition| condition.validate_param(world))
171-
{
172-
// TODO: Warn about system skipping.
173-
return false;
174-
}
175-
176174
// not short-circuiting is intentional
177175
#[allow(clippy::unnecessary_fold)]
178176
conditions
179177
.iter_mut()
180-
.map(|condition| __rust_begin_short_backtrace::readonly_run(&mut **condition, world))
178+
.map(|condition| {
179+
// SAFETY:
180+
// - The caller ensures that `world` has permission to read any data
181+
// required by the condition.
182+
// - `update_archetype_component_access` has been called for condition.
183+
if !unsafe { condition.validate_param_unsafe(world.as_unsafe_world_cell_readonly()) } {
184+
warn_system_skipped!("Condition", condition.name());
185+
return false;
186+
}
187+
__rust_begin_short_backtrace::readonly_run(&mut **condition, world)
188+
})
181189
.fold(true, |acc, res| acc && res)
182190
}

‎crates/bevy_ecs/src/system/adapter_system.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -133,8 +133,8 @@ where
133133
}
134134

135135
#[inline]
136-
fn validate_param(&self, world: &crate::world::World) -> bool {
137-
self.system.validate_param(world)
136+
unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool {
137+
self.system.validate_param_unsafe(world)
138138
}
139139

140140
fn initialize(&mut self, world: &mut crate::prelude::World) {

‎crates/bevy_ecs/src/system/combinator.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -206,8 +206,8 @@ where
206206
}
207207

208208
#[inline]
209-
fn validate_param(&self, world: &World) -> bool {
210-
self.a.validate_param(world) && self.b.validate_param(world)
209+
unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool {
210+
self.a.validate_param_unsafe(world) && self.b.validate_param_unsafe(world)
211211
}
212212

213213
fn initialize(&mut self, world: &mut World) {

‎crates/bevy_ecs/src/system/commands/mod.rs

+5-5
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,8 @@ use crate::{
1212
observer::{Observer, TriggerEvent, TriggerTargets},
1313
system::{RunSystemWithInput, SystemId},
1414
world::{
15-
command_queue::RawCommandQueue, Command, CommandQueue, EntityWorldMut, FromWorld,
16-
SpawnBatchIter, World,
15+
command_queue::RawCommandQueue, unsafe_world_cell::UnsafeWorldCell, Command, CommandQueue,
16+
EntityWorldMut, FromWorld, SpawnBatchIter, World,
1717
},
1818
};
1919
use bevy_ptr::OwningPtr;
@@ -146,10 +146,10 @@ const _: () = {
146146
}
147147

148148
#[inline]
149-
fn validate_param(
149+
unsafe fn validate_param(
150150
state: &Self::State,
151151
system_meta: &bevy_ecs::system::SystemMeta,
152-
world: &World,
152+
world: UnsafeWorldCell,
153153
) -> bool {
154154
<(Deferred<CommandQueue>, &Entities) as bevy_ecs::system::SystemParam>::validate_param(
155155
&state.state,
@@ -162,7 +162,7 @@ const _: () = {
162162
unsafe fn get_param<'w, 's>(
163163
state: &'s mut Self::State,
164164
system_meta: &bevy_ecs::system::SystemMeta,
165-
world: bevy_ecs::world::unsafe_world_cell::UnsafeWorldCell<'w>,
165+
world: UnsafeWorldCell<'w>,
166166
change_tick: bevy_ecs::component::Tick,
167167
) -> Self::Item<'w, 's> {
168168
let(f0, f1) = <(Deferred<'s, CommandQueue>, &'w Entities) as bevy_ecs::system::SystemParam>::get_param(&mut state.state, system_meta, world, change_tick);

‎crates/bevy_ecs/src/system/exclusive_function_system.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ where
145145
}
146146

147147
#[inline]
148-
fn validate_param(&self, _world: &World) -> bool {
148+
unsafe fn validate_param_unsafe(&self, _world: UnsafeWorldCell) -> bool {
149149
true
150150
}
151151

‎crates/bevy_ecs/src/system/function_system.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -637,7 +637,7 @@ where
637637
}
638638

639639
#[inline]
640-
fn validate_param(&self, world: &World) -> bool {
640+
unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool {
641641
let param_state = self.param_state.as_ref().expect(Self::PARAM_MESSAGE);
642642
F::Param::validate_param(param_state, &self.system_meta, world)
643643
}

‎crates/bevy_ecs/src/system/system.rs

+9-4
Original file line numberDiff line numberDiff line change
@@ -97,10 +97,15 @@ pub trait System: Send + Sync + 'static {
9797
/// Validates that all systems parameters can be acquired.
9898
/// If not, the system should not be ran.
9999
///
100-
/// NOTE: `tick` value isn't the exact same as the one
101-
/// that will be passed to `run_unsafe`, none the less
102-
/// it should be enough to check monothicity.
103-
fn validate_param(&self, world: &World) -> bool;
100+
/// # Safety
101+
///
102+
/// - The caller must ensure that `world` has permission to read any world data
103+
/// registered in [`Self::archetype_component_access`]. There must be no conflicting
104+
/// simultaneous accesses while the system is running.
105+
/// - The method [`Self::update_archetype_component_access`] must be called at some
106+
/// point before this one, with the same exact [`World`]. If `update_archetype_component_access`
107+
/// panics (or otherwise does not return for any reason), this method must not be called.
108+
unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool;
104109

105110
/// Initialize the system.
106111
fn initialize(&mut self, _world: &mut World);

0 commit comments

Comments
 (0)