Skip to content

Fix system param validation for piped systems #18785

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Apr 10, 2025

Conversation

alice-i-cecile
Copy link
Member

@alice-i-cecile alice-i-cecile commented Apr 10, 2025

Objective

Solution

  • Validate the parameters for both systems, combining the errors if both failed validation by simply using an early out.
  • Also fix the same bug for combinator systems while we're here.

Testing

I've added a large number of tests checking the behavior under various permutations. These are separate tests, rather than one mega test because a) it's easier to track down bugs that way and b) many of these are should_panic tests, which will halt the evaluation of the rest of the test!

I've also added a test for exclusive systems being pipeable because we don't have one and I was very surprised that that works!

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 10, 2025
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Apr 10, 2025
@MiniaczQ
Copy link
Contributor

Looks fine, no point spending more time on this, since the full fix requires checking params inside each System::run_unsafe, where the abstraction of chain will be gone.

@chescock
Copy link
Contributor

Won't doing eager validation like this re-introduce the issues from #16638 (comment)? If a user pipes a system that inserts a resource into a system with Res, then that will succeed today but would fail under this change. (And if they pipe a system that invalidates a Single into one that uses it, then the second system will fail instead of skipping, but that's no worse than now.)

@alice-i-cecile
Copy link
Member Author

Won't doing eager validation like this re-introduce the issues from #16638 (comment)? If a user pipes a system that inserts a resource into a system with Res, then that will succeed today but would fail under this change

I was going to say that this is quite unlikely, since all of our existing validated parameters requires structural changes to invalidate, but system piping doesn't work for exclusive systems. I wrote a test: turns out at some point that changed!

That said, I think this is a better tradeoff for now: piping exclusive systems or doing custom validated system params that are affected by an earlier system in the pipe feels quite rare. By contrast, the existing bug breaks a relatively common pattern (using validated params in a piped system) by changing the "everything is fine" behavior into a confusing panic.

@alice-i-cecile alice-i-cecile added the X-Contentious There are nontrivial implications that should be thought through label Apr 10, 2025
Copy link
Contributor

@chescock chescock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, breaking resource_exists::<MyRes>.and(do_stuff) is what seemed really bad, but I see you're doing this for pipe but not combinators, so this does seem like the right tradeoff!

Co-authored-by: Chris Russell <[email protected]>
@alice-i-cecile
Copy link
Member Author

Yeah, breaking resource_exists::.and(do_stuff) is what seemed really bad

Yeah, I saw that doc test fail, stared at it for a while in dismay, and then gave up and reverted the change. That is a much harder fix than I want for "last minute during 0.16".

Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not an ECS dev but the changes look good to me and has really solid tests.

@MrGVSV MrGVSV 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 Apr 10, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 10, 2025
Merged via the queue into bevyengine:main with commit 31bb878 Apr 10, 2025
34 checks passed
mockersf pushed a commit that referenced this pull request Apr 11, 2025
# Objective

- Piped systems are an edge case that we missed when reworking system
parameter validation.
- Fixes #18755.

## Solution

- Validate the parameters for both systems, ~~combining the errors if
both failed validation~~ by simply using an early out.
- ~~Also fix the same bug for combinator systems while we're here.~~

## Testing

I've added a large number of tests checking the behavior under various
permutations. These are separate tests, rather than one mega test
because a) it's easier to track down bugs that way and b) many of these
are `should_panic` tests, which will halt the evaluation of the rest of
the test!

I've also added a test for exclusive systems being pipeable because we
don't have one and I was very surprised that that works!

---------

Co-authored-by: Chris Russell <[email protected]>
jf908 pushed a commit to jf908/bevy that referenced this pull request May 12, 2025
# Objective

- Piped systems are an edge case that we missed when reworking system
parameter validation.
- Fixes bevyengine#18755.

## Solution

- Validate the parameters for both systems, ~~combining the errors if
both failed validation~~ by simply using an early out.
- ~~Also fix the same bug for combinator systems while we're here.~~

## Testing

I've added a large number of tests checking the behavior under various
permutations. These are separate tests, rather than one mega test
because a) it's easier to track down bugs that way and b) many of these
are `should_panic` tests, which will halt the evaluation of the rest of
the test!

I've also added a test for exclusive systems being pipeable because we
don't have one and I was very surprised that that works!

---------

Co-authored-by: Chris Russell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Single causes piped systems to panic
4 participants