From 40982cd0a2d73d6f9dd7c10c6b357b01c59234a2 Mon Sep 17 00:00:00 2001 From: Afonso Lage Date: Tue, 5 Jul 2022 17:41:54 +0000 Subject: [PATCH] Make reflect_partial_eq return more accurate results (#5210) # Objective Closes #5204 ## Solution - Followed @nicopap suggestion on https://github.com/bevyengine/bevy/pull/4761#discussion_r903982224 ## Changelog - [x] Updated [struct_trait](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/struct_trait.rs#L455-L457), [tuple_struct](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/tuple_struct.rs#L366-L368), [tuple](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/tuple.rs#L386), [array](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/array.rs#L335-L337), [list](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/list.rs#L309-L311) and [map](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/map.rs#L361-L363) to return `None` when comparison couldn't be performed. - [x] Updated docs comments to reflect above changes. --- crates/bevy_reflect/src/array.rs | 7 +++++-- crates/bevy_reflect/src/list.rs | 7 +++++-- crates/bevy_reflect/src/map.rs | 7 +++++-- crates/bevy_reflect/src/struct_trait.rs | 7 +++++-- crates/bevy_reflect/src/tuple.rs | 8 +++++--- crates/bevy_reflect/src/tuple_struct.rs | 7 +++++-- 6 files changed, 30 insertions(+), 13 deletions(-) diff --git a/crates/bevy_reflect/src/array.rs b/crates/bevy_reflect/src/array.rs index 486966813d..a7c1da4185 100644 --- a/crates/bevy_reflect/src/array.rs +++ b/crates/bevy_reflect/src/array.rs @@ -327,13 +327,16 @@ pub fn array_apply(array: &mut A, reflect: &dyn Reflect) { /// Compares two [arrays](Array) (one concrete and one reflected) to see if they /// are equal. +/// +/// Returns [`None`] if the comparison couldn't even be performed. #[inline] pub fn array_partial_eq(array: &A, reflect: &dyn Reflect) -> Option { match reflect.reflect_ref() { ReflectRef::Array(reflect_array) if reflect_array.len() == array.len() => { for (a, b) in array.iter().zip(reflect_array.iter()) { - if let Some(false) | None = a.reflect_partial_eq(b) { - return Some(false); + let eq_result = a.reflect_partial_eq(b); + if let failed @ (Some(false) | None) = eq_result { + return failed; } } } diff --git a/crates/bevy_reflect/src/list.rs b/crates/bevy_reflect/src/list.rs index 25d2f179b4..9e8c0c65f6 100644 --- a/crates/bevy_reflect/src/list.rs +++ b/crates/bevy_reflect/src/list.rs @@ -293,6 +293,8 @@ pub fn list_apply(a: &mut L, b: &dyn Reflect) { /// - `b` is a list; /// - `b` is the same length as `a`; /// - [`Reflect::reflect_partial_eq`] returns `Some(true)` for pairwise elements of `a` and `b`. +/// +/// Returns [`None`] if the comparison couldn't even be performed. #[inline] pub fn list_partial_eq(a: &L, b: &dyn Reflect) -> Option { let list = if let ReflectRef::List(list) = b.reflect_ref() { @@ -306,8 +308,9 @@ pub fn list_partial_eq(a: &L, b: &dyn Reflect) -> Option { } for (a_value, b_value) in a.iter().zip(list.iter()) { - if let Some(false) | None = a_value.reflect_partial_eq(b_value) { - return Some(false); + let eq_result = a_value.reflect_partial_eq(b_value); + if let failed @ (Some(false) | None) = eq_result { + return failed; } } diff --git a/crates/bevy_reflect/src/map.rs b/crates/bevy_reflect/src/map.rs index 5824ba05a8..080b077039 100644 --- a/crates/bevy_reflect/src/map.rs +++ b/crates/bevy_reflect/src/map.rs @@ -344,6 +344,8 @@ impl<'a> ExactSizeIterator for MapIter<'a> {} /// - `b` is the same length as `a`; /// - For each key-value pair in `a`, `b` contains a value for the given key, /// and [`Reflect::reflect_partial_eq`] returns `Some(true)` for the two values. +/// +/// Returns [`None`] if the comparison couldn't even be performed. #[inline] pub fn map_partial_eq(a: &M, b: &dyn Reflect) -> Option { let map = if let ReflectRef::Map(map) = b.reflect_ref() { @@ -358,8 +360,9 @@ pub fn map_partial_eq(a: &M, b: &dyn Reflect) -> Option { for (key, value) in a.iter() { if let Some(map_value) = map.get(key) { - if let Some(false) | None = value.reflect_partial_eq(map_value) { - return Some(false); + let eq_result = value.reflect_partial_eq(map_value); + if let failed @ (Some(false) | None) = eq_result { + return failed; } } else { return Some(false); diff --git a/crates/bevy_reflect/src/struct_trait.rs b/crates/bevy_reflect/src/struct_trait.rs index 6f5075d990..60a9ba9e61 100644 --- a/crates/bevy_reflect/src/struct_trait.rs +++ b/crates/bevy_reflect/src/struct_trait.rs @@ -437,6 +437,8 @@ impl Typed for DynamicStruct { /// - For each field in `a`, `b` contains a field with the same name and /// [`Reflect::reflect_partial_eq`] returns `Some(true)` for the two field /// values. +/// +/// Returns [`None`] if the comparison couldn't even be performed. #[inline] pub fn struct_partial_eq(a: &S, b: &dyn Reflect) -> Option { let struct_value = if let ReflectRef::Struct(struct_value) = b.reflect_ref() { @@ -452,8 +454,9 @@ pub fn struct_partial_eq(a: &S, b: &dyn Reflect) -> Option { for (i, value) in struct_value.iter_fields().enumerate() { let name = struct_value.name_at(i).unwrap(); if let Some(field_value) = a.field(name) { - if let Some(false) | None = field_value.reflect_partial_eq(value) { - return Some(false); + let eq_result = field_value.reflect_partial_eq(value); + if let failed @ (Some(false) | None) = eq_result { + return failed; } } else { return Some(false); diff --git a/crates/bevy_reflect/src/tuple.rs b/crates/bevy_reflect/src/tuple.rs index c01f798c89..7651f7f5e3 100644 --- a/crates/bevy_reflect/src/tuple.rs +++ b/crates/bevy_reflect/src/tuple.rs @@ -369,6 +369,8 @@ pub fn tuple_apply(a: &mut T, b: &dyn Reflect) { /// - `b` is a tuple; /// - `b` has the same number of elements as `a`; /// - [`Reflect::reflect_partial_eq`] returns `Some(true)` for pairwise elements of `a` and `b`. +/// +/// Returns [`None`] if the comparison couldn't even be performed. #[inline] pub fn tuple_partial_eq(a: &T, b: &dyn Reflect) -> Option { let b = if let ReflectRef::Tuple(tuple) = b.reflect_ref() { @@ -382,9 +384,9 @@ pub fn tuple_partial_eq(a: &T, b: &dyn Reflect) -> Option { } for (a_field, b_field) in a.iter_fields().zip(b.iter_fields()) { - match a_field.reflect_partial_eq(b_field) { - Some(false) | None => return Some(false), - Some(true) => {} + let eq_result = a_field.reflect_partial_eq(b_field); + if let failed @ (Some(false) | None) = eq_result { + return failed; } } diff --git a/crates/bevy_reflect/src/tuple_struct.rs b/crates/bevy_reflect/src/tuple_struct.rs index 27ded3e919..507b6b4bfd 100644 --- a/crates/bevy_reflect/src/tuple_struct.rs +++ b/crates/bevy_reflect/src/tuple_struct.rs @@ -349,6 +349,8 @@ impl Typed for DynamicTupleStruct { /// - `b` is a tuple struct; /// - `b` has the same number of fields as `a`; /// - [`Reflect::reflect_partial_eq`] returns `Some(true)` for pairwise fields of `a` and `b`. +/// +/// Returns [`None`] if the comparison couldn't even be performed. #[inline] pub fn tuple_struct_partial_eq(a: &S, b: &dyn Reflect) -> Option { let tuple_struct = if let ReflectRef::TupleStruct(tuple_struct) = b.reflect_ref() { @@ -363,8 +365,9 @@ pub fn tuple_struct_partial_eq(a: &S, b: &dyn Reflect) -> Option for (i, value) in tuple_struct.iter_fields().enumerate() { if let Some(field_value) = a.field(i) { - if let Some(false) | None = field_value.reflect_partial_eq(value) { - return Some(false); + let eq_result = field_value.reflect_partial_eq(value); + if let failed @ (Some(false) | None) = eq_result { + return failed; } } else { return Some(false);