bevy_reflect: Deprecate PartialReflect::clone_value (#18284)

# Objective

#13432 added proper reflection-based cloning. This is a better method
than cloning via `clone_value` for reasons detailed in the description
of that PR. However, it may not be immediately apparent to users why one
should be used over the other, and what the gotchas of `clone_value`
are.

## Solution

This PR marks `PartialReflect::clone_value` as deprecated, with the
deprecation notice pointing users to `PartialReflect::reflect_clone`.
However, it also suggests using a new method introduced in this PR:
`PartialReflect::to_dynamic`.

`PartialReflect::to_dynamic` is essentially a renaming of
`PartialReflect::clone_value`. By naming it `to_dynamic`, we make it
very obvious that what's returned is a dynamic type. The one caveat to
this is that opaque types still use `reflect_clone` as they have no
corresponding dynamic type.

Along with changing the name, the method is now optional, and comes with
a default implementation that calls out to the respective reflection
subtrait method. This was done because there was really no reason to
require manual implementors provide a method that almost always calls
out to a known set of methods.

Lastly, to make this default implementation work, this PR also did a
similar thing with the `clone_dynamic ` methods on the reflection
subtraits. For example, `Struct::clone_dynamic` has been marked
deprecated and is superseded by `Struct::to_dynamic_struct`. This was
necessary to avoid the "multiple names in scope" issue.

### Open Questions

This PR maintains the original signature of `clone_value` on
`to_dynamic`. That is, it takes `&self` and returns `Box<dyn
PartialReflect>`.

However, in order for this to work, it introduces a panic if the value
is opaque and doesn't override the default `reflect_clone`
implementation.

One thing we could do to avoid the panic would be to make the conversion
fallible, either returning `Option<Box<dyn PartialReflect>>` or
`Result<Box<dyn PartialReflect>, ReflectCloneError>`.

This makes using the method a little more involved (i.e. users have to
either unwrap or handle the rare possibility of an error), but it would
set us up for a world where opaque types don't strictly need to be
`Clone`. Right now this bound is sort of implied by the fact that
`clone_value` is a required trait method, and the default behavior of
the macro is to use `Clone` for opaque types.

Alternatively, we could keep the signature but make the method required.
This maintains that implied bound where manual implementors must provide
some way of cloning the value (or YOLO it and just panic), but also
makes the API simpler to use.

Finally, we could just leave it with the panic. It's unlikely this would
occur in practice since our macro still requires `Clone` for opaque
types, and thus this would only ever be an issue if someone were to
manually implement `PartialReflect` without a valid `to_dynamic` or
`reflect_clone` method.

## Testing

You can test locally using the following command:

```
cargo test --package bevy_reflect --all-features
```

---

## Migration Guide

`PartialReflect::clone_value` is being deprecated. Instead, use
`PartialReflect::to_dynamic` if wanting to create a new dynamic instance
of the reflected value. Alternatively, use
`PartialReflect::reflect_clone` to attempt to create a true clone of the
underlying value.

Similarly, the following methods have been deprecated and should be
replaced with these alternatives:
- `Array::clone_dynamic` → `Array::to_dynamic_array`
- `Enum::clone_dynamic` → `Enum::to_dynamic_enum`
- `List::clone_dynamic` → `List::to_dynamic_list`
- `Map::clone_dynamic` → `Map::to_dynamic_map`
- `Set::clone_dynamic` → `Set::to_dynamic_set`
- `Struct::clone_dynamic` → `Struct::to_dynamic_struct`
- `Tuple::clone_dynamic` → `Tuple::to_dynamic_tuple`
- `TupleStruct::clone_dynamic` → `TupleStruct::to_dynamic_tuple_struct`

Author: MrGVSV

benches/benches/bevy_reflect/list.rs

@@ -10,7 +10,7 @@ use criterion::{
 criterion_group!(
     benches,
     concrete_list_apply,
-    concrete_list_clone_dynamic,
+    concrete_list_to_dynamic_list,
     dynamic_list_apply,
     dynamic_list_push
 );
@@ -81,20 +81,20 @@ fn concrete_list_apply(criterion: &mut Criterion) {
     list_apply(&mut group, "empty_base_concrete_patch", empty_base, patch);
 
     list_apply(&mut group, "empty_base_dynamic_patch", empty_base, |size| {
-        patch(size).clone_dynamic()
+        patch(size).to_dynamic_list()
     });
 
     list_apply(&mut group, "same_len_concrete_patch", full_base, patch);
 
     list_apply(&mut group, "same_len_dynamic_patch", full_base, |size| {
-        patch(size).clone_dynamic()
+        patch(size).to_dynamic_list()
     });
 
     group.finish();
 }
 
-fn concrete_list_clone_dynamic(criterion: &mut Criterion) {
-    let mut group = create_group(criterion, bench!("concrete_list_clone_dynamic"));
+fn concrete_list_to_dynamic_list(criterion: &mut Criterion) {
+    let mut group = create_group(criterion, bench!("concrete_list_to_dynamic_list"));
 
     for size in SIZES {
         group.throughput(Throughput::Elements(size as u64));
@@ -105,7 +105,7 @@ fn concrete_list_clone_dynamic(criterion: &mut Criterion) {
             |bencher, &size| {
                 let v = iter::repeat_n(0, size).collect::<Vec<_>>();
 
-                bencher.iter(|| black_box(&v).clone_dynamic());
+                bencher.iter(|| black_box(&v).to_dynamic_list());
             },
         );
     }
@@ -127,7 +127,7 @@ fn dynamic_list_push(criterion: &mut Criterion) {
                 let dst = DynamicList::default();
 
                 bencher.iter_batched(
-                    || (src.clone(), dst.clone_dynamic()),
+                    || (src.clone(), dst.to_dynamic_list()),
                     |(src, mut dst)| {
                         for item in src {
                             dst.push(item);
@@ -145,20 +145,20 @@ fn dynamic_list_push(criterion: &mut Criterion) {
 fn dynamic_list_apply(criterion: &mut Criterion) {
     let mut group = create_group(criterion, bench!("dynamic_list_apply"));
 
-    let empty_base = |_: usize| || Vec::<u64>::new().clone_dynamic();
+    let empty_base = |_: usize| || Vec::<u64>::new().to_dynamic_list();
     let full_base = |size: usize| move || iter::repeat_n(0, size).collect::<Vec<u64>>();
     let patch = |size: usize| iter::repeat_n(1, size).collect::<Vec<u64>>();
 
     list_apply(&mut group, "empty_base_concrete_patch", empty_base, patch);
 
     list_apply(&mut group, "empty_base_dynamic_patch", empty_base, |size| {
-        patch(size).clone_dynamic()
+        patch(size).to_dynamic_list()
     });
 
     list_apply(&mut group, "same_len_concrete_patch", full_base, patch);
 
     list_apply(&mut group, "same_len_dynamic_patch", full_base, |size| {
-        patch(size).clone_dynamic()
+        patch(size).to_dynamic_list()
     });
 
     group.finish();

benches/benches/bevy_reflect/map.rs

@@ -108,7 +108,7 @@ fn concrete_map_apply(criterion: &mut Criterion) {
     );
 
     map_apply(&mut group, "empty_base_dynamic_patch", empty_base, |size| {
-        key_range_patch(size).clone_dynamic()
+        key_range_patch(size).to_dynamic_map()
     });
 
     map_apply(
@@ -122,7 +122,7 @@ fn concrete_map_apply(criterion: &mut Criterion) {
         &mut group,
         "same_keys_dynamic_patch",
         key_range_base,
-        |size| key_range_patch(size).clone_dynamic(),
+        |size| key_range_patch(size).to_dynamic_map(),
     );
 
     map_apply(
@@ -136,7 +136,7 @@ fn concrete_map_apply(criterion: &mut Criterion) {
         &mut group,
         "disjoint_keys_dynamic_patch",
         key_range_base,
-        |size| disjoint_patch(size).clone_dynamic(),
+        |size| disjoint_patch(size).to_dynamic_map(),
     );
 }
 
@@ -159,7 +159,7 @@ fn dynamic_map_apply(criterion: &mut Criterion) {
             (0..size as u64)
                 .zip(iter::repeat(0))
                 .collect::<HashMap<u64, u64>>()
-                .clone_dynamic()
+                .to_dynamic_map()
         }
     };
 
@@ -183,7 +183,7 @@ fn dynamic_map_apply(criterion: &mut Criterion) {
     );
 
     map_apply(&mut group, "empty_base_dynamic_patch", empty_base, |size| {
-        key_range_patch(size).clone_dynamic()
+        key_range_patch(size).to_dynamic_map()
     });
 
     map_apply(
@@ -197,7 +197,7 @@ fn dynamic_map_apply(criterion: &mut Criterion) {
         &mut group,
         "same_keys_dynamic_patch",
         key_range_base,
-        |size| key_range_patch(size).clone_dynamic(),
+        |size| key_range_patch(size).to_dynamic_map(),
     );
 
     map_apply(
@@ -211,7 +211,7 @@ fn dynamic_map_apply(criterion: &mut Criterion) {
         &mut group,
         "disjoint_keys_dynamic_patch",
         key_range_base,
-        |size| disjoint_patch(size).clone_dynamic(),
+        |size| disjoint_patch(size).to_dynamic_map(),
     );
 }
 

benches/benches/bevy_reflect/struct.rs

@@ -12,8 +12,8 @@ criterion_group!(
     concrete_struct_apply,
     concrete_struct_field,
     concrete_struct_type_info,
-    concrete_struct_clone,
-    dynamic_struct_clone,
+    concrete_struct_to_dynamic_struct,
+    dynamic_struct_to_dynamic_struct,
     dynamic_struct_apply,
     dynamic_struct_get_field,
     dynamic_struct_insert,
@@ -113,7 +113,7 @@ fn concrete_struct_apply(criterion: &mut Criterion) {
                 bencher.iter_batched(
                     || {
                         let (obj, _) = input();
-                        let patch = obj.clone_dynamic();
+                        let patch = obj.to_dynamic_struct();
                         (obj, patch)
                     },
                     |(mut obj, patch)| obj.apply(black_box(&patch)),
@@ -170,8 +170,8 @@ fn concrete_struct_type_info(criterion: &mut Criterion) {
     }
 }
 
-fn concrete_struct_clone(criterion: &mut Criterion) {
-    let mut group = create_group(criterion, bench!("concrete_struct_clone"));
+fn concrete_struct_to_dynamic_struct(criterion: &mut Criterion) {
+    let mut group = create_group(criterion, bench!("concrete_struct_to_dynamic_struct"));
 
     let structs: [(Box<dyn Struct>, Box<dyn Struct>); 5] = [
         (
@@ -203,28 +203,28 @@ fn concrete_struct_clone(criterion: &mut Criterion) {
             BenchmarkId::new("NonGeneric", field_count),
             &standard,
             |bencher, s| {
-                bencher.iter(|| s.clone_dynamic());
+                bencher.iter(|| s.to_dynamic_struct());
             },
         );
         group.bench_with_input(
             BenchmarkId::new("Generic", field_count),
             &generic,
             |bencher, s| {
-                bencher.iter(|| s.clone_dynamic());
+                bencher.iter(|| s.to_dynamic_struct());
             },
         );
     }
 }
 
-fn dynamic_struct_clone(criterion: &mut Criterion) {
-    let mut group = create_group(criterion, bench!("dynamic_struct_clone"));
+fn dynamic_struct_to_dynamic_struct(criterion: &mut Criterion) {
+    let mut group = create_group(criterion, bench!("dynamic_struct_to_dynamic_struct"));
 
     let structs: [Box<dyn Struct>; 5] = [
-        Box::new(Struct1::default().clone_dynamic()),
-        Box::new(Struct16::default().clone_dynamic()),
-        Box::new(Struct32::default().clone_dynamic()),
-        Box::new(Struct64::default().clone_dynamic()),
-        Box::new(Struct128::default().clone_dynamic()),
+        Box::new(Struct1::default().to_dynamic_struct()),
+        Box::new(Struct16::default().to_dynamic_struct()),
+        Box::new(Struct32::default().to_dynamic_struct()),
+        Box::new(Struct64::default().to_dynamic_struct()),
+        Box::new(Struct128::default().to_dynamic_struct()),
     ];
 
     for s in structs {
@@ -234,7 +234,7 @@ fn dynamic_struct_clone(criterion: &mut Criterion) {
             BenchmarkId::from_parameter(field_count),
             &s,
             |bencher, s| {
-                bencher.iter(|| s.clone_dynamic());
+                bencher.iter(|| s.to_dynamic_struct());
             },
         );
     }
@@ -265,7 +265,7 @@ fn dynamic_struct_apply(criterion: &mut Criterion) {
             &patch,
             |bencher, patch| {
                 bencher.iter_batched(
-                    || (base.clone_dynamic(), patch()),
+                    || (base.to_dynamic_struct(), patch()),
                     |(mut base, patch)| base.apply(black_box(&*patch)),
                     BatchSize::SmallInput,
                 );
@@ -289,7 +289,7 @@ fn dynamic_struct_apply(criterion: &mut Criterion) {
                 }
 
                 bencher.iter_batched(
-                    || base.clone_dynamic(),
+                    || base.to_dynamic_struct(),
                     |mut base| base.apply(black_box(&patch)),
                     BatchSize::SmallInput,
                 );
@@ -315,7 +315,7 @@ fn dynamic_struct_insert(criterion: &mut Criterion) {
 
                 let field = format!("field_{}", field_count);
                 bencher.iter_batched(
-                    || s.clone_dynamic(),
+                    || s.to_dynamic_struct(),
                     |mut s| {
                         s.insert(black_box(&field), ());
                     },

crates/bevy_asset/src/handle.rs

@@ -658,7 +658,7 @@ mod tests {
         assert_eq!(UntypedHandle::from(typed.clone()), untyped);
     }
 
-    /// `Reflect::clone_value` should increase the strong count of a strong handle
+    /// `PartialReflect::reflect_clone`/`PartialReflect::to_dynamic` should increase the strong count of a strong handle
     #[test]
     fn strong_handle_reflect_clone() {
         use crate::{AssetApp, AssetPlugin, Assets, VisitAssetDependencies};
@@ -689,18 +689,26 @@ mod tests {
                 );
 
                 let reflected: &dyn Reflect = &handle;
-                let cloned_handle: Box<dyn PartialReflect> = reflected.clone_value();
+                let _cloned_handle: Box<dyn Reflect> = reflected.reflect_clone().unwrap();
 
                 assert_eq!(
                     Arc::strong_count(strong),
                     2,
                     "Cloning the handle with reflect should increase the strong count to 2"
                 );
 
+                let dynamic_handle: Box<dyn PartialReflect> = reflected.to_dynamic();
+
+                assert_eq!(
+                    Arc::strong_count(strong),
+                    3,
+                    "Converting the handle to a dynamic should increase the strong count to 3"
+                );
+
                 let from_reflect_handle: Handle<MyAsset> =
-                    FromReflect::from_reflect(&*cloned_handle).unwrap();
+                    FromReflect::from_reflect(&*dynamic_handle).unwrap();
 
-                assert_eq!(Arc::strong_count(strong), 3, "Converting the reflected value back to a handle should increase the strong count to 3");
+                assert_eq!(Arc::strong_count(strong), 4, "Converting the reflected value back to a handle should increase the strong count to 4");
                 assert!(
                     from_reflect_handle.is_strong(),
                     "The cloned handle should still be strong"

crates/bevy_ecs/src/component.rs

@@ -2911,12 +2911,14 @@ pub fn component_clone_via_clone<C: Clone + Component>(
 /// - Component has [`TypeId`]
 /// - Component is registered
 /// - Component has [`ReflectFromPtr`](bevy_reflect::ReflectFromPtr) registered
-/// - Component has one of the following registered: [`ReflectFromReflect`](bevy_reflect::ReflectFromReflect),
+/// - Component can be cloned via [`PartialReflect::reflect_clone`] _or_ has one of the following registered: [`ReflectFromReflect`](bevy_reflect::ReflectFromReflect),
 ///   [`ReflectDefault`](bevy_reflect::std_traits::ReflectDefault), [`ReflectFromWorld`](crate::reflect::ReflectFromWorld)
 ///
 /// If any of the conditions is not satisfied, the component will be skipped.
 ///
 /// See [`EntityClonerBuilder`](crate::entity::EntityClonerBuilder) for details.
+///
+/// [`PartialReflect::reflect_clone`]: bevy_reflect::PartialReflect::reflect_clone
 #[cfg(feature = "bevy_reflect")]
 pub fn component_clone_via_reflect(
     commands: &mut Commands,
@@ -2934,6 +2936,21 @@ pub fn component_clone_via_reflect(
     // checked in read_source_component_reflect
     let type_id = component_info.type_id().unwrap();
 
+    // Try to clone using `reflect_clone`
+    if let Ok(mut component) = source_component_reflect.reflect_clone() {
+        if let Some(reflect_component) =
+            registry.get_type_data::<crate::reflect::ReflectComponent>(type_id)
+        {
+            reflect_component.visit_entities_mut(&mut *component, &mut |entity| {
+                *entity = ctx.entity_mapper().get_mapped(*entity);
+            });
+        }
+        drop(registry);
+
+        ctx.write_target_component_reflect(component);
+        return;
+    }
+
     // Try to clone using ReflectFromReflect
     if let Some(reflect_from_reflect) =
         registry.get_type_data::<bevy_reflect::ReflectFromReflect>(type_id)
@@ -2977,7 +2994,7 @@ pub fn component_clone_via_reflect(
                 mapped_entities.push(entity);
             });
         }
-        let source_component_cloned = source_component_reflect.clone_value();
+        let source_component_cloned = source_component_reflect.to_dynamic();
         let component_layout = component_info.layout();
         let target = ctx.target();
         let component_id = ctx.component_id();
@@ -3008,7 +3025,11 @@ pub fn component_clone_via_reflect(
                 world
                     .entity_mut(target)
                     .insert_by_id(component_id, OwningPtr::new(raw_component_ptr));
-                alloc::alloc::dealloc(raw_component_ptr.as_ptr(), component_layout);
+
+                if component_layout.size() > 0 {
+                    // Ensure we don't attempt to deallocate zero-sized components
+                    alloc::alloc::dealloc(raw_component_ptr.as_ptr(), component_layout);
+                }
             }
         });
     }