-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bevy_reflect: Reflection-based cloning #13432
base: main
Are you sure you want to change the base?
Conversation
fn parse_clone(&mut self, input: ParseStream) -> syn::Result<()> { | ||
let ident = input.parse::<kw::Clone>()?; | ||
|
||
if input.peek(token::Paren) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -509,6 +533,24 @@ impl ContainerAttributes { | |||
} | |||
} | |||
|
|||
pub fn get_clone_impl(&self, bevy_reflect_path: &Path) -> Option<proc_macro2::TokenStream> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
0a05f93
to
51f9b2d
Compare
51f9b2d
to
ad1146e
Compare
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:What can we expect to be the underlying type of
clone
? If you guessedusize
, then you're correct! Let's try another:What about this code? What is the underlying type of
clone
? If you guessedFoo
, unfortunately you'd be wrong. It's actuallyDynamicStruct
.It's not obvious that the generated
Reflect
impl actually callsStruct::clone_dynamic
under the hood, which always returnsDynamicStruct
.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 returnBox<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 derivedClone
on it. But even without the derive, this seems like somethingReflect
should be able to handle. Almost all types that implementReflect
either contain no data (trivially clonable), they contain a#[reflect_value]
type (which, by definition, must implementClone
), or they contain anotherReflect
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 returnsResult<Box<dyn Reflect>, ReflectCloneError>
, where theBox<dyn Reflect>
is guaranteed to be the same type asSelf
.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:
If we did derive
Clone
, we can tellReflect
to rely on that instead:Generated Code
Or, we can specify our own cloning function:
Generated Code
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.Generated Code
If we don't supply a
clone
attribute for an ignored field, then the method will automatically returnErr(ReflectCloneError::FieldNotClonable {/* ... */})
.Err
values "bubble up" to the caller. So ifFoo
containsBar
and thereflect_clone
method forBar
returnsErr
, then thereflect_clone
method forFoo
also returnsErr
.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 stillOption<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
toPartialReflect
and possibly renaming it topartial_reflect_clone
orclone_dynamic
to better indicate how it differs fromreflect_clone
.Testing
You can test locally by running the following command:
Changelog
Reflect::reflect_clone
methodReflectCloneError
error enum#[reflect(Clone)]
container attribute#[reflect(clone)]
field attribute