Implement Reflect for Box<dyn Reflect>

[franciscoaguirre]

Objective

Closes #3392

Solution

Used code snippet from @Davier and made a few changes on DynamicList for the type checker to work.

[alice-i-cecile]

Awesome, thank you. This is a nice addition, and your test looks nice :)

[Davier]

Looks good to me!
A possible improvement would be to show its usage in the reflection.rs example, perhaps by adding a field of type Box<dyn Reflect> in Foo?

[alice-i-cecile]

@cart do your concerns from #2505 apply here?

[cart]

Yup "boxing boxes" is definitely a concern here, although the scope of the problem is different because in core bevy apis we aren't implicitly boxing T: Reflect parameters that might later be retrieved and passed back in (at least to my knowledge).

However Box<dyn Reflect> is used/returned in a number of places (Reflect::clone_value(), scenes, etc), so this feels risky to me.

Thaaaat being said, having reflection work for "everything", including Boxed reflect types, does seem like it could be valuable. Can we outline specific "real world" scenarios that need this (that can't be resolved in other ways?). So far I've only seen that a user wants to do this, which isn't enough context to evaluate the need here.

[djeedai]

Note that any impl Trait for Box<dyn Trait> opens the door to double-boxing (nested boxing) and especially to silently doing so by mistake. Coincidentally we're hitting this in 🍃Bevy Tweening with djeedai/bevy_tweening#27. See discussion highlighting a few important points:

1. even rustlang/futures-rs struggles with this, it's a hard issue, that trait specialization will solve once it's stable, but until then is hard to fix.
2. you don't need to not have that impl in the current crate for the problem to be avoided with any public trait, because any crate could add that impl for you, and your back to square one.
3. incidentally 2. makes it harder to work around double boxing.
4. there's some runtime workaround via Any::try_downcast() that was suggested to me on the Rust Discord, but it's a runtime check not a build time guarantee.

Given all of that I think it's reasonable to still add the impl, and thoroughly document places where nested boxing might occur by mistake. And consider adding that runtime check as a guard.

[djeedai]

Here's the runtime workaround : https://discord.com/channels/442252698964721669/443150878111694848/981137955773095936 (I didn't try it yet)

Use this function:

fn try_downcast<T, K>(k: K) -> Result<T, K>
where
    T: 'static,
    K: 'static,
{
    let mut k = Some(k);
    if let Some(k) = <dyn std::any::Any>::downcast_mut::<Option<T>>(&mut k) {
        Ok(k.take().unwrap())
    } else {
        Err(k.unwrap())
    }
}

like so:

fn box_trait<T: 'static + Trait>(val: T) -> Box<dyn Trait> {
    try_downcast::<Box<dyn Trait>, _>(val)
        .unwrap_or_else(|val| Box::new(val))
}

[PROMETHIA-27]

as a real-world example; working on my editor I've stumbled across this. I want to send messages over network between the editor and the game client, and those messages are serialized into yaml (could be anything, picked arbitrarily) via reflection. In order to send reflected types, I sometimes need to send a Box (serialized components), but it's not Reflect so I ended up implementing a ReflectObject newtype around it. This PR would allow me to throw that (likely somewhat buggy) code out and reduce annoying boilerplate/restrictions

[PROMETHIA-27]

Looks decent, but I have a few questions:

[PROMETHIA-27]

why is .deref() necessary here? Clarity?

[MrGVSV]

@franciscoaguirre Would you be able to rebase this off main?

[MrGVSV]

Adding the S-Adopt-Me tag.

@franciscoaguirre let us know if you'd rather continue working on this yourself and I'll remove it! :)

[franciscoaguirre]

@MrGVSV Yeah I can work on this. There was so much debate that I got lost 😂 and I wasn't having much time. I have more time now so I can get it done

[rdrpenguin04]

@franciscoaguirre Are you still intending to work on this?

[franciscoaguirre]

@rdrpenguin04 I've been heavily caught in finishing university. Now that I'll soon graduate I'll have time for open source and I'd love to contribute to Bevy. That said, I can only work on it in January, since the holidays are coming up. I've seen people requesting this PR to be merged so if you need it before holidays I don't mind you taking the torch. If not, I can work on it in January :)

[franciscoaguirre]

@rdrpenguin04 I got around to rebasing and updating the code with the latest on main 😄
@PROMETHIA-27 Is this PR okay now with your change to the Reflect trait?
@alice-i-cecile Is this issue still relevant?

[djeedai]

@franciscoaguirre FYI you have at least one test panicking, looks like it's directly related to your change.

[franciscoaguirre]

Yep, fixed it.

The thing is, I was implementing DynamicTypePath on Box<dyn Reflect> which lets me use self to forward the call to the inner value, but I needed to implement TypePath for the type and having both implementations resulted in conflicts.
I think it's more useful to get the type of the inner value, maybe not? Is there a way I can accomplish that?

[djeedai]

I have no idea, I don't have enough domain knowledge to answer whether we should implement the dynamic version or not. My gut feeling is that TypePath would be more useful, but I really don't have a backing argument sorry. Maybe @alice-i-cecile or @MrGVSV can help here?

[JMS55]

Maintainers: What are we doing with this PR? It's been S-Ready-For-Final-Review for a while now

[alice-i-cecile]

Deferring to the Reflection SMEs. I'll bring it up on Discord.

[MrGVSV]

Maintainers: What are we doing with this PR? It's been S-Ready-For-Final-Review for a while now

Sorry to everyone that's been waiting on this :/

I'm currently looking into a possible alternative approach that aims to avoid the double-boxing issue by going through remote reflection. However, if that doesn't work out or it runs into the same problems, then we'll probably move forward with this PR.

I'll keep experimenting and return with details soon!

Either way, I'd like to get something in for Box<dyn Reflect> and Box<dyn PartialReflect> for 0.15.

[MrGVSV]

I've just pushed #14776 if anyone wants to give it a look!

The TL;DR is that we can avoid the risk of double-boxing by adding a separate ReflectBox type (that users can't construct—otherwise we'd have the same problem) and using remote reflection to reflect it:

use bevy_reflect::boxed::ReflectBox;

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

The main consideration is that this ReflectBox approach comes with many limitations. Some are the same as this PR (i.e. doesn't work with FromReflect, can't be used in enums), but some are unique to remote reflection (i.e. difficulty using ReflectBox as a type parameter).

So the question becomes, do we go for the more flexible PR (this one) or the more conservative PR (#14776)?

I'm leaning towards starting out conservative to see how far that can take us. We can then revisit this PR in the future if necessary.

However, I'm more than happy to hear alternative perspectives or even other ideas!

---

Also, I wanted to point out that another cool benefit to ReflectBox is its ability to work with any type T that can be converted to a dyn PartialReflect (using a couple traits). This includes other trait objects like dyn MyCoolTrait.

If we do end up going with this PR, it might be worthwhile trying to get that behavior to work as well (possibly using a similar approach as ReflectBox). Either in this PR or a followup one.

[alice-i-cecile]

Closing in favor of #14776.