[Merged by Bors] - bevy_reflect: Relax bounds on `Option<T>` #5658

MrGVSV · 2022-08-11T22:30:49Z

Objective

The reflection impls on Option<T> have the bound T: Reflect + Clone. This means that using FromReflect requires Clone even though we can normally get away with just FromReflect.

Solution

Update the bounds on Option<T> to match that of Vec<T>, where T: FromReflect.

This helps remove a Clone implementation that may be undesired but added for the sole purpose of getting the code to compile.

Changelog

Reflection on Option<T> now has T bound by FromReflect rather than Reflect + Clone
Added a FromReflect impl for Instant

Migration Guide

If using Option<T> with Bevy's reflection API, T now needs to implement FromReflect rather than just Clone. This can be achieved easily by simply deriving FromReflect:

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

let reflected: Box<dyn Reflect> = Box::new(Some(Foo));

// NEW
#[derive(Reflect, FromReflect)]
struct Foo;

let reflected: Box<dyn Reflect> = Box::new(Some(Foo));

Note: You can still derive Clone, but it's not required in order to compile.

cart · 2022-08-16T20:48:41Z

bors r+

cart · 2022-08-16T23:19:07Z

bors retry

# Objective The reflection impls on `Option<T>` have the bound `T: Reflect + Clone`. This means that using `FromReflect` requires `Clone` even though we can normally get away with just `FromReflect`. ## Solution Update the bounds on `Option<T>` to match that of `Vec<T>`, where `T: FromReflect`. This helps remove a `Clone` implementation that may be undesired but added for the sole purpose of getting the code to compile. --- ## Changelog * Reflection on `Option<T>` now has `T` bound by `FromReflect` rather than `Reflect + Clone` * Added a `FromReflect` impl for `Instant` ## Migration Guide If using `Option<T>` with Bevy's reflection API, `T` now needs to implement `FromReflect` rather than just `Clone`. This can be achieved easily by simply deriving `FromReflect`: ```rust // OLD #[derive(Reflect, Clone)] struct Foo; let reflected: Box<dyn Reflect> = Box::new(Some(Foo)); // NEW #[derive(Reflect, FromReflect)] struct Foo; let reflected: Box<dyn Reflect> = Box::new(Some(Foo)); ``` > Note: You can still derive `Clone`, but it's not required in order to compile.

bors · 2022-08-16T23:37:53Z

Build failed (retrying...):

check-doc

# Objective The reflection impls on `Option<T>` have the bound `T: Reflect + Clone`. This means that using `FromReflect` requires `Clone` even though we can normally get away with just `FromReflect`. ## Solution Update the bounds on `Option<T>` to match that of `Vec<T>`, where `T: FromReflect`. This helps remove a `Clone` implementation that may be undesired but added for the sole purpose of getting the code to compile. --- ## Changelog * Reflection on `Option<T>` now has `T` bound by `FromReflect` rather than `Reflect + Clone` * Added a `FromReflect` impl for `Instant` ## Migration Guide If using `Option<T>` with Bevy's reflection API, `T` now needs to implement `FromReflect` rather than just `Clone`. This can be achieved easily by simply deriving `FromReflect`: ```rust // OLD #[derive(Reflect, Clone)] struct Foo; let reflected: Box<dyn Reflect> = Box::new(Some(Foo)); // NEW #[derive(Reflect, FromReflect)] struct Foo; let reflected: Box<dyn Reflect> = Box::new(Some(Foo)); ``` > Note: You can still derive `Clone`, but it's not required in order to compile.

bors · 2022-08-16T23:40:20Z

Build failed:

check-doc

cart · 2022-08-17T00:20:59Z

bors r+

# Objective The reflection impls on `Option<T>` have the bound `T: Reflect + Clone`. This means that using `FromReflect` requires `Clone` even though we can normally get away with just `FromReflect`. ## Solution Update the bounds on `Option<T>` to match that of `Vec<T>`, where `T: FromReflect`. This helps remove a `Clone` implementation that may be undesired but added for the sole purpose of getting the code to compile. --- ## Changelog * Reflection on `Option<T>` now has `T` bound by `FromReflect` rather than `Reflect + Clone` * Added a `FromReflect` impl for `Instant` ## Migration Guide If using `Option<T>` with Bevy's reflection API, `T` now needs to implement `FromReflect` rather than just `Clone`. This can be achieved easily by simply deriving `FromReflect`: ```rust // OLD #[derive(Reflect, Clone)] struct Foo; let reflected: Box<dyn Reflect> = Box::new(Some(Foo)); // NEW #[derive(Reflect, FromReflect)] struct Foo; let reflected: Box<dyn Reflect> = Box::new(Some(Foo)); ``` > Note: You can still derive `Clone`, but it's not required in order to compile.

