Skip to content

Commit 2ad5908

Browse files
Make Query::single (and friends) return a Result (#18082)
# 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::single` panics 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 else` statements in Rust and the fancy system error handling work in 0.16 have changed my mind on the right balance here. ## Solution 1. Make `Query::single` and `Query::single_mut` (and other random related methods) return a `Result`. 2. Handle all of Bevy's internal usage of these APIs. 3. Deprecate `Query::get_single` and friends, since we've moved their functionality to the nice names. 4. Add detailed advice on how to best handle these errors. 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 `.single` to track down any missed deprecated usages. As to whether or not all the migrations were successful, that's what CI is for :) ## Future work ~~Rename `Query::get_single` and friends to `Query::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_mut` and their `QueryState` equivalents now return a `Result`. Generally, you'll want to: 1. Use Bevy 0.16's system error handling to return a `Result` using the `?` operator. 2. Use a `let else Ok(data)` block to early return if it's an expected failure. 3. Use `unwrap()` or `Ok` destructuring inside of tests. The old `Query::get_single` (etc) methods which did this have been deprecated.
1 parent ff1ae62 commit 2ad5908

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

50 files changed

+261
-268
lines changed

crates/bevy_core_pipeline/src/oit/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ fn configure_depth_texture_usages(
162162
}
163163

164164
// Find all the render target that potentially uses OIT
165-
let primary_window = p.get_single().ok();
165+
let primary_window = p.single().ok();
166166
let mut render_target_has_oit = <HashSet<_>>::default();
167167
for (camera, has_oit) in &cameras {
168168
if has_oit {

crates/bevy_dev_tools/src/picking_debug.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -260,7 +260,7 @@ pub fn debug_draw(
260260
.map(|(entity, camera)| {
261261
(
262262
entity,
263-
camera.target.normalize(primary_window.get_single().ok()),
263+
camera.target.normalize(primary_window.single().ok()),
264264
)
265265
})
266266
.filter_map(|(entity, target)| Some(entity).zip(target))

crates/bevy_ecs/compile_fail/tests/ui/query_lifetime_safety.rs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -27,29 +27,29 @@ fn main() {
2727
}
2828

2929
{
30-
let data: &Foo = query.single();
31-
let mut data2: Mut<Foo> = query.single_mut();
30+
let data: &Foo = query.single().unwrap();
31+
let mut data2: Mut<Foo> = query.single_mut().unwrap();
3232
//~^ E0502
3333
assert_eq!(data, &mut *data2); // oops UB
3434
}
3535

3636
{
37-
let mut data2: Mut<Foo> = query.single_mut();
38-
let data: &Foo = query.single();
37+
let mut data2: Mut<Foo> = query.single_mut().unwrap();
38+
let data: &Foo = query.single().unwrap();
3939
//~^ E0502
4040
assert_eq!(data, &mut *data2); // oops UB
4141
}
4242

4343
{
44-
let data: &Foo = query.get_single().unwrap();
45-
let mut data2: Mut<Foo> = query.get_single_mut().unwrap();
44+
let data: &Foo = query.single().unwrap();
45+
let mut data2: Mut<Foo> = query.single_mut().unwrap();
4646
//~^ E0502
4747
assert_eq!(data, &mut *data2); // oops UB
4848
}
4949

5050
{
51-
let mut data2: Mut<Foo> = query.get_single_mut().unwrap();
52-
let data: &Foo = query.get_single().unwrap();
51+
let mut data2: Mut<Foo> = query.single_mut().unwrap();
52+
let data: &Foo = query.single().unwrap();
5353
//~^ E0502
5454
assert_eq!(data, &mut *data2); // oops UB
5555
}

crates/bevy_ecs/compile_fail/tests/ui/query_lifetime_safety.stderr

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,9 @@ error[E0502]: cannot borrow `query` as immutable because it is also borrowed as
4545
error[E0502]: cannot borrow `query` as mutable because it is also borrowed as immutable
4646
--> tests/ui/query_lifetime_safety.rs:45:39
4747
|
48-
44 | let data: &Foo = query.get_single().unwrap();
48+
44 | let data: &Foo = query.single().unwrap();
4949
| ----- immutable borrow occurs here
50-
45 | let mut data2: Mut<Foo> = query.get_single_mut().unwrap();
50+
45 | let mut data2: Mut<Foo> = query.single_mut().unwrap();
5151
| ^^^^^^^^^^^^^^^^^^^^^^ mutable borrow occurs here
5252
46 |
5353
47 | assert_eq!(data, &mut *data2); // oops UB
@@ -56,9 +56,9 @@ error[E0502]: cannot borrow `query` as mutable because it is also borrowed as im
5656
error[E0502]: cannot borrow `query` as immutable because it is also borrowed as mutable
5757
--> tests/ui/query_lifetime_safety.rs:52:30
5858
|
59-
51 | let mut data2: Mut<Foo> = query.get_single_mut().unwrap();
59+
51 | let mut data2: Mut<Foo> = query.single_mut().unwrap();
6060
| ----- mutable borrow occurs here
61-
52 | let data: &Foo = query.get_single().unwrap();
61+
52 | let data: &Foo = query.single().unwrap();
6262
| ^^^^^ immutable borrow occurs here
6363
53 |
6464
54 | assert_eq!(data, &mut *data2); // oops UB

crates/bevy_ecs/compile_fail/tests/ui/query_to_readonly.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,13 +35,13 @@ fn for_loops(mut query: Query<&mut Foo>) {
3535
fn single_mut_query(mut query: Query<&mut Foo>) {
3636
// this should fail to compile
3737
{
38-
let mut mut_foo = query.single_mut();
38+
let mut mut_foo = query.single_mut().unwrap();
3939

4040
// This solves "temporary value dropped while borrowed"
4141
let readonly_query = query.as_readonly();
4242
//~^ E0502
4343

44-
let ref_foo = readonly_query.single();
44+
let ref_foo = readonly_query.single().unwrap();
4545

4646
*mut_foo = Foo;
4747

@@ -55,7 +55,7 @@ fn single_mut_query(mut query: Query<&mut Foo>) {
5555

5656
let ref_foo = readonly_query.single();
5757

58-
let mut mut_foo = query.single_mut();
58+
let mut mut_foo = query.single_mut().unwrap();
5959
//~^ E0502
6060

6161
println!("{ref_foo:?}");

crates/bevy_ecs/compile_fail/tests/ui/query_transmute_safety.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,8 @@ fn main() {
2222
let mut query_a = lens_a.query();
2323
let mut query_b = lens_b.query();
2424

25-
let a = query_a.single_mut();
26-
let b = query_b.single_mut(); // oops 2 mutable references to same Foo
25+
let a = query_a.single_mut().unwrap();
26+
let b = query_b.single_mut().unwrap(); // oops 2 mutable references to same Foo
2727
assert_eq!(*a, *b);
2828
}
2929

@@ -34,8 +34,8 @@ fn main() {
3434
let mut query_b = lens.query();
3535
//~^ E0499
3636

37-
let a = query_a.single_mut();
38-
let b = query_b.single_mut(); // oops 2 mutable references to same Foo
37+
let a = query_a.single_mut().unwrap();
38+
let b = query_b.single_mut().unwrap(); // oops 2 mutable references to same Foo
3939
assert_eq!(*a, *b);
4040
}
4141
}

crates/bevy_ecs/src/change_detection.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1557,7 +1557,7 @@ mod tests {
15571557
// Since the world is always ahead, as long as changes can't get older than `u32::MAX` (which we ensure),
15581558
// the wrapping difference will always be positive, so wraparound doesn't matter.
15591559
let mut query = world.query::<Ref<C>>();
1560-
assert!(query.single(&world).is_changed());
1560+
assert!(query.single(&world).unwrap().is_changed());
15611561
}
15621562

15631563
#[test]

crates/bevy_ecs/src/query/builder.rs

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ use super::{FilteredAccess, QueryData, QueryFilter};
3333
/// .build();
3434
///
3535
/// // Consume the QueryState
36-
/// let (entity, b) = query.single(&world);
36+
/// let (entity, b) = query.single(&world).unwrap();
3737
/// ```
3838
pub struct QueryBuilder<'w, D: QueryData = (), F: QueryFilter = ()> {
3939
access: FilteredAccess<ComponentId>,
@@ -297,13 +297,13 @@ mod tests {
297297
.with::<A>()
298298
.without::<C>()
299299
.build();
300-
assert_eq!(entity_a, query_a.single(&world));
300+
assert_eq!(entity_a, query_a.single(&world).unwrap());
301301

302302
let mut query_b = QueryBuilder::<Entity>::new(&mut world)
303303
.with::<A>()
304304
.without::<B>()
305305
.build();
306-
assert_eq!(entity_b, query_b.single(&world));
306+
assert_eq!(entity_b, query_b.single(&world).unwrap());
307307
}
308308

309309
#[test]
@@ -319,13 +319,13 @@ mod tests {
319319
.with_id(component_id_a)
320320
.without_id(component_id_c)
321321
.build();
322-
assert_eq!(entity_a, query_a.single(&world));
322+
assert_eq!(entity_a, query_a.single(&world).unwrap());
323323

324324
let mut query_b = QueryBuilder::<Entity>::new(&mut world)
325325
.with_id(component_id_a)
326326
.without_id(component_id_b)
327327
.build();
328-
assert_eq!(entity_b, query_b.single(&world));
328+
assert_eq!(entity_b, query_b.single(&world).unwrap());
329329
}
330330

331331
#[test]
@@ -385,7 +385,7 @@ mod tests {
385385
.data::<&B>()
386386
.build();
387387

388-
let entity_ref = query.single(&world);
388+
let entity_ref = query.single(&world).unwrap();
389389

390390
assert_eq!(entity, entity_ref.id());
391391

@@ -408,7 +408,7 @@ mod tests {
408408
.ref_id(component_id_b)
409409
.build();
410410

411-
let entity_ref = query.single(&world);
411+
let entity_ref = query.single(&world).unwrap();
412412

413413
assert_eq!(entity, entity_ref.id());
414414

crates/bevy_ecs/src/query/error.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ impl<'w> PartialEq for QueryEntityError<'w> {
104104
impl<'w> Eq for QueryEntityError<'w> {}
105105

106106
/// An error that occurs when evaluating a [`Query`](crate::system::Query) or [`QueryState`](crate::query::QueryState) as a single expected result via
107-
/// [`get_single`](crate::system::Query::get_single) or [`get_single_mut`](crate::system::Query::get_single_mut).
107+
/// [`single`](crate::system::Query::single) or [`single_mut`](crate::system::Query::single_mut).
108108
#[derive(Debug, Error)]
109109
pub enum QuerySingleError {
110110
/// No entity fits the query.

crates/bevy_ecs/src/query/mod.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -763,8 +763,8 @@ mod tests {
763763
let _: Option<&Foo> = q.get(&world, e).ok();
764764
let _: Option<&Foo> = q.get_manual(&world, e).ok();
765765
let _: Option<[&Foo; 1]> = q.get_many(&world, [e]).ok();
766-
let _: Option<&Foo> = q.get_single(&world).ok();
767-
let _: &Foo = q.single(&world);
766+
let _: Option<&Foo> = q.single(&world).ok();
767+
let _: &Foo = q.single(&world).unwrap();
768768

769769
// system param
770770
let mut q = SystemState::<Query<&mut Foo>>::new(&mut world);
@@ -776,9 +776,9 @@ mod tests {
776776

777777
let _: Option<&Foo> = q.get(e).ok();
778778
let _: Option<[&Foo; 1]> = q.get_many([e]).ok();
779-
let _: Option<&Foo> = q.get_single().ok();
779+
let _: Option<&Foo> = q.single().ok();
780780
let _: [&Foo; 1] = q.many([e]);
781-
let _: &Foo = q.single();
781+
let _: &Foo = q.single().unwrap();
782782
}
783783

784784
// regression test for https://github.com/bevyengine/bevy/pull/8029

0 commit comments

Comments
 (0)