Skip to content
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

Reduce runtime panics through SystemParam validation #15276

Merged
merged 33 commits into from
Sep 23, 2024

Conversation

MiniaczQ
Copy link
Contributor

@MiniaczQ MiniaczQ commented Sep 17, 2024

Objective

The goal of this PR is to introduce SystemParam validation in order to reduce runtime panics.

Fixes #15265

Solution

SystemParam now has a new method validate_param(...) -> bool, which takes immutable variants of get_param arguments. The returned value indicates whether the parameter can be acquired from the world. If parameters cannot be acquired for a system, it won't be executed, similarly to run conditions. This reduces panics when using params like Res, ResMut, etc. as well as allows for new, ergonomic params like #15264 or #15302.

Param validation happens at the level of executors. All validation happens directly before executing a system, in case of normal systems they are skipped, in case of conditions they return false.

Warning about system skipping is primitive and subject to change in subsequent PRs.

Testing

Two executor tests check that all executors:

  • skip systems which have invalid parameters:
    • piped systems get skipped together,
    • dependent systems still run correctly,
  • skip systems with invalid run conditions:
    • system conditions have invalid parameters,
    • system set conditions have invalid parameters.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Release-Note Work that should be called out in the blog due to impact X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Sep 17, 2024
@alice-i-cecile
Copy link
Member

The opinions of the SME-ECS (@JoJoJet @maniwani @james7132 and myself) and frankly the rest of the ECS community are very positive on the broad direction of this work, so I'm marking it as X-Blessed.

@MiniaczQ MiniaczQ marked this pull request as ready for review September 18, 2024 14:46
@MiniaczQ
Copy link
Contributor Author

MiniaczQ commented Sep 18, 2024

validate_param is technically not unsafe, but we might want to mark it as so.
In theory, this method shouldn't access component values, despite having a readonly world access.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Very pleased about how non-intrusive this is!

Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

I made some comments on the safety for the single_threaded executor and I believe the simple executor has the same issues. It's late here so I might be missing something obvious, but I don't see update_archetype_component_access being called for the conditions for either of those two executors.

crates/bevy_ecs/src/schedule/executor/single_threaded.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/schedule/executor/single_threaded.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/schedule/executor/single_threaded.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/schedule/executor/single_threaded.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/system_param.rs Show resolved Hide resolved
@alice-i-cecile 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 Sep 22, 2024
crates/bevy_ecs/src/system/function_system.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/system.rs Outdated Show resolved Hide resolved
/// # Safety
///
/// - The caller must ensure that `world` has permission to read any world data
/// registered in [`Self::archetype_component_access`]. There must be no conflicting
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this mention update_archetype_component_access while the other validate_params mention init_state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was copy-paste from get_param/run methods, this means that they are also inconsistent with each other.
Agreed it should refer to update_archetype_component_access and some reference to new_archetype instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed, let me know if that's sufficient.
If not I'm afraid we'll need a follow up PR since Alice wants this merged in the next train

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it's good enough now, I'm ok with merging it and maybe improve the wording in a follow up PR

crates/bevy_ecs/src/system/system_param.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/macros/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/macros/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/system.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/system_param.rs Outdated Show resolved Hide resolved
@alice-i-cecile 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-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Sep 22, 2024
@alice-i-cecile 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 Sep 23, 2024
@MiniaczQ MiniaczQ removed the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Sep 23, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 23, 2024
Merged via the queue into bevyengine:main with commit e312da8 Sep 23, 2024
27 checks passed
@alice-i-cecile
Copy link
Member

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1691 if you'd like to help out.

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-Usability A targeted quality-of-life change that makes Bevy easier to use D-Unsafe Touches with unsafe code in some way M-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Systems should be skipped if their resources cannot be fetched
9 participants