Make Reflect safe to implement (#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 <42193387+PROMETHIA-27@users.noreply.github.com>
This commit is contained in:
PROMETHIA-27 2022-06-27 16:52:25 +00:00
parent 332cfa1b3a
commit c27a3cff6d
15 changed files with 206 additions and 96 deletions

View file

@ -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 {

View file

@ -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())
}
}
})

View file

@ -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 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
}
@ -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 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
}
@ -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 {

View file

@ -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
}

View file

@ -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
}

View file

@ -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 any_mut(&mut self) -> &mut dyn Any {
fn as_any(&self) -> &dyn Any {
self
}
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(),
)
}
}

View file

@ -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)]

View file

@ -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
}

View file

@ -212,8 +212,7 @@ impl Map for DynamicMap {
}
}
// SAFE: any and any_mut both return self
unsafe impl Reflect for DynamicMap {
impl Reflect for DynamicMap {
fn type_name(&self) -> &str {
&self.name
}
@ -223,11 +222,15 @@ unsafe impl Reflect for DynamicMap {
<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
}

View file

@ -2,7 +2,10 @@ use crate::{
array_debug, list_debug, map_debug, serde::Serializable, struct_debug, tuple_debug,
tuple_struct_debug, Array, List, Map, Struct, Tuple, TupleStruct, TypeInfo, Typed, ValueInfo,
};
use std::{any::Any, fmt::Debug};
use std::{
any::{self, Any, TypeId},
fmt::Debug,
};
use crate::utility::NonGenericTypeInfoCell;
pub use bevy_utils::AHasher as ReflectHasher;
@ -46,15 +49,8 @@ pub enum ReflectMut<'a> {
///
/// When using `#[derive(Reflect)]` with a struct or tuple struct, the suitable subtrait for that
/// type (`Struct` or `TupleStruct`) is derived automatically.
///
/// # Safety
/// Implementors _must_ ensure that [`Reflect::any`] and [`Reflect::any_mut`] both return the `self`
/// value passed in. If this is not done, [`Reflect::downcast`](trait.Reflect.html#method.downcast)
/// will be UB (and also just logically broken).
pub unsafe trait Reflect: Any + Send + Sync {
/// Returns the [type name] of the underlying type.
///
/// [type name]: std::any::type_name
pub trait Reflect: Any + Send + Sync {
/// Returns the [type name][std::any::type_name] of the underlying type.
fn type_name(&self) -> &str;
/// Returns the [`TypeInfo`] of the underlying type.
@ -67,11 +63,14 @@ pub unsafe trait Reflect: Any + Send + Sync {
/// [`TypeRegistry::get_type_info`]: crate::TypeRegistry::get_type_info
fn get_type_info(&self) -> &'static TypeInfo;
/// Returns the value as a [`Box<dyn Any>`][std::any::Any].
fn into_any(self: Box<Self>) -> Box<dyn Any>;
/// Returns the value as a [`&dyn Any`][std::any::Any].
fn any(&self) -> &dyn Any;
fn as_any(&self) -> &dyn Any;
/// Returns the value as a [`&mut dyn Any`][std::any::Any].
fn any_mut(&mut self) -> &mut dyn Any;
fn as_any_mut(&mut self) -> &mut dyn Any;
/// Casts this type to a reflected value
fn as_reflect(&self) -> &dyn Reflect;
@ -210,18 +209,14 @@ impl Typed for dyn Reflect {
}
}
#[deny(rustdoc::broken_intra_doc_links)]
impl dyn Reflect {
/// Downcasts the value to type `T`, consuming the trait object.
///
/// If the underlying value is not of type `T`, returns `Err(self)`.
pub fn downcast<T: Reflect>(self: Box<dyn Reflect>) -> Result<Box<T>, Box<dyn Reflect>> {
// SAFE?: Same approach used by std::any::Box::downcast. ReflectValue is always Any and type
// has been checked.
if self.is::<T>() {
unsafe {
let raw: *mut dyn Reflect = Box::into_raw(self);
Ok(Box::from_raw(raw as *mut T))
}
Ok(self.into_any().downcast().unwrap())
} else {
Err(self)
}
@ -234,11 +229,26 @@ impl dyn Reflect {
self.downcast::<T>().map(|value| *value)
}
/// Returns `true` if the underlying value represents a value of type `T`, or `false`
/// otherwise.
///
/// Read `is` for more information on underlying values and represented types.
#[inline]
pub fn represents<T: Reflect>(&self) -> bool {
self.type_name() == any::type_name::<T>()
}
/// Returns `true` if the underlying value is of type `T`, or `false`
/// otherwise.
///
/// The underlying value is the concrete type that is stored in this `dyn` object;
/// it can be downcasted to. In the case that this underlying value "represents"
/// a different type, like the Dynamic\*\*\* types do, you can call `represents`
/// to determine what type they represent. Represented types cannot be downcasted
/// to, but you can use [`FromReflect`] to create a value of the represented type from them.
#[inline]
pub fn is<T: Reflect>(&self) -> bool {
self.any().is::<T>()
self.type_id() == TypeId::of::<T>()
}
/// Downcasts the value to type `T` by reference.
@ -246,7 +256,7 @@ impl dyn Reflect {
/// If the underlying value is not of type `T`, returns `None`.
#[inline]
pub fn downcast_ref<T: Reflect>(&self) -> Option<&T> {
self.any().downcast_ref::<T>()
self.as_any().downcast_ref::<T>()
}
/// Downcasts the value to type `T` by mutable reference.
@ -254,6 +264,6 @@ impl dyn Reflect {
/// If the underlying value is not of type `T`, returns `None`.
#[inline]
pub fn downcast_mut<T: Reflect>(&mut self) -> Option<&mut T> {
self.any_mut().downcast_mut::<T>()
self.as_any_mut().downcast_mut::<T>()
}
}

View file

@ -337,8 +337,7 @@ impl Struct for DynamicStruct {
}
}
// SAFE: any and any_mut both return self
unsafe impl Reflect for DynamicStruct {
impl Reflect for DynamicStruct {
#[inline]
fn type_name(&self) -> &str {
&self.name
@ -350,12 +349,17 @@ unsafe impl Reflect for DynamicStruct {
}
#[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
}

View file

@ -267,8 +267,7 @@ impl Tuple for DynamicTuple {
}
}
// SAFE: any and any_mut both return self
unsafe impl Reflect for DynamicTuple {
impl Reflect for DynamicTuple {
#[inline]
fn type_name(&self) -> &str {
self.name()
@ -280,12 +279,17 @@ unsafe impl Reflect for DynamicTuple {
}
#[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
}
@ -460,8 +464,7 @@ macro_rules! impl_reflect_tuple {
}
}
// SAFE: any and any_mut both return self
unsafe impl<$($name: Reflect),*> Reflect for ($($name,)*) {
impl<$($name: Reflect),*> Reflect for ($($name,)*) {
fn type_name(&self) -> &str {
std::any::type_name::<Self>()
}
@ -470,11 +473,15 @@ macro_rules! impl_reflect_tuple {
<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
}

View file

@ -251,8 +251,7 @@ impl TupleStruct for DynamicTupleStruct {
}
}
// SAFE: any and any_mut both return self
unsafe impl Reflect for DynamicTupleStruct {
impl Reflect for DynamicTupleStruct {
#[inline]
fn type_name(&self) -> &str {
self.name.as_str()
@ -264,12 +263,17 @@ unsafe impl Reflect for DynamicTupleStruct {
}
#[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
}

View file

@ -45,11 +45,12 @@ use std::any::{Any, TypeId};
/// }
///
/// #
/// # unsafe impl Reflect for MyStruct {
/// # impl Reflect for MyStruct {
/// # fn type_name(&self) -> &str { todo!() }
/// # fn get_type_info(&self) -> &'static TypeInfo { todo!() }
/// # fn any(&self) -> &dyn Any { todo!() }
/// # fn any_mut(&mut self) -> &mut dyn Any { todo!() }
/// # fn into_any(self: Box<Self>) -> Box<dyn Any> { todo!() }
/// # fn as_any(&self) -> &dyn Any { todo!() }
/// # fn as_any_mut(&mut self) -> &mut dyn Any { todo!() }
/// # fn as_reflect(&self) -> &dyn Reflect { todo!() }
/// # fn as_reflect_mut(&mut self) -> &mut dyn Reflect { todo!() }
/// # fn apply(&mut self, value: &dyn Reflect) { todo!() }

View file

@ -34,11 +34,12 @@ use std::any::{Any, TypeId};
/// }
/// }
/// #
/// # unsafe impl Reflect for Foo {
/// # impl Reflect for Foo {
/// # fn type_name(&self) -> &str { todo!() }
/// # fn get_type_info(&self) -> &'static TypeInfo { todo!() }
/// # fn any(&self) -> &dyn Any { todo!() }
/// # fn any_mut(&mut self) -> &mut dyn Any { todo!() }
/// # fn into_any(self: Box<Self>) -> Box<dyn Any> { todo!() }
/// # fn as_any(&self) -> &dyn Any { todo!() }
/// # fn as_any_mut(&mut self) -> &mut dyn Any { todo!() }
/// # fn as_reflect(&self) -> &dyn Reflect { todo!() }
/// # fn as_reflect_mut(&mut self) -> &mut dyn Reflect { todo!() }
/// # fn apply(&mut self, value: &dyn Reflect) { todo!() }
@ -94,11 +95,12 @@ impl NonGenericTypeInfoCell {
/// }
/// }
/// #
/// # unsafe impl<T: Reflect> Reflect for Foo<T> {
/// # impl<T: Reflect> Reflect for Foo<T> {
/// # fn type_name(&self) -> &str { todo!() }
/// # fn get_type_info(&self) -> &'static TypeInfo { todo!() }
/// # fn any(&self) -> &dyn Any { todo!() }
/// # fn any_mut(&mut self) -> &mut dyn Any { todo!() }
/// # fn into_any(self: Box<Self>) -> Box<dyn Any> { todo!() }
/// # fn as_any(&self) -> &dyn Any { todo!() }
/// # fn as_any_mut(&mut self) -> &mut dyn Any { todo!() }
/// # fn as_reflect(&self) -> &dyn Reflect { todo!() }
/// # fn as_reflect_mut(&mut self) -> &mut dyn Reflect { todo!() }
/// # fn apply(&mut self, value: &dyn Reflect) { todo!() }