bors · 2022-08-17T00:40:04Z

Pull request successfully merged into main.

Build succeeded:

# Objective #5658 made it so that `FromReflect` was used as the bound for `T` in `Option<T>`. However, it did not use this change effectively for the implementation of `Reflect::apply` (it was still using `take`, which would fail for Dynamic types). Additionally, the changes were not consistent with other methods within the file, such as the ones for `Vec<T>` and `HashMap<K, V>`. ## Solution Update `Option<T>` to fallback on `FromReflect` if `take` fails, instead of wholly relying on one or the other. I also chose to update the error messages, as they weren't all too descriptive before. --- ## Changelog - Use `FromReflect::from_reflect` as a fallback in the `Reflect::apply` implementation for `Option<T>`

# Objective The reflection impls on `Option<T>` have the bound `T: Reflect + Clone`. This means that using `FromReflect` requires `Clone` even though we can normally get away with just `FromReflect`. ## Solution Update the bounds on `Option<T>` to match that of `Vec<T>`, where `T: FromReflect`. This helps remove a `Clone` implementation that may be undesired but added for the sole purpose of getting the code to compile. --- ## Changelog * Reflection on `Option<T>` now has `T` bound by `FromReflect` rather than `Reflect + Clone` * Added a `FromReflect` impl for `Instant` ## Migration Guide If using `Option<T>` with Bevy's reflection API, `T` now needs to implement `FromReflect` rather than just `Clone`. This can be achieved easily by simply deriving `FromReflect`: ```rust // OLD #[derive(Reflect, Clone)] struct Foo; let reflected: Box<dyn Reflect> = Box::new(Some(Foo)); // NEW #[derive(Reflect, FromReflect)] struct Foo; let reflected: Box<dyn Reflect> = Box::new(Some(Foo)); ``` > Note: You can still derive `Clone`, but it's not required in order to compile.

# Objective bevyengine#5658 made it so that `FromReflect` was used as the bound for `T` in `Option<T>`. However, it did not use this change effectively for the implementation of `Reflect::apply` (it was still using `take`, which would fail for Dynamic types). Additionally, the changes were not consistent with other methods within the file, such as the ones for `Vec<T>` and `HashMap<K, V>`. ## Solution Update `Option<T>` to fallback on `FromReflect` if `take` fails, instead of wholly relying on one or the other. I also chose to update the error messages, as they weren't all too descriptive before. --- ## Changelog - Use `FromReflect::from_reflect` as a fallback in the `Reflect::apply` implementation for `Option<T>`

# Objective The reflection impls on `Option<T>` have the bound `T: Reflect + Clone`. This means that using `FromReflect` requires `Clone` even though we can normally get away with just `FromReflect`. ## Solution Update the bounds on `Option<T>` to match that of `Vec<T>`, where `T: FromReflect`. This helps remove a `Clone` implementation that may be undesired but added for the sole purpose of getting the code to compile. --- ## Changelog * Reflection on `Option<T>` now has `T` bound by `FromReflect` rather than `Reflect + Clone` * Added a `FromReflect` impl for `Instant` ## Migration Guide If using `Option<T>` with Bevy's reflection API, `T` now needs to implement `FromReflect` rather than just `Clone`. This can be achieved easily by simply deriving `FromReflect`: ```rust // OLD #[derive(Reflect, Clone)] struct Foo; let reflected: Box<dyn Reflect> = Box::new(Some(Foo)); // NEW #[derive(Reflect, FromReflect)] struct Foo; let reflected: Box<dyn Reflect> = Box::new(Some(Foo)); ``` > Note: You can still derive `Clone`, but it's not required in order to compile.

# Objective bevyengine#5658 made it so that `FromReflect` was used as the bound for `T` in `Option<T>`. However, it did not use this change effectively for the implementation of `Reflect::apply` (it was still using `take`, which would fail for Dynamic types). Additionally, the changes were not consistent with other methods within the file, such as the ones for `Vec<T>` and `HashMap<K, V>`. ## Solution Update `Option<T>` to fallback on `FromReflect` if `take` fails, instead of wholly relying on one or the other. I also chose to update the error messages, as they weren't all too descriptive before. --- ## Changelog - Use `FromReflect::from_reflect` as a fallback in the `Reflect::apply` implementation for `Option<T>`

