bevy_reflect: `reflect_hash` and `reflect_partial_eq` inconsistent on Dynamic types

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<u64, usize>.

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:

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

let mut map = DynamicMap::default();

let foo = Foo(123);
let foo_clone: Box<dyn Reflect> = 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:

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

let a = Foo(123);
let b = Foo(123);
let c: Box<dyn Reflect> = 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. 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.

#[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<T>, the functions stored in these structs no longer need to return Option<bool> and Option<u64> 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 discussion with @soqb.

great write up! is the only case for returning None from from_reflect when all fields have the #[reflect(skip_equivalence/ignore)] attribute (and similarly when they have no fields)? couldn't these types be thought of as "always equivalent" and just return true from reflect_partial_eq and write nothing to the hasher in reflect_hash?

is the only case for returning None from from_reflect when all fields have the #[reflect(skip_equivalence/ignore)] attribute (and similarly when they have no fields)? couldn't these types be thought of as "always equivalent" and just return true from reflect_partial_eq and write nothing to the hasher in reflect_hash?

Since each container boils down to a list of base value types, we only return None if one of those types cannot perform the operation. For example, f32 cannot be hashed. This means that the following wouldn't normally compile:

#[derive(Hash)]
struct Foo(f32);

To replicate this dynamically, f32's reflect_hash would return None. And Foo's reflect_hash would also return None unless the f32 field is skipped/ignored.

So no, ignoring/skipping all fields should not return simply return None.

In fact, we might might want to compare type_names as well (we currently make use of this in the reflect_hash operation anyways). Since Dynamic types takes on the type_name of the value they are clone_value'd from. So we may not always return true or None (hash).

That might be too restrictive and we can think about if we really want that behavior. Or we can even have a more restrictive method like reflect_strict_eq or something.

On a side note, I realize there are actually cases where we want to separate out equality and hashing. I gave the example of f32 above. This type can be compared using PartialEq but it cannot be hashed. Therefore, containers may want to use it for equality comparisons but not for their hashing strategy. For example, they may have a u64 ID field in the struct and prefer to use only that. This might be a case where we want to use attributes like #[reflect(skip_hash)] and #[reflect(skip_partial_eq)] over #[reflect(skip_equivalence)].

Here's another issue we need to consider with this proposed solution: Dynamic types are unaware of type information, such as which fields are marked skip_hash/skip_partial_eq.

I can't think of a perfect solution to this, but one idea I had was to store Option<&'static TypeInfo> in these Dynamic types. This would allow us to access field-specific data (assuming we store them in NamedField/UnnamedField). And so Reflect::clone_value would copy this static reference to their generated Dynamics so most users won't need to worry about this themselves.

The idea is that this would allow us to respect skip_hash even in something like a DynamicStruct.

Any thoughts on this?

imo the correct reflect_hash and reflect_partial_eq implementations are crucial behavior - dynamics should behave as close to their concrete counterparts as we can reasonably support. storing an Option<&'static TypeInfo> seems like a reasonable way to do this.

in terms of storing the info on NamedField/UnnamedField this would make sense as it allows the info to be retrieved from both a dyn Reflect and the type registry. in this case we should probably merge SerializationData into TypeInfo for consistency (in a different pr).

i can think of four Typed implementation for dynamics:

current behavior - return TypeInfo::Dynamic which does not contain any information on internal types.
keep returning TypeInfo::Dynamic but with information on fields, variants etc.
if the Option<&'static TypeInfo> field is available return that or else return TypeInfo::Dynamic as described in either of the solutions outlined above.
same as 2/3 but don't wrap in TypeInfo::Dynamic e.g. a DynamicStruct will always return TypeInfo::Struct.

in this case we should probably merge SerializationData into TypeInfo for consistency (in a different pr).

Hm, I'm still hesitant to fully tie reflection to serialization like this. And keeping SerializationData separate allows it to be configured at runtime if needed (but maybe that's not great anyways). We can certainly discuss that more in a future PR though haha.

i can think of four Typed implementation for dynamics:

current behavior - return TypeInfo::Dynamic which does not contain any information on internal types.

keep returning TypeInfo::Dynamic but with information on fields, variants etc.

if the Option<&'static TypeInfo> field is available return that or else return TypeInfo::Dynamic as described in either of the solutions outlined above.

same as 2/3 but don't wrap in TypeInfo::Dynamic e.g. a DynamicStruct will always return TypeInfo::Struct.

I'm not sure 2 is possible since this info is returned statically. Regardless, I think we'd be better off adding a separate method outside Typed that returns the Option<&'static TypeInfo>. This way we can still branch on whether or not the type is Dynamic:

fn foo(value: &dyn Reflect) {
  match value.reflect_ref() {
    ReflectRef::Struct(value) => {
      match value.type_info() {
        TypeInfo::Dynamic(_) => {
          // Probably should add a cleaner way to get here lol
          let value: DynamicStruct = value.downcast_ref().unwrap();
          let info: Option<&'static TypeInfo> = value.represents();
          // ...
        }
        // ...
      }
    }
    // ...
  }
}

And doing this I think sets us up better for Unique Reflect, but I could be wrong.

Additionally, knowing that a type is Dynamic is important for certain operations (such as for serde stuff), so I don't think we should hide the (currently) only method for getting that information.

Starting work on this now!

bevyengine / bevy