Improvements to execution order ambiguity reporting

[alice-i-cecile]

Context

This PR was largely completed by @alfonsolage in #2766. I've simply rebased it and put a few finishing touches on it to get it over the finish line.

Massive thanks and full credit for all his hard work.

Objective

• Execution order ambiguities suck, but our existing tool to discover them isn't very nice.
• It also isn't very discoverable.
• Fixes Simple improvements to ambiguity reporting clarity #1484

Solution

1. Number the reported ambiguities, to make them easier to discuss in a team.
2. Display function signatures of conflicting systems.
3. Explain which component(s) / resource(s) the systems are conflicting on.
4. Report ambiguities where all pairwise combinations of systems conflict on the same data as a group.
5. Create a simple .ignore_all_ambiguities() method that causes a system to be ignored completely in the ambiguity checker. This would only be useful for inconsequential systems like particle effect creation.
6. Replace ambiguity sets with a simple .ambiguous_with(system_label) method, which allows your system to be ambiguous with the provided label (or system, via [Merged by Bors] - Auto-label function systems with SystemTypeIdLabel #4224).
7. Provide an "X unresolved ambiguities detected" message that runs by default. Rather than toggling ambiguity detection on/off, have three states: off / minimal / verbose. This would provide discoverability that system ambiguities are a thing and let developers see when the change that they just made added / removed some ambiguities at a glance.
8. Provide an way to ignore/filter crates when using ambiguity checker.
9. By default, ambiguity checker should ignore internal bevy crates.
10. Improve docs.
11. Add an example of using ambiguity reporting
12. Add a WarnInternal reporting level, and included an example to the tools folder that reports all of the Bevy-internal ambiguities. Should be helpful for developers until we quash the last few.
13. Add a Forbid reporting level, for developers who prefer to force themselves to fix ambiguities ASAP.
14. Add a Deny reporting level, which reports literally every ambiguity and panics, for applications that need true determinism.
15. Use short version of system and type names, rather than full path (generally, this is just noise and makes the output much harder to read).
16. Use tracing's warn and error macros, rather than println.

Changelog

Execution order ambiguity reporting has been significantly improved and a minimal report informing you of the number of ambiguous system pairs present in your schedule is now on by default.

It is now much easier to disable false positive ordering ambiguities, bevy-internal ambiguities are silenced by default and the output is much easier to read and act on.

Sample output:

One of your stages contains 4 pairs of systems with unknown order and conflicting data access.
You may want to add `.before()` or `.after()` constraints between some of these systems to prevent bugs.

1. `system_b` conflicts with `system_e` on ["MyResource"]
2. `system_b` conflicts with `system_d` on ["MyResource", "MyOtherResource"]
3. `system_e` conflicts with `system_c` on ["MyResource"]
4. `system_d` conflicts with `system_c` on ["MyResource"]

Migration Guide

The ReportExecutionOrderAmbiguities resource is now ExecutionOrderAmbiguities: you can adjust the level of the enum to match your preferred strictness level.

The concept of "ambiguity sets" has been simplified away: simply use .label(SharedLabel).ambiguous_with(SharedLabel) with a standard SystemLabel instead.

Status

Rebasing this is proving to be a disaster; I'm going to remake this, ideally in a series of small PRs.

Future work

If you think any of these are dealbreakers, do let me know. For now though, this PR is already way too large.

• solve Detecting system order ambiguities requires a specific world #4364
• solve Ambiguity reporting does not account for effects of Without query filters, leading to false positives #4381
• report stage name (requires piping some metadata through a lot of plumbing), or yeet stages
• remove all internal execution order ambiguities, the example to check for these, and the ReportInternal enum variant (Tracking issue: eliminate or explicitly silence all Bevy-internal execution order ambiguities #4377)

[alice-i-cecile]

Comments

1. There's a bit of a mess when trying to report all of the ambiguities for each stage, as we can't collect exclusive and non-exclusive systems together. Once Add &mut World as a system param and make .exclusive_system() optional #4166 lands, that should be fixable.

2. Because these ambiguities are reported per stage, I really want it to report which stage it's coming from. Unfortunately, I couldn't see a good way to do so: this information is erased at the SystemStage level. I'm considering piping in the stage name somehow, but this PR is already too big.

EDIT: I took a look, and it's not going to be nice. We need this information at SystemStage, but those are constructed from a Box dyn ParallelExecutor, and adding the name at that step breaks the trait object. Officially punted.

3. I'm not thrilled with the way that the ambiguities are disabled per plugin still: it feels very manual for something that should be a setting. Let me see what I can do without a full refactor of how plugins work.

[maniwani]

Looks good. Thanks for following up on my request in the original PR. I also like that the resource is enabled and set to minimal by default.

Just a small nit about wording.

Would it be interesting for CI purposes to have a way to exit the app with the number of conflicts as the status code? Or at least not 0 if there are conflicts

Would it be interesting for CI purposes to have a way to exit the app with the number of conflicts as the status code? Or at least not 0 if there are conflicts

IMO the way to do this is to allow users to forbid ambiguities, at both a plugin and app level. This is fundamentally what we need for determinism. I think that's very much worth pursuing, but out of scope for this.

Is there a way to un-ignore the internal plugins?

Is there a way to un-ignore the internal plugins?

Yes. Change the ignore_crates field of ReportExecutionOrderAmbiguties.

That said, this whole design is eh... I'm going to see if I can come up with a more reasonable design for this.

That said, this whole design [for ignoring crates] is eh... I'm going to see if I can come up with a more reasonable design for this.

I took a pass at this, and it's not going to be triviak. Current problems:

1. This scatters the execution order ignoring all over bevy, with no rhyme or reason.
2. It's very hard to discover that you can do this.
3. We have to store two fields in ReportExecutionOrderAmbiguities, and rely on a overwrought builder struct.

Blockers:

1. We can't reasonably fix all of the internal ambiguities in this PR, for merge-ability reasons.
2. Simply adding the crates to the ignored_crates field doesn't stop ambiguities between them
3. We don't want internally ignored crates to also ignore ambiguities with user systems.
4. We can't reasonably have a "ignore conflicts on this component / resource" config without a Resource trait.

I think I can make this work though, and I think that hard-coding a list internally is the least bad solution.

This test is very fragile and hard to use due to the elaborate "label" reporting strategy.

I've done my best to replace the functionality over in the tests in ambiguity_detection.rs.

This functionality is useful to the public, so I refactored it out into its own method.

In the long-term I'd prefer a type-safe pattern for this sort of initialized vs uninitailized state, but that's way out of scope.

The alternative strategy, of making this a configurable field on a per plugin basis or otherwise enabling this resulted in a much more complex, less ergonomic API.

The hard-coded list still existed, it was just scattered across a dozen different files.

This could probably be done with a single gnarly regex, but I'm not actually sure that would be more readable or maintainable.

There's a function TypeRegistration::get_short_name that does something very similar to this function. Not sure if it matches exactly.

This is an exact match. I've refactored this out into bevy_utils and reused it.

Thanks!

Since you're using println! is this going to try to print to the console in production if people forget to turn this off?

Since you're using println! is this going to try to print to the console in production if people forget to turn this off?

Probably. I'd like to use info!() or warn!() but their logging output is very noisy: #4299 (comment)

@hymm @maniwani @MrGVSV I managed to swap over to the tracing warn and error macros: it's much less noisy now that I condensed the output for each stage. I think this is ready for final review now: it's in a good place, and I would really like to avoid making this PR larger.

Going to carve this up for easier reviewing and less rebasing hell. Based on the original PR description, let's carve this up as follows.

Foundational:

1. Provide an "X unresolved ambiguities detected" message that runs by default. Rather than toggling ambiguity detection on/off, have five states: Allow / WarnMinimal / WarnVerbose / Deny / Forbid. This would provide discoverability that system ambiguities are a thing and let developers see when the change that they just made added / removed some ambiguities at a glance.
2. Solve all remaining Bevy-internal ambiguities.

Disjoint improvements:

1. Number the reported ambiguities, to make them easier to discuss in a team.
2. Display function signatures of conflicting systems.
3. Report ambiguities where all pairwise combinations of systems conflict on the same data as a group.
4. Create a simple .ignore_all_ambiguities() method that causes a system to be ignored completely in the ambiguity checker. This would only be useful for inconsequential systems like particle effect creation.
5. Replace ambiguity sets with a simple .ambiguous_with(system_label) method, which allows your system to be ambiguous with the provided label (or system, via [Merged by Bors] - Auto-label function systems with SystemTypeIdLabel #4224).
6. Explain which component(s) / resource(s) the systems are conflicting on.
7. Provide an way to ignore/filter crates when using ambiguity checker.
8. Use short version of system and type names, rather than full path (generally, this is just noise and makes the output much harder to read).
9. Add an example of using ambiguity reporting

Incremental:

1. Improve docs.
2. Use tracing's warn and error macros, rather than println.

Yeet (fixed by 2 of foundational work):

1. By default, ambiguity checker should ignore internal bevy crates.
2. Add a WarnInternal reporting level, and included an example to the tools folder that reports all of the Bevy-internal ambiguities. Should be helpful for developers until we quash the last few.