Reflection allows users to clone arbitrary reflected components, but Reflect does not have a Clone bound

[alice-i-cecile]

Problem

1. ReflectComponent::copy_component allows users to copy any arbitrary component data to different entities, and across worlds, as long as it is reflect-able.
2. Neither Reflect nor Component has a Clone bound, and so we can sneakily clone components that are not expecting to be cloned. I'm pretty sure this is ultimately done by just copying their raw memory.
3. Components are effectively cloned in a way that bypasses the Clone method, which can cause issues with things like ref-counting (used for Handle).

Possible Solutions

1. Give Reflect and/or Component a Clone bound. This fixes problem 2, but not problem 3, and may be painful in niche use cases (although I have not seen any concrete use cases for components that cannot be cloned raised, see Command to clone entities #1515).

Furthermore, not all fields of a component should be reflected. Cloning components with some cloneable field and defaulting the others is a reasonable strategy here, but does not permit a Clone bound.

2. Use the appropriate Clone methods when they exist.

3. Leave the existing behavior in place, and very clearly document what's going on and how to use it safely.

[DJMcNab]

I believe this copy would happen based on creating a new default, then reflecting the old onto the new, not be some extremely bad UB

[alice-i-cecile]

@mockersf points out that this Clone-bypassing behavior is not uniquely isolated to this reflection method.

If we implement both Serialize and Deserialize for a type (which we need to, for reflection to be useful to store scenes), we can (tediously) clone arbitrary structs, bypassing any limitations. I've documented this behavior in #3366 at least, and I suspect that's probably the best we can do.

This does however make me increasingly convinced that we should have either Reflect: Clone or Component: Clone, as uncloneable structs cannot be reasonably reflected, and almost all components should be both reflected and cloneable.

[Davier]

This does however make me increasingly convinced that we should have either Reflect: Clone or Component: Clone, as uncloneable structs cannot be reasonably reflected, and almost all components should be both reflected and cloneable.

FromWorld, which is a superset of Default, is required to use ReflectComponent:

bevy/crates/bevy_ecs/src/reflect.rs

Line 81 in ed9d45f

impl<C: Component + Reflect + FromWorld> FromType<C> for ReflectComponent {

bevy/crates/bevy_ecs/src/world/mod.rs

Lines 1171 to 1175 in ed9d45f

	impl<T: Default> FromWorld for T {
	fn from_world(_world: &mut World) -> Self {
	T::default()
	}
	}

[alice-i-cecile]

FromWorld, which is a superset of Default, is required to use ReflectComponent:

Yes, which is a nice feature (although I still like FromReflect from #1395). This should avoid unsoundness, but won't save us from logic errors around things like ref-counting. Handle for example could easily implement Default, but handles cloned in this fashion wouldn't keep assets alive in the proper feature.

As Clone is not a subtrait of Default, we could also have Default-able components that aren't clone-able at all, although it's again hard to imagine what those might be used for.

[alice-i-cecile]

Yeah, this is just an ecosystem-wide "problem". Closing.