Skip to content

Commit 52ff80c

Browse files
PROMETHIA-27james7132
PROMETHIA-27
authored and
james7132
committed
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 <42193387+PROMETHIA-27@users.noreply.github.com>
1 parent cfd5389 commit 52ff80c

File tree

15 files changed

+206
-96
lines changed

15 files changed

+206
-96
lines changed

‎crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,7 @@ impl ReflectTraits {
208208
match &self.partial_eq {
209209
TraitImpl::Implemented => Some(quote! {
210210
fn reflect_partial_eq(&self, value: &dyn #bevy_reflect_path::Reflect) -> Option<bool> {
211-
let value = value.any();
211+
let value = value.as_any();
212212
if let Some(value) = value.downcast_ref::<Self>() {
213213
Some(std::cmp::PartialEq::eq(self, value))
214214
} else {

‎crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ pub(crate) fn impl_value(
2626
TokenStream::from(quote! {
2727
impl #impl_generics #bevy_reflect_path::FromReflect for #type_name #ty_generics #where_clause {
2828
fn from_reflect(reflect: &dyn #bevy_reflect_path::Reflect) -> Option<Self> {
29-
Some(reflect.any().downcast_ref::<#type_name #ty_generics>()?.clone())
29+
Some(reflect.as_any().downcast_ref::<#type_name #ty_generics>()?.clone())
3030
}
3131
}
3232
})

‎crates/bevy_reflect/bevy_reflect_derive/src/impls.rs

+27-13
Original file line numberDiff line numberDiff line change
@@ -125,8 +125,7 @@ pub(crate) fn impl_struct(derive_data: &ReflectDeriveData) -> TokenStream {
125125
}
126126
}
127127

128-
// SAFE: any and any_mut both return self
129-
unsafe impl #impl_generics #bevy_reflect_path::Reflect for #struct_name #ty_generics #where_clause {
128+
impl #impl_generics #bevy_reflect_path::Reflect for #struct_name #ty_generics #where_clause {
130129
#[inline]
131130
fn type_name(&self) -> &str {
132131
std::any::type_name::<Self>()
@@ -138,11 +137,17 @@ pub(crate) fn impl_struct(derive_data: &ReflectDeriveData) -> TokenStream {
138137
}
139138

140139
#[inline]
141-
fn any(&self) -> &dyn std::any::Any {
140+
fn into_any(self: Box<Self>) -> Box<dyn std::any::Any> {
142141
self
143142
}
143+
144144
#[inline]
145-
fn any_mut(&mut self) -> &mut dyn std::any::Any {
145+
fn as_any(&self) -> &dyn std::any::Any {
146+
self
147+
}
148+
149+
#[inline]
150+
fn as_any_mut(&mut self) -> &mut dyn std::any::Any {
146151
self
147152
}
148153

@@ -275,8 +280,7 @@ pub(crate) fn impl_tuple_struct(derive_data: &ReflectDeriveData) -> TokenStream
275280
}
276281
}
277282

278-
// SAFE: any and any_mut both return self
279-
unsafe impl #impl_generics #bevy_reflect_path::Reflect for #struct_name #ty_generics #where_clause {
283+
impl #impl_generics #bevy_reflect_path::Reflect for #struct_name #ty_generics #where_clause {
280284
#[inline]
281285
fn type_name(&self) -> &str {
282286
std::any::type_name::<Self>()
@@ -288,11 +292,17 @@ pub(crate) fn impl_tuple_struct(derive_data: &ReflectDeriveData) -> TokenStream
288292
}
289293

290294
#[inline]
291-
fn any(&self) -> &dyn std::any::Any {
295+
fn into_any(self: Box<Self>) -> Box<dyn std::any::Any> {
296+
self
297+
}
298+
299+
#[inline]
300+
fn as_any(&self) -> &dyn std::any::Any {
292301
self
293302
}
303+
294304
#[inline]
295-
fn any_mut(&mut self) -> &mut dyn std::any::Any {
305+
fn as_any_mut(&mut self) -> &mut dyn std::any::Any {
296306
self
297307
}
298308

@@ -372,8 +382,7 @@ pub(crate) fn impl_value(
372382

373383
#typed_impl
374384

375-
// SAFE: any and any_mut both return self
376-
unsafe impl #impl_generics #bevy_reflect_path::Reflect for #type_name #ty_generics #where_clause {
385+
impl #impl_generics #bevy_reflect_path::Reflect for #type_name #ty_generics #where_clause {
377386
#[inline]
378387
fn type_name(&self) -> &str {
379388
std::any::type_name::<Self>()
@@ -385,12 +394,17 @@ pub(crate) fn impl_value(
385394
}
386395

387396
#[inline]
388-
fn any(&self) -> &dyn std::any::Any {
397+
fn into_any(self: Box<Self>) -> Box<dyn std::any::Any> {
398+
self
399+
}
400+
401+
#[inline]
402+
fn as_any(&self) -> &dyn std::any::Any {
389403
self
390404
}
391405

392406
#[inline]
393-
fn any_mut(&mut self) -> &mut dyn std::any::Any {
407+
fn as_any_mut(&mut self) -> &mut dyn std::any::Any {
394408
self
395409
}
396410

@@ -411,7 +425,7 @@ pub(crate) fn impl_value(
411425

412426
#[inline]
413427
fn apply(&mut self, value: &dyn #bevy_reflect_path::Reflect) {
414-
let value = value.any();
428+
let value = value.as_any();
415429
if let Some(value) = value.downcast_ref::<Self>() {
416430
*self = value.clone();
417431
} else {

‎crates/bevy_reflect/src/array.rs

+8-4
Original file line numberDiff line numberDiff line change
@@ -151,8 +151,7 @@ impl DynamicArray {
151151
}
152152
}
153153

154-
// SAFE: any and any_mut both return self
155-
unsafe impl Reflect for DynamicArray {
154+
impl Reflect for DynamicArray {
156155
#[inline]
157156
fn type_name(&self) -> &str {
158157
self.name.as_str()
@@ -164,12 +163,17 @@ unsafe impl Reflect for DynamicArray {
164163
}
165164

166165
#[inline]
167-
fn any(&self) -> &dyn Any {
166+
fn into_any(self: Box<Self>) -> Box<dyn Any> {
168167
self
169168
}
170169

171170
#[inline]
172-
fn any_mut(&mut self) -> &mut dyn Any {
171+
fn as_any(&self) -> &dyn Any {
172+
self
173+
}
174+
175+
#[inline]
176+
fn as_any_mut(&mut self) -> &mut dyn Any {
173177
self
174178
}
175179

‎crates/bevy_reflect/src/impls/smallvec.rs

+7-4
Original file line numberDiff line numberDiff line change
@@ -55,8 +55,7 @@ where
5555
}
5656
}
5757

58-
// SAFE: any and any_mut both return self
59-
unsafe impl<T: smallvec::Array + Send + Sync + 'static> Reflect for SmallVec<T>
58+
impl<T: smallvec::Array + Send + Sync + 'static> Reflect for SmallVec<T>
6059
where
6160
T::Item: FromReflect + Clone,
6261
{
@@ -68,11 +67,15 @@ where
6867
<Self as Typed>::type_info()
6968
}
7069

71-
fn any(&self) -> &dyn Any {
70+
fn into_any(self: Box<Self>) -> Box<dyn Any> {
7271
self
7372
}
7473

75-
fn any_mut(&mut self) -> &mut dyn Any {
74+
fn as_any(&self) -> &dyn Any {
75+
self
76+
}
77+
78+
fn as_any_mut(&mut self) -> &mut dyn Any {
7679
self
7780
}
7881

‎crates/bevy_reflect/src/impls/std.rs

+37-19
Original file line numberDiff line numberDiff line change
@@ -105,8 +105,7 @@ impl<T: FromReflect> List for Vec<T> {
105105
}
106106
}
107107

108-
// SAFE: any and any_mut both return self
109-
unsafe impl<T: FromReflect> Reflect for Vec<T> {
108+
impl<T: FromReflect> Reflect for Vec<T> {
110109
fn type_name(&self) -> &str {
111110
std::any::type_name::<Self>()
112111
}
@@ -115,11 +114,15 @@ unsafe impl<T: FromReflect> Reflect for Vec<T> {
115114
<Self as Typed>::type_info()
116115
}
117116

118-
fn any(&self) -> &dyn Any {
117+
fn into_any(self: Box<Self>) -> Box<dyn Any> {
119118
self
120119
}
121120

122-
fn any_mut(&mut self) -> &mut dyn Any {
121+
fn as_any(&self) -> &dyn Any {
122+
self
123+
}
124+
125+
fn as_any_mut(&mut self) -> &mut dyn Any {
123126
self
124127
}
125128

@@ -230,8 +233,7 @@ impl<K: Reflect + Eq + Hash, V: Reflect> Map for HashMap<K, V> {
230233
}
231234
}
232235

233-
// SAFE: any and any_mut both return self
234-
unsafe impl<K: Reflect + Eq + Hash, V: Reflect> Reflect for HashMap<K, V> {
236+
impl<K: Reflect + Eq + Hash, V: Reflect> Reflect for HashMap<K, V> {
235237
fn type_name(&self) -> &str {
236238
std::any::type_name::<Self>()
237239
}
@@ -240,11 +242,15 @@ unsafe impl<K: Reflect + Eq + Hash, V: Reflect> Reflect for HashMap<K, V> {
240242
<Self as Typed>::type_info()
241243
}
242244

243-
fn any(&self) -> &dyn Any {
245+
fn into_any(self: Box<Self>) -> Box<dyn Any> {
244246
self
245247
}
246248

247-
fn any_mut(&mut self) -> &mut dyn Any {
249+
fn as_any(&self) -> &dyn Any {
250+
self
251+
}
252+
253+
fn as_any_mut(&mut self) -> &mut dyn Any {
248254
self
249255
}
250256

@@ -350,8 +356,7 @@ impl<T: Reflect, const N: usize> Array for [T; N] {
350356
}
351357
}
352358

353-
// SAFE: any and any_mut both return self
354-
unsafe impl<T: Reflect, const N: usize> Reflect for [T; N] {
359+
impl<T: Reflect, const N: usize> Reflect for [T; N] {
355360
#[inline]
356361
fn type_name(&self) -> &str {
357362
std::any::type_name::<Self>()
@@ -362,12 +367,17 @@ unsafe impl<T: Reflect, const N: usize> Reflect for [T; N] {
362367
}
363368

364369
#[inline]
365-
fn any(&self) -> &dyn Any {
370+
fn into_any(self: Box<Self>) -> Box<dyn Any> {
366371
self
367372
}
368373

369374
#[inline]
370-
fn any_mut(&mut self) -> &mut dyn Any {
375+
fn as_any(&self) -> &dyn Any {
376+
self
377+
}
378+
379+
#[inline]
380+
fn as_any_mut(&mut self) -> &mut dyn Any {
371381
self
372382
}
373383

@@ -465,8 +475,7 @@ impl_array_get_type_registration! {
465475
30 31 32
466476
}
467477

468-
// SAFE: any and any_mut both return self
469-
unsafe impl Reflect for Cow<'static, str> {
478+
impl Reflect for Cow<'static, str> {
470479
fn type_name(&self) -> &str {
471480
std::any::type_name::<Self>()
472481
}
@@ -475,11 +484,15 @@ unsafe impl Reflect for Cow<'static, str> {
475484
<Self as Typed>::type_info()
476485
}
477486

478-
fn any(&self) -> &dyn Any {
487+
fn into_any(self: Box<Self>) -> Box<dyn Any> {
488+
self
489+
}
490+
491+
fn as_any(&self) -> &dyn Any {
479492
self
480493
}
481494

482-
fn any_mut(&mut self) -> &mut dyn Any {
495+
fn as_any_mut(&mut self) -> &mut dyn Any {
483496
self
484497
}
485498

@@ -492,7 +505,7 @@ unsafe impl Reflect for Cow<'static, str> {
492505
}
493506

494507
fn apply(&mut self, value: &dyn Reflect) {
495-
let value = value.any();
508+
let value = value.as_any();
496509
if let Some(value) = value.downcast_ref::<Self>() {
497510
*self = value.clone();
498511
} else {
@@ -525,7 +538,7 @@ unsafe impl Reflect for Cow<'static, str> {
525538
}
526539

527540
fn reflect_partial_eq(&self, value: &dyn Reflect) -> Option<bool> {
528-
let value = value.any();
541+
let value = value.as_any();
529542
if let Some(value) = value.downcast_ref::<Self>() {
530543
Some(std::cmp::PartialEq::eq(self, value))
531544
} else {
@@ -552,7 +565,12 @@ impl GetTypeRegistration for Cow<'static, str> {
552565

553566
impl FromReflect for Cow<'static, str> {
554567
fn from_reflect(reflect: &dyn crate::Reflect) -> Option<Self> {
555-
Some(reflect.any().downcast_ref::<Cow<'static, str>>()?.clone())
568+
Some(
569+
reflect
570+
.as_any()
571+
.downcast_ref::<Cow<'static, str>>()?
572+
.clone(),
573+
)
556574
}
557575
}
558576

‎crates/bevy_reflect/src/lib.rs

+36
Original file line numberDiff line numberDiff line change
@@ -485,6 +485,42 @@ mod tests {
485485
assert!(foo.reflect_partial_eq(&dynamic_struct).unwrap());
486486
}
487487

488+
#[test]
489+
fn reflect_downcast() {
490+
#[derive(Reflect, Clone, Debug, PartialEq)]
491+
struct Bar {
492+
y: u8,
493+
z: ::glam::Mat4,
494+
}
495+
496+
#[derive(Reflect, Clone, Debug, PartialEq)]
497+
struct Foo {
498+
x: i32,
499+
s: String,
500+
b: Bar,
501+
u: usize,
502+
t: (Vec3, String),
503+
}
504+
505+
let foo = Foo {
506+
x: 123,
507+
s: "String".to_string(),
508+
b: Bar {
509+
y: 255,
510+
z: ::glam::Mat4::from_cols_array(&[
511+
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,
512+
15.0,
513+
]),
514+
},
515+
u: 1111111111111,
516+
t: (Vec3::new(3.0, 2.0, 1.0), "Tuple String".to_string()),
517+
};
518+
519+
let foo2: Box<dyn Reflect> = Box::new(foo.clone());
520+
521+
assert_eq!(foo, *foo2.downcast::<Foo>().unwrap());
522+
}
523+
488524
#[test]
489525
fn reflect_take() {
490526
#[derive(Reflect, Debug, PartialEq)]

‎crates/bevy_reflect/src/list.rs

+8-4
Original file line numberDiff line numberDiff line change
@@ -163,8 +163,7 @@ impl List for DynamicList {
163163
}
164164
}
165165

166-
// SAFE: any and any_mut both return self
167-
unsafe impl Reflect for DynamicList {
166+
impl Reflect for DynamicList {
168167
#[inline]
169168
fn type_name(&self) -> &str {
170169
self.name.as_str()
@@ -176,12 +175,17 @@ unsafe impl Reflect for DynamicList {
176175
}
177176

178177
#[inline]
179-
fn any(&self) -> &dyn Any {
178+
fn into_any(self: Box<Self>) -> Box<dyn Any> {
180179
self
181180
}
182181

183182
#[inline]
184-
fn any_mut(&mut self) -> &mut dyn Any {
183+
fn as_any(&self) -> &dyn Any {
184+
self
185+
}
186+
187+
#[inline]
188+
fn as_any_mut(&mut self) -> &mut dyn Any {
185189
self
186190
}
187191

0 commit comments

Comments
 (0)