More informations for query errors

[Selene-Amanita]

Objective

1. Fix Return missing component types in QueryDoesNotMatch errors #8917 : add information about query/entity mismatch for get/get_mut, ...
2. Fix Return missing component types in QueryDoesNotMatch errors #8917 (comment) : add information about query errors in general.
3. Fix Query::get_component should mention that the compenent must be part of the query #9011 : fix documentation for get_component/get_component_mut
4. Try to make the documentation for queries better in general.

Solution

1. Query Entity Component Mismatch:

• Add QueryEntityMismatchDetail and QueryEntityComponentMismatch.
• Add get_mismatches_detail to get the QueryEntityComponentMismatch array.
• I didn't handle the logic to get more information in case of a ChangeDetectionMismatch, because I have no idea how to get this information.

2. More information in general

• Add QueryComponentErrorDetail and QueryEntityErrorDetail, I think it's better to put a struct instead of a tuple to know what is what, but I left a singleton tuple in QuerySingleError because there's only one information to get here.
• Use std::any to fill the type representations of the query/component.

For 3 and 4, just update the documentation.

Summary of the new structures

(I didn't put all the comments/derive, and didn't put QuerySingleError since it didn't change)

For QueryComponentError:

pub enum QueryComponentError {
    /// The [`Query`] does not have read access to the requested component.
    MissingReadAccess(QueryComponentErrorDetail),
    /// The [`Query`] does not have write access to the requested component.
    MissingWriteAccess(QueryComponentErrorDetail),
    /// The given [`Entity`] does not have the requested component.
    MissingComponent(QueryComponentErrorDetail),
    /// The requested [`Entity`] does not exist.
    NoSuchEntity(QueryComponentErrorDetail),
}

/// Additional information in the context of a [`QueryComponentError`].
pub struct QueryComponentErrorDetail {
    /// Specific entity which was requested when encountering the error.
    pub requested_entity: Entity,
    /// Component which was requested when encountering the error.
    pub requested_component: &'static str,
    /// Representation of the query's type.
    pub query_type: &'static str,
}

For QueryEntityError:

pub enum QueryEntityError {
    /// The given [`Entity`]'s components do not match the query, see [`QueryEntityMismatchDetail`].
    QueryDoesNotMatch(QueryEntityMismatchDetail, QueryEntityErrorDetail),
    /// The given [`Entity`] does not exist.
    NoSuchEntity(QueryEntityErrorDetail),
    /// The [`Entity`] was requested mutably more than once.
    AliasedMutability(QueryEntityErrorDetail),
}

/// Additional information in the context of a [`QueryEntityError`].
pub struct QueryEntityErrorDetail {
    /// Specific entity which was requested when encountering the error.
    pub requested_entity: Entity,
    /// Representation of the query's type.
    pub query_type: &'static str,
}

/// Diagnosis about why an [`Entity`] and a [`Query`](crate::system::Query) or [`QueryState`] don't match.
pub enum QueryEntityMismatchDetail {
    /// The entity is missing components required by the query,
    /// or it has components that are filtered out by the query.
    ComponentMismatch(Vec<QueryEntityComponentMismatch>),
    /// The entity has the correct components, but some of them are requested
    /// with change detection ([`Added`](crate::query::Added) or [`Changed`](crate::query::Changed))
    /// and the component for that entity is not in that change state.
    ChangeDetectionMismatch,
}

/// Represents which [`Entity`]'s component do not match a [`Query`](crate::system::Query) or [`QueryState`].
///
/// This is usually used in an array because of a query like `QueryState<A, Or<(With<B>, With<C)>>`
/// is separated into two queries `QueryState<A, With<B>>` and `QueryState<A, With<B>>`,
/// in that case a single `QueryEntityMismatchDetail` represents the reasons the entity
/// doesn't match one of those "sub-queries".
pub struct QueryEntityComponentMismatch {
    /// Components that the query requires but the entity doesn't have,
    /// with a representation of their name.
    pub query_with_components_not_in_entity: Vec<(String, ComponentId)>,
    /// Components that the query filters out but the entity have,
    /// with a representation of their name.
    pub query_without_components_in_entity: Vec<(String, ComponentId)>,
    /// [`Archetype`] of the entity, used to retrieve its components.
    pub entity_archetype: ArchetypeId,
    /// Components that the query requires.
    pub query_with_components: FixedBitSet,
    /// Components that the query filters out.
    pub query_without_components: FixedBitSet,
}

Code to test quickly

If you think of other scenarios to test please tell me or test yourself.

use bevy::prelude::*;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, setup)
        .add_systems(Update, debug)
        .run();
}

#[derive(Resource)]
struct TestResource {
    entity: Entity
}

