Our API suggests that panicking should be the default

[janhohenheim]

In spirit of #12660
Adapted from #14268

What problem does this solve or what need does it fill?

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.

This is an issue for the following APIs:

• 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

What solution would you like?

Deprecate and remove panicking variants. Improve ergonomics by adding macros for early returns. Using a macro for get_single as an example:

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

Which expands to:

fn do_stuff(primary_window: Query<&Window, With<PrimaryWindow>>) {
    match primary_window.get_single() {
        Ok(item) => item,
        Err => return default(),
    };
    // Do stuff with the window. If there's not exactly one, we have already returned.
}

Note that returning default() will allow the macro to work with systems that return an Option and get piped into error handling methods.

Similar macros are already being used by some users, as indicated by https://github.com/tbillington/bevy_best_practices?tab=readme-ov-file#getter-macros

Paraphrasing @alice-i-cecile:

I'm also coming around on the macro approach: 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.

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.

What alternative(s) have you considered?

• Do nothing
• Leave the API as-is and heavily recommend the get_ variants in documentation
• Print a warning when calling panicking variants
• Keep the panicking variants and rename them to something clunky like _unchecked
• Make the panicking API opt-in via a feature
• Leave the API as-is and let the schedule handle panics
• Use a #[system] macro that modifies the code to allow us to use ? in it, like in bevy_mod_sysfail

Open Questions

Naming

There is a sentiment that get_ is used by the standard library in general when returning an Option.
Quoting @benfrankel:

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.

In a world where Bevy is not panicking by default and always hands out Options and Results, I believe there is little reason to stick to a get_ prefix. It is unnecessary noise on otherwise very concise functions. As such, I would advise to change Bevy's naming convention and drop it as part of this initiative. This may sound like something for another issue, but I'll argue that it should be discussed at least in the same release cycle. Users will already have to go and touch every instance of mut, single, etc. so I don't want to further annoy them by having to change them yet a second time when we drop the get_ prefix.

I realize that this is more controversial however and I'm fine with leaving it. I just want to point out that this is probably the best chance we will get to change it.

Macro parameters

It would be nice to have some influence over what should be done in the unhappy path. For example, we could have a parameter that controls an error! log:

get_single!(foo, "Oh heck");

which expands to:

match foo.get_single() {
    Ok(item) => item,
    Err => {
        error!("Oh heck");
        return default();
    }
};

Of course, the use of error! is arbitrary here and the user may want to use warn! instead or something else altogether. To accommodate for this, we could pass a closure:

get_single!(foo, || error!("Oh heck"));

Which expands to the same as above. Note that this closure could even dictate what we return. Since error! returns (), this works out for the common case, and a user is free to build their own Error variant. However at that point, I don't know if a macro call is saving much typing work anymore.

One could also combine these variants by using labeled parameters:

get_single!(foo, warn: "Oh heck");
get_single!(bar, error: "Oh heck");
get_single!(baz, returns: || error!("Oh heck"));

This would allow the user to have a terse version for the common case usage and a more verbose one for custom error handling.

continue

This discussion has focused on the case where we want an early return. I think that is the common case, at least in my experience. What about continue however? Do we want a _continue variant for all macros? An extra parameter? Is there some kind of Rust hack that turns into either continue or return depending on context? Would that even be what the user expects to happen?

[janhohenheim]

Pinging SME-ECS: @JoJoJet @maniwani

[joseph-gio]

I agree with this I think, however it would be unfortunate for singleton-style components to become more verbose. single is one of those panicking methods that kind of makes sense, since if you use it with a component type that you only spawn one of it's always correct.
Maybe we could remove panicking-by-default and still avoid the verbosity by having a marker trait for singleton components, which would unlock the infallible accessor for that type

[janhohenheim]

@JoJoJet you could also do the following, assuming we remove the get_ suffix and allow passing a function to the macro as a second param:

let singleton = single!(singleton); // early return
let singleton = single!(singleton, panic); // panic if there's not exactly one

This would require some panic util fn just like we have for warn, error, etc.

[benfrankel]

The RFC for postfix macros would make this pattern so much nicer. Waiting for a language feature isn't practical, though.

let window = window_query.single().or_return!("optional warning msg");

[IceSentry]

At work we started using a slightly different pattern. We use the early return with the fallible functions, but we use a debug_assert so it still panics in debug to help us catch those issues. That way it doesn't panic for our users if something unexpected happens.

[cart]

My take is generally still what I discussed here: #12660 (comment)

Specifically, I think we should investigate the viability of the following plan:

1. Make Result systems feel more automatic / first class (id like to stress that result systems are already possible, they just currently require an addition function call at registration). Ideally it is just as simple as returning Result in an existing system. If this isn't possible (while retaining support for non-result systems) for type-system reasons, investigate further.

   app.add_systems(Update, window_thing)
   
   fn window_thing(windows: Query<&Window>) -> Result {
     let window = windows.get_single()?;
     OK // this is a const export of Ok(())
   }

2. Replace the majority of Option-returning functions with Results, which then enables ? to work seamlessly for pretty much everything:

   fn world_thing(world: &mut World) -> Result {
     let entity = world.get_entity(SOME_ENTITY)?;
     OK
   }

3. Replace panicking nice-name api names (ex: entity()) with the Result-returning variant (and remove the old get_entity() function):

   fn world_thing(world: &mut World) -> Result {
     let entity = world.entity(SOME_ENTITY)?;
     OK
   }

Imo it is critical to prove steps (2) and (3) out first and do it "as a single transaction" from a user perspective. Skipping straight to step (3) (while easy) puts the cart before the horse and imo makes Bevy UX worse for a period of time (at best) and at worst, commits us to a path that may not be viable or ideal (due to unknowns not yet discovered). Skipping (2) would mean that ? isn't the "ergonomic one size fits all" solution it needs to be.

I do think the macro approach is an interesting "middle ground", but it feels like we're just re-implementing the ? behavior in a clunkier / less idiomatic / less flexible way. Rather than create a zoo of ways to solve this problem (and encourage people to migrate once to a middle ground solution, then again to the "final" solution), I think I'd prefer to just invest in a single canonical ideal solution.

That being said, if it doesn't already exist, I think it makes a lot of sense to build the macro approach as a 3rd party crate. That seems like an appropriate "quick no bureaucracy" middle ground solution while we sort out the details.

[cart]

As a supplement, I think it would also be interesting to investigate an Option extension trait that lets you do this:

fn system() -> Result {
  let list = vec![1, 2];
  let x = list.get(0).unpack()?;
  let y = list.get(1).assume("index 1 should exist")?;
  OK
}

Where unpack() (non panicking unwrap variant) maps Option<T> to Result<T, ContextlessError>. And assume() (non panicking expect variant) maps to Result<T, MessageError>.