Expect<&T> for "safe" queries

[Zeenobit]

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

Since I've started using Bevy, a problem I consistently run into is that systems which iterate over a query can silently "skip" over entities which unintentionally, or erroneously do not match the query. This often leads to subtle bugs that are hard to debug.

For example, consider the system below:

#[derive(Bundle)]
struct FooBundle {
    a: A,
    b: B,
}

void foo(query: Query<(&A, &B)>) {
    for (a, b) in &query {
        ...
    }
}

In such a scenario, this system expects all entities with A to also have a B, which is kinda enforced by using an FooBundle. However, if A or B is removed for whatever reason, this system would now skip over the invalid entities; when in reality I would like a panic or an error to catch the invalid entity as early as possible.

One workaround I've found to solve this issue is to use Option<T>:

void foo(query: Query<(&A, Option<&B>)>) {
    for (a, b) in &query {
        let b = b.unwrap(); // <-- Panic if 'A' exists without 'B'!
        ...
    }
}

Another way to achieve the same result is to split the query:

void foo(query: Query<(Entity, &A)>, b_query: Query<&B>) {
    for (entity, a) in &query {
        let b = b_query.get(entity).unwrap(); // <-- Panic if 'A' exists without 'B'!
        ...
    }
}

What solution would you like?

I would like a more ergonomic implementation of the workaround I've mentioned above.
I imagine this could take the form of Expect<&T>, such that:

void foo(query: Query<(&A, Expect<&B>)>) {
    for (a, b) in &query { // <-- Panic if 'A' exists without 'B'!
        ...
    }
}

Internally, this could behave like an Option<&T> query, but instead of its output being an Option<&T>, it would be &T, and it would panic if internally it is None.

What alternative(s) have you considered?

Workaround described above.
If there are already ways to achieve what I want, I'm unaware of them!

Additional context

I think we could further extend this terminology to make query get more ergonomic as well.

Instead of:

let b = query.get(entity).unwrap();

We could have:

let b = query.expect(entity); // Shorthand for "get(entity).unwrap()"

I would also be open to implementing this with a PR; but I wanted to raise the idea to community before I start implementing.

[alice-i-cecile]

Hmm, I'm not totally sold.

Since I've started using Bevy, a problem I consistently run into is that systems which iterate over a query can silently "skip" over entities which unintentionally, or erroneously do not match the query

This hasn't been my experience. Increasingly, on more complex Bevy projects I've found that I've been programming really defensively, and eliminating panics wherever possible, typically logging a warning or error.

To detect surprising archetype issues, I'll write my own simple logging systems that will be feature flagged, which go and check and log for these sorts of problems.

Given that this is just sugar for a controversial pattern, I think it's likely a better fit for a 3rd party crate (but do let us know if you need any upstream changes to support it).

[Zeenobit]

Sure, I could try to implement this as a crate.

I'd also be open to Expect being an error instead. Maybe renamed to Require as to not be confused with expect from std.

Edit: Granted, then the issue would be "what to return?" If it's an error, it'd impose a Default restriction on T...and would have to insert the component. 🤔

[Adamkob12]

Just wanted to say that even if this doesn't end up being implemented, you can do it yourself on your side (in your game):

1. Define an Expect<T> struct
2. Implement WorldQuery for Expect<&T> & Expect<&mut T> -- should be almost identical to Bevy's internal Option<&T> WorldQuery implementation (but you change WorldQuery::Item to the unwrapped item and change the methods a bit to unwrap) so just copy it.
3. Implement QueryData for Expect<&T> & Expect<&mut T> (trivial)
4. You're all done!

[alice-i-cecile]

You may actually be able to get away with a WorldQuery derive even :)

[Zeenobit]

Minimal implementation of this:
https://gist.github.com/Zeenobit/6faad301b75754e26aa3a25475e8db55 (Based on 0.12)

In my opinion, the feature is too small to warrant a crate. While I understand the advantage of defensive programming, I also don't really see the harm in providing this as a tool to be used as needed. Users would have to opt into the feature consciously.

We could also turn this from a panic into an error without having to worry about constructing/inserting a missing component.
I think we could just log an error here and instead return false:

    fn matches_component_set(
        _state: &T::State,
        set_contains_id: &impl Fn(ComponentId) -> bool,
    ) -> bool {
        if set_contains_id(state) {
            return true;
        }
        error!(...); // <--- Graceful error
        false
    }

[Adamkob12]

You may actually be able to get away with a WorldQuery derive even :)

TIL it was deprecated or am I missing something?

I guess it's QueryData now right?

[alice-i-cecile]

Yep, QueryData is the derive you'll need.

I also don't really see the harm in providing this as a tool to be used as needed

The cost isn't in users using it: it's about the cost to learn and explore Bevy, and the ongoing maintenance costs. As an ECS SME, I'm opposed: I think this can and should be done externally.

[bushrat011899]

Perhaps the better option would be to include this as a part of a broader Query extension crate, such as bevy_query_ext?

[DasLixou]

Related to #1481 ?