fn setup(
    mut commands: Commands,
) {
    let entity = commands.spawn((
        Transform::default(),
        Visibility::default(),
    )).id();
    commands.insert_resource(TestResource{entity});
}

fn debug(
    test_res: Res<TestResource>,
    test_q1: Query<&Transform, Without<Visibility>>,
    test_q2: Query<&Transform, With<GlobalTransform>>,
    test_q3: Query<&Transform, &GlobalTransform>,
    test_q4: Query<&Transform, Or<(Without<Visibility>, With<GlobalTransform>)>>,
    test_q5: Query<&Transform, Added<Visibility>>,
    test_q6: Query<&Transform, Changed<Visibility>>,
) {
    let e = test_res.entity;
    dbg!("_______________________1");
    dbg!(test_q1.get(e));
    dbg!("_______________________2");
    dbg!(test_q2.get(e));
    dbg!("_______________________3");
    dbg!(test_q3.get(e));
    dbg!("_______________________4");
    dbg!(test_q4.get(e));
    dbg!("_______________________5");
    dbg!(test_q5.get(e));
    dbg!("_______________________6");
    dbg!(test_q6.get(e));
}

Drawbacks

This gives more information about why an error happened, and can allow easier generic logic to handle them, but at three costs:

• the Result can take more memory if the components requested take less space than the error struct
• the error struct itself takes more memory when an error occurs
• when an error occurs, more computation is needed
  All of this even if the end user doesn't really need that information.

I don't think it's a really big deal since the error struct is basically pointers to Vec and String, errors are not supposed to happen very often, and functions like contains can be used to just check if an entity is fetched by a query.

Maybe the error could be computed in debug mode only or be behind a feature or something?

Notes

• It's the first time I really dive into the ECS code, some stuff aren't clear for me so I may have made mistakes.
• QueryEntityMismatchDetail is just an enum, that could be merged with QueryEntityError (in QueryEntityError, have a variant QueryDoesNotMatchComponents and QueryDoesNotMatchChangeDetection for example), but I decided against it to avoid a too big breaking change, and to allow for potential additional "mismatch" causes in the future.
• I left get_mismatches_detail public but it maybe doesn't need to be, also to not make it unsafe I put specialized arguments instead of taking an UnsafeWorldCell. Maybe a more user-friendly get_mismatches_detail can be created alongside this one.
• My logic in get_mismatches_detail isn't very "rusty" (for loops inside of iterators), you can make it better if you have suggestions to change it.
• As stated above, I didn't handle the logic with change detection, if you have an idea on how to do that, it would be appreciated, I'm not sure where that information is in the QueryState (filter_state?) and how to retrieve it.

---

Changelog

• Changed QueryEntityError's variants to add more information on the error.
• Changed QueryComponentError's variants to add more information on the error.

Migration Guide

• QueryEntityError's variants now have a QueryEntityErrorDetail singleton instead of Entity, and QueryDoesNotMatch also have QueryEntityMismatchDetail, you can ignore them in your pattern matching, or get the Entity from QueryEntityErrorDetail: QueryEntityError::QueryDoesNotMatch(entity) => entity becomes QueryEntityError::QueryDoesNotMatch(_, error_details) => error_details.requested_entity.
• QueryComponentError's variants now have a QueryComponentErrorDetail singleton, you can just ignore it in your pattern matching: QueryComponentError::MissingReadAccess => "missing read access" becomes
  QueryComponentError::MissingReadAccess(_) => "missing read access"

[nicopap]

