Skip to content

Have System::run_unsafe return Result. #19145

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 21 commits into from
Jul 3, 2025
Merged

Have System::run_unsafe return Result. #19145

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
8ba30ec
Have `System::run_unsafe` return `Result`.
chescock Apr 23, 2025
a9c6c84
Error message may include backtrace.
chescock May 9, 2025
f4960bd
Add missing semicolons.
chescock May 9, 2025
c2b009b
Fenced code blocks should have a language specified.
chescock May 9, 2025
8c8c67e
Update PR number in migration guide.
chescock May 9, 2025
809514c
Error message may include backtrace.
chescock May 9, 2025
632e3d6
Unused import.
chescock May 9, 2025
d3e72ca
Merge remote-tracking branch 'remotes/origin/main' into result-pipe
chescock May 20, 2025
ba48caa
Merge remote-tracking branch 'remotes/origin/main' into result-pipe
chescock May 30, 2025
80546b2
Merge remote-tracking branch 'remotes/origin/main' into result-pipe
chescock Jun 6, 2025
26c723f
Replace `FallibleFunctionSystem` wrapper with `IntoResult` converter.
chescock Jun 9, 2025
f0db7b4
Merge remote-tracking branch 'remotes/origin/main' into result-pipe
chescock Jun 23, 2025
ad6be24
Revert "Implement SystemCondition for systems returning Result<bool, …
chescock Jun 23, 2025
b006848
Rework unit tests for fallible conditions to match new semantics.
chescock Jun 23, 2025
202efa1
Revert "Support fallible one-shot systems (#19678)"
chescock Jun 23, 2025
f03e910
Add test that fallible commands report errors.
chescock Jun 23, 2025
1de29ca
Merge remote-tracking branch 'remotes/origin/main' into result-pipe
chescock Jun 23, 2025
cdbd498
Mention changes to `run_system_cached` inference in migration guide.
chescock Jun 25, 2025
a30cc18
Merge remote-tracking branch 'remotes/origin/main' into result-pipe
chescock Jun 30, 2025
e78e0cd
Fix new lints.
chescock Jun 30, 2025
9c591d8
Merge remote-tracking branch 'remotes/origin/main' into result-pipe
chescock Jul 3, 2025
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
2 changes: 1 addition & 1 deletion benches/benches/bevy_ecs/iteration/iter_simple_system.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,6 @@ impl Benchmark {

#[inline(never)]
pub fn run(&mut self) {
self.1.run((), &mut self.0);
self.1.run((), &mut self.0).unwrap();
}
}
20 changes: 10 additions & 10 deletions crates/bevy_ecs/src/change_detection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,10 +188,10 @@ pub trait DetectChangesMut: DetectChanges {
/// #
/// # // first time `reset_score` runs, the score is changed.
/// # schedule.run(&mut world);
/// # assert!(score_changed.run((), &mut world));
/// # assert!(score_changed.run((), &mut world).unwrap());
/// # // second time `reset_score` runs, the score is not changed.
/// # schedule.run(&mut world);
/// # assert!(!score_changed.run((), &mut world));
/// # assert!(!score_changed.run((), &mut world).unwrap());
/// ```
#[inline]
#[track_caller]
Expand Down Expand Up @@ -263,12 +263,12 @@ pub trait DetectChangesMut: DetectChanges {
/// #
/// # // first time `reset_score` runs, the score is changed.
/// # schedule.run(&mut world);
/// # assert!(score_changed.run((), &mut world));
/// # assert!(score_changed_event.run((), &mut world));
/// # assert!(score_changed.run((), &mut world).unwrap());
/// # assert!(score_changed_event.run((), &mut world).unwrap());
/// # // second time `reset_score` runs, the score is not changed.
/// # schedule.run(&mut world);
/// # assert!(!score_changed.run((), &mut world));
/// # assert!(!score_changed_event.run((), &mut world));
/// # assert!(!score_changed.run((), &mut world).unwrap());
/// # assert!(!score_changed_event.run((), &mut world).unwrap());
/// ```
#[inline]
#[must_use = "If you don't need to handle the previous value, use `set_if_neq` instead."]
Expand Down Expand Up @@ -315,10 +315,10 @@ pub trait DetectChangesMut: DetectChanges {
/// #
/// # // first time `reset_score` runs, the score is changed.
/// # schedule.run(&mut world);
/// # assert!(message_changed.run((), &mut world));
/// # assert!(message_changed.run((), &mut world).unwrap());
/// # // second time `reset_score` runs, the score is not changed.
/// # schedule.run(&mut world);
/// # assert!(!message_changed.run((), &mut world));
/// # assert!(!message_changed.run((), &mut world).unwrap());
/// ```
fn clone_from_if_neq<T>(&mut self, value: &T) -> bool
where
Expand Down Expand Up @@ -1590,7 +1590,7 @@ mod tests {

// world: 1, system last ran: 0, component changed: 1
// The spawn will be detected since it happened after the system "last ran".
assert!(change_detected_system.run((), &mut world));
assert!(change_detected_system.run((), &mut world).unwrap());

// world: 1 + MAX_CHANGE_AGE
let change_tick = world.change_tick.get_mut();
Expand All @@ -1600,7 +1600,7 @@ mod tests {
// Since we clamp things to `MAX_CHANGE_AGE` for determinism,
// `ComponentTicks::is_changed` will now see `MAX_CHANGE_AGE > MAX_CHANGE_AGE`
// and return `false`.
assert!(!change_expired_system.run((), &mut world));
assert!(!change_expired_system.run((), &mut world).unwrap());
}

#[test]
Expand Down
16 changes: 8 additions & 8 deletions crates/bevy_ecs/src/event/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,20 +525,20 @@ mod tests {
});
reader.initialize(&mut world);

let last = reader.run((), &mut world);
let last = reader.run((), &mut world).unwrap();
assert!(last.is_none(), "EventReader should be empty");

world.send_event(TestEvent { i: 0 });
let last = reader.run((), &mut world);
let last = reader.run((), &mut world).unwrap();
assert_eq!(last, Some(TestEvent { i: 0 }));

world.send_event(TestEvent { i: 1 });
world.send_event(TestEvent { i: 2 });
world.send_event(TestEvent { i: 3 });
let last = reader.run((), &mut world);
let last = reader.run((), &mut world).unwrap();
assert_eq!(last, Some(TestEvent { i: 3 }));

let last = reader.run((), &mut world);
let last = reader.run((), &mut world).unwrap();
assert!(last.is_none(), "EventReader should be empty");
}

Expand All @@ -555,20 +555,20 @@ mod tests {
});
mutator.initialize(&mut world);

let last = mutator.run((), &mut world);
let last = mutator.run((), &mut world).unwrap();
assert!(last.is_none(), "EventMutator should be empty");

world.send_event(TestEvent { i: 0 });
let last = mutator.run((), &mut world);
let last = mutator.run((), &mut world).unwrap();
assert_eq!(last, Some(TestEvent { i: 0 }));

world.send_event(TestEvent { i: 1 });
world.send_event(TestEvent { i: 2 });
world.send_event(TestEvent { i: 3 });
let last = mutator.run((), &mut world);
let last = mutator.run((), &mut world).unwrap();
assert_eq!(last, Some(TestEvent { i: 3 }));

let last = mutator.run((), &mut world);
let last = mutator.run((), &mut world).unwrap();
assert!(last.is_none(), "EventMutator should be empty");
}

