-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Improve error message for missing resources #18593
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
alice-i-cecile
merged 3 commits into
bevyengine:main
from
chescock:param-validation-error
Mar 30, 2025
Merged
Improve error message for missing resources #18593
alice-i-cecile
merged 3 commits into
bevyengine:main
from
chescock:param-validation-error
Mar 30, 2025
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JaySpruce
approved these changes
Mar 28, 2025
alice-i-cecile
approved these changes
Mar 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for polishing this up: this is important follow-up that I deliberately punted on :) I think I agree that it would be irresponsible to ship without this.
mockersf
pushed a commit
that referenced
this pull request
Mar 30, 2025
# Objective Fixes #18515 After the recent changes to system param validation, the panic message for a missing resource is currently: ``` Encountered an error in system `missing_resource_error::res_system`: SystemParamValidationError { skipped: false } ``` Add the parameter type name and a descriptive message, improving the panic message to: ``` Encountered an error in system `missing_resource_error::res_system`: SystemParamValidationError { skipped: false, message: "Resource does not exist", param: "bevy_ecs::change_detection::Res<missing_resource_error::MissingResource>" } ``` ## Solution Add fields to `SystemParamValidationError` for error context. Include the `type_name` of the param and a message. Store them as `Cow<'static, str>` and only format them into a friendly string in the `Display` impl. This lets us create errors using a `&'static str` with no allocation or formatting, while still supporting runtime `String` values if necessary. Add a unit test that verifies the panic message. ## Future Work If we change the default error handling to use `Display` instead of `Debug`, and to use `ShortName` for the system name, the panic message could be further improved to: ``` Encountered an error in system `res_system`: Parameter `Res<MissingResource>` failed validation: Resource does not exist ``` However, `BevyError` currently includes the backtrace in `Debug` but not `Display`, and I didn't want to try to change that in this PR.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 31, 2025
# Objective Improve error messages for missing resources. The default error handler currently prints the `Debug` representation of the error type instead of `Display`. Most error types use `#[derive(Debug)]`, resulting in a dump of the structure, but will have a user-friendly message for `Display`. Follow-up to #18593 ## Solution Change the default error handler to use `Display` instead of `Debug`. Change `BevyError` to include the backtrace in the `Display` format in addition to `Debug` so that it is still included. ## Showcase Before: ``` Encountered an error in system `system_name`: SystemParamValidationError { skipped: false, message: "Resource does not exist", param: "bevy_ecs::change_detection::Res<app_name::ResourceType>" } Encountered an error in system `other_system_name`: "String message with\nmultiple lines." ``` After ``` Encountered an error in system `system_name`: Parameter `Res<ResourceType>` failed validation: Resource does not exist Encountered an error in system `other_system_name`: String message with multiple lines. ```
mockersf
pushed a commit
that referenced
this pull request
Mar 31, 2025
# Objective Improve error messages for missing resources. The default error handler currently prints the `Debug` representation of the error type instead of `Display`. Most error types use `#[derive(Debug)]`, resulting in a dump of the structure, but will have a user-friendly message for `Display`. Follow-up to #18593 ## Solution Change the default error handler to use `Display` instead of `Debug`. Change `BevyError` to include the backtrace in the `Display` format in addition to `Debug` so that it is still included. ## Showcase Before: ``` Encountered an error in system `system_name`: SystemParamValidationError { skipped: false, message: "Resource does not exist", param: "bevy_ecs::change_detection::Res<app_name::ResourceType>" } Encountered an error in system `other_system_name`: "String message with\nmultiple lines." ``` After ``` Encountered an error in system `system_name`: Parameter `Res<ResourceType>` failed validation: Resource does not exist Encountered an error in system `other_system_name`: String message with multiple lines. ```
github-merge-queue bot
pushed a commit
that referenced
this pull request
Apr 2, 2025
# Objective Improve the parameter validation error message for `Event(Reader|Writer|Mutator)`. System parameters defined using `#[derive(SystemParam)]`, including the parameters for events, currently propagate the validation errors from their subparameters. The error includes the type of the failing parameter, so the resulting error includes the type of the failing subparameter instead of the derived parameter. In particular, `EventReader<T>` will report an error from a `Res<Events<T>>`, even though the user has no parameter of that type! This is a follow-up to #18593. ## Solution Have `#[derive]`d system parameters map errors during propagation so that they report the outer parameter type. To continue to provide context, add a field to `SystemParamValidationError` that identifies the subparameter by name, and is empty for non-`#[derive]`d parameters. Allow them to override the failure message for individual parameters. Use this to convert "Resource does not exist" to "Event not initialized" for `Event(Reader|Writer|Mutator)`. ## Showcase The validation error for a `EventReader<SomeEvent>` parameter when `add_event` has not been called changes from: Before: ``` Parameter `Res<Events<SomeEvent>>` failed validation: Resource does not exist ``` After ``` Parameter `EventReader<SomeEvent>::events` failed validation: Event not initialized ```
mockersf
pushed a commit
that referenced
this pull request
Apr 3, 2025
# Objective Improve the parameter validation error message for `Event(Reader|Writer|Mutator)`. System parameters defined using `#[derive(SystemParam)]`, including the parameters for events, currently propagate the validation errors from their subparameters. The error includes the type of the failing parameter, so the resulting error includes the type of the failing subparameter instead of the derived parameter. In particular, `EventReader<T>` will report an error from a `Res<Events<T>>`, even though the user has no parameter of that type! This is a follow-up to #18593. ## Solution Have `#[derive]`d system parameters map errors during propagation so that they report the outer parameter type. To continue to provide context, add a field to `SystemParamValidationError` that identifies the subparameter by name, and is empty for non-`#[derive]`d parameters. Allow them to override the failure message for individual parameters. Use this to convert "Resource does not exist" to "Event not initialized" for `Event(Reader|Writer|Mutator)`. ## Showcase The validation error for a `EventReader<SomeEvent>` parameter when `add_event` has not been called changes from: Before: ``` Parameter `Res<Events<SomeEvent>>` failed validation: Resource does not exist ``` After ``` Parameter `EventReader<SomeEvent>::events` failed validation: Event not initialized ```
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
S-Ready-For-Final-Review
This PR has been approved by the community. It's ready for a maintainer to consider merging it
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Fixes #18515
After the recent changes to system param validation, the panic message for a missing resource is currently:
Add the parameter type name and a descriptive message, improving the panic message to:
Solution
Add fields to
SystemParamValidationError
for error context. Include thetype_name
of the param and a message.Store them as
Cow<'static, str>
and only format them into a friendly string in theDisplay
impl. This lets us create errors using a&'static str
with no allocation or formatting, while still supporting runtimeString
values if necessary.Add a unit test that verifies the panic message.
Future Work
If we change the default error handling to use
Display
instead ofDebug
, and to useShortName
for the system name, the panic message could be further improved to:However,
BevyError
currently includes the backtrace inDebug
but notDisplay
, and I didn't want to try to change that in this PR.