Replace ValidationOutcome with Result by chescock · Pull Request #18541 · bevyengine/bevy

chescock · 2025-03-25T17:33:06Z

Objective

Make it easier to short-circuit system parameter validation.

Simplify the API surface by combining ValidationOutcome with SystemParamValidationError.

Solution

Replace ValidationOutcome with Result<(), SystemParamValidationError>. Move the docs from ValidationOutcome to SystemParamValidationError.

Add a skipped field to SystemParamValidationError to distinguish the Skipped and Invalid variants.

Use the ? operator to short-circuit validation in tuples of system params.

alice-i-cecile · 2025-03-25T19:20:12Z

crates/bevy_ecs/src/system/system_param.rs

-                // combining the results into a single `ValidationOutcome`.
-                ValidationOutcome::Valid$(.combine($param::validate_param($param, system_meta, world)))*
+                $(
+                    $param::validate_param($param, system_meta, world)?;


This is the tricky macro code that I was struggling to make short-circuit before.

alice-i-cecile

Semantically this makes sense to me: parma-validation is very Result-like. I also like cutting down on the code needed, and you managed to get the macro short-circuiting properly!

Good cleanup; thanks for chewing on this :)

ElliottjPierce

A few suggestions but nothing blocking approval.

There's other ways to do the short circuiting in the macro, but I agree that Result is semantically valuable here. Might be worth a type alias to provide documentation for what the result means more directly, but that's not a huge deal.

ElliottjPierce · 2025-03-25T19:33:53Z

crates/bevy_ecs/src/system/system_param.rs

+    }
+
+    /// Constructs a `SystemParamValidationError` that skips the system.
+    pub fn skipped() -> Self {


This is a small thing, but could we make these constants instead? Or make them const functions with inline?(Though they're probably inlined already)

I don't want to make them actual constants because I'm anticipating we'll want to add parameters (#18515) and it should be a smoother transition if they're already functions.

Making them const fn is a good idea, though! I'll do that.

ElliottjPierce · 2025-03-25T19:36:21Z

crates/bevy_ecs/src/system/system_param.rs

-pub struct SystemParamValidationError;
+pub struct SystemParamValidationError {
+    /// Whether the system should be skipped.
+    pub skipped: bool,


Do we need this to be public? If we're already exposing is_skipped, etc, I don't see the need for this. Making it private would make this easier to change in the future, and it forces users to use the better documented functions.

Suggested change

pub skipped: bool,

skipped: bool,

I'd prefer to keep this pub and kill the helper method if we're deduplicating.

Whoops, I had started with it pub and failed to remove it after I added the method. Good catch!

But it sounds like we want it pub, so I'll go remove the helper methods instead. (I guess I still have some old C# habits.)

Works for me!

ElliottjPierce · 2025-03-25T19:40:06Z

crates/bevy_ecs/src/system/system_param.rs

 /// and handled using the unified error handling mechanisms defined in [`bevy_ecs::error`].
 #[derive(Debug, PartialEq, Eq, Clone, Display, Error)]
-pub struct SystemParamValidationError;
+pub struct SystemParamValidationError {


This is a thiserror derive, but I don't see the #[error(msg)] attribute. Instead Display is derived directly. Nothing wrong with that, but is this intentional?

No clue, sorry. It was like that when I got here, and I'm not familliar with thiserror.

That's fine. We can revisit this later if we need to.

ElliottjPierce · 2025-03-25T19:45:20Z

crates/bevy_ecs/src/schedule/executor/single_threaded.rs

-                    );
+                Ok(()) => (),
+                Err(e) => {
+                    if !e.is_skipped() {


This is repeated in a bunch of places. It was before too, but it might be worth pulling out into an associated function on the error type. Definitely not a problem, but worth mentioning.

Yup, I said the same thing on the original PR :) #18504 (comment) It sounded like it was tricky to solve at the moment, so I didn't try.

Well, it's only a little duplication, and it may be subject to change anyway. So we can come back to it if it's final form still has duplication and it becomes boiler plate.

alice-i-cecile · 2025-03-25T20:08:54Z

I'll give you a couple days to respond to feedback here, but I'm going to merge this before the next release candidate regardless <3

# Objective Make it easier to short-circuit system parameter validation. Simplify the API surface by combining `ValidationOutcome` with `SystemParamValidationError`. ## Solution Replace `ValidationOutcome` with `Result<(), SystemParamValidationError>`. Move the docs from `ValidationOutcome` to `SystemParamValidationError`. Add a `skipped` field to `SystemParamValidationError` to distinguish the `Skipped` and `Invalid` variants. Use the `?` operator to short-circuit validation in tuples of system params.

…stemError (#18666) # Objective Provide more useful errors when `World::run_system` and related methods fail parameter validation. Let callers determine whether the validation failure would have skipped or failed the system. Follow-up to #18541. ## Solution Add a `SystemParamValidationError` value to the `RunSystemError::InvalidParams` and `RegisteredSystemError::InvalidParams` variants. That includes the complete context of the parameter validation error, including the `skipped` flag.

chescock added 2 commits March 25, 2025 13:22

Replace ValidationOutcome with Result.

86e8289

Semicolon to fix clippy lint.

97142c4

alice-i-cecile added this to the 0.16 milestone Mar 25, 2025

alice-i-cecile reviewed Mar 25, 2025

View reviewed changes

alice-i-cecile approved these changes Mar 25, 2025

View reviewed changes

ElliottjPierce approved these changes Mar 25, 2025

View reviewed changes

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

chescock added 2 commits March 25, 2025 20:36

Make skipped() and invalid() be const.

ce43771

Remove is_skipped() method in favor of using the field directly.

3f6bb5a

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-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Mar 26, 2025

alice-i-cecile added this pull request to the merge queue Mar 26, 2025

Merged via the queue into bevyengine:main with commit 837991a Mar 26, 2025
33 checks passed

chescock mentioned this pull request Apr 1, 2025

Include SystemParamValidationError in RunSystemError and RegisteredSystemError #18666

Merged

Uh oh!

Conversation

chescock commented Mar 25, 2025

Objective

Solution

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!

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!

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 commented Mar 25, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants