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.
This commit is contained in:
parent
0f6d532a15
commit
5f936aefc8
@ -562,6 +562,10 @@ impl World {
|
||||
/// // ...
|
||||
/// });
|
||||
/// ```
|
||||
///
|
||||
/// # Panics
|
||||
///
|
||||
/// Panics if the given system is an exclusive system.
|
||||
pub fn add_observer<E: Event, B: Bundle, M>(
|
||||
&mut self,
|
||||
system: impl IntoObserverSystem<E, B, M>,
|
||||
|
@ -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<E: Event, B: Bundle, M, I: IntoObserverSystem<E, B, M>>(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::<E, B, I::System>,
|
||||
error_handler: None,
|
||||
@ -529,4 +542,14 @@ mod tests {
|
||||
world.trigger(TriggerEvent);
|
||||
assert!(world.resource::<Ran>().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<TriggerEvent>, _world: &mut World) {}
|
||||
let mut world = World::default();
|
||||
world.add_observer(system);
|
||||
}
|
||||
}
|
||||
|
@ -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<E: Event, B: Bundle, M>(
|
||||
&mut self,
|
||||
observer: impl IntoObserverSystem<E, B, M>,
|
||||
|
@ -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<E: Event, B: Bundle, M>(
|
||||
&mut self,
|
||||
|
@ -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.
|
Loading…
Reference in New Issue
Block a user