Remove single and single_mut

[janhohenheim]

Objective

In spirit of #12660

I believe it is not controversial to say that when an API offers two similar functions with similar names, the shorter will seem like the default. As such, I believe many people will instinctively gravitate to single and single_mut over get_single and get_single_mut. This means we are subtly pushing users to prefer the implicitly panicking version over the fallible one. This is bad, as it leads to games that may run well on the developers machine but then panic in an edge case.
It is currently understood in the wider Rust community that panics should be an exceptional case, a nuclear option for when recovery is impossible.
We should do our best to make it as easy to do the right thing by default (See Falling Into The Pit of Success). This means that it should be easier to handle an error by propagating it with ? to a logger or similar than to panic. Our current API does the opposite.

Solution

• Remove the panicking single and single_mut

Alternatives

Per the discussion below, renaming get_single to single is seen as a bad move for naming consistency reasons.

Future Work

I'd like to increase the ergonomics of single and single_mut by introducing the following macro, adapted from https://github.com/tbillington/bevy_best_practices?tab=readme-ov-file#getter-macros

#[macro_export]
macro_rules! single {
    ($q:expr) => {
        match $q.single() {
            Ok(m) => m,
            _ => return default(),
        }
    };
}

Which allows doing the following:

fn do_stuff(primary_window: Query<&Window, With<PrimaryWindow>>) {
    let window = single!(primary_window);
    // Do stuff with the window. If there's not exactly one, we have already returned.
}

I'll leave that change to another PR however, as that is more controversial.

Migration Guide

• Replace calls to single() and single_mut() with single().unwrap() and single_mut().unwrap()
• Rename instances of get_single() and get_single_mut() to single() and single_mut()

[ChristopherBiscardi]

I don't have a strong opinion on specifics, but since this kind of change was made before I'd like to see something more global in analysis compared to locally swapping back again. For example, whatever the semantics, I'd prefer to see similar APIs (World.resource/get_resource) be similar in API design. It would be awkward to teach that .single was Option but .resource wasn't.

[janhohenheim]

@ChristopherBiscardi thanks for the link! I'm very open to changing this to only removing single() and single_mut() or renaming them to something clunky like single_unchecked(). If usability ends up being a blocker, as suggested by the linked comment, I can also include the macro I mentioned above into this PR.

[alice-i-cecile]

I agree with the notes on consistency. I'm also coming around on the macro approach (but yes, different PR!): I'd rather this be a Rust feature where we can just ? in function that return (), but failing that this is better than the endless let else return pattern.

As the engine matures, I'm increasingly against panicking APIs, especially in seemingly innocuous cases like this. While it's nice for prototyping, it's a serious hazard for refactors and production-grade apps.

As I see it there are three reasonable options:

1. Leave the APIs as they are now, prioritizing the panicking variants.
2. Remove the panicking APIs and let users call .unwrap.
3. Remove the panicking APIs, and add macros to extract or return and extract or continue (since this is often done within a loop).

My preferences are 3 > 2 > 1, with aggressive documentation cross-linking the macros and the methods and explaining exactly what the macro is sugar for.

There are several areas that should use a unified approach here:

• Query::single
• Query::get
• World::resource
• World::non_send_resource (although eventually this will likely be on App)
• all of the other non-deferred entity-component-manipulating APIs listed in Use traits to make ECS APIs for working with entities more consistent #14231

We should decide on what to do for all of these areas at the same time and make a consistent decision across the board, pulling in both SME-ECS and @cart. I think they're better implemented as separate PRs to avoid extreme size, but they should be shipped in the same cycle if we make the change.

[janhohenheim]

@alice-i-cecile should I close this PR and start an issue for that?
In the meantime, I'll change it to simply remove single

[alice-i-cecile]

Keep this PR open, but make an issue please :)

[alice-i-cecile]

Actually, there's a fourth option: a #[system] macro that modifies the code to allow us to use ? in it, like in bevy_mod_sysfail. This is my least favorite solution due to the magic and non-locality, but it should be listed for consideration.

[NthTensor]

Unwrapping stuff is easy, catching unwinds is not. I strongly feel that panics are only acceptable in two contexts:

• When invariant are violating going into an unsafe block.
• When the end application author wants to panic.

Safe library code should never panic, and should instead return an Option or Result, or otherwise disable itself and log an error. This applies to both core and ecosystem crates, but not our examples or tests (obviously).

I like this in as far as it promotes a non-panicing api, but the get_single/single split seems backwards compared to the existing methods.

[janhohenheim]

@NthTensor

but the get_single/single split seems backwards compared to the existing methods.

Could you elaborate on what you mean by this?

[NthTensor]

Could you elaborate on what you mean by this?

Mostly our api uses X::foo() -> T panics and X::get_foo() -> Option<T> is non-panicing. This is also how the standard library tends to be set up.

[janhohenheim]

@NthTensor ah gotcha. I've changed the PR so it only removes single and get_single instead :)
Will fix the CI problems and write an issue later.

[benfrankel]

Mostly our api uses X::foo() -> T panics and X::get_foo() -> Option<T> is non-panicing. This is also how the standard library tends to be set up.

This isn't true for the standard library: Vec::first, Iterator::next, Path::parent, Error::source, etc.

The get_ prefix is only used to differentiate from a panicking version. If there's no panicking version, there's no need for the get_ prefix. For some reason Bevy has latched onto foo vs get_foo as the only API choice when foo may fail, but it doesn't have to be this way.

[SkiFire13]

While panicking is obviously unwanted, I feel like this focuses on the wrong issue as just changing .single() to .get_single().unwrap() won't change anything other than making it slightly more visible. The fundamental problem is that actually reacting to unexpected situations is hard. The let else return pattern will lose any information about the unexpected event unless you manually add yet another line with a warn!, going from 1 short line for the old panicking code to 4 for the new one. Returning the Option/Result also similarly loses most of the information, as you won't know which part of the system returned that error. And embedding the warning in get_single doesn't seem possible, as there are cases where a Err is possibly expected and handled accordingly.

And of course after all this the game state might be in an even more unexpected state since the current system didn't run, which will likely will to even more unexpected situations. A reasonable thing to do might be showing an error screen and returning to an earlier "safe" state (e.g. the title screen), but that's very tiresome to do from every single system that could possibly fail. Instead it would be nice if the scheduler could help with that, though I can't see a practical way right now.

Unwrapping stuff is easy, catching unwinds is not.

The scheduler already catches unwinds (except on WASM I guess), though it doesn't do anything with them other than resuming the unwind in the main thread.

[NthTensor]

Instead it would be nice if the scheduler could help with that, though I can't see a practical way right now.

This sounds interesting. I do kind of feel like the impulse to panic stems from the scheduler's lack of support for monadic error handling. Perhaps, when it stabilizes, we could simply adjust the system trait to allow for functions that return impl Try and pass off residuals to observers? Anyway, very off topic.

[NthTensor]

Either way, I think we should still introduce a fallible version of single.

[notmd]

If we want to remove these APIs, please consider marking these deprecated and removed in 0.16. Otherwise, it would be massive breaking and harder to migrate, especially for people who rely on the main branch, they will have to patch all the deps.

[janhohenheim]

@notmd that's a fair point. I'll change my PR tomorrow to leave them in and deprecate them.

[alice-i-cecile]

Either way, I think we should still introduce a fallible version of single.

This exists, in the form of get_single. I use this exclusively within my code base, generally paired with a warn.

[janhohenheim]

Closing for now as the discussion on #14275 points towards a more coordinated effort.