From 7f10d53fd68f1b1363ef4ad1d69e62896d360d55 Mon Sep 17 00:00:00 2001
From: Gino Valente <gino.valente.code@gmail.com>
Date: Wed, 24 Aug 2022 20:44:35 +0000
Subject: [PATCH] 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>`
---
 crates/bevy_reflect/src/impls/std.rs | 87 ++++++++++++++++++++++------
 1 file changed, 69 insertions(+), 18 deletions(-)

diff --git a/crates/bevy_reflect/src/impls/std.rs b/crates/bevy_reflect/src/impls/std.rs
index 90fa58dc372ec..693ca39d7e88e 100644
--- a/crates/bevy_reflect/src/impls/std.rs
+++ b/crates/bevy_reflect/src/impls/std.rs
@@ -696,8 +696,7 @@ impl<T: FromReflect> Reflect for Option<T> {
             if self.variant_name() == value.variant_name() {
                 // Same variant -> just update fields
                 for (index, field) in value.iter_fields().enumerate() {
-                    let name = value.name_at(index).unwrap();
-                    if let Some(v) = self.field_mut(name) {
+                    if let Some(v) = self.field_at_mut(index) {
                         v.apply(field.value());
                     }
                 }
@@ -707,14 +706,23 @@ impl<T: FromReflect> Reflect for Option<T> {
                     "Some" => {
                         let field = value
                             .field_at(0)
-                            .expect("Field at index 0 should exist")
-                            .clone_value();
-                        let field = field.take::<T>().unwrap_or_else(|_| {
-                            panic!(
-                                "Field at index 0 should be of type {}",
-                                std::any::type_name::<T>()
-                            )
-                        });
+                            .unwrap_or_else(|| {
+                                panic!(
+                                    "Field in `Some` variant of {} should exist",
+                                    std::any::type_name::<Option<T>>()
+                                )
+                            })
+                            .clone_value()
+                            .take::<T>()
+                            .unwrap_or_else(|value| {
+                                T::from_reflect(&*value).unwrap_or_else(|| {
+                                    panic!(
+                                        "Field in `Some` variant of {} should be of type {}",
+                                        std::any::type_name::<Option<T>>(),
+                                        std::any::type_name::<T>()
+                                    )
+                                })
+                            });
                         *self = Some(field);
                     }
                     "None" => {
@@ -761,14 +769,23 @@ impl<T: FromReflect> FromReflect for Option<T> {
                 "Some" => {
                     let field = dyn_enum
                         .field_at(0)
-                        .expect("Field at index 0 should exist")
-                        .clone_value();
-                    let field = T::from_reflect(field.as_ref()).unwrap_or_else(|| {
-                        panic!(
-                            "Field at index 0 should be of type {}",
-                            std::any::type_name::<T>()
-                        )
-                    });
+                        .unwrap_or_else(|| {
+                            panic!(
+                                "Field in `Some` variant of {} should exist",
+                                std::any::type_name::<Option<T>>()
+                            )
+                        })
+                        .clone_value()
+                        .take::<T>()
+                        .unwrap_or_else(|value| {
+                            T::from_reflect(&*value).unwrap_or_else(|| {
+                                panic!(
+                                    "Field in `Some` variant of {} should be of type {}",
+                                    std::any::type_name::<Option<T>>(),
+                                    std::any::type_name::<T>()
+                                )
+                            })
+                        });
                     Some(Some(field))
                 }
                 "None" => Some(None),
@@ -953,6 +970,40 @@ mod tests {
         assert_eq!(expected, output);
     }
 
+    #[test]
+    fn option_should_apply() {
+        #[derive(Reflect, FromReflect, PartialEq, Debug)]
+        struct Foo(usize);
+
+        // === None on None === //
+        let patch = None::<Foo>;
+        let mut value = None;
+        Reflect::apply(&mut value, &patch);
+
+        assert_eq!(patch, value, "None apply onto None");
+
+        // === Some on None === //
+        let patch = Some(Foo(123));
+        let mut value = None;
+        Reflect::apply(&mut value, &patch);
+
+        assert_eq!(patch, value, "Some apply onto None");
+
+        // === None on Some === //
+        let patch = None::<Foo>;
+        let mut value = Some(Foo(321));
+        Reflect::apply(&mut value, &patch);
+
+        assert_eq!(patch, value, "None apply onto Some");
+
+        // === Some on Some === //
+        let patch = Some(Foo(123));
+        let mut value = Some(Foo(321));
+        Reflect::apply(&mut value, &patch);
+
+        assert_eq!(patch, value, "Some apply onto Some");
+    }
+
     #[test]
     fn option_should_impl_typed() {
         type MyOption = Option<i32>;