bevy_reflect: Add ReflectBox to remotely reflect Box

[MrGVSV]

Objective

Closes #3392
Closes #3400
Closes #6098
Closes #9929

There are plenty of times where we want to reflect a type that itself contains reflected data:

struct Player {
    weapon: Box<dyn Reflect>
}

Unfortunately, Box<dyn Reflect>, does not implement Reflect, making this impossible.

#3400 does a great job at implementing Reflect for Box<dyn Reflect>, but it has one potential issue: double-boxing.

Double-boxing occurs when we erase the type of Box<T> and put it inside another Box. When Box<T> implements the same bounds required by T, this becomes very easy to do accidentally.

For instance, suppose Box<dyn Reflect> did implement Reflect. That would make this possible:

fn make_box<T: Reflect>(value: T) -> Box<dyn Reflect> {
    Box::new(value)
}

let foo = make_box(123); // Creates: Box<i32>
let bar = make_box(foo); // Creates: Box<Box<i32>>
let baz = make_box(bar); // Creates: Box<Box<Box<i32>>>

And while we can get back to a single box through each Box recursively calling a method like Reflect::into_reflect (i.e. Box<Box<Box<i32>>> delegates to Box<Box<i32>> which delegates to Box<i32> which delegates to i32 which results in Box<i32>), this is not very efficient—both in terms of performance and memory.

