Make Reflect safe to implement (bevyengine#5010)

# Objective Currently, `Reflect` is unsafe to implement because of a contract in which `any` and `any_mut` must return `self`, or `downcast` will cause UB. This PR makes `Reflect` safe, makes `downcast` not use unsafe, and eliminates this contract. ## Solution This PR adds a method to `Reflect`, `any`. It also renames the old `any` to `as_any`. `any` now takes a `Box<Self>` and returns a `Box<dyn Any>`. --- ## Changelog ### Added: - `any()` method - `represents()` method ### Changed: - `Reflect` is now a safe trait - `downcast()` is now safe - The old `any` is now called `as_any`, and `any_mut` is now `as_mut_any` ## Migration Guide - Reflect derives should not have to change anything - Manual reflect impls will need to remove the `unsafe` keyword, add `any()` implementations, and rename the old `any` and `any_mut` to `as_any` and `as_mut_any`. - Calls to `any`/`any_mut` must be changed to `as_any`/`as_mut_any` ## Points of discussion: - Should renaming `any` be avoided and instead name the new method `any_box`? - ~~Could there be a performance regression from avoiding the unsafe? I doubt it, but this change does seem to introduce redundant checks.~~ - ~~Could/should `is` and `type_id()` be implemented differently? For example, moving `is` onto `Reflect` as an `fn(&self, TypeId) -> bool`~~ Co-authored-by: PROMETHIA-27 <[email protected]>
james7132 · Oct 28, 2022 · 52ff80c · 52ff80c
1 parent cfd5389
commit 52ff80c
Show file tree

Hide file tree

Showing 15 changed files with 206 additions and 96 deletions.
diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs b/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs
@@ -208,7 +208,7 @@ impl ReflectTraits {
         match &self.partial_eq {
             TraitImpl::Implemented => Some(quote! {
                 fn reflect_partial_eq(&self, value: &dyn #bevy_reflect_path::Reflect) -> Option<bool> {
-                    let value = value.any();
+                    let value = value.as_any();
                     if let Some(value) = value.downcast_ref::<Self>() {
                         Some(std::cmp::PartialEq::eq(self, value))
                     } else {

diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs b/crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs
@@ -26,7 +26,7 @@ pub(crate) fn impl_value(
     TokenStream::from(quote! {
         impl #impl_generics #bevy_reflect_path::FromReflect for #type_name #ty_generics #where_clause  {
             fn from_reflect(reflect: &dyn #bevy_reflect_path::Reflect) -> Option<Self> {
-                Some(reflect.any().downcast_ref::<#type_name #ty_generics>()?.clone())
+                Some(reflect.as_any().downcast_ref::<#type_name #ty_generics>()?.clone())
             }
         }
     })

diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/impls.rs b/crates/bevy_reflect/bevy_reflect_derive/src/impls.rs
@@ -125,8 +125,7 @@ pub(crate) fn impl_struct(derive_data: &ReflectDeriveData) -> TokenStream {
             }
         }
 
-        // SAFE: any and any_mut both return self
-        unsafe impl #impl_generics #bevy_reflect_path::Reflect for #struct_name #ty_generics #where_clause {
+        impl #impl_generics #bevy_reflect_path::Reflect for #struct_name #ty_generics #where_clause {
             #[inline]
             fn type_name(&self) -> &str {
                 std::any::type_name::<Self>()
@@ -138,11 +137,17 @@ pub(crate) fn impl_struct(derive_data: &ReflectDeriveData) -> TokenStream {
             }
 
             #[inline]
-            fn any(&self) -> &dyn std::any::Any {
+            fn into_any(self: Box<Self>) -> Box<dyn std::any::Any> {
                 self
             }
+
             #[inline]
-            fn any_mut(&mut self) -> &mut dyn std::any::Any {
+            fn as_any(&self) -> &dyn std::any::Any {
+                self
+            }
+
+            #[inline]
+            fn as_any_mut(&mut self) -> &mut dyn std::any::Any {
                 self
             }
 
@@ -275,8 +280,7 @@ pub(crate) fn impl_tuple_struct(derive_data: &ReflectDeriveData) -> TokenStream
             }
         }
 
-        // SAFE: any and any_mut both return self
-        unsafe impl #impl_generics #bevy_reflect_path::Reflect for #struct_name #ty_generics #where_clause {
+        impl #impl_generics #bevy_reflect_path::Reflect for #struct_name #ty_generics #where_clause {
             #[inline]
             fn type_name(&self) -> &str {
                 std::any::type_name::<Self>()
@@ -288,11 +292,17 @@ pub(crate) fn impl_tuple_struct(derive_data: &ReflectDeriveData) -> TokenStream
             }
 
             #[inline]
-            fn any(&self) -> &dyn std::any::Any {
+            fn into_any(self: Box<Self>) -> Box<dyn std::any::Any> {
+                self
+            }
+
+            #[inline]
+            fn as_any(&self) -> &dyn std::any::Any {
                 self
             }
+
             #[inline]
-            fn any_mut(&mut self) -> &mut dyn std::any::Any {
+            fn as_any_mut(&mut self) -> &mut dyn std::any::Any {
                 self
             }
 
@@ -372,8 +382,7 @@ pub(crate) fn impl_value(
 
         #typed_impl
 
-        // SAFE: any and any_mut both return self
-        unsafe impl #impl_generics #bevy_reflect_path::Reflect for #type_name #ty_generics #where_clause  {
+        impl #impl_generics #bevy_reflect_path::Reflect for #type_name #ty_generics #where_clause  {
             #[inline]
             fn type_name(&self) -> &str {
                 std::any::type_name::<Self>()
@@ -385,12 +394,17 @@ pub(crate) fn impl_value(
             }
 
             #[inline]
-            fn any(&self) -> &dyn std::any::Any {
+            fn into_any(self: Box<Self>) -> Box<dyn std::any::Any> {
+                self
+            }
+
+            #[inline]
+            fn as_any(&self) -> &dyn std::any::Any {
                 self
             }
 
             #[inline]
-            fn any_mut(&mut self) -> &mut dyn std::any::Any {
+            fn as_any_mut(&mut self) -> &mut dyn std::any::Any {
                 self
             }
 
@@ -411,7 +425,7 @@ pub(crate) fn impl_value(
 
             #[inline]
             fn apply(&mut self, value: &dyn #bevy_reflect_path::Reflect) {
-                let value = value.any();
+                let value = value.as_any();
                 if let Some(value) = value.downcast_ref::<Self>() {
                     *self = value.clone();
                 } else {

diff --git a/crates/bevy_reflect/src/array.rs b/crates/bevy_reflect/src/array.rs
@@ -151,8 +151,7 @@ impl DynamicArray {
     }
 }
 
-// SAFE: any and any_mut both return self
-unsafe impl Reflect for DynamicArray {
+impl Reflect for DynamicArray {
     #[inline]
     fn type_name(&self) -> &str {
         self.name.as_str()
@@ -164,12 +163,17 @@ unsafe impl Reflect for DynamicArray {
     }
 
     #[inline]
-    fn any(&self) -> &dyn Any {
+    fn into_any(self: Box<Self>) -> Box<dyn Any> {
         self
     }
 
     #[inline]
-    fn any_mut(&mut self) -> &mut dyn Any {
+    fn as_any(&self) -> &dyn Any {
+        self
+    }
+
+    #[inline]
+    fn as_any_mut(&mut self) -> &mut dyn Any {
         self
     }
 

diff --git a/crates/bevy_reflect/src/impls/smallvec.rs b/crates/bevy_reflect/src/impls/smallvec.rs
@@ -55,8 +55,7 @@ where
     }
 }
 
-// SAFE: any and any_mut both return self
-unsafe impl<T: smallvec::Array + Send + Sync + 'static> Reflect for SmallVec<T>
+impl<T: smallvec::Array + Send + Sync + 'static> Reflect for SmallVec<T>
 where
     T::Item: FromReflect + Clone,
 {
@@ -68,11 +67,15 @@ where
         <Self as Typed>::type_info()
     }
 
-    fn any(&self) -> &dyn Any {
+    fn into_any(self: Box<Self>) -> Box<dyn Any> {
         self
     }
 
-    fn any_mut(&mut self) -> &mut dyn Any {
+    fn as_any(&self) -> &dyn Any {
+        self
+    }
+
+    fn as_any_mut(&mut self) -> &mut dyn Any {
         self
     }
 

diff --git a/crates/bevy_reflect/src/impls/std.rs b/crates/bevy_reflect/src/impls/std.rs
@@ -105,8 +105,7 @@ impl<T: FromReflect> List for Vec<T> {
     }
 }
 
-// SAFE: any and any_mut both return self
-unsafe impl<T: FromReflect> Reflect for Vec<T> {
+impl<T: FromReflect> Reflect for Vec<T> {
     fn type_name(&self) -> &str {
         std::any::type_name::<Self>()
     }
@@ -115,11 +114,15 @@ unsafe impl<T: FromReflect> Reflect for Vec<T> {
         <Self as Typed>::type_info()
     }
 
-    fn any(&self) -> &dyn Any {
+    fn into_any(self: Box<Self>) -> Box<dyn Any> {
         self
     }
 
-    fn any_mut(&mut self) -> &mut dyn Any {
+    fn as_any(&self) -> &dyn Any {
+        self
+    }
+
+    fn as_any_mut(&mut self) -> &mut dyn Any {
         self
     }
 
@@ -230,8 +233,7 @@ impl<K: Reflect + Eq + Hash, V: Reflect> Map for HashMap<K, V> {
     }
 }
 
-// SAFE: any and any_mut both return self
-unsafe impl<K: Reflect + Eq + Hash, V: Reflect> Reflect for HashMap<K, V> {
+impl<K: Reflect + Eq + Hash, V: Reflect> Reflect for HashMap<K, V> {
     fn type_name(&self) -> &str {
         std::any::type_name::<Self>()
     }
@@ -240,11 +242,15 @@ unsafe impl<K: Reflect + Eq + Hash, V: Reflect> Reflect for HashMap<K, V> {
         <Self as Typed>::type_info()
     }
 
-    fn any(&self) -> &dyn Any {
+    fn into_any(self: Box<Self>) -> Box<dyn Any> {
         self
     }
 
-    fn any_mut(&mut self) -> &mut dyn Any {
+    fn as_any(&self) -> &dyn Any {
+        self
+    }
+
+    fn as_any_mut(&mut self) -> &mut dyn Any {
         self
     }
 
@@ -350,8 +356,7 @@ impl<T: Reflect, const N: usize> Array for [T; N] {
     }
 }
 
-// SAFE: any and any_mut both return self
-unsafe impl<T: Reflect, const N: usize> Reflect for [T; N] {
+impl<T: Reflect, const N: usize> Reflect for [T; N] {
     #[inline]
     fn type_name(&self) -> &str {
         std::any::type_name::<Self>()
@@ -362,12 +367,17 @@ unsafe impl<T: Reflect, const N: usize> Reflect for [T; N] {
     }
 
     #[inline]
-    fn any(&self) -> &dyn Any {
+    fn into_any(self: Box<Self>) -> Box<dyn Any> {
         self
     }
 
     #[inline]
-    fn any_mut(&mut self) -> &mut dyn Any {
+    fn as_any(&self) -> &dyn Any {
+        self
+    }
+
+    #[inline]
+    fn as_any_mut(&mut self) -> &mut dyn Any {
         self
     }
 
@@ -465,8 +475,7 @@ impl_array_get_type_registration! {
     30 31 32
 }
 
-// SAFE: any and any_mut both return self
-unsafe impl Reflect for Cow<'static, str> {
+impl Reflect for Cow<'static, str> {
     fn type_name(&self) -> &str {
         std::any::type_name::<Self>()
     }
@@ -475,11 +484,15 @@ unsafe impl Reflect for Cow<'static, str> {
         <Self as Typed>::type_info()
     }
 
-    fn any(&self) -> &dyn Any {
+    fn into_any(self: Box<Self>) -> Box<dyn Any> {
+        self
+    }
+
+    fn as_any(&self) -> &dyn Any {
         self
     }
 
-    fn any_mut(&mut self) -> &mut dyn Any {
+    fn as_any_mut(&mut self) -> &mut dyn Any {
         self
     }
 
@@ -492,7 +505,7 @@ unsafe impl Reflect for Cow<'static, str> {
     }
 
     fn apply(&mut self, value: &dyn Reflect) {
-        let value = value.any();
+        let value = value.as_any();
         if let Some(value) = value.downcast_ref::<Self>() {
             *self = value.clone();
         } else {
@@ -525,7 +538,7 @@ unsafe impl Reflect for Cow<'static, str> {
     }
 
     fn reflect_partial_eq(&self, value: &dyn Reflect) -> Option<bool> {
-        let value = value.any();
+        let value = value.as_any();
         if let Some(value) = value.downcast_ref::<Self>() {
             Some(std::cmp::PartialEq::eq(self, value))
         } else {
@@ -552,7 +565,12 @@ impl GetTypeRegistration for Cow<'static, str> {
 
 impl FromReflect for Cow<'static, str> {
     fn from_reflect(reflect: &dyn crate::Reflect) -> Option<Self> {
-        Some(reflect.any().downcast_ref::<Cow<'static, str>>()?.clone())
+        Some(
+            reflect
+                .as_any()
+                .downcast_ref::<Cow<'static, str>>()?
+                .clone(),
+        )
     }
 }
 

diff --git a/crates/bevy_reflect/src/lib.rs b/crates/bevy_reflect/src/lib.rs
@@ -485,6 +485,42 @@ mod tests {
         assert!(foo.reflect_partial_eq(&dynamic_struct).unwrap());
     }
 
+    #[test]
+    fn reflect_downcast() {
+        #[derive(Reflect, Clone, Debug, PartialEq)]
+        struct Bar {
+            y: u8,
+            z: ::glam::Mat4,
+        }
+
+        #[derive(Reflect, Clone, Debug, PartialEq)]
+        struct Foo {
+            x: i32,
+            s: String,
+            b: Bar,
+            u: usize,
+            t: (Vec3, String),
+        }
+
+        let foo = Foo {
+            x: 123,
+            s: "String".to_string(),
+            b: Bar {
+                y: 255,
+                z: ::glam::Mat4::from_cols_array(&[
+                    0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0, 11.0, 12.0, 13.0, 14.0,
+                    15.0,
+                ]),
+            },
+            u: 1111111111111,
+            t: (Vec3::new(3.0, 2.0, 1.0), "Tuple String".to_string()),
+        };
+
+        let foo2: Box<dyn Reflect> = Box::new(foo.clone());
+
+        assert_eq!(foo, *foo2.downcast::<Foo>().unwrap());
+    }
+
     #[test]
     fn reflect_take() {
         #[derive(Reflect, Debug, PartialEq)]

diff --git a/crates/bevy_reflect/src/list.rs b/crates/bevy_reflect/src/list.rs
@@ -163,8 +163,7 @@ impl List for DynamicList {
     }
 }
 
-// SAFE: any and any_mut both return self
-unsafe impl Reflect for DynamicList {
+impl Reflect for DynamicList {
     #[inline]
     fn type_name(&self) -> &str {
         self.name.as_str()
@@ -176,12 +175,17 @@ unsafe impl Reflect for DynamicList {
     }
 
     #[inline]
-    fn any(&self) -> &dyn Any {
+    fn into_any(self: Box<Self>) -> Box<dyn Any> {
         self
     }
 
     #[inline]
-    fn any_mut(&mut self) -> &mut dyn Any {
+    fn as_any(&self) -> &dyn Any {
+        self
+    }
+
+    #[inline]
+    fn as_any_mut(&mut self) -> &mut dyn Any {
         self
     }