Different #[derive(Reflect, PartialEq)] enums/structs with the same structure/naming are `partial_eq` to each other
- Dominant language
- Rust
- Stars
- 48.2k
- Forks
- 4.8k
- Avg merge
- 3d 22h
- Merged PRs (30d)
- 161
Description
## Bevy version and features
`main` branch (at c6ab064c4095c6ecfc563be60e3b98b25299765e as of time of writing)
## What you did
Caveat: I am not too familiar with bevy-reflect, so if the behavior I am describing is actually intended, my apologies.
I wrote this test while reviewing #22452
```rust
#[test]
fn these_two_enums_should_not_partial_eq() {
#[derive(Reflect, Debug, PartialEq)]
enum MyFirstEnum {
A, B, C
}
#[derive(Reflect, Debug, PartialEq)]
enum MySecondEnum {
A, B, C
}
let a_first: &dyn PartialReflect = &MyFirstEnum::A;
let a_second: &dyn PartialReflect = &MySecondEnum::A;
assert!(
!a_first.reflect_partial_eq(a_second).unwrap_or_default(),
"expected MyFirstEnum::A != MySecondEnum::A"
);
}
```
and @CorvusPrudens also was curious about this test as a result:
```rust
#[test]
fn these_two_structs_should_not_partial_eq() {
#[derive(Reflect, PartialEq)]
struct A(f32);
#[derive(Reflect, PartialEq)]
struct B(f32);
let a: &dyn PartialReflect = &A(1.0);
let b: &dyn PartialReflect = &B(1.0);
assert!(a.reflect_partial_eq(b).unwrap_or_default());
}
```
## What went wrong
The above tests fail on main. I would expect those two different enum values or two different structs to not be `partial_eq`, but the code is returning that they are `partial_eq` based on structure alone.
However, if this behavior is intentional, it should be made clear somewhere in the code.
## Additional information
This has implications for `partial_cmp`, which is currently being coded with the existing `partial_eq` implementation in mind.
Simply adding
```rust
if a.reflect_type_path() != b.reflect_type_path() {
return Some(false);
}
```
to the `partial_eq` methods is not an option. Adding this if-branch for `enum_partial_eq` breaks an existing test (`enum_should_deserialize`). This makes me think that we may desire to only compare structurally in certain cases, or perhaps `reflect_type_path()` is not the right thing to compare at all times.
Also something else to ponder from Corvus:
> Another question that comes up is how should this work for a concrete PartialReflect and a dynamic one? Are we only returning false for two similarly-shaped types if they're both concrete?
Contributor guide
Assessment
This issue has not been assessed yet.