bevy_reflect: Default attribute paths by MrGVSV · Pull Request #9323 · bevyengine/bevy

MrGVSV · 2023-07-31T20:50:08Z

Objective

Followup to #9322 but with a breaking change.

With syn2, it's a whole lot easier to customize attributes. One small improvement we could make would be to change the default attribute to take a type path directly instead of a stringified path.

Solution

Updated the parsing logic to take an ExprPath directly.

Changelog

Default functions passed to #[reflect(default)] no longer need to be string literals (i.e. they can specify the path directly like #[reflect(default = path::to::func)])

Migration Guide

The optional function passed to #[reflect(default)] is no longer a stringified path. It can now be a standard type path:

mod defaults {
  fn foo() -> i32 {
    123
  }
}

// BEFORE
#[derive(Reflect)]
struct Foo {
  #[reflect(default = "defaults::foo")]
  value: i32
}

// AFTER
#[derive(Reflect)]
struct Foo {
  #[reflect(default = defaults::foo)]
  value: i32
}

killercup

This should also allow autocomplete to work in these cases, right?

MrGVSV · 2023-08-30T17:13:19Z

This should also allow autocomplete to work in these cases, right?

Ideally yes, but I can't speak for all editors.

# 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: ```rust 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: ```rust #[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`. ```rust #[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: ```rust 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: ```rust #[derive(Reflect, Clone)] #[reflect(Clone)] struct Foo(usize); ``` <details> <summary>Generated Code</summary> ```rust fn reflect_clone(&self) -> Result<Box<dyn Reflect>, ReflectCloneError> { Ok(Box::new(Clone::clone(self))) } ``` </details> Or, we can specify our own cloning function: ```rust #[derive(Reflect)] #[reflect(Clone(incremental_clone))] struct Foo(usize); fn incremental_clone(value: &usize) -> usize { *value + 1 } ``` <details> <summary>Generated Code</summary> ```rust fn reflect_clone(&self) -> Result<Box<dyn Reflect>, ReflectCloneError> { Ok(Box::new(incremental_clone(self))) } ``` </details> 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. ```rust #[derive(Reflect)] struct Foo { #[reflect(ignore, clone)] bar: usize, #[reflect(ignore, clone = "incremental_clone")] baz: usize, } fn incremental_clone(value: &usize) -> usize { *value + 1 } ``` <details> <summary>Generated Code</summary> ```rust fn reflect_clone(&self) -> Result<Box<dyn Reflect>, ReflectCloneError> { Ok(Box::new(Self { bar: Clone::clone(&self.bar), baz: incremental_clone(&self.baz), })) } ``` </details> 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

Improve field attribute parsing

9e04df0

MrGVSV force-pushed the reflect-default-attribute-parsing branch from 6a62619 to 9e04df0 Compare August 30, 2023 00:37

MrGVSV removed the S-Blocked This cannot move forward until something else changes label Aug 30, 2023

MrGVSV marked this pull request as ready for review August 30, 2023 00:38

killercup approved these changes Aug 30, 2023

View reviewed changes

MrGVSV mentioned this pull request May 20, 2024

bevy_reflect: Reflection-based cloning #13432

Merged

BenjaminBrienen added D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 23, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

bevy_reflect: Default attribute paths#9323

bevy_reflect: Default attribute paths#9323
MrGVSV wants to merge 1 commit intobevyengine:mainfrom
MrGVSV:reflect-default-attribute-parsing

MrGVSV commented Jul 31, 2023

Uh oh!

killercup left a comment

Uh oh!

MrGVSV commented Aug 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

MrGVSV commented Jul 31, 2023

Objective

Solution

Changelog

Migration Guide

Uh oh!

killercup left a comment

Choose a reason for hiding this comment

Uh oh!

MrGVSV commented Aug 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants