Have Single and Populated fail instead of skipping

[chescock]

Objective

Change the behavior of Single and Populated to fail validation instead of skipping systems. Users who want the skipping behavior can recover it by wrapping them in When<T>.

This is a very controversial change, and there has been a lot of discussion about it! See some discussion on #18504 (comment), which was spun out into #18516, and some later discussion on #18927 (comment).

I'll try to summarize the arguments here so we don't need to repeat all of them in comments, but let me know if I'm missing or misrepresenting anything and I'll try to update the summary. (And maintainers should feel free to update it!)

Pros

• A failing version of Single is useful for asserting invariants about your model, just as the failing Res parameter is useful.
  If we don't change the behavior here, we may need to offer another way to do that.
• A consistent story for when systems skip and when they fail is easier to understand.
  Currently users need to remember the behavior separately for each kind of system parameter, but with this change they would know that systems skip if and only if they write When.
• Once we do resources-as-entities, consistency between Single and Res will be important.
  I expect users to first learn about resources as a separate concept, and later learn that resources are just a special kind of entity. It will be helpful to be able to teach "Res is just a special kind of Single" without needing to qualify "except that it fails instead of skipping".
• Making the default behavior be loud helps catch bugs.
  Regardless of the failure behavior, Single is more convenient than Query when you know you have exactly one entity, so it will be used when the user expects it never to fail. Most of the uses in the Bevy examples are in this category! If it does fail, then this is something the user didn't expect and we should inform them. If they do want to skip the system at that point, the error message can guide them to add When. And global error handlers can be used to ensure that anything missed still won't crash the game.
• This is a breaking change, so if we ever want to change it, it's better to change it sooner.
  Most code that uses Bevy hasn't been written yet!

Cons

• A failing version of Single is unnecessary now that we have fallible systems.
  Users can use an ordinary Query parameter and write a simple let value = query.single()? to get erroring behavior, but there is no concise way to get skipping behavior.
• Common cases should be simple.
  The skipping behavior is much more useful than the failing behavior, and will be needed more often. We don't want Bevy to have a reputation for requiring useless boilerplate for common tasks!
• This is too much churn, part 1
  Single is very new, and we should take time to learn how it's being used in practice before making big changes.
• This is too much churn, part 2
  Single was introduced in 15.0 with the skipping behavior, and was changed to panic in 15.1 with no workaround. This was a major pain for users who had been eager to use the skipping behavior. The skipping behavior was just restored in 16.0, and breaking it again in 17.0 would be really irritating for some of our most enthusiastic users.

Solution

Change the behavior of Single and Populated to fail validation instead of skipping systems.

Wrap them in When where needed to fix tests.

[alice-i-cecile]

Unfortunately I think that this is the right call. It's clear, consistent and the error messages will guide users to discover how to do this. If we're loudly failing for resources, we should do the same here.

When<Single<D, F>> isn't that bad to write, and it does make it clearer what's going on.

[alice-i-cecile]

#19490 is important for reviewers to consider when deciding how they feel about this behavior.

[janhohenheim]

In general, not a fan of how we are going back-and-forth on the failure behavior of Single.
In spirit of #14275, I can imagine a future API change where we invert the default again and make all Res, Single, and Populated params skip instead of fail with a new wrapper like Required<Res<&TestComponent>> to explicitly make it panic.
Since 2 maintainers have approved this, I trust this is the right call. But I personally would not merge it because I believe Bevy should generally make panicky APIs opt-in, not opt-out.

[chescock]

In general, not a fan of how we are going back-and-forth on the failure behavior of Single.

Yeah, I agree that the constant breaking changes are bad! I'm optimistic that this will be the end of them, though. The last changes were made late in the release cycle when we wanted to get something working, and there wasn't time for a lot of debate or for big changes like introducing When. Now that we aren't rushed, I'm hopeful that the maintainers will be able to make a more permanent decision.

In spirit of #14275, I can imagine a future API change where we invert the default again and make all Res, Single, and Populated params skip instead of fail with a new wrapper like Required<Res<&TestComponent>> to explicitly make it panic. Since 2 maintainers have approved this, I trust this is the right call. But I personally would not merge it because I believe Bevy should generally make panicky APIs opt-in, not opt-out.

In case it helps, note that parameter validation errors do go through the error handler. If you set a non-panicking DefaultErrorHandler then they'll never crash the application. So I see this behavior as similar to using ? with Result-returning APIs, and not problematic in the way panicky APIs were.

[janhohenheim]

That's a very fair point! I would still want Res and co to not panic out of the box, but knowing that I can just change the error handler and build by own little Required<T> on top to get the semantics I want puts me much more at ease from a user standpoint :)

Note: Since all my dependencies would still expect failing Res to be a "hard error", such a custom error handler would need to make sure that only my own crates' fallible systems are silently skipped. The ErrorContext can be used to deduce crates via system names, which is very brittle, but good enough for my userland code.
It's a bit jank, but it means that I can work around this PR in my own code, so I'm happy.

[Diddykonga]

Hmm, I like When<T> the semantics are nice, but I also can see the opposite with Requires<T> /Needs<T>.
I tend to lean towards the side of skip-by-default.

[janhohenheim]

Per @viridia, Expect<T> would be even neater, as it already fits pre-existing semantics :)

[NthTensor]

