system combinators short circuiting with system failure

[janis-bhm]

Objective

Fixes #20376:

CombinatorSystems short circuit if one of the systems fails to validate, e.g. because it queries a Component that isn't resident in the world.

Solution

Instead, we can treat the system failing as the system yielding false, appropriately querying the second system depending on the logical operator.

Testing

I added a new test that calls the combinators in different configurations with a failing system as both the rhs and lhs of the combinator and counts the invocations of the system and the condition.

---

[github-actions]

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

[alice-i-cecile]

This test needs a clearer name.

[alice-i-cecile]

I don't understand why this change, or the sibling change below, was required for this PR.

[janis-bhm]

IMO the change from a simple a(input)? || b(input)? to the match cases makes the logic of the combinators less clear and not as understandable at a glance, and I figured it was easier to reason about by reusing the OrMarker code here.
I guess it's an aesthetic choice in the end though?

[alice-i-cecile]

It's a bit of an aesthetic choice, but more importantly, it's important to avoid mixing in unrelated / quasi-related changes to make understanding git history and review easier.

Can you please revert these here? I'd be happy to review them in a seperate PR (although my gut reaction is that this is less clear).

[alice-i-cecile]

I agree with this change in principle, but:

1. This needs migration guides.
2. The existing integration-y test would really benefit from improved clarity, either via comments or simplification.
3. All of the combinators need to be tested for this behavior in some way.

[janis-bhm]

I've never done a migration guide before so I just looked at the help and other guides but I'm not sure if this is correct

[janis-bhm]

This is specifically because I wanted the expected closure to exactly mirror the combinator systems. could do without allowing these but I think it would be less clear what's being tested against.

[chescock]

I agree that we should treat Skipped like false in run conditions, but I'm not sure we should treat Failed that way. A Res with a missing resource is a bug, and we should report it to the error handler as soon as its encountered. That's also consistent with the pre-0.16 behavior where a missing resource would cause a panic and not evaluate the second system.

That could lead to a simpler implementation. If we define a conversion method like

fn skipped_as_false(result: Result<bool, RunSystemError>) -> Result<bool, RunSystemError> {
    match result {
        Err(RunSystemError::Skipped(_)) => Ok(false),
        result => result,
    }
}

then we could write all the combinators like Ok(skipped_as_false(a(input))? || skipped_as_false(b(input))?), using the ordinary Boolean operators instead of having to write out all the cases exhaustively.

[alice-i-cecile]

Hmm. This is well implemented, and I think that the semantics are simpler with the current implementation. I'm neutral on the current design vs @chescock's proposal above on the whole.

[chescock]

I agree this is well-implemented! I'd still recommend the alternate semantics I proposed above, since I think errors should be reported as early as possible, but if @alice-i-cecile is happy with this behavior then I'm happy to approve this implementation!

[alice-i-cecile]

I think that in practice this behavior is going to be more useful to users: skipping a system if a missing resource exists and another reason to skip the system is already in place will allow us to be more permissive about unusual gameplay states that have no observable impact on the actual running of the game.

This in turn will avoid us proliferating run conditions, or forcing users to spam skipped_as_false simply to silence warnings. If the most likely outcome of a warning is "the user adds boilerplate to silence a warning", it probably shouldn't be a warning.

That said, I suppose there's an argument to be made for "just set your error handler to log a warning"?

@janis-bhm, I value your opinion here, so I'll wait for you to have a chance to respond before we merge this.

[chescock]

This in turn will avoid us proliferating run conditions, or forcing users to spam skipped_as_false simply to silence warnings.

Just to be clear: I wasn't proposing asking users to call that method! It would be a non-pub implementation detail of the combinators.

I'll also note that either formulation means we can use If<Res<R>> as a parameter in a run condition to skip the system if the resource is missing, and it will work properly through or! I'll concede that the If is still boilerplate, though :).

My background may be giving me the wrong instincts here. I've mostly worked on business applications where we want to abort on errors before they can spread, but a game engine will usually want to keep running and hope something fun happens. I'd still vote for "just set your error handler to log a warning", since then you can at least see that something unexpected happened.

[janis-bhm]

While If<Res<R>> is the correct way to express this, I still think result of Err(missing_resource) || Ok(true) is intuitively true. That said; thinking about this, I do think that swallowing errors (which is what I'm doing here atm) is probably not correct.

[alice-i-cecile]

Just to be clear: I wasn't proposing asking users to call that method! It would be a non-pub implementation detail of the combinators.

Ah, I totally missed this. @janis-bhm, let us know when you have changes for us to consider :)

[janis-bhm]

@alice-i-cecile I'm not totally happy with this because it moves the error handling away from the executor and into the combinator system, but this makes the combinators implementations easy to read again, and in the case where both systems return an error, I think this is the simplest way to let them both be sent to the world's error handler.