bevyengine / bevyengine/bevy

Different #[derive(Reflect, PartialEq)] enums/structs with the same structure/naming are `partial_eq` to each other

Open
#22,471 1 comment 0 reactions 0 assignees View on GitHub
A-Reflection C-Bug D-Modest S-Needs-Design
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

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.