Expand Down
41 changes: 15 additions & 26 deletions crates/bevy_ecs/src/observer/runner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::{
observer::{ObserverDescriptor, ObserverTrigger},
prelude::*,
query::DebugCheckedUnwrap,
system::{IntoObserverSystem, ObserverSystem},
system::{IntoObserverSystem, ObserverSystem, RunSystemError},
world::DeferredWorld,
};
use bevy_ptr::PtrMut;
Expand Down Expand Up @@ -374,31 +374,20 @@ fn observer_system_runner<E: Event, B: Bundle, S: ObserverSystem<E, B>>(
// - system is the same type erased system from above
unsafe {
(*system).update_archetype_component_access(world);
match (*system).validate_param_unsafe(world) {
Ok(()) => {
if let Err(err) = (*system).run_unsafe(trigger, world) {
error_handler(
err,
ErrorContext::Observer {
name: (*system).name(),
last_run: (*system).get_last_run(),
},
);
};
(*system).queue_deferred(world.into_deferred());
}
Err(e) => {
if !e.skipped {
error_handler(
e.into(),
ErrorContext::Observer {
name: (*system).name(),
last_run: (*system).get_last_run(),
},
);
}
}
}
if let Err(RunSystemError::Failed(err)) = (*system)
Copy link
Contributor

Choose a reason for hiding this comment

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

Idly interested in benchmarks of this. Removing the branch here should be net positive.

.validate_param_unsafe(world)
.map_err(From::from)
.and_then(|()| (*system).run_unsafe(trigger, world))
{
error_handler(
err,
ErrorContext::Observer {
name: (*system).name(),
last_run: (*system).get_last_run(),
},
);
};
(*system).queue_deferred(world.into_deferred());
}
}

Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_ecs/src/query/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -717,7 +717,7 @@ mod tests {
}
let mut system = IntoSystem::into_system(system);
system.initialize(&mut world);
system.run((), &mut world);
system.run((), &mut world).unwrap();
}
{
fn system(has_a: Query<Entity, With<A>>, mut b_query: Query<&mut B>) {
Expand All @@ -728,7 +728,7 @@ mod tests {
}
let mut system = IntoSystem::into_system(system);
system.initialize(&mut world);
system.run((), &mut world);
system.run((), &mut world).unwrap();
}
{
fn system(query: Query<(Option<&A>, &B)>) {
Expand All @@ -741,7 +741,7 @@ mod tests {
}
let mut system = IntoSystem::into_system(system);
system.initialize(&mut world);
system.run((), &mut world);
system.run((), &mut world).unwrap();
}
}

Expand Down
60 changes: 30 additions & 30 deletions crates/bevy_ecs/src/schedule/condition.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ use alloc::{borrow::Cow, boxed::Box, format};
use core::ops::Not;

use crate::system::{
Adapt, AdapterSystem, CombinatorSystem, Combine, IntoSystem, ReadOnlySystem, System, SystemIn,
SystemInput,
Adapt, AdapterSystem, CombinatorSystem, Combine, IntoSystem, ReadOnlySystem, RunSystemError,
System, SystemIn, SystemInput,
};

/// A type-erased run condition stored in a [`Box`].
Expand Down Expand Up @@ -59,7 +59,7 @@ pub type BoxedCondition<In = ()> = Box<dyn ReadOnlySystem<In = In, Out = bool>>;
/// ```
/// # use bevy_ecs::prelude::*;
/// fn identity() -> impl Condition<(), In<bool>> {
/// IntoSystem::into_system(|In(x)| x)
/// IntoSystem::into_system(|In(x): In<bool>| x)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is another type inference failure. It has something to do with the fact that the output type is tied to the input type, as changing it to return true instead of x works, but I don't entirely understand why the compiler can't figure out In<bool> from the return type of identity.

/// }
///
/// # fn always_true() -> bool { true }
Expand Down Expand Up @@ -1110,9 +1110,9 @@ impl<S: System<Out: Not>> Adapt<S> for NotMarker {
fn adapt(
&mut self,
input: <Self::In as SystemInput>::Inner<'_>,
run_system: impl FnOnce(SystemIn<'_, S>) -> S::Out,
) -> Self::Out {
!run_system(input)
run_system: impl FnOnce(SystemIn<'_, S>) -> Result<S::Out, RunSystemError>,
) -> Result<Self::Out, RunSystemError> {
run_system(input).map(Not::not)
}
}

Expand Down Expand Up @@ -1148,10 +1148,10 @@ where

fn combine(
input: <Self::In as SystemInput>::Inner<'_>,
a: impl FnOnce(SystemIn<'_, A>) -> A::Out,
b: impl FnOnce(SystemIn<'_, A>) -> B::Out,
) -> Self::Out {
a(input) && b(input)
a: impl FnOnce(SystemIn<'_, A>) -> Result<A::Out, RunSystemError>,
b: impl FnOnce(SystemIn<'_, A>) -> Result<B::Out, RunSystemError>,
) -> Result<Self::Out, RunSystemError> {
Ok(a(input)? && b(input)?)
}
}

Expand All @@ -1169,10 +1169,10 @@ where

fn combine(
input: <Self::In as SystemInput>::Inner<'_>,
a: impl FnOnce(SystemIn<'_, A>) -> A::Out,
b: impl FnOnce(SystemIn<'_, B>) -> B::Out,
) -> Self::Out {
!(a(input) && b(input))
a: impl FnOnce(SystemIn<'_, A>) -> Result<A::Out, RunSystemError>,
b: impl FnOnce(SystemIn<'_, A>) -> Result<B::Out, RunSystemError>,
) -> Result<Self::Out, RunSystemError> {
Ok(!(a(input)? && b(input)?))
}
}

Expand All @@ -1190,10 +1190,10 @@ where

fn combine(
input: <Self::In as SystemInput>::Inner<'_>,
a: impl FnOnce(SystemIn<'_, A>) -> A::Out,
b: impl FnOnce(SystemIn<'_, B>) -> B::Out,
) -> Self::Out {
!(a(input) || b(input))
a: impl FnOnce(SystemIn<'_, A>) -> Result<A::Out, RunSystemError>,
b: impl FnOnce(SystemIn<'_, A>) -> Result<B::Out, RunSystemError>,
) -> Result<Self::Out, RunSystemError> {
Ok(!(a(input)? || b(input)?))
}
}

Expand All @@ -1211,10 +1211,10 @@ where

fn combine(
input: <Self::In as SystemInput>::Inner<'_>,
a: impl FnOnce(SystemIn<'_, A>) -> A::Out,
b: impl FnOnce(SystemIn<'_, B>) -> B::Out,
) -> Self::Out {
a(input) || b(input)
a: impl FnOnce(SystemIn<'_, A>) -> Result<A::Out, RunSystemError>,
b: impl FnOnce(SystemIn<'_, A>) -> Result<B::Out, RunSystemError>,
) -> Result<Self::Out, RunSystemError> {
Ok(a(input)? || b(input)?)
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 wrote it this way for consistency, but the short-circuiting behavior of a.or(b) is a little odd. We normally treat failures in run conditions as false-like, but or short circuits on true and failure, and only calls the second condition on Ok(false).

Copy link
Member

Choose a reason for hiding this comment

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

I was told this short-circuiting was preferable in review to avoid breaking existing code and reduce pointless failures.

I'm not sure I agree still, but it's best to split that change out.

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 was told this short-circuiting was preferable in review to avoid breaking existing code and reduce pointless failures.

I'm not sure I agree still, but it's best to split that change out.

Sorry, I don't quite follow. What would you like me to split out?

The core change here is making System::run return a Result, so we have to do something to handle the case where fallible systems are used with an or combinator. Adding ?s and wrapping it in Ok seemed like the simplest change, and was consistent with the other combinators, so that's what I did. But it's also kind of weird, so I wanted to highlight it.

I think the only behavior change to existing code is that a failing Single or Populated in the second system will now return Err and be treated as false, while before this it would skip validation entirely and then panic.

Copy link
Member

Choose a reason for hiding this comment

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

My preference was to leave the behavior completely unchanged in this PR. That said, I prefer this behavior and won't block on it.

}
}

Expand All @@ -1232,10 +1232,10 @@ where

fn combine(
input: <Self::In as SystemInput>::Inner<'_>,
a: impl FnOnce(SystemIn<'_, A>) -> A::Out,
b: impl FnOnce(SystemIn<'_, B>) -> B::Out,
) -> Self::Out {
!(a(input) ^ b(input))
a: impl FnOnce(SystemIn<'_, A>) -> Result<A::Out, RunSystemError>,
b: impl FnOnce(SystemIn<'_, A>) -> Result<B::Out, RunSystemError>,
) -> Result<Self::Out, RunSystemError> {
Ok(!(a(input)? ^ b(input)?))
}
}

Expand All @@ -1253,10 +1253,10 @@ where

fn combine(
input: <Self::In as SystemInput>::Inner<'_>,
a: impl FnOnce(SystemIn<'_, A>) -> A::Out,
b: impl FnOnce(SystemIn<'_, B>) -> B::Out,
) -> Self::Out {
a(input) ^ b(input)
a: impl FnOnce(SystemIn<'_, A>) -> Result<A::Out, RunSystemError>,
b: impl FnOnce(SystemIn<'_, A>) -> Result<B::Out, RunSystemError>,
) -> Result<Self::Out, RunSystemError> {
Ok(a(input)? ^ b(input)?)
}
}

Expand Down
36 changes: 3 additions & 33 deletions crates/bevy_ecs/src/schedule/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,14 @@ use alloc::{boxed::Box, vec, vec::Vec};
use variadics_please::all_tuples;

use crate::{
error::Result,
never::Never,
schedule::{
auto_insert_apply_deferred::IgnoreDeferred,
condition::{BoxedCondition, Condition},
graph::{Ambiguity, Dependency, DependencyKind, GraphInfo},
set::{InternedSystemSet, IntoSystemSet, SystemSet},
Chain,
},
system::{BoxedSystem, InfallibleSystemWrapper, IntoSystem, ScheduleSystem, System},
system::{BoxedSystem, IntoSystem, ScheduleSystem, System},
};

fn new_condition<M>(condition: impl Condition<M>) -> BoxedCondition {
Expand Down Expand Up @@ -557,45 +555,17 @@ impl<T: Schedulable<Metadata = GraphInfo, GroupMetadata = Chain>> IntoScheduleCo
}
}

/// Marker component to allow for conflicting implementations of [`IntoScheduleConfigs`]
#[doc(hidden)]
pub struct Infallible;

impl<F, Marker> IntoScheduleConfigs<ScheduleSystem, (Infallible, Marker)> for F
impl<F, Marker> IntoScheduleConfigs<ScheduleSystem, Marker> for F
where
F: IntoSystem<(), (), Marker>,
{
fn into_configs(self) -> ScheduleConfigs<ScheduleSystem> {
let wrapper = InfallibleSystemWrapper::new(IntoSystem::into_system(self));
ScheduleConfigs::ScheduleConfig(ScheduleSystem::into_config(Box::new(wrapper)))
}
}

impl<F, Marker> IntoScheduleConfigs<ScheduleSystem, (Never, Marker)> for F
where
F: IntoSystem<(), Never, Marker>,
{
fn into_configs(self) -> ScheduleConfigs<ScheduleSystem> {
let wrapper = InfallibleSystemWrapper::new(IntoSystem::into_system(self));
ScheduleConfigs::ScheduleConfig(ScheduleSystem::into_config(Box::new(wrapper)))
}
}

/// Marker component to allow for conflicting implementations of [`IntoScheduleConfigs`]
#[doc(hidden)]
pub struct Fallible;

impl<F, Marker> IntoScheduleConfigs<ScheduleSystem, (Fallible, Marker)> for F
where
F: IntoSystem<(), Result, Marker>,
{
fn into_configs(self) -> ScheduleConfigs<ScheduleSystem> {
let boxed_system = Box::new(IntoSystem::into_system(self));
ScheduleConfigs::ScheduleConfig(ScheduleSystem::into_config(boxed_system))
}
}

impl IntoScheduleConfigs<ScheduleSystem, ()> for BoxedSystem<(), Result> {
impl IntoScheduleConfigs<ScheduleSystem, ()> for BoxedSystem<(), ()> {
fn into_configs(self) -> ScheduleConfigs<ScheduleSystem> {
ScheduleConfigs::ScheduleConfig(ScheduleSystem::into_config(self))
}
Expand Down
Loading