From d45bcfd0434702c0b97de1727e5d5a9b2055dad3 Mon Sep 17 00:00:00 2001
From: Wuketuke <59253838+Wuketuke@users.noreply.github.com>
Date: Fri, 7 Jun 2024 20:56:16 +0000
Subject: [PATCH] improved the error message by insert_boxed (issue #13646)
 (again) (#13706)

previously I worked on fixing issue #13646, back when the error message
did not include the type at all.
But that error message had room for improvement, so I included the
feedback of @alice-i-cecile and @MrGVSV.
The error message will now read `the given key (of type
bevy_reflect::tests::Foo) does not support hashing` or 'the given key
(of type bevy_reflect::DynamicStruct) does not support hashing' in case
of a dynamic struct that represents a hashable struct

i also added a new unit test for this new behaviour
(`reflect_map_no_hash_dynamic`).
Fixes #13646 (again)

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
---
 crates/bevy_reflect/src/lib.rs | 44 +++++++++++++++++++++++++++++++++-
 crates/bevy_reflect/src/map.rs | 26 +++++++++++++++-----
 2 files changed, 63 insertions(+), 7 deletions(-)

diff --git a/crates/bevy_reflect/src/lib.rs b/crates/bevy_reflect/src/lib.rs
index b67a028236..29b73c5985 100644
--- a/crates/bevy_reflect/src/lib.rs
+++ b/crates/bevy_reflect/src/lib.rs
@@ -596,6 +596,7 @@ mod tests {
         any::TypeId,
         borrow::Cow,
         fmt::{Debug, Formatter},
+        hash::Hash,
         marker::PhantomData,
     };
 
@@ -756,7 +757,9 @@ mod tests {
     }
 
     #[test]
-    #[should_panic(expected = "the given key bevy_reflect::tests::Foo does not support hashing")]
+    #[should_panic(
+        expected = "the given key of type `bevy_reflect::tests::Foo` does not support hashing"
+    )]
     fn reflect_map_no_hash() {
         #[derive(Reflect)]
         struct Foo {
@@ -764,11 +767,50 @@ mod tests {
         }
 
         let foo = Foo { a: 1 };
+        assert!(foo.reflect_hash().is_none());
 
         let mut map = DynamicMap::default();
         map.insert(foo, 10u32);
     }
 
+    #[test]
+    #[should_panic(
+        expected = "the dynamic type `bevy_reflect::DynamicStruct` (representing `bevy_reflect::tests::Foo`) does not support hashing"
+    )]
+    fn reflect_map_no_hash_dynamic_representing() {
+        #[derive(Reflect, Hash)]
+        #[reflect(Hash)]
+        struct Foo {
+            a: u32,
+        }
+
+        let foo = Foo { a: 1 };
+        assert!(foo.reflect_hash().is_some());
+        let dynamic = foo.clone_dynamic();
+
+        let mut map = DynamicMap::default();
+        map.insert(dynamic, 11u32);
+    }
+
+    #[test]
+    #[should_panic(
+        expected = "the dynamic type `bevy_reflect::DynamicStruct` does not support hashing"
+    )]
+    fn reflect_map_no_hash_dynamic() {
+        #[derive(Reflect, Hash)]
+        #[reflect(Hash)]
+        struct Foo {
+            a: u32,
+        }
+
+        let mut dynamic = DynamicStruct::default();
+        dynamic.insert("a", 4u32);
+        assert!(dynamic.reflect_hash().is_none());
+
+        let mut map = DynamicMap::default();
+        map.insert(dynamic, 11u32);
+    }
+
     #[test]
     fn reflect_ignore() {
         #[derive(Reflect)]
diff --git a/crates/bevy_reflect/src/map.rs b/crates/bevy_reflect/src/map.rs
index 690d5b47f4..b50c6975b5 100644
--- a/crates/bevy_reflect/src/map.rs
+++ b/crates/bevy_reflect/src/map.rs
@@ -197,12 +197,26 @@ impl MapInfo {
 #[macro_export]
 macro_rules! hash_error {
     ( $key:expr ) => {{
-        let type_name = match (*$key).get_represented_type_info() {
-            None => "Unknown",
-            Some(s) => s.type_path(),
-        };
-        format!("the given key {} does not support hashing", type_name).as_str()
-    }};
+        let type_path = (*$key).reflect_type_path();
+        if !$key.is_dynamic() {
+            format!(
+                "the given key of type `{}` does not support hashing",
+                type_path
+            )
+        } else {
+            match (*$key).get_represented_type_info() {
+                // Handle dynamic types that do not represent a type (i.e a plain `DynamicStruct`):
+                None => format!("the dynamic type `{}` does not support hashing", type_path),
+                // Handle dynamic types that do represent a type (i.e. a `DynamicStruct` proxying `Foo`):
+                Some(s) => format!(
+                    "the dynamic type `{}` (representing `{}`) does not support hashing",
+                    type_path,
+                    s.type_path()
+                ),
+            }
+        }
+        .as_str()
+    }}
 }
 
 /// An ordered mapping between reflected values.