# Objective The reflection impls on `Option<T>` have the bound `T: Reflect + Clone`. This means that using `FromReflect` requires `Clone` even though we can normally get away with just `FromReflect`. ## Solution Update the bounds on `Option<T>` to match that of `Vec<T>`, where `T: FromReflect`. This helps remove a `Clone` implementation that may be undesired but added for the sole purpose of getting the code to compile. --- ## Changelog * Reflection on `Option<T>` now has `T` bound by `FromReflect` rather than `Reflect + Clone` * Added a `FromReflect` impl for `Instant` ## Migration Guide If using `Option<T>` with Bevy's reflection API, `T` now needs to implement `FromReflect` rather than just `Clone`. This can be achieved easily by simply deriving `FromReflect`: ```rust // OLD #[derive(Reflect, Clone)] struct Foo; let reflected: Box<dyn Reflect> = Box::new(Some(Foo)); // NEW #[derive(Reflect, FromReflect)] struct Foo; let reflected: Box<dyn Reflect> = Box::new(Some(Foo)); ``` > Note: You can still derive `Clone`, but it's not required in order to compile.

# Objective bevyengine#5658 made it so that `FromReflect` was used as the bound for `T` in `Option<T>`. However, it did not use this change effectively for the implementation of `Reflect::apply` (it was still using `take`, which would fail for Dynamic types). Additionally, the changes were not consistent with other methods within the file, such as the ones for `Vec<T>` and `HashMap<K, V>`. ## Solution Update `Option<T>` to fallback on `FromReflect` if `take` fails, instead of wholly relying on one or the other. I also chose to update the error messages, as they weren't all too descriptive before. --- ## Changelog - Use `FromReflect::from_reflect` as a fallback in the `Reflect::apply` implementation for `Option<T>`

MrGVSV added C-Usability A simple quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Aug 11, 2022

NiklasEi approved these changes Aug 13, 2022

View reviewed changes

maccesch approved these changes Aug 14, 2022

View reviewed changes

NiklasEi added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Aug 14, 2022

MrGVSV added 3 commits August 16, 2022 17:02

Bound Option reflection by FromReflect

c81ca57

Add FromReflect impl for Instant

50a2588

Update derives with FromReflect

c24a85a

MrGVSV force-pushed the reflect-option-bounds branch from 207efb3 to c24a85a Compare August 17, 2022 00:02

Import FromReflect

50f7a5e

bors bot changed the title ~~bevy_reflect: Relax bounds on Option<T>~~ [Merged by Bors] - bevy_reflect: Relax bounds on Option<T> Aug 17, 2022

bors bot closed this Aug 17, 2022

MrGVSV mentioned this pull request Aug 24, 2022

[Merged by Bors] - bevy_reflect: Fix apply method for Option<T> #5780

Closed

MrGVSV deleted the reflect-option-bounds branch August 24, 2022 21:06

rparrett mentioned this pull request Oct 26, 2022

[Merged by Bors] - Migration guide 0.9 bevyengine/bevy-website#470

Closed

ickk added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Oct 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - bevy_reflect: Relax bounds on `Option<T>` #5658

[Merged by Bors] - bevy_reflect: Relax bounds on `Option<T>` #5658

MrGVSV commented Aug 11, 2022 •

edited

cart commented Aug 16, 2022

cart commented Aug 16, 2022

bors bot commented Aug 16, 2022

bors bot commented Aug 16, 2022

cart commented Aug 17, 2022

bors bot commented Aug 17, 2022

[Merged by Bors] - bevy_reflect: Relax bounds on Option<T> #5658

[Merged by Bors] - bevy_reflect: Relax bounds on Option<T> #5658

Conversation

MrGVSV commented Aug 11, 2022 • edited

Objective

Solution

Changelog

Migration Guide

cart commented Aug 16, 2022

cart commented Aug 16, 2022

bors bot commented Aug 16, 2022

bors bot commented Aug 16, 2022

cart commented Aug 17, 2022

bors bot commented Aug 17, 2022

[Merged by Bors] - bevy_reflect: Relax bounds on `Option<T>` #5658

[Merged by Bors] - bevy_reflect: Relax bounds on `Option<T>` #5658

MrGVSV commented Aug 11, 2022 •

edited