bevy_reflect: Reflection-based cloning

[MrGVSV]

Objective

Using Reflect::clone_value can be somewhat confusing to those unfamiliar with how Bevy's reflection crate works. For example take the following code:

let value: usize = 123;
let clone: Box<dyn Reflect> = value.clone_value();

What can we expect to be the underlying type of clone? If you guessed usize, then you're correct! Let's try another:

#[derive(Reflect, Clone)]
struct Foo(usize);

let value: Foo = Foo(123);
let clone: Box<dyn Reflect> = value.clone_value();

What about this code? What is the underlying type of clone? If you guessed Foo, unfortunately you'd be wrong. It's actually DynamicStruct.

It's not obvious that the generated Reflect impl actually calls Struct::clone_dynamic under the hood, which always returns DynamicStruct.

There are already some efforts to make this a bit more apparent to the end-user: #7207 changes the signature of Reflect::clone_value to instead return Box<dyn PartialReflect>, signaling that we're potentially returning a dynamic type.

But why can't we return Foo?

Foo can obviously be cloned— in fact, we already derived Clone on it. But even without the derive, this seems like something Reflect should be able to handle. Almost all types that implement Reflect either contain no data (trivially clonable), they contain a #[reflect_value] type (which, by definition, must implement Clone), or they contain another Reflect type (which recursively fall into one of these three categories).

This PR aims to enable true reflection-based cloning where you get back exactly the type that you think you do.

Solution

Add a Reflect::reflect_clone method which returns Result<Box<dyn Reflect>, ReflectCloneError>, where the Box<dyn Reflect> is guaranteed to be the same type as Self.

#[derive(Reflect)]
struct Foo(usize);

let value: Foo = Foo(123);
let clone: Box<dyn Reflect> = value.reflect_clone().unwrap();
assert!(clone.is::<Foo>());

Notice that we didn't even need to derive Clone for this to work: it's entirely powered via reflection!

Under the hood, the macro generates something like this:

fn reflect_clone(&self) -> Result<Box<dyn Reflect>, ReflectCloneError> {
    Ok(Box::new(Self {
        // The `reflect_clone` impl for `usize` just makes use of its `Clone` impl
        0: Reflect::reflect_clone(&self.0)?.take().map_err(/* ... */)?,
    }))
}

If we did derive Clone, we can tell Reflect to rely on that instead:

#[derive(Reflect, Clone)]
#[reflect(Clone)]
struct Foo(usize);

Generated Code

fn reflect_clone(&self) -> Result<Box<dyn Reflect>, ReflectCloneError> {
    Ok(Box::new(Clone::clone(self)))
}

Or, we can specify our own cloning function:

#[derive(Reflect)]
#[reflect(Clone(incremental_clone))]
struct Foo(usize);

fn incremental_clone(value: &usize) -> usize {
  *value + 1
}

Generated Code

fn reflect_clone(&self) -> Result<Box<dyn Reflect>, ReflectCloneError> {
    Ok(Box::new(incremental_clone(self)))
}

Similarly, we can specify how fields should be cloned. This is important for fields that are #[reflect(ignore)]'d as we otherwise have no way to know how they should be cloned.

#[derive(Reflect)]
struct Foo {
 #[reflect(ignore, clone)]
  bar: usize,
  #[reflect(ignore, clone = "incremental_clone")]
  baz: usize,
}

fn incremental_clone(value: &usize) -> usize {
  *value + 1
}

Generated Code

fn reflect_clone(&self) -> Result<Box<dyn Reflect>, ReflectCloneError> {
    Ok(Box::new(Self {
        bar: Clone::clone(&self.bar),
        baz: incremental_clone(&self.baz),
    }))
}

If we don't supply a clone attribute for an ignored field, then the method will automatically return Err(ReflectCloneError::FieldNotClonable {/* ... */}).

Err values "bubble up" to the caller. So if Foo contains Bar and the reflect_clone method for Bar returns Err, then the reflect_clone method for Foo also returns Err.

Attribute Syntax

You might have noticed the differing syntax between the container attribute and the field attribute.

This was purely done for consistency with the current attributes. There are PRs aimed at improving this. #7317 aims at making the "special-cased" attributes more in line with the field attributes syntactically. And #9323 aims at moving away from the stringified paths in favor of just raw function paths.

Compatibility with Unique Reflect

This PR was designed with Unique Reflect (#7207) in mind. This method actually wouldn't change that much (if at all) under Unique Reflect. It would still exist on Reflect and it would still Option<Box<dyn Reflect>>. In fact, Unique Reflect would only improve the user's understanding of what this method returns.

We may consider moving what's currently Reflect::clone_value to PartialReflect and possibly renaming it to partial_reflect_clone or clone_dynamic to better indicate how it differs from reflect_clone.

Testing

You can test locally by running the following command:

cargo test --package bevy_reflect

---

Changelog

• Added Reflect::reflect_clone method
• Added ReflectCloneError error enum
• Added #[reflect(Clone)] container attribute
• Added #[reflect(clone)] field attribute

[cBournhonesque]

I wonder if we should use darling as a helper crate to parse attributes?
It seems easier than what you're doing, especially if the attributes become more complex later on

[MrGVSV]

We could utilize something like darling in the future (not this PR though). I'm not sure our parsing logic is so complex we really need it, but it's certainly worth considering.

[cBournhonesque]

Where is this used? I couldn't understand this part.
I get the clone_impl on struct,tuple,enum,value,etc. but not this.

Or is this the top-level impl?

[MrGVSV]

It's used by derive_data.rs. I could probably move this logic into that file directly, since it's the only place where it's used.

[cBournhonesque]

Looks good to me, I like the change!
I think the naming will be clearer when the PartialReflect/UniqueReflect PR gets merged as well.
Had some small comments, and i'll probably wait for the blocking PR to be merged before approving

[pablo-lua]

Very good PR, I just have some small questions :D

[pablo-lua]

If I understood it right, if we have an ignored field, we can only fallback to Derive impl, right?

[pablo-lua]

Question: Should we use #[reflect(Clone)] Whenever possible?

[pablo-lua]

I think we must warn the user which cases can return None
From what I read in this PR, this returns None if Clone isn't implemented for all types, but when I first read the PR description, this left me very confused: How can Clone fail?

[MrGVSV]

Rebased! Some notable changes:

• I did end up putting the method on PartialReflect. I think it makes sense there as an alternative way to go from dyn PartialReflect to dyn Reflect. It's also not possible to put it on Reflect since fields are only required to be PartialReflect. This may change with bevy_reflect: Add casting traits to support Box and other wrapper types #15532 but not super sure on that
• HashSets and HashMaps now require S: Default
• Had to account for remote reflection

Note that as a followup, it might be nice to add the #[reflect(Clone)] container attributes to all Bevy methods that can support it. As well as #[reflect(clone)] field attributes on all ignored fields that can support it.

[MrGVSV]

Also, I'm curious if we should rename the method since it returns a Result. Should we call it try_reflect_clone?

Any thoughts?

[alice-i-cecile]

Mild preference for reflect_clone: I think it reads better with ? syntax.

[tychedelia]

Don't have any specific comments wrt the implementation here as I'm less familiar with this subsystem, but the code seems good and appears to follow existing patterns for other similar features. I've definitely had use for something like this before.

Tested on macOS.

[MrGVSV]

Put up a PR to add ReflectClone as type data as well!

[bushrat011899]

Excellent addition! LGTM

[alice-i-cecile]

CI failure looks spurious, trying again.