We had a good long bikeshed about this last night. This is effectively our current approach (with slightly different keywords):

Semantics	Res	Single
Error	Res	Expect<Single>
Skip	If<Res>	Single

This PR advocates instead for this, for consistency:

Semantics	Res	Single
Error	Res	Single
Skip	If<Res>	If<Single>

But some other people (@janhohenheim especially) want to invert this:

Semantics	Res	Single
Error	Expect<Res>	Expect<Single>
Skip	Res	Single

And other people (@CorvusPrudens) want to have more semantic options:

Semantics	Res	Single
Panic	Expect<Res>	Expect<Single>
Warn	Res	Single
Skip	If<Res>	If<Single>

This last one would require the default error handler to have the power to differentiate error coming from Expect<T> vs Res/Single so that it can choose to warn or panic. This is currently difficult or impossible.

I'd like to point out that both the Panic and Warn branches are both just an Error in system param validation. So this last table is actually a version of the one suggested by this PR.

My proposal would be to commit to the semantics defined by this PR, which error by default, and then as follow up work to improve the information on our error types so that the default error handler can implement more complex behavior.

[NthTensor]

I'd like to respond to @janhohenheim as well.

Bevy should generally make panicky APIs opt-in, not opt-out.

I don't think you should be able to "opt-in" to panicking on the local level; panic behavior should be controlled centrally by the end-user app, not by libraries. And whether panics are opt-in or opt-out on the global level doesn't really matter, because it's easy to change as a blanket policy. What we are talking about here is opt-in/opt-out local error semantics, which is different. Not every error is a panic, and errors still exist no matter what you choose to do with them.

Hot take: I do actually think errors should be opt-out, which is to say errors should be handled explicitly. When errors are opt-in it's way to easy to make mistakes and not realize it, especially as a beginner.

[janhohenheim]

Extremely good assessment!

I think my only remaining issue is that I don’t see a way to say "This Single failure came from a dependency, so we should treat it as an error. This other Single failure came from my own crate, so treat it as a skip. This third Skip also came from my crate, but it’s marked as upholding an invariant, so treat it as an error"
But going ahead with the proposed syntax would enable us to add these semantics at least in theory in future versions, if I understand you correctly. Until then, users wanting the described skip behavior either

• use If everywhere
• set the error handler to blanket skip
• hack the error handler through system names to only skip crate-internally

All in all, that temporary hit in usability until the error handler becomes more flexible could be worth it in the long run.

I think you’ve convinced me that this PR may be a good path forward 🙂

[aleqdev]

Personally while Single version was panicking, i asked ai to write macro for me that looked like this:

use your_crate::silent; // your proc macro crate

#[silent]
fn my_system(
    player: Single<&Player>,
    camera: Single<&Camera>,
    other_things: Query<Entity, ...>
) {
    // ...
}

and it unravels to this:

fn my_system(
    player: Option<Single<&Player>>,
    camera: Option<Single<&Camera>>,
    other_things: Query<Entity, ...>
) {
    let Some(player) = player else { return; };
    let Some(camera) = camera else { return; };

    // ...
}

and macro itself looked something like this:

use proc_macro::TokenStream;
use quote::{quote, format_ident};
use syn::{
    parse_macro_input, FnArg, ItemFn, Pat, PatIdent, PatType, Type, TypePath,
};

#[proc_macro_attribute]
pub fn silent(_attr: TokenStream, item: TokenStream) -> TokenStream {
    // Parse the input function
    let mut input_fn = parse_macro_input!(item as ItemFn);

    // Collect the names of parameters to unwrap, and modify them to Option<Single<T>>
    let mut unwrap_pats = vec![];

    for input in &mut input_fn.sig.inputs {
        if let FnArg::Typed(PatType { pat, ty, .. }) = input {
            // Check if type is `Single<...>`
            if let Type::Path(TypePath { path, .. }) = &**ty {
                let segments = &path.segments;
                if segments.len() == 1 && segments[0].ident == "Single" {
                    // Change type to Option<Single<...>>
                    *ty = syn::parse_quote! { Option<#ty> };

                    // Extract the variable name from the pattern
                    if let Pat::Ident(PatIdent { ident, .. }) = &**pat {
                        unwrap_pats.push(ident.clone());
                    }
                }
            }
        }
    }

    // Insert early return code at the beginning of the function block:
    // let Some(param) = param else { return; };
    let mut unwrap_stmts = vec![];
    for var in unwrap_pats {
        unwrap_stmts.push(quote! {
            let Some(#var) = #var else { return; };
        });
    }

    let orig_block = &input_fn.block;
    input_fn.block = Box::new(syn::parse_quote!({
        #(#unwrap_stmts)*
        #orig_block
    }));

    TokenStream::from(quote! { #input_fn })
}

and that way i could choose what option of system i wanted - panicking or not. I dont think that macro is a good solution to this in particular because it feels more magic than the rest of bevy. maybe instead introduce Single and TrySingle-like types that revert to their respective behaviours when entity is not found so users could choose one they want

[alice-i-cecile]

Cutting from the 0.17 milestone. It's not critical that we make a decision now, and I don't think we should ship a change to make this fail without a corresponding way to downgrade failures from panics in the default error handler.

[NthTensor]

I think that's a fair decision. I will try to prioritize the work for error-levels, I think that will make this decision a lot easier.