Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/bevy_ecs/src/observer/runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,7 @@ fn observer_system_runner<E: Event, B: Bundle, S: ObserverSystem<E, B>>(
// - `update_archetype_component_access` is called first
// - there are no outstanding references to world except a private component
// - system is an `ObserverSystem` so won't mutate world beyond the access of a `DeferredWorld`
// and is never exclusive
// - system is the same type erased system from above
unsafe {
(*system).update_archetype_component_access(world);
Expand Down
1 change: 1 addition & 0 deletions crates/bevy_ecs/src/schedule/executor/multi_threaded.rs
Original file line number Diff line number Diff line change
Expand Up @@ -609,6 +609,7 @@ impl ExecutorState {
// SAFETY:
// - The caller ensures that we have permission to
// access the world data used by the system.
// - `is_exclusive` returned false
// - `update_archetype_component_access` has been called.
unsafe {
if let Err(err) = __rust_begin_short_backtrace::run_unsafe(
Expand Down
6 changes: 0 additions & 6 deletions crates/bevy_ecs/src/system/adapter_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,12 +162,6 @@ where
})
}

#[inline]
fn run(&mut self, input: SystemIn<'_, Self>, world: &mut crate::prelude::World) -> Self::Out {
self.func
.adapt(input, |input| self.system.run(input, world))
}

#[inline]
fn apply_deferred(&mut self, world: &mut crate::prelude::World) {
self.system.apply_deferred(world);
Expand Down
19 changes: 1 addition & 18 deletions crates/bevy_ecs/src/system/combinator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ where
input,
// SAFETY: The world accesses for both underlying systems have been registered,
// so the caller will guarantee that no other systems will conflict with `a` or `b`.
// If either system has `is_exclusive()`, then the combined system also has `is_exclusive`.
// Since these closures are `!Send + !Sync + !'static`, they can never be called
// in parallel, so their world accesses will not conflict with each other.
// Additionally, `update_archetype_component_access` has been called,
Expand All @@ -186,19 +187,6 @@ where
)
}

fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Self::Out {
let world = world.as_unsafe_world_cell();
Func::combine(
input,
// SAFETY: Since these closures are `!Send + !Sync + !'static`, they can never
// be called in parallel. Since mutable access to `world` only exists within
// the scope of either closure, we can be sure they will never alias one another.
|input| self.a.run(input, unsafe { world.world_mut() }),
// SAFETY: See the above safety comment.
|input| self.b.run(input, unsafe { world.world_mut() }),
)
}

#[inline]
fn apply_deferred(&mut self, world: &mut World) {
self.a.apply_deferred(world);
Expand Down Expand Up @@ -416,11 +404,6 @@ where
self.b.run_unsafe(value, world)
}

fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Self::Out {
let value = self.a.run(input, world);
self.b.run(value, world)
}

fn apply_deferred(&mut self, world: &mut World) {
self.a.apply_deferred(world);
self.b.apply_deferred(world);
Expand Down
12 changes: 3 additions & 9 deletions crates/bevy_ecs/src/system/exclusive_function_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,18 +110,12 @@ where

#[inline]
unsafe fn run_unsafe(
&mut self,
_input: SystemIn<'_, Self>,
_world: UnsafeWorldCell,
) -> Self::Out {
panic!("Cannot run exclusive systems with a shared World reference");
}

fn run_without_applying_deferred(
&mut self,
input: SystemIn<'_, Self>,
world: &mut World,
world: UnsafeWorldCell,
) -> Self::Out {
// SAFETY: `is_exclusive` returned true, so the caller ensured we had `&mut World` access
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This safety comment doesn't make sense to me. We never check is_exclusive in this method. My understanding here is that you've passed the safety requirements for calling world_mut to the caller as it can't be verified here. https://docs.rs/bevy/latest/bevy/ecs/world/unsafe_world_cell/struct.UnsafeWorldCell.html#safety-1

And so need to update the safety comment on run_unsafe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I did update the safety comment on run_unsafe. It now has the additional requirement that

    /// - If [`System::is_exclusive`] returns `true`, then it must be valid to call
    ///   [`UnsafeWorldCell::world_mut`] on `world`.

What I wanted to communicate here is that because ExclusiveFunctionSystem::is_exclusive returns true, that clause triggered and the caller had to ensure that world_mut was valid.

If you have ideas on how to make the wording more clear, I'm happy to update it!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try changing it to

-        // SAFETY: `is_exclusive` returned true, so the caller ensured we had `&mut World` access
+        // SAFETY: `Self::is_exclusive` returns true, so the caller is
+        // required to ensure that the it's valid to call `world_mut()`

Is that any better?

let world = unsafe { world.world_mut() };
world.last_change_tick_scope(self.system_meta.last_run, |world| {
#[cfg(feature = "trace")]
let _span_guard = self.system_meta.system_span.enter();
Expand Down
6 changes: 0 additions & 6 deletions crates/bevy_ecs/src/system/observer_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,12 +138,6 @@ where
Ok(())
}

#[inline]
fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Self::Out {
self.observer.run(input, world);
Ok(())
}

#[inline]
fn apply_deferred(&mut self, world: &mut World) {
self.observer.apply_deferred(world);
Expand Down
6 changes: 0 additions & 6 deletions crates/bevy_ecs/src/system/schedule_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,6 @@ impl<S: System<In = (), Out = ()>> System for InfallibleSystemWrapper<S> {
Ok(())
}

#[inline]
fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Self::Out {
self.0.run(input, world);
Ok(())
}

#[inline]
fn apply_deferred(&mut self, world: &mut World) {
self.0.apply_deferred(world);
Expand Down
2 changes: 2 additions & 0 deletions crates/bevy_ecs/src/system/system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ pub trait System: Send + Sync + 'static {
/// - The caller must ensure that [`world`](UnsafeWorldCell) has permission to access any world data
/// registered in `archetype_component_access`. There must be no conflicting
/// simultaneous accesses while the system is running.
/// - If [`System::is_exclusive`] returns `true`, then it must be valid to call
/// [`UnsafeWorldCell::world_mut`] on `world`.
/// - The method [`System::update_archetype_component_access`] must be called at some
/// point before this one, with the same exact [`World`]. If [`System::update_archetype_component_access`]
/// panics (or otherwise does not return for any reason), this method must not be called.
Expand Down
9 changes: 9 additions & 0 deletions crates/bevy_ecs/src/system/system_registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -894,4 +894,13 @@ mod tests {

assert_eq!(INVOCATIONS_LEFT.get(), 0);
}

#[test]
fn run_system_exclusive_adapters() {
let mut world = World::new();
fn system(_: &mut World) {}
world.run_system_cached(system).unwrap();
world.run_system_cached(system.pipe(system)).unwrap();
world.run_system_cached(system.map(|()| {})).unwrap();
}
}