Have System::run_unsafe return Result.

[chescock]

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 run conditions.

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.

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.

[chescock]

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]

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]

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]

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

[alice-i-cecile]

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.

[chescock]

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.

[chescock]

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.