hashing error in bevy_reflect now includes the type (bevyengine#13646)

[Wuketuke]

Objective

If you try to add an object to the hashmap that is not capable of hashing, the program panics. For easier debugging, the type for that object should be included in the error message.

Fixes #13646.

Solution

initially i tried calling std::any::type_name_of_val. this had the problem that it would print something like dyn Box, not helpful. But since these objects all implement Reflect, i used Reflect::type_path() instead. Previously, the error message was part of a constant called HASH_ERROR. i changed that to a macro called hash_error to print the type of that object more easily

Testing

i adapted the unit test reflect_map_no_hash to expect the type in that panic aswell

since this is my first contribution, please let me know if i have done everything properly

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[alice-i-cecile]

Nice stuff :) No complaints! I think the error message might be a bit clearer as "the supplied key (of type Foo) does support hashing", but I don't feel strongly.

P.S. If you use "Fixes #ISSUE_NUMBER" or "Closes #ISSUE_NUMBER" in your PR description it'll link the issue and PR and when the PR gets merged the issue will get closed :) I've edited your initial PR description to do that here.

[MrGVSV]

I think the current error message might be a little misleading (dynamic types in bevy_reflect are tricky 😅).

[MrGVSV]

A dynamic type will always fail to be "hashable" regardless of whether the represented type is hashable or not. In other words, even if Foo is hashable, DynamicStruct(Foo) won't be.

So this error message might be a bit misleading to users who are using hashable types.

Could we instead use the type name of the $key directly via DynamicTypePath::reflect_type_path? That should print out the actual type path (e.g. bevy_reflect::DynamicStruct).

Or perhaps we can combine the two somehow to print out something like bevy_reflect::DynamicStruct(my_crate::foo::Foo)? Although, I worry that this might also be a little misleading.

Another idea could also be to use Reflect::is_dynamic to print a separate error for dynamic types which can give more info and also indicate what type is being proxied (i.e. dynamic types cannot be hashed (trying to hash a dynamic instance of `my_crate::foo::Foo`) or something like that).

[Wuketuke]

i tried your suggestion, and changing s.type_path() to s.reflect_type_path() makes it not compile.
also i it out on a dynamic struct (in the reflect_map_no_hash unit test):
let mut s: DynamicStruct = DynamicStruct::default(); s.insert("a", Foo{a:2}); map.insert(s, 11u32);

the error message was: the given key Unknown does not support hashing, i dont think this is misleading, only unhelpful.

i dont exactly understand what problem you are trying to raise, but that probably says more about me than you. Im new to bevy & rust, so i might have missed something.

[MrGVSV]

i tried your suggestion, and changing s.type_path() to s.reflect_type_path() makes it not compile.

Sorry, to clarify, you'd do this on $key.

also i it out on a dynamic struct (in the reflect_map_no_hash unit test):
let mut s: DynamicStruct = DynamicStruct::default(); s.insert("a", Foo{a:2}); map.insert(s, 11u32);

the error message was: the given key Unknown does not support hashing, i dont think this is misleading, only unhelpful.

Unfortunately, that only tests a plain DynamicStruct, whereas we also need to consider a DynamicStruct that proxies/represents another type.

So for example:

#[derive(Reflect, Hash)]
#[reflect(Hash)]
struct Foo;

// `Foo` is hashable
let foo = Foo;
assert!(foo.reflect_hash().is_some());

// `DynamicStruct` representing `Foo` is not
let dynamic = foo.clone_dynamic();
assert!(dynamic.reflect_hash().is_none());

If we tried to use dynamic as a key, we'd get back the error the given key my_crate::foo::Foo does not support hashing, which is untrue.

I think it might be better if we had a separate case for dynamic types so that their error message can give a bit more detail since we've had a number of people run into this issue.

i dont exactly understand what problem you are trying to raise, but that probably says more about me than you. Im new to bevy & rust, so i might have missed something.

No worries! It's totally not your fault! This is just one of those weird quirks in bevy_reflect we are trying to solve (see #8695). You did great! :)

[MrGVSV]

@Wuketuke you are more than welcome to take another stab at this issue if you want to open another PR! If not, that's fine too, just let me know!

[Wuketuke]

@MrGVSV i just created a new pull request #13706 that addresses the issue u raised. i didnt open a new issue, since i think that just wastes everyones time, or do you think i should have done that?

[MrGVSV]

No need for an issue! I'll take a look at the PR. Thanks!