Allow users to choose how Commands and EntityCommands should fail

[JaySpruce]

Objective

• Fixes Configurable error handling in commands #2004
• Fixes EntityCommands should have methods to fail gracefully #3845
• Fixes Potential surprising "entity does not exist" panic due to asynchronous command execution #7118
• Fixes Commands should be infallible whenever possible #10166

Solution

Added the following Commands:

• ignore_on_error
• log_on_error (sends messages using info!)
• warn_on_error (sends messages using warn!)
• panic_on_error

and the following EntityCommands:

• ignore_if_missing
• log_if_missing
• warn_if_missing
• panic_if_missing

These change how subsequent commands will respond if errors occur.

Internally:

• Added a failure_handling_mode field to Commands.
• When a fallible Command or an EntityCommand is queued, this field is sent with it.
• Fallible Commands should check for errors themselves.
• When any EntityCommand is applied, it automatically checks if the entity exists.
• If an error occurs, the command matches against failure_handling_mode to determine how to respond.

Showcase

#[derive(Component)]
struct X;

let mut world = World::default();

let mut queue_a = CommandQueue::default();
let mut queue_b = CommandQueue::default();

let mut commands_a = Commands::new(&mut queue_a, &world);
let mut commands_b = Commands::new(&mut queue_b, &world);

let entity = commands_a.spawn_empty().id();

commands_a.entity(entity).despawn();
commands_b.entity(entity).warn_if_missing().insert(X);

queue_a.apply(&mut world);
queue_b.apply(&mut world); // Doesn't panic

Review Notes

The real changes are all in system/commands/mod.rs and world/mod.rs.
All the other files are just returning some commands to their old behavior:

• despawn() -> warn_if_missing().despawn()
• remove() -> ignore_if_missing().remove()

These two are the only commands that both 1) didn't already panic by default and 2) were used internally (according to VS Code's "Find All References" anyway).
They might not all need to be kept non-panicking, I just didn't want to regress anything.

Bikeshedding

• ignore vs silent vs something else?
• failure_handling_mode vs failure_response vs something else? (Used to be failure_mode, but that's wrong)
• info! vs debug!?

Possible Additions / Future PRs

• Deprecate try_ variants of commands
  • I added some comments explaining the change and directing the user to the normal variants, but held off on deprecating since AFAIK that's usually a separate PR.
• Location tracking
  • Just call Location::caller in the user-facing command (if not set to ignore) and pass it in, so the user can see exactly which command caused an error. Didn't seem to hurt performance meaningfully in my testing, but maybe I'm not the best at benchmarking.
• Generalized errors for Commands using Result
  • We can't return a Result from a command to the caller, but we could wrap commands in something that handles any Err returned from the internal function call.

Migration Guide

The following commands that used to fail silently will now panic by default:

• EntityCommands::remove
• EntityCommands::remove_by_id
• EntityCommands::remove_with_requires
• EntityCommands::clear
• EntityCommands::retain
• EntityCommands::observe
• EntityEntryCommands::and_modify

Use ignore_if_missing to achieve the previous behavior:

commands.entity(entity).ignore_if_missing().clear();

The command EntityCommands::despawn used to warn upon failure and will now panic by default.
Use warn_if_missing to achieve the previous behavior:

commands.entity(entity).warn_if_missing().despawn();

try variants of commands are unchanged and will always fail silently; these will be removed in the future.

[JaySpruce]

I don't know how to do Tracy stuff, but I ran the "spawn_commands" benchmark which does this:

for i in 0..entity_count {
    let mut entity = commands.spawn_empty();
    entity
        .insert_if(A, || black_box(i % 2 == 0))
        .insert_if(B, || black_box(i % 3 == 0))
        .insert_if(C, || black_box(i % 4 == 0));

    if black_box(i % 5 == 0) {
        entity.despawn();
    }
}

and got these results:

main

this PR

[JaySpruce]

Maybe controversial, but since invalid EntityCommands just do nothing now, I think it's okay to let Commands::entity return one if the entity doesn't exist. It'll panic if the user wants it to panic, but otherwise it'll just complain or do nothing when they try to use it. Commands::get_entity is still useful if they want to handle it properly.

[JaySpruce]

I benchmarked again with a modified version of spawn_commands that despawns some entities early:

main version

for i in 0..entity_count {
    let mut entity = commands.spawn_empty();
    if black_box(i % 3 == 0) {
        entity.despawn();
    }
    entity
        .try_insert_if(A, || black_box(i % 2 == 0))
        .try_insert_if(B, || black_box(i % 3 == 0))
        .try_insert_if(C, || black_box(i % 4 == 0));
    if black_box(i % 5 == 0) {
        entity.despawn();
    }
}

this PR version

for i in 0..entity_count {
    let mut entity = commands.spawn_empty();
    if black_box(i % 3 == 0) {
        entity.despawn();
    }
    entity
        .X_if_missing()
        .insert_if(A, || black_box(i % 2 == 0))
        .insert_if(B, || black_box(i % 3 == 0))
        .insert_if(C, || black_box(i % 4 == 0));
    if black_box(i % 5 == 0) {
        entity.despawn();
    }
}

and got these results:

ignore_if_missing vs main

bad warn_if_missing vs main

new warn_if_missing vs main

So, ignore_if_missing is essentially identical in performance to try_ functions in main.

warn_if_missing is about 2x slower, but I think that's to be expected. If it's causing a problem, either fix whatever's despawning entities early or set it to ignore. Scratch that, I fixed it. Apparently Strings are no good.

tl;dr: there's maybe a 1% performance impact across the board, if you don't want to call that noise.

Should note that warn! isn't actually printing anything in these benches (I guess benches and tests just don't have the infrastructure for it), so it'll probably be slower in a real program since printing in general is slow. Still good to know that there's nothing else slowing it down

[JaySpruce]

I've got a version working that tracks the calling location of EntityCommands, so that the error messages can be more useful:

It hurts performance of warn_if_missing by another ~2%. Other than that, the biggest downside is that EntityCommands all look like this:

pub fn insert(&mut self, bundle: impl Bundle) -> &mut Self {
    if self.failure_mode().should_respond() {
        self.queue_with_caller(insert(bundle, InsertMode::Replace), Location::caller())
    } else {
        self.queue(insert(bundle, InsertMode::Replace))
    }
}

Could leave it for another PR, or could merge it here if it's wanted

[BenjaminBrienen]

Bikeshedding: I thought a "failure mode" or a "mode of failure" is the way something went wrong. This type/field describes the strategy for handling when something goes wrong.

[JaySpruce]

Hadn't heard of that, but that does seem to be the case. Maybe just failure_handling_mode? Doesn't really need to be short since it's not very public-facing.

Totally open to more bikeshedding, I'm not committed to any of the names here

[BenjaminBrienen]

I love it!