Have `System::run_unsafe` return `Result`. #19145

chescock · 2025-05-09T15:27:02Z

Objective

Allow combinator and pipe systems to delay validation of the second system, while still allowing the second system to be skipped.

Fixes #18796

Allow fallible systems to be used as one-shot systems, reporting errors to the error handler when used through commands.

Fixes #19722

Allow fallible systems to be used as run conditions, including when used with combinators. Alternative to #19580.

Always validate parameters when calling the safe run_without_applying_deferred, run, and run_readonly methods on a System.

Solution

Have System::run_unsafe return a Result.

We want pipe systems to run the first system before validating the second, since the first system may affect whether the second system has valid parameters. But if the second system skips then we have no output value to return! So, pipe systems must return a Result that indicates whether the second system ran.

But if we just make pipe systems have Out = Result<B::Out>, then chaining a.pipe(b).pipe(c) becomes difficult. c would need to accept the Result from a.pipe(b), which means it would likely need to return Result itself, giving Result<Result<Out>>!

Instead, we make all systems return a Result! We move the handling of fallible systems from IntoScheduleConfigs and IntoObserverSystem to SystemParamFunction and ExclusiveSystemParamFunction, so that an infallible system can be wrapped before being passed to a combinator.

As a side effect, this enables fallible systems to be used as run conditions and one-shot systems.

Now that the safe run_without_applying_deferred, run, and run_readonly methods return a Result, we can have them perform parameter validation themselves instead of requiring each caller to remember to call them. run_unsafe will continue to not validate parameters, since it is used in the multi-threaded executor when we want to validate and run in separate tasks.

Note that this makes type inference a little more brittle. A function that returns Result<T> can be considered either a fallible system returning T or an infallible system returning Result<T> (and this is important to continue supporting pipe-based error handling)! So there are some cases where the output type of a system can no longer be inferred. It will work fine when directly adding to a schedule, since then the output type is fixed to () (or bool for run conditions). And it will work fine when pipeing to a system with a typed input parameter.

I used a dedicated RunSystemError for the error type instead of plain BevyError so that skipping a system does not box an error or capture a backtrace.

Move safety comments outside of their unsafe blocks.

chescock · 2025-05-09T15:27:54Z

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

@@ -443,7 +443,7 @@ impl<'w, 's> Commands<'w, 's> {
    ///     // Return from the system successfully.
    ///     Ok(())
    /// }
-    /// # bevy_ecs::system::assert_is_system(example_system);
+    /// # bevy_ecs::system::assert_is_system::<(), (), _>(example_system);


This is one of the type inference failures. example_system returns Result, so it may be a fallible () system or an infallible Result system.

chescock · 2025-05-09T15:30:22Z

crates/bevy_ecs/src/schedule/condition.rs

@@ -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)


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.

chescock · 2025-05-09T15:31:29Z

crates/bevy_ecs/src/system/adapter_system.rs

+///         run_system: impl FnOnce(SystemIn<'_, S>) -> Result<S::Out, RunSystemError>,
+///     ) -> Result<Self::Out, RunSystemError> {
+///         let result = run_system(input)?;
+///         Ok(!result)


Writing this in one expression as Ok(!(run_system(input)?)) fails, and I don't entirely understand why.

error[E0308]: `?` operator has incompatible types --> crates\bevy_ecs\src\system\adapter_system.rs:39:14 | 28 | Ok(!(run_system(input)?)) | ^^^^^^^^^^^^^^^^^^ expected `std::ops::Not::Output`, found `bevy_ecs::system::System::Out` | = note: `?` operator cannot convert from `<S as bevy_ecs::system::System>::Out` to `<<S as bevy_ecs::system::System>::Out as Not>::Output` = note: expected associated type `<<S as bevy_ecs::system::System>::Out as Not>::Output` found associated type `<S as bevy_ecs::system::System>::Out` = note: an associated type was expected, but a different one was found

Why would it expect Not::Output as the input to !???

chescock · 2025-05-09T15:33:14Z

crates/bevy_ecs/src/schedule/condition.rs

+        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)?)


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).

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.

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.

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

chescock · 2025-05-09T15:36:00Z

crates/bevy_ecs/src/system/system.rs

