Better `ScheduleBuildError` introspection and handling #20256

ItsDoot · 2025-07-23T02:29:27Z

Objective

Part of Extensible Schedule Graphs #20115

I want to pull out the ad-hoc graph manipulation and analysis functions into generalized reusable functions, but in doing so encounter some borrow-checker issues. This is because the graphs are mutably borrowed during the build process at the points where we need to render warning and error messages (which require full access to the ScheduleGraph).

Solution

So, lets defer message rendering until the consumer actually needs it:

Replaced Strings in ScheduleBuildError variants with just the NodeId/SystemKey/SystemSetKey.
Added ScheduleBuildError::to_string to render the messages as they were previously.
Added ScheduleBuildWarning, a subset of the error enum of just the possible warnings.
Collected warnings into a Vec and returned them from the build process, caching them in the Schedule and made accessible via Schedule::warnings. Additionally automatically warn!() these warnings after the schedule is built.
Exposed ScheduleGraph::get_node_name so external consumers can do any special rendering themselves.
Finally, moved ScheduleBuildError and ScheduleBuildWarning to their own files to help incrementally minimize the large schedule.rs module.

Testing

Reusing current tests.

alice-i-cecile

Good cleanup. The context about the borrow checker was really helpful in understanding why this is necessary.

hymm · 2025-07-23T05:22:59Z

crates/bevy_ecs/src/schedule/error.rs

+/// Category of warnings encountered during [`Schedule::initialize`](crate::schedule::Schedule::initialize).
+#[non_exhaustive]
+#[derive(Error, Debug)]
+pub enum ScheduleBuildWarning {


Can we just alias ScheduleBuildError as ScheduleBuildWarning? It doesn't feel like it deserves it's own type as it's just the difference in how the error is reported/handled.

I actually added it last minute expecting someone to say something along the lines of "well if only two of the variants can actually be warnings then there should be a separate enum". Happy to remove it, though!

IMO, the error enum should have an ElevatedWarning variant that wraps these warnings. I think that would be best of both worlds.

Wouldn't that make the type recursive and hence unsized?

Added ScheduleBuildError::Elevated and removed the duplicate variants in ScheduleBuildError.

Ah I misunderstood what the suggestion was. I think I still prefer my solution as now you have to look in two places to see all the errors, but not going to block on it.

ElliottjPierce

A couple of small ideas, but looks good to me! I don't know enough about how schedules are built to do a full correctness review, but the code quality looks good to me!

I have also been annoyed with the previous state because of borrow checking when I was working on queued component registration, so I'm very glad for this to be cleaned up!

ElliottjPierce · 2025-07-23T14:50:28Z

crates/bevy_ecs/src/schedule/schedule.rs

-        self.initialize(world)
-            .unwrap_or_else(|e| panic!("Error when initializing schedule {:?}: {e}", self.label));
+        self.initialize(world).unwrap_or_else(|e| {
+            panic!(


This panic feels like it should be an error returned out, but same behavior as before, so I'm fine with it.

Replacing panics with errors is always good, but I'll leave that for a small follow up PR.

ElliottjPierce · 2025-07-23T14:55:55Z

crates/bevy_ecs/src/schedule/error.rs

+impl ScheduleBuildError {
+    /// Renders the error as a human-readable string with node identifiers
+    /// replaced with their names.
+    pub fn to_string(&self, graph: &ScheduleGraph, world: &World) -> String {


What happens when the wrong graph or world is passed in? I don't really care as long as it doesn't panic. And documenting what would happen would be good too IMO.

IDK, but another idea would be to keep a reference to the graph and world in the error types. That way, using the wrong world and graph would be impossible. As long as the reference is only tied in at the end, I don't think it would cause any borrow checking issues while calculating the errors.

What happens when the wrong graph or world is passed in? I don't really care as long as it doesn't panic. And documenting what would happen would be good too IMO.

It just leads to incorrect node names and component names; I added this as a note for the function.

IDK, but another idea would be to keep a reference to the graph and world in the error types. That way, using the wrong world and graph would be impossible. As long as the reference is only tied in at the end, I don't think it would cause any borrow checking issues while calculating the errors.

Eh, I think we've had annoyances in the past with error types that hold references.

ElliottjPierce · 2025-07-23T14:58:40Z

crates/bevy_ecs/src/schedule/error.rs

+/// Category of warnings encountered during [`Schedule::initialize`](crate::schedule::Schedule::initialize).
+#[non_exhaustive]
+#[derive(Error, Debug)]
+pub enum ScheduleBuildWarning {


IMO, the error enum should have an ElevatedWarning variant that wraps these warnings. I think that would be best of both worlds.

ElliottjPierce · 2025-07-23T15:02:00Z

crates/bevy_ecs/src/schedule/error.rs

+/// Category of errors encountered during [`Schedule::initialize`](crate::schedule::Schedule::initialize).
+#[non_exhaustive]
+#[derive(Error, Debug)]
+pub enum ScheduleBuildError {


I don't think this needs to change since it was like this before, but It sounds like multiple of these errors could happen at once. Like there could be a cross-dependency and a dependency loop I would guess. Not returning all errors could turn fixing them into a game of wack-a-mole. It might be nice to just have a struct that could represent all possible permutations of errors, like vecs of dependency loops, a bool for uninitialized, etc. IDK if that's actually a good idea; just something to think about.

I want to move in this direction at some point, but would rather let the larger changes surface first.

alice-i-cecile

I like the elevated warning variant. Nice!

better schedule build error introspection

8f66744

better docs

eeefc15

ItsDoot requested review from hymm, alice-i-cecile, chescock and ElliottjPierce July 23, 2025 03:08

ItsDoot added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jul 23, 2025

ItsDoot mentioned this pull request Jul 23, 2025

Extensible Schedule Graphs #20115

Open

alice-i-cecile approved these changes Jul 23, 2025

View reviewed changes

hymm reviewed Jul 23, 2025

View reviewed changes

ElliottjPierce approved these changes Jul 23, 2025

View reviewed changes

alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 23, 2025

ItsDoot added 3 commits July 23, 2025 19:33

add docs about params passed to ScheduleBuildError::to_string

a9f5207

wrap elevated warning types into ScheduleBuildError::Elevated

131dcf5

update migration guide

3314636

alice-i-cecile approved these changes Jul 24, 2025

View reviewed changes

alice-i-cecile added this pull request to the merge queue Jul 24, 2025

Merged via the queue into bevyengine:main with commit 0090846 Jul 24, 2025
32 checks passed

Uh oh!

Better ScheduleBuildError introspection and handling #20256

Better ScheduleBuildError introspection and handling #20256

Conversation

ItsDoot commented Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Testing

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ItsDoot Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElliottjPierce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Better `ScheduleBuildError` introspection and handling #20256

Better `ScheduleBuildError` introspection and handling #20256

ItsDoot commented Jul 23, 2025 •

edited

Loading

ItsDoot Jul 24, 2025 •

edited

Loading