This PR aims to bring an alternative approach to the table, based on the newly merged remote reflection feature (#6042). The original idea for this PR comes from @soqb.

Solution

Introduce a new ReflectBox<T> wrapper type that can be used to remotely reflect Box<T>.

use bevy_reflect::boxed::ReflectBox;

#[derive(Reflect)]
#[reflect(from_reflect = false)]
struct Player {
    #[reflect(remote = ReflectBox<dyn Reflect>)]
    weapon: Box<dyn Reflect>,
}

ReflectBox<T> cannot be publicly constructed or downcast to. It only exists hidden as the underlying machinery for reflecting Box<T>. This makes it very difficult to accidentally double-box it. In fact, I'm 99% sure you'll never accidentally have that happen (there are ways to force this if you're feeling rebellious though).

The way it works is by defining a couple new traits (both of which are hidden using #[doc(hidden)]): ToPartialReflect and ToReflect. These traits have a blanket impl defined for all types T: PartialReflect and T: Reflect, respectively.

However, they also have custom impls for the corresponding trait objects: dyn PartialReflect and dyn Reflect.

And because ReflectBox<T> requires T: ToPartialReflect, it works for all three: Box<T>, Box<dyn PartialReflect>, and Box<dyn Reflect>.

I chose to make these traits public to allow for other trait objects to join in the fun! For example, we could refactor our Player struct to look like this:

use bevy_reflect::boxed::ReflectBox;

trait Weapon: Reflect {
    fn damage(&self) -> u32;
}

impl ToPartialReflect for dyn Weapon {/* ... */}
impl ToReflect for dyn Weapon {/* ... */}
impl TypePath for dyn Weapon {/* ... */}

#[derive(Reflect)]
#[reflect(from_reflect = false)]
struct Player {
    // Now our Rust API can directly call `Weapon` methods
    // AND we can still reflect it
    #[reflect(remote = ReflectBox<dyn Weapon>)]
    weapon: Box<dyn Weapon>,
}

Caveats

FromReflect

Note

This limitation is also an issue for #3400.

You might have noticed the #[reflect(from_reflect = false)] attributes.

We have to opt out of FromReflect because dyn PartialReflect and dyn Reflect are not aware of FromReflect (i.e. they are not subtraits of FromReflect). This means we can't delegate to the FromReflect impl, even if it does exist for the underlying type.

Enums

Note

This limitation is also an issue for #3400.

Unfortunately, due to the lack of FromReflect, this approach can't be used in enums.

This is due to enums internally relying on FromReflect within their PartialReflect::try_apply method body, which calls FromReflect when it needs to replace the current variant with a new one.

Usage as a Type Parameter

Note

This limitation only applies to #3400 where the type parameter has a FromReflect bound (or any other bound that Box<dyn Reflect> can't satisfy). For example, you also couldn't define a Vec<Box<dyn Reflect>> using #3400.

Since this approach requires the use of remote reflection, it may not be possible to handle types where Box is being used as a type parameter. For example, Vec<Box<dyn Reflect>> and Option<Box<dyn Reflect>> are not possible, even with this PR in place.

In some cases, it may be possible to remedy this with additional remote reflection. However, it's tricky to appease the compiler without monomorphizing the type down to a dedicated type (i.e. instead of supporting MyType<Box<dyn Reflect>>, having a separate MyTypeReflectBox which removes the generic and assumes Box<dyn Reflect> in its place).

Future PRs may explore adding additional types similar to ReflectBox for common types like Vec and Option, but these will also be limited in the same way ReflectBox is now.

Serialization

While the current serialization logic isn't the most efficient (we have to do some string comparisons to determine if the type is a ReflectBox or not), we are able to serialize ReflectBox just like any other type.

Currently, it will serialize concrete Box<T> instances as normal and Box<dyn Trait> as a new type map.

For example, if we serialized the following type:

#[derive(Reflect)]
#[reflect(from_reflect = false)]
struct MyStruct {
    #[reflect(remote = ReflectBox<dyn PartialReflect>)]
    partial_reflect: Box<dyn PartialReflect>,
    #[reflect(remote = ReflectBox<dyn Reflect>)]
    full_reflect: Box<dyn Reflect>,
    #[reflect(remote = ReflectBox<i32>)]
    concrete: Box<i32>,
}

let value = MyStruct {
    partial_reflect: Box::new(123),
    full_reflect: Box::new(456),
    concrete: Box::new(789),
};

We'd get the following RON:

{
    "my_crate::MyStruct": (
        partial_reflect: {"i32": 123},
        full_reflect: {"i32": 456},
        concrete: 789,
    )
}

Open Questions

1. Are the limitations of this PR worth the avoidance of double-boxing? Or should we continue with Implement Reflect for Box<dyn Reflect> #3400 instead?

   Please note that we may still choose to go with this more conservative approach while we debate the true risks of Implement Reflect for Box<dyn Reflect> #3400. We can do this because most usages of Box under this PR will be fully compatible with Implement Reflect for Box<dyn Reflect> #3400—the only exception would be custom Box<dyn SomeOtherTrait> usages, which will need their own solution (maybe a macro?). Of the compatible cases, the only migration needed will be to remove the #[reflect(remote = ReflectBox<...>)] field attributes wherever they are used.

2. Are there better ways to handle serialization other than fetching the type path and comparing the prefix? We already do this for Option types, but that's at least only performed in specific scenarios.

   One thing we could do is add a dedicated method to PartialReflect called is_pointer or is_indirect, which we can then call directly to determine whether we're a ReflectBox or not.

   Alternatively, we could change the PartialReflect::is_dynamic method on ReflectBox to return true instead of delegating to the inner type. I don't really like this option because it would be the only method where we do this and it would almost certainly break places that rely on is_dynamic for their control flow.

   One last option could be to change the signature of the largely unused PartialReflect::serializable method to take in a reference to the TypeRegistry. Then we could have ReflectBox return a ReflectSerializer containing the inner data. I think this would be my preferred approach considering it doesn't really change the behavior in an unexpected way (unlike modifying is_dynamic). It would just technically be a breaking change.

3. Should the ToPartialReflect and ToReflect traits be #[doc(hidden)]? I did this to prevent them from coming up during autocomplete and so that users don't accidentally misuse them (although, I don't think you can really "misuse" them). But since it's really nice for users to be able to get this feature to work with their own trait objects, should we consider making it more public?

Testing

You can test the changes locally by running:

cargo test --package bevy_reflect

---

Showcase

A common occurrence when reflecting a type is wanting to store reflected data within a reflected type. This wasn't possible before, but now it is using the new bevy_reflect::boxed::ReflectBox type and remote reflection:

use bevy_reflect::boxed::ReflectBox;

#[derive(Reflect)]
struct Sword {
    damage: u32
}

#[derive(Reflect)]
// Keep in mind that `ReflectBox` does not implement `FromReflect`.
// Because of this, we will need to opt-out of the automatic `FromReflect` impl:
#[reflect(from_reflect = false)]
struct Player {
    // Tell the `Reflect` derive to remotely reflect our `Box<dyn Reflect>`
    // using the `ReflectBox<dyn Reflect>` wrapper type:
    #[reflect(remote = ReflectBox<dyn Reflect>)]
    weapon: Box<dyn Reflect>
}

let player = Player {
    // We can create our `Box<dyn Reflect>` as normal:
    weapon: Box::new(Sword { damage: 100 })
};

// Now we can reflect `weapon`!
// Under the hood, this `dyn Reflect` is actually a `ReflectBox<dyn Reflect>`,
// and is automatically delegating all reflection methods to the inner `dyn Reflect`.
// It will automatically convert to and from `ReflectBox<dyn Reflect>` as needed.
let weapon: &dyn Reflect = player.weapon.as_reflect();
assert!(weapon.reflect_partial_eq(&Sword { damage: 100 }).unwrap_or_default());

[SkiFire13]

ReflectBox<T> cannot be publicly constructed

Nitpick: it can, since ReflectRemote::into_wrapper offers a way to do so.

---

I would also add to the caveats that this way of reflecting a Box<dyn Reflect> will limit the operations to those that you can do on the actual underlying type contained in the Box<dyn Reflect>. Most notably you won't be able to entirely replace the Box<dyn Reflect>, changing the type of the contained value.

[alice-i-cecile]

1. Are the limitations of this PR worth the avoidance of double-boxing?

IMO no: there's a lot of complexity here, both for users and maintainers. The limitations are also unpleasant, especially the one raised by SkiFire. Avoiding a modest footgun is not worth the complexity cost. I would much rather move forward this than nothing though.

2. Are there better ways to handle serialization other than fetching the type path and comparing the prefix?

I can't think of anything unfortunately :(

3. Should the ToPartialReflect and ToReflect traits be #[doc(hidden)]?

IMO no: doc(hidden) items are mysterious and frustrating to users. I'd like to avoid them in general without an extremely compelling reason.

[soqb]

Most notably you won't be able to entirely replace the Box, changing the type of the contained value.

i apologise if i'm really not getting it:

let mut boxed: Box<dyn Reflect> = Box::new(123u8);
boxed = Box::new(456u16);

[soqb]

looking at the PR description, i'm very confident all of the mentioned caveats can be solved:

1. FromReflect

we can definitely implement FromReflect for ReflectBox<T> by following these semantics:

• for T: Sized + FromReflect, defer to individual FromReflect impls.
• for T == dyn PartialReflect do nothing, the value is already the type we want.
• for T == dyn Reflect, use PartialReflect::try_into_reflect to (fallibly) downcast.

2. usage as a type parameter.
   solving this is more complicated but not outlandish and will give ergonomic benefit down the line.

essentially:

• change RemoteReflect to be generic over the remote type.
• introduce a reflexive impl so T: Reflect to also impl RemoteReflect<T>
• introduce a new Reflectable trait:

// loosely defined as:
trait Reflectable {
    type Via: Reflect + RemoteReflect<Self>;
}

impl<T: Reflect> Reflectable for T {
    type Via = T;
}

impl<T: ToPartialReflect> Reflectable for Box<T> {
    type Via = ReflectBox<T>;
}

and then, in all relevant Reflect impls, types bind to Reflectable instead of Reflect, so something like Vec<Foo<Box<dyn Reflect>>> works seamlessly.

[SkiFire13]

let mut boxed: Box<dyn Reflect> = Box::new(123u8);
boxed = Box::new(456u16);

What I mean is that you won't be able to do this if boxed is actually a field of a type and you're trying to mutate it through reflection. Something like this (I have not tested whether this compiles):

#[derive(Reflect)]
#[reflect(from_reflect = false)]
struct Foo {
    #[reflect(remote = ReflectBox<dyn Reflect>)]
    boxed: Box<dyn Reflect>,
}

let mut foo = Foo {
    boxed: Box::new(123u8),
};
let replacement: Box<dyn Reflect> = Box::new(456u16);

// Imagine that somewhere else gets `foo` as a `&mut dyn Reflect` and
// pattern matches it against a `ReflectMut::Struct`. This may an UI for example.
let reflected: &mut dyn Reflect = foo;
let ReflectMut::Struct(s) = reflected.reflect_mut() else { panic!() };
// Then it wants to change its `boxed` field to a `Box<dyn Reflect>` containing a different type.
// This is one thing it might try, but I don't think there's a way to make this work because
// any mutation will be forward to the underlying type, which of course can't change its own type.
s.field_mut("boxed").unwrap().apply(&replacement);

[alice-i-cecile]

I'd love to have tests for all of these nasty edge cases ;)

[JMS55]

What about Arc<dyn Reflect>?

[MrGVSV]

What about Arc<dyn Reflect>?

I think we can do a similar thing here as well. It might be slightly trickier due to getting mutable access to dyn Reflect being a fallible operation. But it's possible.

At worst, we just keep the existing impl for Arc (which treats it like a value type rather than wrapping it like ReflectBox does).

[MrGVSV]

1. Are the limitations of this PR worth the avoidance of double-boxing?

IMO no: there's a lot of complexity here, both for users and maintainers. The limitations are also unpleasant, especially the one raised by SkiFire. Avoiding a modest footgun is not worth the complexity cost. I would much rather move forward this than nothing though.

Yeah this PR is definitely more involved. I don't think it's too bad to maintain since most of the logic is just delegating to the inner Box, but it is a lot less ergonomic from a user perspective.

It should be noted that most limitations, including the one posted by SkiFire, exists on #3400 as well. The only limitation (I think) that they don't face is being able to use Box<dyn Reflect> as a type parameter. We face that issue due to remote reflection not currently being expressive enough for such cases.

I do still think that this is the safer option (and I really like that I can reflect reflection-compatible trait objects), but the user ergonomics are probably the most compelling reason to go with #3400.

2. Are there better ways to handle serialization other than fetching the type path and comparing the prefix?

I can't think of anything unfortunately :(

Yeah I was doing some experiments on being able to store and compare the TypeId of a "base type" via TypeInfo, but that ultimately ended up going nowhere due to issues with trait bounds.

3. Should the ToPartialReflect and ToReflect traits be #[doc(hidden)]?

IMO no: doc(hidden) items are mysterious and frustrating to users. I'd like to avoid them in general without an extremely compelling reason.

Fair enough. Any suggestions for making them public? Is the naming clear? Should we update the docs in any meaningful way?

[MrGVSV]

What I mean is that you won't be able to do this if boxed is actually a field of a type and you're trying to mutate it through reflection.

Oof didn't think about that. Great point! Unfortunately, this is likely an issue with both this PR and #3400. We would have to either not delegate certain methods or we would have to add a new way of changing a value (which would return an Err for concrete types).

I think we can modify the methods for Reflect::set and PartialReflect::try_apply for ReflectBox to first check if the underlying type is the same or different, then decide whether to delegate or replace the value. I'll look into it!

[MrGVSV]

looking at the PR description, i'm very confident all of the mentioned caveats can be solved:

1. FromReflect

we can definitely implement FromReflect for ReflectBox<T> by following these semantics:

• for T: Sized + FromReflect, defer to individual FromReflect impls.
• for T == dyn PartialReflect do nothing, the value is already the type we want.
• for T == dyn Reflect, use PartialReflect::try_into_reflect to (fallibly) downcast.

Yeah I like this. It allows FromReflect to still work for types where the fields are concrete types behind the dyn Reflect/dyn PartialReflect objects.

It's still not perfect since dynamic types (such as those you get when deserializing), will always fail. But we can look into solutions for this as that's just an inherent limitation of working with proxies.

2. usage as a type parameter.
   solving this is more complicated but not outlandish and will give ergonomic benefit down the line.

essentially:

• change RemoteReflect to be generic over the remote type.
• introduce a reflexive impl so T: Reflect to also impl RemoteReflect<T>
• introduce a new Reflectable trait:

// loosely defined as:
trait Reflectable {
    type Via: Reflect + RemoteReflect<Self>;
}

impl<T: Reflect> Reflectable for T {
    type Via = T;
}

impl<T: ToPartialReflect> Reflectable for Box<T> {
    type Via = ReflectBox<T>;
}

and then, in all relevant Reflect impls, types bind to Reflectable instead of Reflect, so something like Vec<Foo<Box<dyn Reflect>>> works seamlessly.

I almost forgot about this! I'll try looking into it, because it would vastly improve the ergonomics.

[alice-i-cecile]

Alright, thanks to both @MrGVSV and @soqb for addressing my concerns :) If the limitations aren't any worse, then I'm fine to defer to you about maintenance burden. ToReflect / ToPartialReflect are both good names, and the docs are adequate. A doc test or two would be quite helpful in showing off how they're used.

As for end user ergonomics / complexity, can we get a relatively simple top-level example for how to reflect a component that has a Box<dyn Reflect>? Your PR example was nice, and it would be good to have a starting point for the workflow.

[MrGVSV]

I think we can modify the methods for Reflect::set and PartialReflect::try_apply for ReflectBox to first check if the underlying type is the same or different, then decide whether to delegate or replace the value. I'll look into it!

Starting experimenting with this. I'm not sure it's possible when using T: ?Sized, there's no way for us to downcast back to T from a Box<dyn Reflect> (Any requires Sized).

So unless we remove the generic impl (preventing custom trait object support), then it appears we aren't able to achieve this.

Feel free to correct me if I'm wrong, though!

[SkiFire13]

Since the example I gave in #14776 (comment) may seem a bit weird/artificial, here's another one that might seem more reasonable:

#[derive(Reflect)]
#[reflect(from_reflect = false)]
struct Foo {
    #[reflect(remote = crate::boxed::ReflectBox<dyn Reflect>)]
    boxed: Box<dyn Reflect>,
}

let mut foo1 = Foo {
    boxed: Box::new(123u8),
};

let foo2 = Foo {
    boxed: Box::new("foo"),
};

foo1.apply(&foo2);

Given foo1 and foo2 having the same type I would expect apply to never fail, and instead it does with:

called `Result::unwrap()` on an `Err` value: MismatchedTypes { from_type: "bevy_reflect::boxed::ReflectBox<dyn bevy_reflect::Reflect>", to_type: "u8" }

To be fair though I think #3400 has the same issue, so there isn't a better approach (yet).

[MrGVSV]

Given foo1 and foo2 having the same type I would expect apply to never fail, and instead it does with:

called `Result::unwrap()` on an `Err` value: MismatchedTypes { from_type: "bevy_reflect::boxed::ReflectBox<dyn bevy_reflect::Reflect>", to_type: "u8" }

To be fair though I think #3400 has the same issue, so there isn't a better approach (yet).

I actually think #3400 might be okay because it currently only supports Box<dyn Reflect>. But if it were to try and support other trait objects it would face the same problem.

I wonder if we made T: Sized by doing ReflectBox<Box<dyn Reflect>> instead of ReflectBox<dyn Reflect> we could make the downcast work 🤔 Actually I doubt that will work since then T will be Box<MyType> instead of MyType, which dyn Reflect definitely won't downcast to.

[MrGVSV]

looking at the PR description, i'm very confident all of the mentioned caveats can be solved:

1. FromReflect

we can definitely implement FromReflect for ReflectBox<T> by following these semantics:

• for T: Sized + FromReflect, defer to individual FromReflect impls.
• for T == dyn PartialReflect do nothing, the value is already the type we want.
• for T == dyn Reflect, use PartialReflect::try_into_reflect to (fallibly) downcast.

Yeah I like this. It allows FromReflect to still work for types where the fields are concrete types behind the dyn Reflect/dyn PartialReflect objects.

It's still not perfect since dynamic types (such as those you get when deserializing), will always fail. But we can look into solutions for this as that's just an inherent limitation of working with proxies.

So I also looked into adding this. The only one we can potentially support right now is T: Sized + FromReflect.

Attempting to do so for dyn Reflect and dyn PartialReflect run into the same issue: the argument to FromReflect::from_reflect is a &dyn PartialReflect. So we can't just downcast use PartialReflect::try_into_reflect for ReflectBox<dyn PartialReflect>, since we're not given an owned value.

And we can't call PartialReflect::clone_value because it will almost always return a dynamic type, which will always fail PartialReflect::try_into_reflect.

Like I mentioned on Discord, we might be able to get around this by making FromReflect accessible via TypeInfo, or by using a global registry to access ReflectFromReflect. Until then, though, I don't know if it's worth adding. Maybe for T: FromReflect, but I don't expect Box<ConcreteTypeThatImplementsFromReflect> to see a lot of usage.

[MrGVSV]

Alright, thanks to both @MrGVSV and @soqb for addressing my concerns :) If the limitations aren't any worse, then I'm fine to defer to you about maintenance burden. ToReflect / ToPartialReflect are both good names, and the docs are adequate. A doc test or two would be quite helpful in showing off how they're used.

Moved these to a dedicated module and updated the docs to include an example of how to implement it on custom trait objects.

As for end user ergonomics / complexity, can we get a relatively simple top-level example for how to reflect a component that has a Box<dyn Reflect>? Your PR example was nice, and it would be good to have a starting point for the workflow.

You mean an example in the repo? Yeah I can try to put one together 😄

[soqb]

Something like this (I have not tested whether this compiles)

could easily become:

s.field_mut("boxed").unwrap().set(replacement);

NB: i'm not sure if set actually works like this right now, but it should.

[MrGVSV]

Something like this (I have not tested whether this compiles)

could easily become:

s.field_mut("boxed").unwrap().set(replacement);

NB: i'm not sure if set actually works like this right now, but it should.

I think the problem is that ReflectBox<T>::set has to do one of two things:

1. If the types are the same, call T::set with the given Box<dyn Reflect>
2. If the types are different, set ReflectBox<T> to a new ReflectBox<T> with the given Box<dyn Reflect>

The real issue is 2, since there is no way to go from Box<dyn Reflect> to Box<T>, while still allowing T: ?Sized (for dyn Reflect, dyn MyTrait, etc.), since Any::downcast does not allow for ?Sized types.

[MrGVSV]

I may have a solution for this actually 🤔

Update: I mentioned my idea on Discord, but I'm having a lot of concern over the complexity of the solution—both in terms of maintainability and user understandability. We may have to look into solving the Reflect::set issue as a followup.

[github-actions]

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

[MrGVSV]

FYI I just put up a new PR that uses a different approach that I think is a bit cleaner (at least from an end-user perspective) and will be easier to extend: #15532

If we decide we prefer this PR better, I'll rebase and we can continue forward with this one.

[alice-i-cecile]

I prefer that PR too, so I'm closing this for now.