-    #[error("System {system} did not run due to failed parameter validation: {err}")]
-    InvalidParams {
-        /// The identifier of the system that was run.
-        system: Cow<'static, str>,


This used to include the system name for context, but I removed it so that I could implement From and use ?, since the system name isn't available in From. The caller will know which system they were running, so they can recover the context if necessary.

# Conflicts: # crates/bevy_ecs/src/observer/runner.rs

# Conflicts: # crates/bevy_ecs/src/observer/runner.rs # crates/bevy_ecs/src/schedule/condition.rs # crates/bevy_ecs/src/schedule/executor/multi_threaded.rs # crates/bevy_ecs/src/system/observer_system.rs # crates/bevy_ecs/src/system/schedule_system.rs # crates/bevy_ecs/src/system/system.rs

# Conflicts: # crates/bevy_ecs/src/observer/runner.rs # crates/bevy_ecs/src/schedule/executor/mod.rs # crates/bevy_ecs/src/schedule/executor/simple.rs # crates/bevy_ecs/src/schedule/executor/single_threaded.rs # crates/bevy_ecs/src/system/exclusive_function_system.rs # crates/bevy_ecs/src/system/function_system.rs # crates/bevy_ecs/src/system/observer_system.rs # crates/bevy_ecs/src/system/schedule_system.rs # crates/bevy_ecs/src/system/system.rs # crates/bevy_ecs/src/system/system_param.rs # crates/bevy_ecs/src/system/system_registry.rs

This uses less code, and is more compatible with the hotpatching implementation.

# Conflicts: # crates/bevy_ecs/src/schedule/executor/mod.rs # crates/bevy_ecs/src/schedule/executor/multi_threaded.rs # crates/bevy_ecs/src/schedule/executor/simple.rs # crates/bevy_ecs/src/schedule/executor/single_threaded.rs # crates/bevy_ecs/src/system/exclusive_function_system.rs # crates/bevy_ecs/src/system/function_system.rs # crates/bevy_ecs/src/system/observer_system.rs # crates/bevy_ecs/src/system/schedule_system.rs # crates/bevy_ecs/src/system/system.rs

…BevyError> and Result<(), BevyError> (bevyengine#19553)" This reverts commit 0e805eb.

This reverts commit d1c6fbe.

# Conflicts: # crates/bevy_ecs/src/observer/runner.rs # crates/bevy_ecs/src/system/observer_system.rs # crates/bevy_ecs/src/system/system.rs # crates/bevy_ecs/src/system/system_registry.rs

chescock · 2025-06-23T15:01:20Z

Okay, I got this working again! My original approach conflicted with the changes for hotpatching, but I managed to make a simpler approach work that removes the conflict.

I did run into a few conflicts with other recent PRs that were making fallible systems work in more places. I mostly reverted them in the hope that this PR supersedes them, but tried to take any unit tests.

#19678 made one-shot systems accept any output type. When combined with this PR, passing a Result was ambiguous as to whether it was a fallible system returning () or an infallible system returning Result. But we don't actually want to allow systems to return a Result that is silently dropped, as noted in #19722. So I reverted #19678, and added another unit test showing that errors are passed to the error handler.

#19553 made conditions accept Result<bool, BevyError> and Result<(), BevyError>, but treated errors as false without passing them to the error handler. This PR instead passes them to the error handler, and then removes support for ().

One of the goals of this PR is to treat returned errors through the same path as parameter validation errors, so treating them differently in conditions would be inconsistent. In particular, pipe and combinator systems return validation errors from the second system as errors, so run_if(a.and(b)) will report validation errors from b as errors. If we ignored returned errors in conditions, then we would wind up ignoring validation errors from b.

But based on the comments in #19403, I expect that change to be controversial.

NthTensor · 2025-06-25T15:59:55Z

crates/bevy_ecs/src/observer/runner.rs

-                }
-            }
-        }
+        if let Err(RunSystemError::Failed(err)) = (*system)


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

NthTensor · 2025-06-25T16:04:34Z

crates/bevy_ecs/src/system/system.rs

 pub enum RunSystemError {
    /// System could not be run due to parameters that failed validation.
-    /// This should not be considered an error if [`field@SystemParamValidationError::skipped`] is `true`.
-    #[error("System {system} did not run due to failed parameter validation: {err}")]
-    InvalidParams {
-        /// The identifier of the system that was run.
-        system: DebugName,
-        /// The returned parameter validation error.
-        err: SystemParamValidationError,
-    },
+    /// This is not considered an error.
+    Skipped(SystemParamValidationError),
+    /// System returned an error or failed required parameter validation.
+    Failed(BevyError),
+}


Ooh I prefer this framing. There's been some ambiguity with what returning Result::Err from a system really means. Is it an error, or is it just part of normal control flow. This tells us: The system failed to complete, which is explicitly an error.

I like this framing too.

NthTensor · 2025-06-25T16:05:39Z

crates/bevy_ecs/src/system/system.rs

    }
 }

 /// Running system failed.
