Remove lifetime from QueryEntityError (#18157)

# Objective

- Allow `Query` methods such as `Query::get` to have their error
short-circuited using `?` in systems using Bevy's `Error` type

## Solution

- Removed `UnsafeWorldCell<'w>` from `QueryEntityError` and instead
store `ArchetypeId` (the information the error formatter was extracting
anyway).
- Replaced trait implementations with derives now that the type is
plain-old-data.

## Testing

- CI

---

## Migration Guide

- `QueryEntityError::QueryDoesNotMatch.1` is of type `ArchetypeId`
instead of `UnsafeWorldCell`. It is up to the caller to obtain an
`UnsafeWorldCell` now.
- `QueryEntityError` no longer has a lifetime parameter, remove it from
type signatures where required.

## Notes

This was discussed on Discord and accepted by Cart as the desirable path
forward in [this
message](https://discord.com/channels/691052431525675048/749335865876021248/1346611950527713310).
Scroll up from this point for context.

---------

Co-authored-by: SpecificProtagonist <30270112+SpecificProtagonist@users.noreply.github.com>

crates/bevy_ecs/src/query/error.rs

@@ -1,18 +1,18 @@
 use thiserror::Error;
 
 use crate::{
+    archetype::ArchetypeId,
     entity::{Entity, EntityDoesNotExistError},
-    world::unsafe_world_cell::UnsafeWorldCell,
 };
 
 /// An error that occurs when retrieving a specific [`Entity`]'s query result from [`Query`](crate::system::Query) or [`QueryState`](crate::query::QueryState).
 // TODO: return the type_name as part of this error
-#[derive(Clone, Copy)]
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
-pub enum QueryEntityError<'w> {
+pub enum QueryEntityError {
     /// The given [`Entity`]'s components do not match the query.
     ///
     /// Either it does not have a requested component, or it has a component which the query filters out.
-    QueryDoesNotMatch(Entity, UnsafeWorldCell<'w>),
+    QueryDoesNotMatch(Entity, ArchetypeId),
     /// The given [`Entity`] does not exist.
     EntityDoesNotExist(EntityDoesNotExistError),
     /// The [`Entity`] was requested mutably more than once.
@@ -21,23 +21,19 @@ pub enum QueryEntityError<'w> {
     AliasedMutability(Entity),
 }
 
-impl<'w> From<EntityDoesNotExistError> for QueryEntityError<'w> {
+impl From<EntityDoesNotExistError> for QueryEntityError {
     fn from(error: EntityDoesNotExistError) -> Self {
         QueryEntityError::EntityDoesNotExist(error)
     }
 }
 
-impl<'w> core::error::Error for QueryEntityError<'w> {}
+impl core::error::Error for QueryEntityError {}
 
-impl<'w> core::fmt::Display for QueryEntityError<'w> {
+impl core::fmt::Display for QueryEntityError {
     fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
         match *self {
-            Self::QueryDoesNotMatch(entity, world) => {
+            Self::QueryDoesNotMatch(entity, _) => {
-                write!(
+                write!(f, "The query does not match entity {entity}")
-                    f,
-                    "The query does not match entity {entity}, which has components "
-                )?;
-                format_archetype(f, world, entity)
             }
             Self::EntityDoesNotExist(error) => {
                 write!(f, "{error}")
@@ -52,57 +48,6 @@ impl<'w> core::fmt::Display for QueryEntityError<'w> {
     }
 }
 
-impl<'w> core::fmt::Debug for QueryEntityError<'w> {
-    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
-        match *self {
-            Self::QueryDoesNotMatch(entity, world) => {
-                write!(f, "QueryDoesNotMatch({entity} with components ")?;
-                format_archetype(f, world, entity)?;
-                write!(f, ")")
-            }
-            Self::EntityDoesNotExist(error) => {
-                write!(f, "EntityDoesNotExist({error})")
-            }
-            Self::AliasedMutability(entity) => write!(f, "AliasedMutability({entity})"),
-        }
-    }
-}
-
-fn format_archetype(
-    f: &mut core::fmt::Formatter<'_>,
-    world: UnsafeWorldCell<'_>,
-    entity: Entity,
-) -> core::fmt::Result {
-    // We know entity is still alive
-    let entity = world
-        .get_entity(entity)
-        .expect("entity does not belong to world");
-    for (i, component_id) in entity.archetype().components().enumerate() {
-        if i > 0 {
-            write!(f, ", ")?;
-        }
-        let name = world
-            .components()
-            .get_name(component_id)
-            .expect("entity does not belong to world");
-        write!(f, "{}", disqualified::ShortName(name))?;
-    }
-    Ok(())
-}
-
-impl<'w> PartialEq for QueryEntityError<'w> {
-    fn eq(&self, other: &Self) -> bool {
-        match (self, other) {
-            (Self::QueryDoesNotMatch(e1, _), Self::QueryDoesNotMatch(e2, _)) if e1 == e2 => true,
-            (Self::EntityDoesNotExist(e1), Self::EntityDoesNotExist(e2)) if e1 == e2 => true,
-            (Self::AliasedMutability(e1), Self::AliasedMutability(e2)) if e1 == e2 => true,
-            _ => false,
-        }
-    }
-}
-
-impl<'w> Eq for QueryEntityError<'w> {}
-
 /// An error that occurs when evaluating a [`Query`](crate::system::Query) or [`QueryState`](crate::query::QueryState) as a single expected result via
 /// [`single`](crate::system::Query::single) or [`single_mut`](crate::system::Query::single_mut).
 #[derive(Debug, Error)]
@@ -117,8 +62,7 @@ pub enum QuerySingleError {
 
 #[cfg(test)]
 mod test {
-    use crate::prelude::World;
+    use crate::{prelude::World, query::QueryEntityError};
-    use alloc::format;
     use bevy_ecs_macros::Component;
 
     #[test]
@@ -129,19 +73,18 @@ mod test {
         struct Present1;
         #[derive(Component)]
         struct Present2;
-        #[derive(Component, Debug)]
+        #[derive(Component, Debug, PartialEq)]
         struct NotPresent;
 
-        let entity = world.spawn((Present1, Present2)).id();
+        let entity = world.spawn((Present1, Present2));
 
-        let err = world
+        let (entity, archetype_id) = (entity.id(), entity.archetype().id());
-            .query::<&NotPresent>()
+
-            .get(&world, entity)
+        let result = world.query::<&NotPresent>().get(&world, entity);
-            .unwrap_err();
 
         assert_eq!(
-            format!("{err:?}"),
+            result,
-            "QueryDoesNotMatch(0v1 with components Present1, Present2)"
+            Err(QueryEntityError::QueryDoesNotMatch(entity, archetype_id))
         );
     }
 }

crates/bevy_ecs/src/query/state.rs

@@ -952,7 +952,7 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
         &mut self,
         world: &'w World,
         entity: Entity,
-    ) -> Result<ROQueryItem<'w, D>, QueryEntityError<'w>> {
+    ) -> Result<ROQueryItem<'w, D>, QueryEntityError> {
         self.query(world).get_inner(entity)
     }
 
@@ -993,7 +993,7 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
         &mut self,
         world: &'w World,
         entities: [Entity; N],
-    ) -> Result<[ROQueryItem<'w, D>; N], QueryEntityError<'w>> {
+    ) -> Result<[ROQueryItem<'w, D>; N], QueryEntityError> {
         self.query(world).get_many_inner(entities)
     }
 
@@ -1005,7 +1005,7 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
         &mut self,
         world: &'w mut World,
         entity: Entity,
-    ) -> Result<D::Item<'w>, QueryEntityError<'w>> {
+    ) -> Result<D::Item<'w>, QueryEntityError> {
         self.query_mut(world).get_inner(entity)
     }
 
@@ -1052,7 +1052,7 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
         &mut self,
         world: &'w mut World,
         entities: [Entity; N],
-    ) -> Result<[D::Item<'w>; N], QueryEntityError<'w>> {
+    ) -> Result<[D::Item<'w>; N], QueryEntityError> {
         self.query_mut(world).get_many_inner(entities)
     }
 
@@ -1074,7 +1074,7 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
         &self,
         world: &'w World,
         entity: Entity,
-    ) -> Result<ROQueryItem<'w, D>, QueryEntityError<'w>> {
+    ) -> Result<ROQueryItem<'w, D>, QueryEntityError> {
         self.query_manual(world).get_inner(entity)
     }
 
@@ -1091,7 +1091,7 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
         &mut self,
         world: UnsafeWorldCell<'w>,
         entity: Entity,
-    ) -> Result<D::Item<'w>, QueryEntityError<'w>> {
+    ) -> Result<D::Item<'w>, QueryEntityError> {
         self.query_unchecked(world).get_inner(entity)
     }
 

