Make Query::single (and friends) return a Result #18082
Merged
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
alice-i-cecile
added
A-ECS
alice-i-cecile
added
the
D-Straightforward
bushrat011899
approved these changes
Feb 27, 2025
examples/stress_tests/many_text2d.rs
Outdated
Comment on lines
158
to
160
| let mut camera_transform = camera_query.single_mut(); | ||
| let Ok(mut camera_transform) = camera_query.get_single_mut() else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
Could the signature for move_camera not be changed to return Result, allowing the use of ? here too?
There was a problem hiding this comment.
Yes, but this is an example of a case where silently failing and just returning is always correct. You never want to panic here.
crates/bevy_ecs/src/query/builder.rs
Outdated
| assert_eq!(entity_a, query_a.single(&world)); | ||
| assert_eq!(entity_a, query_a.get_single(&world).unwrap()); |
There was a problem hiding this comment.
Nit: I prefer to wrap the reference value in Ok(...) rather an unwrap in an assert_eq, as that will print a nicer error message. Has the same end-result of failing the test so it's not that big of a deal though.
There was a problem hiding this comment.
Yeah, mildly agree.
2 tasks
cart
reviewed
Feb 28, 2025
Carter0
reviewed
Feb 28, 2025
| /// fn my_system(query: Query<&A>, mut commands: Commands) { | ||
| /// match query.single() { | ||
| /// Ok(a) => (), // Do something with `a` | ||
| /// Err(err) => match err { |
alice-i-cecile
added
S-Waiting-on-Author
|
I still need to fix CI, but once that's done I'm going to call this good to merge. |
alice-i-cecile
changed the title
IQuick143
added
the
M-Needs-Migration-Guide
alice-i-cecile
added
S-Ready-For-Final-Review
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 7, 2025
# Objective Alternative to and closes #18120. Sibling to #18082, see that PR for broader reasoning. Folks weren't sold on the name `many` (get_many is clearer, and this is rare), and that PR is much more complex. ## Solution - Simply deprecate `Query::many` and `Query::many_mut` - Clean up internal usages Mentions of this in the docs can wait until it's fully removed in the 0.17 cycle IMO: it's much easier to catch the problems when doing that. ## Testing CI! ## Migration Guide `Query::many` and `Query::many_mut` have been deprecated to reduce panics and API duplication. Use `Query::get_many` and `Query::get_many_mut` instead, and handle the `Result`. --------- 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
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
As discussed in #14275, Bevy is currently too prone to panic, and makes the easy / beginner-friendly way to do a large number of operations just to panic on failure.
This is seriously frustrating in library code, but also slows down development, as many of the
Query::singlepanics can actually safely be an early return (these panics are often due to a small ordering issue or a change in game state.More critically, in most "finished" products, panics are unacceptable: any unexpected failures should be handled elsewhere. That's where the new
With the advent of good system error handling, we can now remove this.
Note: I was instrumental in a) introducing this idea in the first place and b) pushing to make the panicking variant the default. The introduction of both
let elsestatements in Rust and the fancy system error handling work in 0.16 have changed my mind on the right balance here.Solution
Query::singleandQuery::single_mut(and other random related methods) return aResult.Query::get_singleand friends, since we've moved their functionality to the nice names.Generally I like the diff here, although
get_single().unwrap()in tests is a bit of a downgrade.Testing
I've done a global search for
.singleto track down any missed deprecated usages.As to whether or not all the migrations were successful, that's what CI is for :)
Future work
RenameQuery::get_singleand friends toQuery::single!I've opted not to do this in this PR, and smear it across two releases in order to ease the migration. Successive deprecations are much easier to manage than the semantics and types shifting under your feet.Cart has convinced me to change my mind on this; see #18082 (comment).
Migration guide
Query::single,Query::single_mutand theirQueryStateequivalents now return aResult. Generally, you'll want to:Resultusing the?operator.let else Ok(data)block to early return if it's an expected failure.unwrap()orOkdestructuring inside of tests.The old
Query::get_single(etc) methods which did this have been deprecated.