bevyengine / bevyengine/bevy

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

Open
#6,601 6 comments 0 reactions 1 assignee Claimed by @MrGVSV View on GitHub
A-Reflection C-Feature
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.