Ensure consistency between Un/Typed AssetId and Handle (bevyengin…

…e#10628)

# Objective

- Fixes bevyengine#10629
- `UntypedAssetId` and `AssetId` (along with `UntypedHandle` and
`Handle`) do not hash to the same values when pointing to the same
`Asset`. Additionally, comparison and conversion between these types
does not follow idiomatic Rust standards.

## Solution

- Added new unit tests to validate/document expected behaviour
- Added trait implementations to make working with Un/Typed values more
ergonomic
- Ensured hashing and comparison between Un/Typed values is consistent
- Removed `From` trait implementations that panic, and replaced them
with `TryFrom`

---

## Changelog

- Ensured `Handle::<A>::hash` and `UntypedHandle::hash` will produce the
same value for the same `Asset`
- Added non-panicing `Handle::<A>::try_typed`
- Added `PartialOrd` to `UntypedHandle` to match `Handle<A>` (this will
return `None` for `UntypedHandles` for differing `Asset` types)
- Added `TryFrom<UntypedHandle>` for `Handle<A>`
- Added `From<Handle<A>>` for `UntypedHandle`
- Removed panicing `From<Untyped...>` implementations. These are
currently unused within the Bevy codebase, and shouldn't be used
externally, hence removal.
- Added cross-implementations of `PartialEq` and `PartialOrd` for
`UntypedHandle` and `Handle<A>` allowing direct comparison when
`TypeId`'s match.
- Near-identical changes to `AssetId` and `UntypedAssetId`

## Migration Guide

If you relied on any of the panicing `From<Untyped...>` implementations,
simply call the existing `typed` methods instead. Alternatively, use the
new `TryFrom` implementation instead to directly expose possible
mistakes.

## Notes

I've made these changes since `Handle` is such a fundamental type to the
entire `Asset` ecosystem within Bevy, and yet it had pretty unclear
behaviour with no direct testing. The fact that hashing untyped vs typed
versions of the same handle would produce different values is something
I expect to cause a very subtle bug for someone else one day.

I haven't included it in this PR to avoid any controversy, but I also
believe the `typed_unchecked` methods should be removed from these
types, or marked as `unsafe`. The `texture_atlas` example currently uses
it, and I believe it is a bad choice. The performance gained by not
type-checking before conversion would be entirely dwarfed by the act of
actually loading an asset and creating its handle anyway. If an end user
is in a tight loop repeatedly calling `typed_unchecked` on an
`UntypedHandle` for the entire runtime of their application, I think the
small performance drop caused by that safety check is ~~a form of cosmic
justice~~ reasonable.

crates/bevy_asset/src/handle.rs

@@ -11,6 +11,7 @@ use std::{
     hash::{Hash, Hasher},
     sync::Arc,
 };
+use thiserror::Error;
 
 /// Provides [`Handle`] and [`UntypedHandle`] _for a specific asset type_.
 /// This should _only_ be used for one specific asset type.
@@ -191,10 +192,7 @@ impl<A: Asset> Handle<A> {
     /// [`Handle::Weak`].  
     #[inline]
     pub fn untyped(self) -> UntypedHandle {
-        match self {
-            Handle::Strong(handle) => UntypedHandle::Strong(handle),
-            Handle::Weak(id) => UntypedHandle::Weak(id.untyped()),
-        }
+        self.into()
     }
 }
 
@@ -224,13 +222,14 @@ impl<A: Asset> std::fmt::Debug for Handle<A> {
 impl<A: Asset> Hash for Handle<A> {
     #[inline]
     fn hash<H: Hasher>(&self, state: &mut H) {
-        Hash::hash(&self.id(), state);
+        self.id().hash(state);
+        TypeId::of::<A>().hash(state);
     }
 }
 
 impl<A: Asset> PartialOrd for Handle<A> {
     fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
-        Some(self.id().cmp(&other.id()))
+        Some(self.cmp(other))
     }
 }
 
@@ -249,6 +248,34 @@ impl<A: Asset> PartialEq for Handle<A> {
 
 impl<A: Asset> Eq for Handle<A> {}
 
+impl<A: Asset> From<Handle<A>> for AssetId<A> {
+    #[inline]
+    fn from(value: Handle<A>) -> Self {
+        value.id()
+    }
+}
+
+impl<A: Asset> From<&Handle<A>> for AssetId<A> {
+    #[inline]
+    fn from(value: &Handle<A>) -> Self {
+        value.id()
+    }
+}
+
+impl<A: Asset> From<Handle<A>> for UntypedAssetId {
+    #[inline]
+    fn from(value: Handle<A>) -> Self {
+        value.id().into()
+    }
+}
+
+impl<A: Asset> From<&Handle<A>> for UntypedAssetId {
+    #[inline]
+    fn from(value: &Handle<A>) -> Self {
+        value.id().into()
+    }
+}
+
 /// An untyped variant of [`Handle`], which internally stores the [`Asset`] type information at runtime
 /// as a [`TypeId`] instead of encoding it in the compile-time type. This allows handles across [`Asset`] types
 /// to be stored together and compared.
@@ -326,12 +353,20 @@ impl UntypedHandle {
     /// Converts to a typed Handle. This will panic if the internal [`TypeId`] does not match the given asset type `A`
     #[inline]
     pub fn typed<A: Asset>(self) -> Handle<A> {
-        assert_eq!(
-            self.type_id(),
-            TypeId::of::<A>(),
-            "The target Handle<A>'s TypeId does not match the TypeId of this UntypedHandle"
-        );
-        self.typed_unchecked()
+        let Ok(handle) = self.try_typed() else {
+            panic!(
+                "The target Handle<{}>'s TypeId does not match the TypeId of this UntypedHandle",
+                std::any::type_name::<A>()
+            )
+        };
+
+        handle
+    }
+
+    /// Converts to a typed Handle. This will panic if the internal [`TypeId`] does not match the given asset type `A`
+    #[inline]
+    pub fn try_typed<A: Asset>(self) -> Result<Handle<A>, UntypedAssetConversionError> {
+        Handle::try_from(self)
     }
 
     /// The "meta transform" for the strong handle. This will only be [`Some`] if the handle is strong and there is a meta transform
@@ -383,3 +418,212 @@ impl std::fmt::Debug for UntypedHandle {
         }
     }
 }
+
+impl PartialOrd for UntypedHandle {
+    fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+        if self.type_id() == other.type_id() {
+            self.id().partial_cmp(&other.id())
+        } else {
+            None
+        }
+    }
+}
+
+impl From<UntypedHandle> for UntypedAssetId {
+    #[inline]
+    fn from(value: UntypedHandle) -> Self {
+        value.id()
+    }
+}
+
+impl From<&UntypedHandle> for UntypedAssetId {
+    #[inline]
+    fn from(value: &UntypedHandle) -> Self {
+        value.id()
+    }
+}
+
+// Cross Operations
+
+impl<A: Asset> PartialEq<UntypedHandle> for Handle<A> {
+    #[inline]
+    fn eq(&self, other: &UntypedHandle) -> bool {
+        TypeId::of::<A>() == other.type_id() && self.id() == other.id()
+    }
+}
+
+impl<A: Asset> PartialEq<Handle<A>> for UntypedHandle {
+    #[inline]
+    fn eq(&self, other: &Handle<A>) -> bool {
+        other.eq(self)
+    }
+}
+
+impl<A: Asset> PartialOrd<UntypedHandle> for Handle<A> {
+    #[inline]
+    fn partial_cmp(&self, other: &UntypedHandle) -> Option<std::cmp::Ordering> {
+        if TypeId::of::<A>() != other.type_id() {
+            None
+        } else {
+            self.id().partial_cmp(&other.id())
+        }
+    }
+}
+
+impl<A: Asset> PartialOrd<Handle<A>> for UntypedHandle {
+    #[inline]
+    fn partial_cmp(&self, other: &Handle<A>) -> Option<std::cmp::Ordering> {
+        Some(other.partial_cmp(self)?.reverse())
+    }
+}
+
+impl<A: Asset> From<Handle<A>> for UntypedHandle {
+    fn from(value: Handle<A>) -> Self {
+        match value {
+            Handle::Strong(handle) => UntypedHandle::Strong(handle),
+            Handle::Weak(id) => UntypedHandle::Weak(id.into()),
+        }
+    }
+}
+
+impl<A: Asset> TryFrom<UntypedHandle> for Handle<A> {
+    type Error = UntypedAssetConversionError;
+
+    fn try_from(value: UntypedHandle) -> Result<Self, Self::Error> {
+        let found = value.type_id();
+        let expected = TypeId::of::<A>();
+
+        if found != expected {
+            return Err(UntypedAssetConversionError::TypeIdMismatch { expected, found });
+        }
+
+        match value {
+            UntypedHandle::Strong(handle) => Ok(Handle::Strong(handle)),
+            UntypedHandle::Weak(id) => {
+                let Ok(id) = id.try_into() else {
+                    return Err(UntypedAssetConversionError::TypeIdMismatch { expected, found });
+                };
+                Ok(Handle::Weak(id))
+            }
+        }
+    }
+}
+
+/// Errors preventing the conversion of to/from an [`UntypedHandle`] and an [`Handle`].
+#[derive(Error, Debug, PartialEq, Clone)]
+#[non_exhaustive]
+pub enum UntypedAssetConversionError {
+    /// Caused when trying to convert an [`UntypedHandle`] into an [`Handle`] of the wrong type.
+    #[error(
+        "This UntypedHandle is for {found:?} and cannot be converted into an Handle<{expected:?}>"
+    )]
+    TypeIdMismatch { expected: TypeId, found: TypeId },
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    type TestAsset = ();
+
+    const UUID_1: Uuid = Uuid::from_u128(123);
+    const UUID_2: Uuid = Uuid::from_u128(456);
+
+    /// Simple utility to directly hash a value using a fixed hasher
+    fn hash<T: Hash>(data: &T) -> u64 {
+        let mut hasher = bevy_utils::AHasher::default();
+        data.hash(&mut hasher);
+        hasher.finish()
+    }
+
+    /// Typed and Untyped `Handles` should be equivalent to each other and themselves
+    #[test]
+    fn equality() {
+        let typed = AssetId::<TestAsset>::Uuid { uuid: UUID_1 };
+        let untyped = UntypedAssetId::Uuid {
+            type_id: TypeId::of::<TestAsset>(),
+            uuid: UUID_1,
+        };
+
+        let typed = Handle::Weak(typed);
+        let untyped = UntypedHandle::Weak(untyped);
+
+        assert_eq!(
+            Ok(typed.clone()),
+            Handle::<TestAsset>::try_from(untyped.clone())
+        );
+        assert_eq!(UntypedHandle::from(typed.clone()), untyped);
+        assert_eq!(typed, untyped);
+    }
+
+    /// Typed and Untyped `Handles` should be orderable amongst each other and themselves
+    #[allow(clippy::cmp_owned)]
+    #[test]
+    fn ordering() {
+        assert!(UUID_1 < UUID_2);
+
+        let typed_1 = AssetId::<TestAsset>::Uuid { uuid: UUID_1 };
+        let typed_2 = AssetId::<TestAsset>::Uuid { uuid: UUID_2 };
+        let untyped_1 = UntypedAssetId::Uuid {
+            type_id: TypeId::of::<TestAsset>(),
+            uuid: UUID_1,
+        };
+        let untyped_2 = UntypedAssetId::Uuid {
+            type_id: TypeId::of::<TestAsset>(),
+            uuid: UUID_2,
+        };
+
+        let typed_1 = Handle::Weak(typed_1);
+        let typed_2 = Handle::Weak(typed_2);
+        let untyped_1 = UntypedHandle::Weak(untyped_1);
+        let untyped_2 = UntypedHandle::Weak(untyped_2);
+
+        assert!(typed_1 < typed_2);
+        assert!(untyped_1 < untyped_2);
+
+        assert!(UntypedHandle::from(typed_1.clone()) < untyped_2);
+        assert!(untyped_1 < UntypedHandle::from(typed_2.clone()));
+
+        assert!(Handle::<TestAsset>::try_from(untyped_1.clone()).unwrap() < typed_2);
+        assert!(typed_1 < Handle::<TestAsset>::try_from(untyped_2.clone()).unwrap());
+
+        assert!(typed_1 < untyped_2);
+        assert!(untyped_1 < typed_2);
+    }
+
+    /// Typed and Untyped `Handles` should be equivalently hashable to each other and themselves
+    #[test]
+    fn hashing() {
+        let typed = AssetId::<TestAsset>::Uuid { uuid: UUID_1 };
+        let untyped = UntypedAssetId::Uuid {
+            type_id: TypeId::of::<TestAsset>(),
+            uuid: UUID_1,
+        };
+
+        let typed = Handle::Weak(typed);
+        let untyped = UntypedHandle::Weak(untyped);
+
+        assert_eq!(
+            hash(&typed),
+            hash(&Handle::<TestAsset>::try_from(untyped.clone()).unwrap())
+        );
+        assert_eq!(hash(&UntypedHandle::from(typed.clone())), hash(&untyped));
+        assert_eq!(hash(&typed), hash(&untyped));
+    }
+
+    /// Typed and Untyped `Handles` should be interchangeable
+    #[test]
+    fn conversion() {
+        let typed = AssetId::<TestAsset>::Uuid { uuid: UUID_1 };
+        let untyped = UntypedAssetId::Uuid {
+            type_id: TypeId::of::<TestAsset>(),
+            uuid: UUID_1,
+        };
+
+        let typed = Handle::Weak(typed);
+        let untyped = UntypedHandle::Weak(untyped);
+
+        assert_eq!(typed, Handle::try_from(untyped.clone()).unwrap());
+        assert_eq!(UntypedHandle::from(typed.clone()), untyped);
+    }
+}