-#[derive(Error, Debug)]
+#[derive(Debug)]
 pub enum RunSystemError {


Bikeshed: Might we want to call this RunSystemResult or something? It has Error in the name and then the next line says "this is not considered an error".

It has Error in the name and then the next line says "this is not considered an error".

Hah! Fair :).

Might we want to call this RunSystemResult or something?

I would like that in isolation, but Result<Out, RunSystemResult> seems odd.

I'm happy to change it if there is a strong alternative, but I might be too stuck thinking that the thing that goes in a Result::Err is an "error" to come up with a better name myself.

NthTensor

Excellent migration guide.

# Conflicts: # crates/bevy_utils/src/debug_info.rs

# Conflicts: # crates/bevy_ecs/src/schedule/executor/mod.rs

alice-i-cecile

I prefer the framing, it plugs multiple consistency holes in our API design, it's noticeably simpler and the migration guide is excellent. Very nice work; merging.

chescock added 7 commits May 9, 2025 10:44

Have System::run_unsafe return Result.

8ba30ec

Error message may include backtrace.

a9c6c84

Add missing semicolons.

f4960bd

Move safety comments outside of their unsafe blocks.

Fenced code blocks should have a language specified.

c2b009b

Update PR number in migration guide.

8c8c67e

Error message may include backtrace.

809514c

Unused import.

632e3d6

chescock force-pushed the result-pipe branch from 19941c8 to 632e3d6 Compare May 9, 2025 16:26

chescock added A-ECS Entities, components, systems, and events S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 9, 2025

chescock commented May 9, 2025

View reviewed changes

alice-i-cecile added C-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes D-Unsafe Touches with unsafe code in some way labels May 10, 2025

Merge remote-tracking branch 'remotes/origin/main' into result-pipe

d3e72ca

# Conflicts: # crates/bevy_ecs/src/observer/runner.rs

alice-i-cecile added the X-Uncontroversial This work is generally agreed upon label May 21, 2025

alice-i-cecile added this to the 0.17 milestone May 30, 2025

chescock added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 9, 2025

chescock added 7 commits June 23, 2025 09:28

Replace FallibleFunctionSystem wrapper with IntoResult converter.

26c723f

This uses less code, and is more compatible with the hotpatching implementation.

Revert "Implement SystemCondition for systems returning Result<bool, …

ad6be24

…BevyError> and Result<(), BevyError> (bevyengine#19553)" This reverts commit 0e805eb.

Rework unit tests for fallible conditions to match new semantics.

b006848

Revert "Support fallible one-shot systems (bevyengine#19678)"

202efa1

This reverts commit d1c6fbe.

Add test that fallible commands report errors.

f03e910

Merge remote-tracking branch 'remotes/origin/main' into result-pipe

1de29ca

# Conflicts: # crates/bevy_ecs/src/observer/runner.rs # crates/bevy_ecs/src/system/observer_system.rs # crates/bevy_ecs/src/system/system.rs # crates/bevy_ecs/src/system/system_registry.rs

chescock removed the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Jun 23, 2025

chescock added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Jun 23, 2025

chescock mentioned this pull request Jun 23, 2025

Impl system condition combinators for Result-ful run conditions #19580

Closed

Mention changes to run_system_cached inference in migration guide.

cdbd498

NthTensor self-requested a review June 25, 2025 14:08

NthTensor reviewed Jun 25, 2025

View reviewed changes

NthTensor approved these changes Jun 25, 2025

View reviewed changes

alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jun 25, 2025

chescock added 3 commits June 29, 2025 20:21

Merge remote-tracking branch 'remotes/origin/main' into result-pipe

a30cc18

# Conflicts: # crates/bevy_utils/src/debug_info.rs

Fix new lints.

e78e0cd

Merge remote-tracking branch 'remotes/origin/main' into result-pipe

9c591d8

# Conflicts: # crates/bevy_ecs/src/schedule/executor/mod.rs

alice-i-cecile requested a review from ItsDoot July 3, 2025 21:13

alice-i-cecile approved these changes Jul 3, 2025

View reviewed changes

alice-i-cecile added this pull request to the merge queue Jul 3, 2025

alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 3, 2025

Merged via the queue into bevyengine:main with commit 6e918f5 Jul 3, 2025
34 checks passed

chescock mentioned this pull request Jul 22, 2025

run_system_cached does not allow usage of systems which return Result #19227

Closed

Uh oh!

Have System::run_unsafe return Result. #19145

Have System::run_unsafe return Result. #19145

Uh oh!

Conversation

chescock commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chescock commented Jun 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NthTensor left a comment

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Have `System::run_unsafe` return `Result`. #19145

Have `System::run_unsafe` return `Result`. #19145

chescock commented May 9, 2025 •

edited

Loading