crates/bevy_ecs/src/system/query.rs

@@ -1430,7 +1430,7 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> {
     ///
     /// - [`get_mut`](Self::get_mut) to get the item using a mutable borrow of the [`Query`].
     #[inline]
-    pub fn get_inner(self, entity: Entity) -> Result<D::Item<'w>, QueryEntityError<'w>> {
+    pub fn get_inner(self, entity: Entity) -> Result<D::Item<'w>, QueryEntityError> {
         // SAFETY: system runs without conflicts with other systems.
         // same-system queries have runtime borrow checks when they conflict
         unsafe {
@@ -1444,7 +1444,10 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> {
                 .matched_archetypes
                 .contains(location.archetype_id.index())
             {
-                return Err(QueryEntityError::QueryDoesNotMatch(entity, self.world));
+                return Err(QueryEntityError::QueryDoesNotMatch(
+                    entity,
+                    location.archetype_id,
+                ));
             }
             let archetype = self
                 .world
@@ -1476,7 +1479,10 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> {
             if F::filter_fetch(&mut filter, entity, location.table_row) {
                 Ok(D::fetch(&mut fetch, entity, location.table_row))
             } else {
-                Err(QueryEntityError::QueryDoesNotMatch(entity, self.world))
+                Err(QueryEntityError::QueryDoesNotMatch(
+                    entity,
+                    location.archetype_id,
+                ))
             }
         }
     }
@@ -1573,7 +1579,7 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> {
     pub fn get_many_inner<const N: usize>(
         self,
         entities: [Entity; N],
-    ) -> Result<[D::Item<'w>; N], QueryEntityError<'w>> {
+    ) -> Result<[D::Item<'w>; N], QueryEntityError> {
         // Verify that all entities are unique
         for i in 0..N {
             for j in 0..i {
@@ -1602,7 +1608,7 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> {
     pub fn get_many_readonly<const N: usize>(
         self,
         entities: [Entity; N],
-    ) -> Result<[D::Item<'w>; N], QueryEntityError<'w>>
+    ) -> Result<[D::Item<'w>; N], QueryEntityError>
     where
         D: ReadOnlyQueryData,
     {
@@ -1620,7 +1626,7 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> {
     unsafe fn get_many_impl<const N: usize>(
         self,
         entities: [Entity; N],
-    ) -> Result<[D::Item<'w>; N], QueryEntityError<'w>> {
+    ) -> Result<[D::Item<'w>; N], QueryEntityError> {
         let mut values = [(); N].map(|_| MaybeUninit::uninit());
 
         for (value, entity) in core::iter::zip(&mut values, entities) {