Allow systems to return results

[joseph-gio]

Objective

When writing an app in rust, the main function usually returns nothing, but it can also return a Result; when doing this, returning an Err value will result in a panic. This makes writing fallible code much more ergonomic, since ? can be used to propagate errors.

This flexibility should be extended to systems: users should be able to return a Result from their systems. This can be done by calling system.pipe(unwrap) when adding the system, however this approach requires more boilerplate, incurs a runtime overhead due to the PipeSystem, removes context from the error message (since the panic message has no way of accessing the system's name), and makes the name of the combined system messier (which is stored by allocating a new String).

Solution

A system can only be added to a schedule if it implements IntoSystemConfigs<>. Currently, this is only implemented for systems that take no input and return nothing. Now, this trait is also implemented for any systems that return Result<(), impl Debug>. A custom system adapter is used that panics when the system returns an error and includes the system name in the message.

Initially, I was concerned that this may make the compile error for systems with invalid return types more confusing. However in my testing, the error message does not seem to be any worse.

---

Changelog

• Systems may now return values of type Result<(), E>, where E is any type that implements std::fmt::Debug.

[alice-i-cecile]

I really like this, but it's hard to discover. Can you add a quick handling_errors_in_systems example in bevy_ecs that demonstrates this, one of the built in logging adaptors and a custom adaptor?

[alice-i-cecile]

Minor nit then LGTM.

[joseph-gio]

I'm closing this out. Using ? for errors like this is convenient and pretty when writing code, but I find that it makes the debugging experience worse when something goes wrong, since the error message points to the location where the error was reported rather than where it was produced -- using .unwrap() is actually much nicer here.

If anyone finds a way of improving the panic message reporting, feel free to make a new PR based off of this.