From 5f936aefc800f6ba55e45168a07595fda0a3c921 Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Mon, 5 May 2025 13:46:25 -0400 Subject: [PATCH] Prevent exclusive systems from being used as observers (#19033) # Objective Prevent using exclusive systems as observers. Allowing them is unsound, because observers are only expected to have `DeferredWorld` access, and the observer infrastructure will keep pointers that are invalidated by the creation of `&mut World`. See https://github.com/bevyengine/bevy/actions/runs/14778342801/job/41491517847?pr=19011 for a MIRI failure in a recent PR caused by an exclusive system being used as an observer in a test. ## Solution Have `Observer::new` panic if `System::is_exclusive()` is true. Document that method, and methods that call it, as panicking. (It should be possible to express this in the type system so that the calls won't even compile, but I did not want to attempt that.) ## Testing Added a unit test that calls `World::add_observer` with an exclusive system. --- crates/bevy_ecs/src/observer/mod.rs | 4 +++ crates/bevy_ecs/src/observer/runner.rs | 25 ++++++++++++++++++- crates/bevy_ecs/src/system/commands/mod.rs | 4 +++ crates/bevy_ecs/src/world/entity_ref.rs | 2 ++ .../observers_may_not_be_exclusive.md | 8 ++++++ 5 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 release-content/migration-guides/observers_may_not_be_exclusive.md diff --git a/crates/bevy_ecs/src/observer/mod.rs b/crates/bevy_ecs/src/observer/mod.rs index 4731072d4c..3c1a7a037c 100644 --- a/crates/bevy_ecs/src/observer/mod.rs +++ b/crates/bevy_ecs/src/observer/mod.rs @@ -562,6 +562,10 @@ impl World { /// // ... /// }); /// ``` + /// + /// # Panics + /// + /// Panics if the given system is an exclusive system. pub fn add_observer( &mut self, system: impl IntoObserverSystem, diff --git a/crates/bevy_ecs/src/observer/runner.rs b/crates/bevy_ecs/src/observer/runner.rs index d68c495dab..660460d1ce 100644 --- a/crates/bevy_ecs/src/observer/runner.rs +++ b/crates/bevy_ecs/src/observer/runner.rs @@ -279,9 +279,22 @@ pub struct Observer { impl Observer { /// Creates a new [`Observer`], which defaults to a "global" observer. This means it will run whenever the event `E` is triggered /// for _any_ entity (or no entity). + /// + /// # Panics + /// + /// Panics if the given system is an exclusive system. pub fn new>(system: I) -> Self { + let system = Box::new(IntoObserverSystem::into_system(system)); + assert!( + !system.is_exclusive(), + concat!( + "Exclusive system `{}` may not be used as observer.\n", + "Instead of `&mut World`, use either `DeferredWorld` if you do not need structural changes, or `Commands` if you do." + ), + system.name() + ); Self { - system: Box::new(IntoObserverSystem::into_system(system)), + system, descriptor: Default::default(), hook_on_add: hook_on_add::, error_handler: None, @@ -529,4 +542,14 @@ mod tests { world.trigger(TriggerEvent); assert!(world.resource::().0); } + + #[test] + #[should_panic( + expected = "Exclusive system `bevy_ecs::observer::runner::tests::exclusive_system_cannot_be_observer::system` may not be used as observer.\nInstead of `&mut World`, use either `DeferredWorld` if you do not need structural changes, or `Commands` if you do." + )] + fn exclusive_system_cannot_be_observer() { + fn system(_: Trigger, _world: &mut World) {} + let mut world = World::default(); + world.add_observer(system); + } } diff --git a/crates/bevy_ecs/src/system/commands/mod.rs b/crates/bevy_ecs/src/system/commands/mod.rs index 7f0d7d84de..2eed9f6f2a 100644 --- a/crates/bevy_ecs/src/system/commands/mod.rs +++ b/crates/bevy_ecs/src/system/commands/mod.rs @@ -1123,6 +1123,10 @@ impl<'w, 's> Commands<'w, 's> { /// **Calling [`observe`](EntityCommands::observe) on the returned /// [`EntityCommands`] will observe the observer itself, which you very /// likely do not want.** + /// + /// # Panics + /// + /// Panics if the given system is an exclusive system. pub fn add_observer( &mut self, observer: impl IntoObserverSystem, diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index a9887c5248..a0ef602cfc 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -2773,6 +2773,8 @@ impl<'w> EntityWorldMut<'w> { /// # Panics /// /// If the entity has been despawned while this `EntityWorldMut` is still alive. + /// + /// Panics if the given system is an exclusive system. #[track_caller] pub fn observe( &mut self, diff --git a/release-content/migration-guides/observers_may_not_be_exclusive.md b/release-content/migration-guides/observers_may_not_be_exclusive.md new file mode 100644 index 0000000000..1ffa9e7ed5 --- /dev/null +++ b/release-content/migration-guides/observers_may_not_be_exclusive.md @@ -0,0 +1,8 @@ +--- +title: Exclusive systems may not be used as observers +pull_requests: [19033] +--- + +Exclusive systems may no longer be used as observers. +This was never sound, as the engine keeps references alive during observer invocation that would be invalidated by `&mut World` access, but was accidentally allowed. +Instead of `&mut World`, use either `DeferredWorld` if you do not need structural changes, or `Commands` if you do.