Skip to content

Commit 880ea5d

Browse files
committed
bevy_reflect: Fix apply method for Option<T> (#5780)
# Objective #5658 made it so that `FromReflect` was used as the bound for `T` in `Option<T>`. However, it did not use this change effectively for the implementation of `Reflect::apply` (it was still using `take`, which would fail for Dynamic types). Additionally, the changes were not consistent with other methods within the file, such as the ones for `Vec<T>` and `HashMap<K, V>`. ## Solution Update `Option<T>` to fallback on `FromReflect` if `take` fails, instead of wholly relying on one or the other. I also chose to update the error messages, as they weren't all too descriptive before. --- ## Changelog - Use `FromReflect::from_reflect` as a fallback in the `Reflect::apply` implementation for `Option<T>`
1 parent 886837d commit 880ea5d

File tree

1 file changed

+69
-18
lines changed
  • crates/bevy_reflect/src/impls

1 file changed

+69
-18
lines changed

crates/bevy_reflect/src/impls/std.rs

Lines changed: 69 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -696,8 +696,7 @@ impl<T: FromReflect> Reflect for Option<T> {
696696
if self.variant_name() == value.variant_name() {
697697
// Same variant -> just update fields
698698
for (index, field) in value.iter_fields().enumerate() {
699-
let name = value.name_at(index).unwrap();
700-
if let Some(v) = self.field_mut(name) {
699+
if let Some(v) = self.field_at_mut(index) {
701700
v.apply(field.value());
702701
}
703702
}
@@ -707,14 +706,23 @@ impl<T: FromReflect> Reflect for Option<T> {
707706
"Some" => {
708707
let field = value
709708
.field_at(0)
710-
.expect("Field at index 0 should exist")
711-
.clone_value();
712-
let field = field.take::<T>().unwrap_or_else(|_| {
713-
panic!(
714-
"Field at index 0 should be of type {}",
715-
std::any::type_name::<T>()
716-
)
717-
});
709+
.unwrap_or_else(|| {
710+
panic!(
711+
"Field in `Some` variant of {} should exist",
712+
std::any::type_name::<Option<T>>()
713+
)
714+
})
715+
.clone_value()
716+
.take::<T>()
717+
.unwrap_or_else(|value| {
718+
T::from_reflect(&*value).unwrap_or_else(|| {
719+
panic!(
720+
"Field in `Some` variant of {} should be of type {}",
721+
std::any::type_name::<Option<T>>(),
722+
std::any::type_name::<T>()
723+
)
724+
})
725+
});
718726
*self = Some(field);
719727
}
720728
"None" => {
@@ -761,14 +769,23 @@ impl<T: FromReflect> FromReflect for Option<T> {
761769
"Some" => {
762770
let field = dyn_enum
763771
.field_at(0)
764-
.expect("Field at index 0 should exist")
765-
.clone_value();
766-
let field = T::from_reflect(field.as_ref()).unwrap_or_else(|| {
767-
panic!(
768-
"Field at index 0 should be of type {}",
769-
std::any::type_name::<T>()
770-
)
771-
});
772+
.unwrap_or_else(|| {
773+
panic!(
774+
"Field in `Some` variant of {} should exist",
775+
std::any::type_name::<Option<T>>()
776+
)
777+
})
778+
.clone_value()
779+
.take::<T>()
780+
.unwrap_or_else(|value| {
781+
T::from_reflect(&*value).unwrap_or_else(|| {
782+
panic!(
783+
"Field in `Some` variant of {} should be of type {}",
784+
std::any::type_name::<Option<T>>(),
785+
std::any::type_name::<T>()
786+
)
787+
})
788+
});
772789
Some(Some(field))
773790
}
774791
"None" => Some(None),
@@ -953,6 +970,40 @@ mod tests {
953970
assert_eq!(expected, output);
954971
}
955972

973+
#[test]
974+
fn option_should_apply() {
975+
#[derive(Reflect, FromReflect, PartialEq, Debug)]
976+
struct Foo(usize);
977+
978+
// === None on None === //
979+
let patch = None::<Foo>;
980+
let mut value = None;
981+
Reflect::apply(&mut value, &patch);
982+
983+
assert_eq!(patch, value, "None apply onto None");
984+
985+
// === Some on None === //
986+
let patch = Some(Foo(123));
987+
let mut value = None;
988+
Reflect::apply(&mut value, &patch);
989+
990+
assert_eq!(patch, value, "Some apply onto None");
991+
992+
// === None on Some === //
993+
let patch = None::<Foo>;
994+
let mut value = Some(Foo(321));
995+
Reflect::apply(&mut value, &patch);
996+
997+
assert_eq!(patch, value, "None apply onto Some");
998+
999+
// === Some on Some === //
1000+
let patch = Some(Foo(123));
1001+
let mut value = Some(Foo(321));
1002+
Reflect::apply(&mut value, &patch);
1003+
1004+
assert_eq!(patch, value, "Some apply onto Some");
1005+
}
1006+
9561007
#[test]
9571008
fn option_should_impl_typed() {
9581009
type MyOption = Option<i32>;

0 commit comments

Comments
 (0)