[bevy_ui/layout] Replace unwrap, expect, and panic with Result

[StrikeForceZero]

This is 5 of 5 iterative PR's that affect bevy_ui/layout

• Blocked by [bevy_ui/layout] Extract UiSurface to its own file #12801
• Blocked by [bevy_ui/layout] Update tests to get GlobalTransform properly #12802
• Blocked by [bevy_ui/layout] Add tests, missing asserts, missing debug field, and widen children param in update_children for UiSurface #12803
• Blocked by [bevy_ui/layout] UiSurface update internal mappings #12804

Diff to parent PR

This is a very ambitious attempt to add error handling to UiSurface and the ui_layout_system (WIP) instead of panicking

---

Objective

• Replace unwrap, expect, panic!, with Result
  • unwrap considered harmful #12660

Solution

• Replaces all instances unwrap, expect, panic!, with Result
• Adds UiSurfaceError

---

Changelog

Migration Guide

TBD - probably should be saved for 0.14

[yyogo]

This... doesn't really make sense to me. Unreachable code by definition has no flow control paths leading to it. How is a caller expected to handle this error? Why is avoiding unreachable!() even a goal? You can't get rid of all possible panics, std is riddled with panicking API...

The same applies to internal state errors. If an internal invariant doesn't hold that is a bug, not an exception, and I don't understand how application code is expected to handle this. Panicking with an appropriate message is the most reasonable course of action in these scenarios IMO.

[StrikeForceZero]

Well, you're not wrong, and I agree. I didn't mean all panics, just the ones created by unwrap and such. However, I'm clearly getting carried away by including Unreachable in its scope. Thank you for calling me out on that early.

My original intentions were to bubble everything up to the caller (in this case, the ui_layout_system), expecting it to return a Result as well. Then that could just let unhandled errors trigger error logs (via ui_layout_system.map(error)) instead of crashing. That way, it could maybe allow other UI entities to finish processing. But that increases the complexity as seen here.

You make another excellent point about "how can a caller be expected to handle InvalidState?"

Context: Right now, the internal maps are exposed as pub(super). This could result in someone adding code outside the structs associated functions that interact directly with the internal map or the specific sequence of calls initiated by the calling system that creates an InvalidState. For now, InvalidState is a marker for something intended to be unreachable but not guaranteed. If the maps can be made private and the exposed API made to be infallible, then I think replacing InvalidState back to panic! or unreachable! depending on the context would be appropriate.

There's not much expectation of actually handling InvalidState as the caller. Not saying we should, but theoretically, we could attempt to recreate the UiSurface for transient issues like a child UI node being promoted to the root UI node.

When I circle back around to this I'll

• remove Unreachable and revert to unreachable!.