(comment applies to the whole QueryComponentError rather than this specific line.

Avoid variants all holding the same data

I see that all variants now are FooBar(QueryComponentErrorDetail). Wouldn't it make more sense to convert QueryComponentError into a struct?

This would remove a lot of the boilerplate.

pub enum QueryComponentErrorKind {
    MissingReadAccess,
    MissingWriteAccess,
    // ...
}
pub struct QueryComponentError {
    pub kind: QueryComponentErrorKind,
    pub entity: Entity,
    pub component: &'static str,
    pub query: &'static str,
}

Also QueryComponentErrorKind is a fairly long name, it starts to trigger my java-factory detector. After a certain identifier length, it becomes difficult to read code. But I think it's fine as is right now.

Avoid the &'static str

Instead of holding the str for the component name and query name, it's possible to make QueryComponentError generic over Q and C. It would look like this:

pub struct QueryComponentError<Q, C> {
    pub kind: QueryComponentErrorKind,
    pub entity: Entity,
    pub _component: PhantomData<fn(C)>,
    pub _query: PhantomData<fn(Q)>,
   // alternatively: pub _types: PhantomData<fn(C, Q)>,
}

Then, you'd use the type_name function in the display impl. This way, it avoids bloating the error type (and therefore the result type) with 32 additional bytes, it's basically free until you display the error.

[Selene-Amanita]

First of all thank you for the review!

I want to say that most of my choice were led by "not changing the existing structure too much" : all the "QuerySomethingError" were enums and two of them were already holding the same kind of data in all variants. QuerySingleError in particular was already holding a &'static str. I don't have a strong opinion on them and I did think about the stuff you proposed, but didn't note them all down in the PR, thank you for opening the conversation about them.

About changing enum holding struct to struct with a Kind enum field:

• It makes sense, but I'm a bit afraid the kind of error would be "buried" in the doc a bit
• Should I do the same to QuerySingleError? (probably not with the PhantomData, see bellow)

About using PhantomData instead of &'static str:

• This does make runtime a tiny bit faster if you don't use this data I think, and take less memory I guess, but it would probably make compile time a bit longer and binary size a bit heavier because of monomorphization,
• Why the fn in the PhantomData? Is it to avoid explicit lifetime?
• Should I restrict Q and C with a trait constraint? What should I use for Q?
• Should I add helper functions to this type for end user to be able to get the string representation of query and component without having to know about std::any nor use the Display trait?
• Should I do the same for QuerySingleError?

About get_mismatches_detail, reply here: #9041 (comment)

Also since @james7132 self-requested a review, can I have your opinion on all of this, I would appreciate a third opinion before making the changes (I'll have to change all the tests and docs and stuff).

[nicopap]

Same remark as for QueryComponentError. This type should be converted to a struct, instead of having every variants hold a QueryEntityErrorDetail.

[nicopap]

So the query_type can be converted into a generic parameter here as well.

[nicopap]

I think QueryEntityMismatchDetail is too heavy, both to compute and memory-wise to add to the return result of Query::get. Since Query::get_mismatches_detail exists, I think there is no need to add this to QueryEntityError, users can always call get_mismatches_detail if they have an Err and need to know the exact problems encountered.

It's very common for bevy code to do

if let Ok(foo) = query.get(entity) {
    // ...
}

If the Err is a common case, and is always skipped here, the user is going to create and discard a QueryEntityMismatchDetail multiple times per frame, while they absolutely do not care about it.

[Selene-Amanita]

I kinda hesitated a bit about this yeah.

About removing the QueryEntityMismatchDetail in the QueryDoesNotMatch variant (or the struct holding it) and letting people use get_mismatches_detail:

• It would be a tiny bit more annoying to use (you have to match the variant to check that it's a QueryDoesNotMatch before calling the function, but still need the query itself to call the function so you can't just pass the error to a helper function handle it you need to pass the query too), but I agree that it's still better than what we have now (which is no information at all about the mismatch) and that it's a usecase too specific to calculate it and store it every time.
• I would like to split the variant QueryDoesNotMatch into QueryComponentsMismatch and QueryChangeDetectionMismatch at least, otherwise in the case of a change detection mismatch, people might use get_mismatches_detail and not understand why it returns empty arrays (or None, see bellow).
• I need to change get_mismatches_detail as it's not super user friendly right now, should I make it so it returns an Option so it returns None if there's no mismatch? This would save computing time when it's the case and make it clearer that the entity does in fact match the query (at least components-wise), but maybe people would still want information about "subqueries" in the case of an Or?

[nicopap]

I think the Detail here only contributes to make the type harder to read. I'd rather see a shorter name.

Suggested change

	pub enum QueryEntityMismatchDetail {
	pub enum QueryEntityMismatch {

[nicopap]

So I see two ways someone becomes familiar with this error API.

1. They explore the documentation for the return type of Query::get, they are inquisitive about what failures are possible (I personally never do this)
2. They see the unwrap panic error message for the error.

In case (1), the user is already trying to find what can go wrong. If they don't get their answer initially from reading the QueryComponentError docs, they'll read the QueryComponentErrorKind doc, since they want to know something they yet have to learn.

In case (2), the error message will contain the details as to why a panic occured.

It would also make the match in the Display::fmt impl so much cleaner.

Regarding generic parameters

Fairly certain there is no compiler overhead compared to the current PR. This is basically just delaying the instantiation of the type name to when it's meaningfully used, rather than before.

I wrapped the types in fn() because this way, they don't need to be Send and Sync for QueryComponentError to be Send and Sync.

Q and C should be the query and component used in the get_component respectively.

Whenever you need to instantiate a QueryComponentError, you'll have the type
parameters in scope, you should use them then.

Regarding whether we should go forward with this.

I also am worried about exposing such an intricate error API. It's something that we then have to maintain, and break user code when bevy internals change.

Maybe most fields should stay private?

See how I decided to make a part of an error private, as to avoid having to deal with user-facing API: #8887 (comment)

Why are we exposing this? If it is to provide bevy users with a detailed message about the query error. We should rely on the Error and fmt::Display implementations, not the entirety of the error type.

If it's about giving the tools to dynamically react to missing components on entities… Well, can you give an example of what? dynamic-style queries always require exclusive world access, and we are incuring a cost in situations where this is impossible.

Regarding get_mismatches

There is just too much information in QueryEntityComponentMismatch. And it requires cloning the FixedBitSet of the with/without, potentially several times, each time Query::get returns Err. It also requires creating a Vec and String's for each component. It's standard to use Query::get inside a loop inside a system, And to skip mismatched entities (completely ignore the error).

It's not at all "a tinny bit of performance downgrade" we go from a basically 0 cost operation to something like O(n * m) allocations (n the number of components, m the number of Ors).

Here is a suggestion:

Split the error in two

Consider this. Instead of returning Vec<QueryEntityComponentMismatch>, get could return a very simple unit value (or a minimal error with the "free" to compute error data), which fmt::Display says "use .get_mismatches method for more precise error informations."

Also of note, I much prefer the suggested name in #8917. get_missing_components is far more self-explanatory as to why you'd call it, and what it returns. get_mismatches could be "what mismatch?" Maybe get_mismatch_reason if you are not comfortable with "component".

[Selene-Amanita]

They explore the documentation for the return type of Query::get, they are inquisitive about what failures are possible (I personally never do this)

I do (1) a lot ^^'
I agree it's not a really big deal if people like me have to go to the Kind doc to have more info, but I still find it a tiny bit less clear.

Q and C should be the query and component used in the get_component respectively.

I get that, my question was: should I enforce that by having trait constraints, and if so which one should I use for Q?

Maybe most fields should stay private?
Why are we exposing this? If it is to provide bevy users with a detailed message about the query error. We should rely on the Error and fmt::Display implementations, not the entirety of the error type.

So people, particularly people making libraries, can make their own error messages. I know I would use it in bevy_basic_portals to tell the end user what components they are missing to make their portal work for example (which should never happen if they use the bundle, but yeah).

Consider this. Instead of returning Vec, get could return a very simple unit value (or a minimal error with the "free" to compute error data), which fmt::Display says "use .get_mismatches method for more precise error informations."

I understood that's what you wanted in your previous message and I agree with that. For me it wasn't that big of a deal not because I though the computation was trivial (yes it's just a few components but potentially inside loops etc...) but because I thought errors aren't supposed to happen that often if everything goes well, but if you're telling there are a lot of cases where you might want to call get when you're not sure if the entity would be matched by the query then okay ^^'

get_missing_components is far more self-explanatory as to why you'd call it

I disagree with this, since the "mismatch" can also happen when there is a Without in the Query and the entity has this component, in this case the component isn't missing.

get_mismatches could be "what mismatch?" Maybe get_mismatch_reason if you are not comfortable with "component".

Both of those are not good either since a mismatch can happen also with change detection, and my function returns why there is a component mismatch only. This is also why I wanted to separate the variants.

[Selene-Amanita]

@nicopap I tried implementing your requested changes but I encountered problems for the "getting rid of &'static str" part.

I first tried to have for example QuerySingleError<Q> and use it like Result<Q::Item<'w>, QueryEntityError<Self>> (Self being QueryState/Query), but I had two problems:

• Q needs to implement Debug for implementing Error on QuerySingleError (kinda weird that it doesn't tell me that's necessary for deriving Debug on QuerySingleError<Q>, but whatever I can just have a trait bound on Q)
• the methods on Query use the methods on QueryState, but the Q would be a different type then, so I can just use the error and pass it above. For example get_single on Query_State would return a Err<QuerySingleError<QueryState<_,_>>> but the one on Query returns a Err<QuerySingleError<Query<_,_>>> instead.

I then thought "well no big deal, instead I will define it as pub struct QuerySingleError<Q: WorldQuery, F:ReadOnlyWorldQuery>", but:

• I still have problems with deriving Debug/implementing Error, because WorldQuery and ReadOnlyWorldQuery don't have a trait bound on Debug, I can get around that by implementing Debug manually on QuerySingleError like Query and QueryState do
• I have a new problem (I probably had it before just didn't notice): methods that access data immutably like get_single actually "change" the type of Q and F, so I can't pass the error above either.

I'm not sure how to proceed, I think I can take my first approach again (QuerySingleError<Q>) but just copy the error to make it match the type of the function calling (by implementing From<QuerySingleError<P>> on QuerySingleError<Q>)? The thing is I'm not sure the compiler optimizes something like that and it would create overhead for nothing, on top of longer compile time and bigger binary (a monomorphized QuerySingleError for every Query, QueryState and their read-only variant, same for QueryComponentError and QueryEntityError). What do you think?

[nicopap]

I'm going to submit a PR to your branch to help you out.

[nicopap]

The PR is ready and available at Selene-Amanita#1