From 253a773cc5fe87fdfeafe96b44a1456acb4fe742 Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Sun, 5 May 2024 20:54:53 -0700 Subject: [PATCH] Fix dynamic diffing and cleanup tests --- crates/bevy_reflect/derive/src/impls/enums.rs | 1 + crates/bevy_reflect/src/array.rs | 12 +- crates/bevy_reflect/src/diff/mod.rs | 917 +++++++++--------- crates/bevy_reflect/src/enums/dynamic_enum.rs | 10 +- crates/bevy_reflect/src/list.rs | 10 +- crates/bevy_reflect/src/map.rs | 10 +- crates/bevy_reflect/src/struct_trait.rs | 10 +- crates/bevy_reflect/src/tuple.rs | 12 +- crates/bevy_reflect/src/tuple_struct.rs | 10 +- 9 files changed, 510 insertions(+), 482 deletions(-) diff --git a/crates/bevy_reflect/derive/src/impls/enums.rs b/crates/bevy_reflect/derive/src/impls/enums.rs index 8ef442ae2b734..1622114e35cb9 100644 --- a/crates/bevy_reflect/derive/src/impls/enums.rs +++ b/crates/bevy_reflect/derive/src/impls/enums.rs @@ -162,6 +162,7 @@ pub(crate) fn impl_enum(reflect_enum: &ReflectEnum) -> proc_macro2::TokenStream } fn apply_enum_diff(&mut self, diff: #bevy_reflect_path::diff::EnumDiff) -> #bevy_reflect_path::diff::DiffApplyResult { + println!("Diffing... {}", #bevy_reflect_path::DynamicTypePath::reflect_type_path(self)); let info = ::type_info(); if info.type_id() != diff.type_info().type_id() { diff --git a/crates/bevy_reflect/src/array.rs b/crates/bevy_reflect/src/array.rs index b3fe214092769..bee0d1d4842ce 100644 --- a/crates/bevy_reflect/src/array.rs +++ b/crates/bevy_reflect/src/array.rs @@ -366,14 +366,16 @@ impl Array for DynamicArray { } fn apply_array_diff(&mut self, diff: ArrayDiff) -> DiffApplyResult { - let Some(info) = self.get_represented_type_info() else { - return Err(DiffApplyError::MissingTypeInfo); - }; - - if info.type_id() != diff.type_info().type_id() || self.len() != diff.len() { + if self.len() != diff.len() { return Err(DiffApplyError::TypeMismatch); } + if let Some(info) = self.get_represented_type_info() { + if info.type_id() != diff.type_info().type_id() { + return Err(DiffApplyError::TypeMismatch); + } + }; + for (index, diff) in diff.take_changes().into_iter().enumerate() { self.get_mut(index) .ok_or(DiffApplyError::MissingField)? diff --git a/crates/bevy_reflect/src/diff/mod.rs b/crates/bevy_reflect/src/diff/mod.rs index 6ae6c7a3a6700..7dd682368adcf 100644 --- a/crates/bevy_reflect/src/diff/mod.rs +++ b/crates/bevy_reflect/src/diff/mod.rs @@ -160,38 +160,41 @@ pub use value_diff::*; #[cfg(test)] mod tests { use crate as bevy_reflect; - use crate::diff::{Diff, DiffApplyError, DiffType, ElementDiff, EntryDiff, EnumDiff}; + use crate::diff::{ + Diff, DiffApplyError, DiffResult, DiffType, ElementDiff, EntryDiff, EnumDiff, + }; use crate::Reflect; use bevy_utils::HashMap; /// Generates assertions for applying a diff to a value. /// + /// Note that this macro will modify `$old`. + /// /// # Cases /// /// * [`Diff::NoChange`] - Asserts that applying the diff should result in no change. /// * [`Diff::Replaced`] - Asserts that applying the diff should result in a type mismatch error. - /// * [`Diff::Modified`] - Asserts that applying the diff should result in a conversion from `old` to `new`. + /// * [`Diff::Modified`] - Asserts that applying the diff should result in a conversion from `$old` to `$new`. macro_rules! assert_apply_diff { - ($old: ident, $new: ident) => {{ - let mut _old = $old.clone(); - let diff = Reflect::diff(&$old, $new.as_reflect()).unwrap(); + ($old: expr, $new: expr, $diff: expr) => {{ + let mut old = $old; + let new = $new; + let diff = $diff; match &diff { Diff::NoChange => { assert!( - _old.reflect_partial_eq($new.as_reflect()) - .unwrap_or_default(), + old.reflect_partial_eq(new.as_reflect()).unwrap_or_default(), "old should be the same as new when diff is `Diff::NoChange`" ); - diff.apply(_old.as_reflect_mut()).unwrap(); + diff.apply(old.as_reflect_mut()).unwrap(); assert!( - _old.reflect_partial_eq($new.as_reflect()) - .unwrap_or_default(), + old.reflect_partial_eq(new.as_reflect()).unwrap_or_default(), "applying `Diff::NoChange` should result in no change" ); } Diff::Replaced(..) => { - let result = diff.apply(_old.as_reflect_mut()); + let result = diff.apply(old.as_reflect_mut()); assert_eq!( result, Err(DiffApplyError::TypeMismatch), @@ -199,10 +202,9 @@ mod tests { ); } Diff::Modified(..) => { - diff.apply(_old.as_reflect_mut()).unwrap(); + diff.apply(old.as_reflect_mut()).unwrap(); assert!( - _old.reflect_partial_eq($new.as_reflect()) - .unwrap_or_default(), + old.reflect_partial_eq(new.as_reflect()).unwrap_or_default(), "applying `Diff::Modified` should result in a modified value" ); } @@ -210,6 +212,43 @@ mod tests { }}; } + /// Runs a series of tests for diffing two values using the given callback. + /// + /// This function will generate four tests: + /// * diff(concrete, concrete) + /// * diff(concrete, dynamic) + /// * diff(dynamic, concrete) + /// * diff(dynamic, dynamic) + /// + /// The callback will be given the results of each diff operation. + /// + /// This is useful for testing the consistency of diffing both concrete and dynamic values. + fn test_diff( + old: T1, + new: T2, + callback: impl Fn(Box, &dyn Reflect, DiffResult), + ) { + // 1. diff(concrete, concrete) + let diff = old.diff(new.as_reflect()); + callback(Box::new(old.clone()), new.as_reflect(), diff); + + // 2. diff(concrete, dynamic) + let new_dynamic = new.clone_value(); + let diff = old.diff(new_dynamic.as_reflect()); + callback(Box::new(old.clone()), new_dynamic.as_reflect(), diff); + + // 3. diff(dynamic, concrete) + let old_dynamic = old.clone_value(); + let diff = old_dynamic.diff(new.as_reflect()); + callback(old_dynamic.clone_value(), new.as_reflect(), diff); + + // 4. diff(dynamic, dynamic) + let old_dynamic = old.clone_value(); + let new_dynamic = new.clone_value(); + let diff = old_dynamic.diff(new_dynamic.as_reflect()); + callback(old_dynamic.clone_value(), new_dynamic.as_reflect(), diff); + } + #[test] fn should_diff_value() { let old = 123_i32; @@ -217,209 +256,199 @@ mod tests { let diff = old.diff(&new).unwrap(); assert!(matches!(diff, Diff::NoChange)); - assert_apply_diff!(old, new); + assert_apply_diff!(old, new, diff); let old = 123_i32; let new = 321_i32; let diff = old.diff(&new).unwrap(); assert!(matches!(diff, Diff::Modified(..))); - assert_apply_diff!(old, new); + assert_apply_diff!(old, new, diff); let old = 123_i32; let new = 123_u32; let diff = old.diff(&new).unwrap(); assert!(matches!(diff, Diff::Replaced(..))); - assert_apply_diff!(old, new); + assert_apply_diff!(old, new, diff); } #[test] fn should_diff_tuple() { - let old = (1, 2, 3); - let new = (1, 2, 3); - - let diff = old.diff(&new).unwrap(); - assert!(matches!(diff, Diff::NoChange)); - assert_apply_diff!(old, new); - - let old = (1, 2, 3); - let new = (1, 2, 3, 4); - - let diff = old.diff(&new).unwrap(); - assert!(matches!(diff, Diff::Replaced(..))); - assert_apply_diff!(old, new); - - let old = (1, 2, 3); - let new = (1, 0, 3); - - let diff = old.diff(&new).unwrap(); - if let Diff::Modified(modified) = diff { - if let DiffType::Tuple(tuple_diff) = modified { - let mut fields = tuple_diff.field_iter(); - - assert!(matches!(fields.next(), Some(Diff::NoChange))); - assert!(matches!( - fields.next(), - Some(Diff::Modified(DiffType::Value(..))) - )); - assert!(matches!(fields.next(), Some(Diff::NoChange))); - assert!(fields.next().is_none()); + test_diff((1, 2, 3), (1, 2, 3), |old, new, diff| { + assert!(matches!(diff, Ok(Diff::NoChange))); + assert_apply_diff!(old, new, diff.unwrap()); + }); + + test_diff((1, 2, 3), (1, 2, 3, 4), |old, new, diff| { + assert!(matches!(diff, Ok(Diff::Replaced(..)))); + assert_apply_diff!(old, new, diff.unwrap()); + }); + + test_diff((1, 2, 3), (1, 0, 3), |old, new, diff| { + if let Ok(Diff::Modified(modified)) = &diff { + if let DiffType::Tuple(tuple_diff) = modified { + let mut fields = tuple_diff.field_iter(); + + assert!(matches!(fields.next(), Some(Diff::NoChange))); + assert!(matches!( + fields.next(), + Some(Diff::Modified(DiffType::Value(..))) + )); + assert!(matches!(fields.next(), Some(Diff::NoChange))); + assert!(fields.next().is_none()); + } else { + panic!("expected `DiffType::Tuple`"); + } } else { - panic!("expected `DiffType::Tuple`"); + panic!("expected `Diff::Modified`"); } - } else { - panic!("expected `Diff::Modified`"); - } - assert_apply_diff!(old, new); + assert_apply_diff!(old, new, diff.unwrap()); + }); } #[test] fn should_diff_array() { - let old = [1, 2, 3]; - let new = [1, 2, 3]; - - let diff = old.diff(&new).unwrap(); - assert!(matches!(diff, Diff::NoChange)); - assert_apply_diff!(old, new); - - let old = [1, 2, 3]; - let new = [1, 2, 3, 4]; - - let diff = old.diff(&new).unwrap(); - assert!(matches!(diff, Diff::Replaced(..))); - assert_apply_diff!(old, new); - - let old = [1, 2, 3]; - let new = [1, 0, 3]; - - let diff = old.diff(&new).unwrap(); - if let Diff::Modified(modified) = diff { - if let DiffType::Array(array_diff) = modified { - let mut fields = array_diff.iter(); - - assert!(matches!(fields.next(), Some(Diff::NoChange))); - assert!(matches!( - fields.next(), - Some(Diff::Modified(DiffType::Value(..))) - )); - assert!(matches!(fields.next(), Some(Diff::NoChange))); - assert!(fields.next().is_none()); + test_diff([1, 2, 3], [1, 2, 3], |old, new, diff| { + assert!(matches!(diff, Ok(Diff::NoChange))); + assert_apply_diff!(old, new, diff.unwrap()); + }); + + test_diff([1, 2, 3], [1, 2, 3, 4], |old, new, diff| { + assert!(matches!(diff, Ok(Diff::Replaced(..)))); + assert_apply_diff!(old, new, diff.unwrap()); + }); + + test_diff([1, 2, 3], [1, 0, 3], |old, new, diff| { + if let Ok(Diff::Modified(modified)) = &diff { + if let DiffType::Array(array_diff) = modified { + let mut fields = array_diff.iter(); + + assert!(matches!(fields.next(), Some(Diff::NoChange))); + assert!(matches!( + fields.next(), + Some(Diff::Modified(DiffType::Value(..))) + )); + assert!(matches!(fields.next(), Some(Diff::NoChange))); + assert!(fields.next().is_none()); + } else { + panic!("expected `DiffType::Array`"); + } } else { - panic!("expected `DiffType::Array`"); + panic!("expected `Diff::Modified`"); } - } else { - panic!("expected `Diff::Modified`"); - } - assert_apply_diff!(old, new); + assert_apply_diff!(old, new, diff.unwrap()); + }); } #[test] fn should_diff_list() { - let old = vec![1, 2, 3]; - let new = vec![1, 2, 3]; - - let diff = old.diff(&new).unwrap(); - assert!(matches!(diff, Diff::NoChange)); - assert_apply_diff!(old, new); - - let old: Vec = vec![1, 2, 3]; - let new: Vec = vec![1, 2, 3]; - - let diff = old.diff(&new).unwrap(); - assert!(matches!(diff, Diff::Replaced(_))); - assert_apply_diff!(old, new); - - let old = vec![1, 2, 3]; - let new = vec![9, 1, 2, 3]; - - let diff = old.diff(&new).unwrap(); - if let Diff::Modified(modified) = diff { - if let DiffType::List(list_diff) = modified { - let mut changes = list_diff.iter_changes(); - - assert!(matches!( - changes.next(), - Some(ElementDiff::Inserted(0, _ /* 9 */)) - )); - assert!(changes.next().is_none()); - } else { - panic!("expected `DiffType::List`"); - } - } else { - panic!("expected `Diff::Modified`"); - } - assert_apply_diff!(old, new); - - let old: Vec = vec![]; - let new: Vec = vec![1, 2, 3]; - - let diff = old.diff(&new).unwrap(); - if let Diff::Modified(modified) = diff { - if let DiffType::List(list_diff) = modified { - let mut changes = list_diff.iter_changes(); - - assert!(matches!( - changes.next(), - Some(ElementDiff::Inserted(0, _ /* 1 */)) - )); - assert!(matches!( - changes.next(), - Some(ElementDiff::Inserted(0, _ /* 2 */)) - )); - assert!(matches!( - changes.next(), - Some(ElementDiff::Inserted(0, _ /* 3 */)) - )); - assert!(changes.next().is_none()); + test_diff(vec![1, 2, 3], vec![1, 2, 3], |old, new, diff| { + assert!(matches!(diff, Ok(Diff::NoChange))); + assert_apply_diff!(old, new, diff.unwrap()); + }); + + test_diff( + vec![1, 2, 3] as Vec, + vec![1u32, 2, 3] as Vec, + |old, new, diff| { + assert!(matches!(diff, Ok(Diff::Replaced(..)))); + assert_apply_diff!(old, new, diff.unwrap()); + }, + ); + + test_diff(vec![1, 2, 3], vec![9, 1, 2, 3], |old, new, diff| { + if let Ok(Diff::Modified(modified)) = &diff { + if let DiffType::List(list_diff) = modified { + let mut changes = list_diff.iter_changes(); + + assert!(matches!( + changes.next(), + Some(ElementDiff::Inserted(0, _ /* 9 */)) + )); + assert!(changes.next().is_none()); + } else { + panic!("expected `DiffType::List`"); + } } else { - panic!("expected `DiffType::List`"); + panic!("expected `Diff::Modified`"); } - } else { - panic!("expected `Diff::Modified`"); - } - assert_apply_diff!(old, new); + assert_apply_diff!(old, new, diff.unwrap()); + }); - let old = vec![1, 2, 3, 4, 5]; - let new = vec![1, 0, 3, 6, 8, 4, 7]; + test_diff( + vec![] as Vec, + vec![1, 2, 3] as Vec, + |old, new, diff| { + if let Ok(Diff::Modified(modified)) = &diff { + if let DiffType::List(list_diff) = modified { + let mut changes = list_diff.iter_changes(); - let diff = old.diff(&new).unwrap(); - if let Diff::Modified(modified) = diff { - if let DiffType::List(list_diff) = modified { - let mut changes = list_diff.iter_changes(); + assert!(matches!( + changes.next(), + Some(ElementDiff::Inserted(0, _ /* 1 */)) + )); + assert!(matches!( + changes.next(), + Some(ElementDiff::Inserted(0, _ /* 2 */)) + )); + assert!(matches!( + changes.next(), + Some(ElementDiff::Inserted(0, _ /* 3 */)) + )); + assert!(changes.next().is_none()); + } else { + panic!("expected `DiffType::List`"); + } + } else { + panic!("expected `Diff::Modified`"); + } + assert_apply_diff!(old, new, diff.unwrap()); + }, + ); + + test_diff( + vec![1, 2, 3, 4, 5], + vec![1, 0, 3, 6, 8, 4, 7], + |old, new, diff| { + if let Ok(Diff::Modified(modified)) = &diff { + if let DiffType::List(list_diff) = modified { + let mut changes = list_diff.iter_changes(); - assert!(matches!( - changes.next(), - Some(ElementDiff::Deleted(1 /* 2 */)) - )); - assert!(matches!( - changes.next(), - Some(ElementDiff::Inserted(2, _ /* 0 */)) - )); - assert!(matches!( - changes.next(), - Some(ElementDiff::Inserted(3, _ /* 6 */)) - )); - assert!(matches!( - changes.next(), - Some(ElementDiff::Inserted(3, _ /* 8 */)) - )); - assert!(matches!( - changes.next(), - Some(ElementDiff::Deleted(4 /* 5 */)) - )); - assert!(matches!( - changes.next(), - Some(ElementDiff::Inserted(5, _ /* 7 */)) - )); - assert!(changes.next().is_none()); - } else { - panic!("expected `DiffType::List`"); - } - } else { - panic!("expected `Diff::Modified`"); - } - assert_apply_diff!(old, new); + assert!(matches!( + changes.next(), + Some(ElementDiff::Deleted(1 /* 2 */)) + )); + assert!(matches!( + changes.next(), + Some(ElementDiff::Inserted(2, _ /* 0 */)) + )); + assert!(matches!( + changes.next(), + Some(ElementDiff::Inserted(3, _ /* 6 */)) + )); + assert!(matches!( + changes.next(), + Some(ElementDiff::Inserted(3, _ /* 8 */)) + )); + assert!(matches!( + changes.next(), + Some(ElementDiff::Deleted(4 /* 5 */)) + )); + assert!(matches!( + changes.next(), + Some(ElementDiff::Inserted(5, _ /* 7 */)) + )); + assert!(changes.next().is_none()); + } else { + panic!("expected `DiffType::List`"); + } + } else { + panic!("expected `Diff::Modified`"); + } + assert_apply_diff!(old, new, diff.unwrap()); + }, + ); } #[test] @@ -430,82 +459,92 @@ mod tests { }; } - let old = map! {1: 111, 2: 222, 3: 333}; - let new = map! {3: 333, 1: 111, 2: 222}; + test_diff( + map! {1: 111, 2: 222, 3: 333}, + map! {1: 111, 2: 222, 3: 333}, + |old, new, diff| { + assert!(matches!(diff, Ok(Diff::NoChange))); + assert_apply_diff!(old, new, diff.unwrap()); + }, + ); + + test_diff( + map! {1: 111, 2: 222, 3: 333} as HashMap, + map! {1: 111u32, 2: 222, 3: 333} as HashMap, + |old, new, diff| { + assert!(matches!(diff, Ok(Diff::Replaced(..)))); + assert_apply_diff!(old, new, diff.unwrap()); + }, + ); + + test_diff( + map! {1: 111, 2: 222, 3: 333}, + map! {1: 111, 3: 333}, + |old, new, diff| { + if let Ok(Diff::Modified(modified)) = &diff { + if let DiffType::Map(map_diff) = modified { + let mut changes = map_diff.iter_changes(); - let diff = old.diff(&new).unwrap(); - assert!(matches!(diff, Diff::NoChange)); - assert_apply_diff!(old, new); - - let old: HashMap = map! {1: 111, 2: 222, 3: 333}; - let new: HashMap = map! {1: 111, 2: 222, 3: 333}; - - let diff = old.diff(&new).unwrap(); - assert!(matches!(diff, Diff::Replaced(_))); - assert_apply_diff!(old, new); - - let old = map! {1: 111, 2: 222, 3: 333}; - let new = map! {1: 111, 3: 333}; - - let diff = old.diff(&new).unwrap(); - if let Diff::Modified(modified) = diff { - if let DiffType::Map(map_diff) = modified { - let mut changes = map_diff.iter_changes(); - - assert!(matches!( - changes.next(), - Some(EntryDiff::Deleted(_ /* 2 */)) - )); - assert!(changes.next().is_none()); - } else { - panic!("expected `DiffType::Map`"); - } - } else { - panic!("expected `Diff::Modified`"); - } - assert_apply_diff!(old, new); - - let old = map! {1: 111, 2: 222, 3: 333}; - let new = map! {1: 111, 2: 222, 3: 333, 4: 444}; - - let diff = old.diff(&new).unwrap(); - if let Diff::Modified(modified) = diff { - if let DiffType::Map(map_diff) = modified { - let mut changes = map_diff.iter_changes(); - - assert!(matches!( - changes.next(), - Some(EntryDiff::Inserted(_ /* 4 */, _ /* 444 */)) - )); - assert!(changes.next().is_none()); - } else { - panic!("expected `DiffType::Map`"); - } - } else { - panic!("expected `Diff::Modified`"); - } - assert_apply_diff!(old, new); - - let old = map! {1: 111, 2: 222, 3: 333}; - let new = map! {1: 111, 2: 999, 3: 333}; + assert!(matches!( + changes.next(), + Some(EntryDiff::Deleted(_ /* 2 */)) + )); + assert!(changes.next().is_none()); + } else { + panic!("expected `DiffType::Map`"); + } + } else { + panic!("expected `Diff::Modified`"); + } + assert_apply_diff!(old, new, diff.unwrap()); + }, + ); + + test_diff( + map! {1: 111, 2: 222, 3: 333}, + map! {1: 111, 2: 222, 3: 333, 4: 444}, + |old, new, diff| { + if let Ok(Diff::Modified(modified)) = &diff { + if let DiffType::Map(map_diff) = modified { + let mut changes = map_diff.iter_changes(); - let diff = old.diff(&new).unwrap(); - if let Diff::Modified(modified) = diff { - if let DiffType::Map(map_diff) = modified { - let mut changes = map_diff.iter_changes(); + assert!(matches!( + changes.next(), + Some(EntryDiff::Inserted(_ /* 4 */, _ /* 444 */)) + )); + assert!(changes.next().is_none()); + } else { + panic!("expected `DiffType::Map`"); + } + } else { + panic!("expected `Diff::Modified`"); + } + assert_apply_diff!(old, new, diff.unwrap()); + }, + ); + + test_diff( + map! {1: 111, 2: 222, 3: 333}, + map! {1: 111, 2: 999, 3: 333}, + |old, new, diff| { + if let Ok(Diff::Modified(modified)) = &diff { + if let DiffType::Map(map_diff) = modified { + let mut changes = map_diff.iter_changes(); - assert!(matches!( - changes.next(), - Some(EntryDiff::Modified(_ /* 2 */, _ /* 999 */)) - )); - assert!(changes.next().is_none()); - } else { - panic!("expected `DiffType::Map`"); - } - } else { - panic!("expected `Diff::Modified`"); - } - assert_apply_diff!(old, new); + assert!(matches!( + changes.next(), + Some(EntryDiff::Modified(_ /* 2 */, _ /* 999 */)) + )); + assert!(changes.next().is_none()); + } else { + panic!("expected `DiffType::Map`"); + } + } else { + panic!("expected `Diff::Modified`"); + } + assert_apply_diff!(old, new, diff.unwrap()); + }, + ); } #[test] @@ -515,42 +554,36 @@ mod tests { #[derive(Reflect, Clone)] struct Bar(i32, i32, i32, i32); - let old = Foo(1, 2, 3); - let new = Foo(1, 2, 3); - - let diff = old.diff(&new).unwrap(); - assert!(matches!(diff, Diff::NoChange)); - assert_apply_diff!(old, new); - - let old = Foo(1, 2, 3); - let new = Bar(1, 2, 3, 4); - - let diff = old.diff(&new).unwrap(); - assert!(matches!(diff, Diff::Replaced(..))); - assert_apply_diff!(old, new); - - let old = Foo(1, 2, 3); - let new = Foo(1, 0, 3); - - let diff = old.diff(&new).unwrap(); - if let Diff::Modified(modified) = diff { - if let DiffType::TupleStruct(tuple_struct_diff) = modified { - let mut fields = tuple_struct_diff.field_iter(); - - assert!(matches!(fields.next(), Some(Diff::NoChange))); - assert!(matches!( - fields.next(), - Some(Diff::Modified(DiffType::Value(..))) - )); - assert!(matches!(fields.next(), Some(Diff::NoChange))); - assert!(fields.next().is_none()); + test_diff(Foo(1, 2, 3), Foo(1, 2, 3), |old, new, diff| { + assert!(matches!(diff, Ok(Diff::NoChange))); + assert_apply_diff!(old, new, diff.unwrap()); + }); + + test_diff(Foo(1, 2, 3), Bar(1, 2, 3, 4), |old, new, diff| { + assert!(matches!(diff, Ok(Diff::Replaced(..)))); + assert_apply_diff!(old, new, diff.unwrap()); + }); + + test_diff(Foo(1, 2, 3), Foo(1, 0, 3), |old, new, diff| { + if let Ok(Diff::Modified(modified)) = &diff { + if let DiffType::TupleStruct(tuple_struct_diff) = modified { + let mut fields = tuple_struct_diff.field_iter(); + + assert!(matches!(fields.next(), Some(Diff::NoChange))); + assert!(matches!( + fields.next(), + Some(Diff::Modified(DiffType::Value(..))) + )); + assert!(matches!(fields.next(), Some(Diff::NoChange))); + assert!(fields.next().is_none()); + } else { + panic!("expected `DiffType::TupleStruct`"); + } } else { - panic!("expected `DiffType::TupleStruct`"); + panic!("expected `Diff::Modified`"); } - } else { - panic!("expected `Diff::Modified`"); - } - assert_apply_diff!(old, new); + assert_apply_diff!(old, new, diff.unwrap()); + }); } #[test] @@ -567,45 +600,51 @@ mod tests { c: usize, } - let old = Foo { a: 123, b: 1.23 }; - let new = Foo { a: 123, b: 1.23 }; - - let diff = old.diff(&new).unwrap(); - assert!(matches!(diff, Diff::NoChange)); - assert_apply_diff!(old, new); - - let old = Foo { a: 123, b: 1.23 }; - let new = Bar { - a: 123, - b: 1.23, - c: 123, - }; - - let diff = old.diff(&new).unwrap(); - assert!(matches!(diff, Diff::Replaced(..))); - assert_apply_diff!(old, new); - - let old = Foo { a: 123, b: 1.23 }; - let new = Foo { a: 123, b: 3.21 }; - - let diff = old.diff(&new).unwrap(); - if let Diff::Modified(modified) = diff { - if let DiffType::Struct(struct_diff) = modified { - let mut fields = struct_diff.field_iter(); + test_diff( + Foo { a: 123, b: 1.23 }, + Foo { a: 123, b: 1.23 }, + |old, new, diff| { + assert!(matches!(diff, Ok(Diff::NoChange))); + assert_apply_diff!(old, new, diff.unwrap()); + }, + ); + + test_diff( + Foo { a: 123, b: 1.23 }, + Bar { + a: 123, + b: 1.23, + c: 123, + }, + |old, new, diff| { + assert!(matches!(diff, Ok(Diff::Replaced(..)))); + assert_apply_diff!(old, new, diff.unwrap()); + }, + ); + + test_diff( + Foo { a: 123, b: 1.23 }, + Foo { a: 123, b: 3.21 }, + |old, new, diff| { + if let Ok(Diff::Modified(modified)) = &diff { + if let DiffType::Struct(struct_diff) = modified { + let mut fields = struct_diff.field_iter(); - assert!(matches!(fields.next(), Some(("a", Diff::NoChange)))); - assert!(matches!( - fields.next(), - Some(("b", Diff::Modified(DiffType::Value(..)))) - )); - assert!(fields.next().is_none()); - } else { - panic!("expected `DiffType::Struct`"); - } - } else { - panic!("expected `Diff::Modified`"); - } - assert_apply_diff!(old, new); + assert!(matches!(fields.next(), Some(("a", Diff::NoChange)))); + assert!(matches!( + fields.next(), + Some(("b", Diff::Modified(DiffType::Value(..)))) + )); + assert!(fields.next().is_none()); + } else { + panic!("expected `DiffType::Struct`"); + } + } else { + panic!("expected `Diff::Modified`"); + } + assert_apply_diff!(old, new, diff.unwrap()); + }, + ); } mod enums { @@ -624,29 +663,23 @@ mod tests { B, } - let old = Foo::A; - let new = Foo::A; - - let diff = old.diff(&new).unwrap(); - assert!(matches!(diff, Diff::NoChange)); - assert_apply_diff!(old, new); - - let old = Foo::A; - let new = Foo::B; + test_diff(Foo::A, Foo::A, |old, new, diff| { + assert!(matches!(diff, Ok(Diff::NoChange))); + assert_apply_diff!(old, new, diff.unwrap()); + }); - let diff = old.diff(&new).unwrap(); - assert!(matches!( - diff, - Diff::Modified(DiffType::Enum(EnumDiff::Swapped(..))) - )); - assert_apply_diff!(old, new); - - let old = Foo::A; - let new = Bar::A; + test_diff(Foo::A, Foo::B, |old, new, diff| { + assert!(matches!( + diff, + Ok(Diff::Modified(DiffType::Enum(EnumDiff::Swapped(..)))) + )); + assert_apply_diff!(old, new, diff.unwrap()); + }); - let diff = old.diff(&new).unwrap(); - assert!(matches!(diff, Diff::Replaced(..))); - assert_apply_diff!(old, new); + test_diff(Foo::A, Bar::A, |old, new, diff| { + assert!(matches!(diff, Ok(Diff::Replaced(..)))); + assert_apply_diff!(old, new, diff.unwrap()); + }); } #[test] @@ -662,56 +695,48 @@ mod tests { B(i32, i32, i32), } - let old = Foo::A(1, 2, 3); - let new = Foo::A(1, 2, 3); - - let diff = old.diff(&new).unwrap(); - assert!(matches!(diff, Diff::NoChange)); - assert_apply_diff!(old, new); - - let old = Foo::A(1, 2, 3); - let new = Foo::B(1, 2, 3); - - let diff = old.diff(&new).unwrap(); - assert!(matches!( - diff, - Diff::Modified(DiffType::Enum(EnumDiff::Swapped(..))) - )); - assert_apply_diff!(old, new); - - let old = Foo::A(1, 2, 3); - let new = Bar::A(1, 2, 3); + test_diff(Foo::A(1, 2, 3), Foo::A(1, 2, 3), |old, new, diff| { + assert!(matches!(diff, Ok(Diff::NoChange))); + assert_apply_diff!(old, new, diff.unwrap()); + }); - let diff = old.diff(&new).unwrap(); - assert!(matches!(diff, Diff::Replaced(..))); - assert_apply_diff!(old, new); - - let old = Foo::A(1, 2, 3); - let new = Foo::A(1, 0, 3); - - let diff = old.diff(&new).unwrap(); - if let Diff::Modified(modified) = diff { - if let DiffType::Enum(enum_diff) = modified { - if let EnumDiff::Tuple(tuple_diff) = enum_diff { - let mut fields = tuple_diff.field_iter(); - - assert!(matches!(fields.next(), Some(Diff::NoChange))); - assert!(matches!( - fields.next(), - Some(Diff::Modified(DiffType::Value(..))) - )); - assert!(matches!(fields.next(), Some(Diff::NoChange))); - assert!(fields.next().is_none()); + test_diff(Foo::A(1, 2, 3), Foo::B(1, 2, 3), |old, new, diff| { + assert!(matches!( + diff, + Ok(Diff::Modified(DiffType::Enum(EnumDiff::Swapped(..)))) + )); + assert_apply_diff!(old, new, diff.unwrap()); + }); + + test_diff(Foo::A(1, 2, 3), Bar::A(1, 2, 3), |old, new, diff| { + assert!(matches!(diff, Ok(Diff::Replaced(..)))); + assert_apply_diff!(old, new, diff.unwrap()); + }); + + test_diff(Foo::A(1, 2, 3), Foo::A(1, 0, 3), |old, new, diff| { + if let Ok(Diff::Modified(modified)) = &diff { + if let DiffType::Enum(enum_diff) = modified { + if let EnumDiff::Tuple(tuple_diff) = enum_diff { + let mut fields = tuple_diff.field_iter(); + + assert!(matches!(fields.next(), Some(Diff::NoChange))); + assert!(matches!( + fields.next(), + Some(Diff::Modified(DiffType::Value(..))) + )); + assert!(matches!(fields.next(), Some(Diff::NoChange))); + assert!(fields.next().is_none()); + } else { + panic!("expected `EnumDiff::Tuple`"); + } } else { - panic!("expected `EnumDiff::Tuple`"); + panic!("expected `DiffType::Enum`"); } } else { - panic!("expected `DiffType::Enum`"); + panic!("expected `Diff::Modified`"); } - } else { - panic!("expected `Diff::Modified`"); - } - assert_apply_diff!(old, new); + assert_apply_diff!(old, new, diff.unwrap()); + }); } #[test] @@ -727,55 +752,63 @@ mod tests { B { x: f32, y: f32 }, } - let old = Foo::A { x: 1.23, y: 4.56 }; - let new = Foo::A { x: 1.23, y: 4.56 }; - - let diff = old.diff(&new).unwrap(); - assert!(matches!(diff, Diff::NoChange)); - assert_apply_diff!(old, new); - - let old = Foo::A { x: 1.23, y: 4.56 }; - let new = Foo::B { x: 1.23, y: 4.56 }; - - let diff = old.diff(&new).unwrap(); - assert!(matches!( - diff, - Diff::Modified(DiffType::Enum(EnumDiff::Swapped(..))) - )); - assert_apply_diff!(old, new); - - let old = Foo::A { x: 1.23, y: 4.56 }; - let new = Bar::A { x: 1.23, y: 4.56 }; - - let diff = old.diff(&new).unwrap(); - assert!(matches!(diff, Diff::Replaced(..))); - assert_apply_diff!(old, new); - - let old = Foo::A { x: 1.23, y: 4.56 }; - let new = Foo::A { x: 1.23, y: 7.89 }; - - let diff = old.diff(&new).unwrap(); - if let Diff::Modified(modified) = diff { - if let DiffType::Enum(enum_diff) = modified { - if let EnumDiff::Struct(struct_diff) = enum_diff { - let mut fields = struct_diff.field_iter(); - - assert!(matches!(fields.next(), Some(("x", Diff::NoChange)))); - assert!(matches!( - fields.next(), - Some(("y", Diff::Modified(DiffType::Value(..)))) - )); - assert!(fields.next().is_none()); + test_diff( + Foo::A { x: 1.23, y: 4.56 }, + Foo::A { x: 1.23, y: 4.56 }, + |old, new, diff| { + assert!(matches!(diff, Ok(Diff::NoChange))); + assert_apply_diff!(old, new, diff.unwrap()); + }, + ); + + test_diff( + Foo::A { x: 1.23, y: 4.56 }, + Foo::B { x: 1.23, y: 4.56 }, + |old, new, diff| { + assert!(matches!( + diff, + Ok(Diff::Modified(DiffType::Enum(EnumDiff::Swapped(..)))) + )); + assert_apply_diff!(old, new, diff.unwrap()); + }, + ); + + test_diff( + Foo::A { x: 1.23, y: 4.56 }, + Bar::A { x: 1.23, y: 4.56 }, + |old, new, diff| { + assert!(matches!(diff, Ok(Diff::Replaced(..)))); + assert_apply_diff!(old, new, diff.unwrap()); + }, + ); + + test_diff( + Foo::A { x: 1.23, y: 4.56 }, + Foo::A { x: 1.23, y: 7.89 }, + |old, new, diff| { + if let Ok(Diff::Modified(modified)) = &diff { + if let DiffType::Enum(enum_diff) = modified { + if let EnumDiff::Struct(struct_diff) = enum_diff { + let mut fields = struct_diff.field_iter(); + + assert!(matches!(fields.next(), Some(("x", Diff::NoChange)))); + assert!(matches!( + fields.next(), + Some(("y", Diff::Modified(DiffType::Value(..)))) + )); + assert!(fields.next().is_none()); + } else { + panic!("expected `EnumDiff::Struct`"); + } + } else { + panic!("expected `DiffType::Enum`"); + } } else { - panic!("expected `EnumDiff::Struct`"); + panic!("expected `Diff::Modified`"); } - } else { - panic!("expected `DiffType::Enum`"); - } - } else { - panic!("expected `Diff::Modified`"); - } - assert_apply_diff!(old, new); + assert_apply_diff!(old, new, diff.unwrap()); + }, + ); } } diff --git a/crates/bevy_reflect/src/enums/dynamic_enum.rs b/crates/bevy_reflect/src/enums/dynamic_enum.rs index 95327a80c7a3e..ae9cc8a0ae912 100644 --- a/crates/bevy_reflect/src/enums/dynamic_enum.rs +++ b/crates/bevy_reflect/src/enums/dynamic_enum.rs @@ -288,14 +288,12 @@ impl Enum for DynamicEnum { } fn apply_enum_diff(&mut self, diff: EnumDiff) -> DiffApplyResult { - let Some(info) = self.get_represented_type_info() else { - return Err(DiffApplyError::MissingTypeInfo); + if let Some(info) = self.get_represented_type_info() { + if info.type_id() != diff.type_info().type_id() { + return Err(DiffApplyError::TypeMismatch); + } }; - if info.type_id() != diff.type_info().type_id() { - return Err(DiffApplyError::TypeMismatch); - } - match diff { EnumDiff::Swapped(value_diff) => { self.try_apply(value_diff.as_reflect()).map_err(Into::into) diff --git a/crates/bevy_reflect/src/list.rs b/crates/bevy_reflect/src/list.rs index 77183ba4f11e3..1174cc2ad1eda 100644 --- a/crates/bevy_reflect/src/list.rs +++ b/crates/bevy_reflect/src/list.rs @@ -278,14 +278,12 @@ impl List for DynamicList { } fn apply_list_diff(&mut self, diff: ListDiff) -> DiffApplyResult { - let Some(info) = self.get_represented_type_info() else { - return Err(DiffApplyError::MissingTypeInfo); + if let Some(info) = self.get_represented_type_info() { + if info.type_id() != diff.type_info().type_id() { + return Err(DiffApplyError::TypeMismatch); + } }; - if info.type_id() != diff.type_info().type_id() { - return Err(DiffApplyError::TypeMismatch); - } - let new_len = (self.len() + diff.total_insertions()) - diff.total_deletions(); let mut new = Vec::with_capacity(new_len); diff --git a/crates/bevy_reflect/src/map.rs b/crates/bevy_reflect/src/map.rs index aa5a996383160..d2ee329fdd35f 100644 --- a/crates/bevy_reflect/src/map.rs +++ b/crates/bevy_reflect/src/map.rs @@ -314,14 +314,12 @@ impl Map for DynamicMap { } fn apply_map_diff(&mut self, diff: MapDiff) -> DiffApplyResult { - let Some(info) = self.get_represented_type_info() else { - return Err(DiffApplyError::MissingTypeInfo); + if let Some(info) = self.get_represented_type_info() { + if info.type_id() != diff.type_info().type_id() { + return Err(DiffApplyError::TypeMismatch); + } }; - if info.type_id() != diff.type_info().type_id() { - return Err(DiffApplyError::TypeMismatch); - } - for change in diff.take_changes() { match change { EntryDiff::Deleted(key) => { diff --git a/crates/bevy_reflect/src/struct_trait.rs b/crates/bevy_reflect/src/struct_trait.rs index bb1b133fe6240..8fc6fe70b7c12 100644 --- a/crates/bevy_reflect/src/struct_trait.rs +++ b/crates/bevy_reflect/src/struct_trait.rs @@ -402,12 +402,10 @@ impl Struct for DynamicStruct { } fn apply_struct_diff(&mut self, diff: StructDiff) -> DiffApplyResult { - let Some(info) = self.get_represented_type_info() else { - return Err(DiffApplyError::MissingTypeInfo); - }; - - if info.type_id() != diff.type_info().type_id() { - return Err(DiffApplyError::TypeMismatch); + if let Some(info) = self.get_represented_type_info() { + if info.type_id() != diff.type_info().type_id() { + return Err(DiffApplyError::TypeMismatch); + } } for (field_name, field_diff) in diff.take_changes() { diff --git a/crates/bevy_reflect/src/tuple.rs b/crates/bevy_reflect/src/tuple.rs index 64044b6b73fdb..34829d7efedcd 100644 --- a/crates/bevy_reflect/src/tuple.rs +++ b/crates/bevy_reflect/src/tuple.rs @@ -303,14 +303,16 @@ impl Tuple for DynamicTuple { } fn apply_tuple_diff(&mut self, diff: TupleDiff) -> DiffApplyResult { - let Some(info) = self.get_represented_type_info() else { - return Err(DiffApplyError::MissingTypeInfo); - }; - - if info.type_id() != diff.type_info().type_id() || self.field_len() != diff.field_len() { + if self.field_len() != diff.field_len() { return Err(DiffApplyError::TypeMismatch); } + if let Some(info) = self.get_represented_type_info() { + if info.type_id() != diff.type_info().type_id() { + return Err(DiffApplyError::TypeMismatch); + } + } + for (index, diff) in diff.take_changes().into_iter().enumerate() { self.field_mut(index) .ok_or(DiffApplyError::MissingField)? diff --git a/crates/bevy_reflect/src/tuple_struct.rs b/crates/bevy_reflect/src/tuple_struct.rs index d7f6d7802f86f..5e51ecb4e72e8 100644 --- a/crates/bevy_reflect/src/tuple_struct.rs +++ b/crates/bevy_reflect/src/tuple_struct.rs @@ -313,14 +313,12 @@ impl TupleStruct for DynamicTupleStruct { } fn apply_tuple_struct_diff(&mut self, diff: TupleStructDiff) -> DiffApplyResult { - let Some(info) = self.get_represented_type_info() else { - return Err(DiffApplyError::MissingTypeInfo); + if let Some(info) = self.get_represented_type_info() { + if info.type_id() != diff.type_info().type_id() { + return Err(DiffApplyError::TypeMismatch); + } }; - if info.type_id() != diff.type_info().type_id() { - return Err(DiffApplyError::TypeMismatch); - } - for (index, diff) in diff.take_changes().into_iter().enumerate() { self.fields .get_mut(index)