bevy_reflect: `reflect_hash` and `reflect_partial_eq` inconsistent on Dynamic types
- Dominant language
- Rust
- Stars
- 48.2k
- Forks
- 4.8k
- Avg merge
- 3d 16h
- Merged PRs (30d)
- 171
Description
## What problem does this solve or what need does it fill?
### The Issue with `Reflect::reflect_hash`
Currently, `DynamicMap` is the only place we make use of `Reflect::reflect_hash`. It uses this to dynamically hash a `dyn Reflect` so it can be stored in an internal `HashMap`.
Unfortunately, Dynamic types (e.g. `DynamicStruct`, `DynamicTuple`, etc.), do not implement `refelct_hash`. Why? They can't make use of the `Hash` impl of the concrete type they represent.
This means that doing the following will panic:
```rust
#[derive(Reflect, Hash)]
#[reflect(Hash)]
struct Foo(i32);
let mut map = DynamicMap::default();
let foo = Foo(123);
let foo_clone: Box = foo.clone_value(); // Creates a boxed `DynamicTupleStruct`
map.insert_boxed(foo_clone, Box::new(321)); // PANIC: "the given key does not support hashing"
```
Ideally, a Dynamic value's `reflect_hash` should be the same as its concrete counterpart. This reduces the burden of validating types on the user and reduces their need for unnecessary `FromReflect::from_reflect` calls.
### The Issue with `Reflect::reflect_partial_eq`
Similarly, `Reflect::reflect_partial_eq` can often work completely differently between a concrete type and its Dynamic representation. If a user adds `#[reflect(PartialEq)]` to a type, it suddenly breaks on certain comparisons and only in one direction:
```rust
#[derive(Reflect, PartialEq)]
#[reflect(PartialEq)]
struct Foo(i32);
let a = Foo(123);
let b = Foo(123);
let c: Box = a.clone_value(); // Creates a boxed `DynamicTupleStruct`
// 1. Concrete vs Concrete
assert!(a.reflect_partial_eq(&b).unwrap_or_default()); // PASS
// 2. Dynamic vs Concrete
assert!(c.reflect_partial_eq(&b).unwrap_or_default()); // PASS
// 3. Concrete vs Dynamic
assert!(b.reflect_partial_eq(&*c).unwrap_or_default()); // FAIL
```
In the above example, we fail at Assertion 3 because `Foo` uses its actual `PartialEq` impl when comparing, which will obviously fails when compared to a Dynamic.
## What solution would you like?
We should make both `Reflect::reflect_hash` and `Reflect::reflect_partial_eq` completely dynamic for non-`ReflectRef::Value` types. That is, for a given container type (e.g. a struct, tuple, list, etc.), the results of those operations should be determined based on the values that make up the type.
This means that both `Foo(i32)` and its `DynamicTupleStruct` representation can return the same value for each, and the user can rest a little easier when using `Reflect::clone_value`.
### Requirements
#### Equality
Together, these methods should work much like [`Hash` + `Eq`](https://doc.rust-lang.org/std/hash/trait.Hash.html#hash-and-eq). We should uphold that if two values are equal according to `reflect_partial_eq`, their `reflect_hash` values should be equal as well:
```
reflect_partial_eq(a, b) -> reflect_hash(a) == reflect_hash(b)
```
#### Optionality
There may be cases where a value cannot be hashed or compared. These values should return `None`. If a type contains another type whose `reflect_partial_eq` or `reflect_hash` is `None`, then it should return `None` as well. So if one field of a struct returns `None`, then the entire struct returns `None`. The `None`-ness bubbles up.
##### Skipping Fields
In order to give users more control over this behavior, it would be best to allow them to "skip" certain fields that are either known to return `None` or simply not desired to be included in the operation.
```rust
#[derive(Reflect)]
struct MyStruct {
a: usize,
#[reflect(skip_hash)]
b: f32, // `f32` cannot be hashed
#[reflect(skip_partial_eq)]
c: i32, // We don't want this field to influence equality checks
}
```
The `#[reflect(skip_hash)]` attribute will remove the given field from `reflect_hash` checks, while the `#[reflect(skip_partial_eq)]` attribute will remove it from `reflect_partial_eq` checks.
### Type Data
There may be cases we want to still make use of the concrete impls of `PartialEq` and/or `Hash`.
To do this, we can add two new `TypeData` structs: `ReflectHash` and `ReflectPartialEq`. Both will simply contain function pointers that can be be used to perform the concrete implementations. This data can then be retrieved from the `TypeRegistry`. And since getting `TypeData` returns an `Option`, the functions stored in these structs no longer need to return `Option` and `Option` like they currently do. They can simply return `bool` and `u64`.
## What alternative(s) have you considered?
These issues can be avoided by always checking that a type is not a Dynamic and using `FromReflect::from_reflect` if it is. However, we often don't have access to the concrete type required to make use of the `FromReflect` trait, which makes this solution not ideal.
## Additional context
Based on a [Discord](https://discord.com/channels/691052431525675048/1002362493634629796/1041251100243787807) discussion with @soqb.
Contributor guide
Assessment
This issue has not been assessed yet.