Thoughts on ecs::schedule::StateError and StateError::AlreadyInState

[stephenmartindale]

Once again, I find myself wrangling with ecs::schedule::StateError and I have a lot of thoughts and ideas for potential changes and merge-requests – all of which I would happily code up and submit but, because this is probably worthy of debate, I thought I would start a discussion, first.

Trivialities

First, here are some trivialities:

1. StateError should be in the prelude. → ecs::schedule::StateError now Clone, PartialEq, Eq; added to prelude. #7486
2. StateError, as a fieldless error-kind enum that is already pub, should #[derive(PartialEq, Eq)] at least. → ecs::schedule::StateError now Clone, PartialEq, Eq; added to prelude. #7486

StateError::AlreadyInState

Most of the existing references and uses of StateError are in bevy_ecs/src/schedule/state.rs where it is used for pub fns that mostly return Result<(), StateError>.

In my opinion, a wide change to all that public API that would make it much better would be:

1. Remove StateError::AlreadyInState
2. Change all those pub fn signatures to Result<bool, StateError>, where:
   • Ok(false) is returned if the requested state-change was a NO-OP – i.e. the case where the existing code would return StateError::AlreadyInState
   • Ok(true) would be returned if the state-change proceeded – the existing code would return Ok(())

If one contemplates that, it immediately becomes obvious that many of the functions would no longer need to return Result<_, _> at all because, excepting StateError::AlreadyInState, they don't error. These include:

• pub fn overwrite_set()
• pub fn overwrite_replace()
• pub fn overwrite_push()

All of those, above, could just return bool, not Result.

Also, there's pub fn pop() which currently returns Result<(), StateError> but cannot ever return Err(StateError::AlreadyInState) so, in the strictest terms, it actually shouldn't use StateError as it stands but rather some StateError-{AlreadyInState}. After my proposed refactoring, it could continue to return Result<(), StateError> and that would become [more] correct.

Possible Regressions

None. This would be a breaking change but no functionality would be lost – callers would still be able to distinguish whether a state-related operation was successful, a NO-OP, or a failure.

Argumentative Code Snippets

Consider two call-site cases:

1. The caller cares that the state-transition takes effect and was not redundant with the existing state.
2. The caller only cares that, after the transition, the new state is in effect.

Currently, call-site code might look like this:

// Case 1
my_state.set(MyState::Animating).unwrap();

// Case 2
match my_state.set(MyState::Animating) {
    Ok(_) | Err(StateError::AlreadyInState) => { /*no-op*/ },
    Err(e) => Err(e).unwrap(),
}

If my proposal grants approval:

// Case 1
assert!(my_state.set(MyState::Animating).unwrap());

// Case 2
my_state.set(MyState::Animating).unwrap();

Personally, I like how symmetrical both those two cases are in the second snippet. (Of course, .expect() or arguments to assert! or any myriad mechanisms exist to make nicer errors and panics.)

Outlook

As I said, I am really keen to code up and submit merge-requests for all of the changes I propose – with documentation.

Let me know your opinions and concerns and whether that would be worth the bother – this is an API-breaking-change so I'm starting with a discussion before I start with hacking.