Non-panicking Version of Reflect.apply()

[zicklag]

What problem does this solve or what need does it fill?

While working on a reflect-based scripting solution, I've run into the need to try and apply() one reflect value to another, but I can't have it panic if it fails, I need to return an error instead.

What solution would you like?

It would be nice to have maybe a try_apply method or just return a result from the existing apply method instead of panicking.

What alternative(s) have you considered?

I tried to use catch_unwind to capture the panic, but I'm working with types that are not unwind safe, so it's not an option for me in this case.

I believe I'll have to compare the values that I'm applying manually, with a match on their ReflectRef's in order to see if they are compatible, before doing the apply.

---

Another alternative would be to provide an is_compatible() method or something like that, that's essentially just a check on whether or not an apply would succeed, but I'm not sure if that's more useful or not. I still feel like panicking by default here isn't the best option.

Additional context

None yet.

[zicklag]

I believe this check is enough to ensure that the two reflects are compatible before applying, which is honestly not as bad as I thought it was going to be:

// Assuming reflect1 and reflect2 are both `dyn Reflect`
let is_compatible = match reflect1.reflect_ref() {
    ref1 @ (ReflectRef::Struct(_)
    | ReflectRef::TupleStruct(_)
    | ReflectRef::Tuple(_)
    | ReflectRef::List(_)
    | ReflectRef::Array(_)
    | ReflectRef::Map(_)) => {
        std::mem::discriminant(&ref1)
            == std::mem::discriminant(&reflect2.reflect_ref())
    }
    ReflectRef::Value(value1) => match reflect2.reflect_ref() {
        ReflectRef::Value(value2) => value1.type_id() == value2.type_id(),
        _ => false,
    },
}

Edit: Nevermind, now that I think about it, we'd have to do something like this recursively. :/

[zicklag]

I think this might be enough to validate compatibility between reflect values, but I haven't tested it:

fn reflect_is_compatible(reflect1: &dyn Reflect, reflect2: &dyn Reflect) -> bool {
    match (reflect1.reflect_ref(), reflect2.reflect_ref()) {
        (ReflectRef::Value(value1), ReflectRef::Value(value2)) => {
            value1.type_id() == value2.type_id()
        }
        (ReflectRef::Array(arr1), ReflectRef::Array(arr2)) => {
            arr1.iter()
                .zip(arr2.iter())
                .fold(true, |compatible, (reflect1, reflect2)| {
                    compatible && reflect_is_compatible(reflect1, reflect2)
                })
        }
        (ReflectRef::List(list1), ReflectRef::List(list2)) => {
            list1
                .iter()
                .zip(list2.iter())
                .fold(true, |compatible, (reflect1, reflect2)| {
                    compatible && reflect_is_compatible(reflect1, reflect2)
                })
        }
        (ReflectRef::Tuple(tuple1), ReflectRef::Tuple(tuple2)) => tuple1
            .iter_fields()
            .zip(tuple2.iter_fields())
            .fold(true, |compatible, (reflect1, reflect2)| {
                compatible && reflect_is_compatible(reflect1, reflect2)
            }),
        (ReflectRef::TupleStruct(tuple1), ReflectRef::TupleStruct(tuple2)) => tuple1
            .iter_fields()
            .zip(tuple2.iter_fields())
            .fold(true, |compatible, (reflect1, reflect2)| {
                compatible && reflect_is_compatible(reflect1, reflect2)
            }),
        (ReflectRef::Struct(struct1), ReflectRef::Struct(struct2)) => struct1
            .iter_fields()
            .enumerate()
            .fold(true, |compatible, (i, field1)| {
                if let Some(field2) = struct2.field(struct1.name_at(i).unwrap()) {
                    compatible && reflect_is_compatible(field1, field2)
                } else {
                    compatible
                }
            }),
        _ => false,
    }
}

Edit: OK, I wrote some tests. It's by no means exhausted, but it seems to check out so far:

mod test {
    use super::*;

    #[derive(Reflect, Default)]
    struct S1 {
        a: String,
        b: f32,
    }

    #[derive(Reflect, Default)]
    struct S2 {
        a: String,
        b: f32,
        c: u32,
    }

    #[derive(Reflect, Default)]
    struct S3 {
        a: String,
        b: u32,
    }

    #[test]
    fn test_reflect_is_compatible_check() {
        let string = Box::new(String::default()) as Box<dyn Reflect>;
        let uint = Box::new(0u32) as Box<dyn Reflect>;
        let mut s1 = Box::new(S1::default()) as Box<dyn Reflect>;
        let s2 = Box::new(S2::default()) as Box<dyn Reflect>;
        let s3 = Box::new(S3::default()) as Box<dyn Reflect>;

        assert!(!reflect_is_compatible(
            uint.as_reflect(),
            string.as_reflect()
        ));
        assert!(!reflect_is_compatible(s1.as_reflect(), string.as_reflect()));

        assert!(reflect_is_compatible(s1.as_reflect(), s2.as_reflect()));
        s1.apply(s2.as_reflect());

        assert!(!reflect_is_compatible(s1.as_reflect(), s3.as_reflect()));

        let mut l1 = Box::new(vec![1, 2, 3]) as Box<dyn Reflect>;
        let l2 = Box::new(vec![5, 4, 3, 2, 1]) as Box<dyn Reflect>;

        assert!(reflect_is_compatible(l1.as_reflect(), l2.as_reflect()));
        l1.apply(l2.as_reflect());
        assert!(!reflect_is_compatible(l1.as_reflect(), s1.as_reflect()));

        let mut t1 = Box::new((0u32, String::from("hi"))) as Box<dyn Reflect>;
        let t2 = Box::new((1u32, String::from("bye"))) as Box<dyn Reflect>;
        let t3 = Box::new((0f32, String::from("bye"))) as Box<dyn Reflect>;

        assert!(reflect_is_compatible(t1.as_reflect(), t2.as_reflect()));
        t1.apply(t2.as_reflect());
        assert!(!reflect_is_compatible(t1.as_reflect(), t3.as_reflect()));
    }
}

[alice-i-cecile]

I definitely like either the try_apply or simply returning a Result over having to remember to check first. That sort of guard pattern is very easy to forget.

I think my preference is probably to just return a Result'; this API is easy to panic with and the ergonomics aren't essential due to how rare it is in most code.

[MrGVSV]

Yeah I prefer returning a Result from Reflect::apply as well.

[feyokorenhof]

I'm looking for a fun issue to make my first contribution and I'd like to try drafting something up to potentially fix this issue.
I am not new to programming but I am very new to Rust so it will probably take a few tries before I get it right, I hope that's okay :).

Correct me if I'm wrong but I understand this has to be changed:

The apply function for every iterable in bevy_reflect/src and bevy_reflect/src/impls has to be changed to return a Result instead of panicking.
I notice that it is propagating the call to the actual apply function depending on the type eg. array_apply for arrays. These would also return this same result so the user can deal with it on their side.

I am however unsure of the type of Error I would have to return. Should I make a new Error enum especially for this situation like a ReflectError or ApplyError, or would a more generic Error be okay?

[alice-i-cecile]

Yep, that sounds about right :) Make a new error type for sure, and use the thiserror crate to help write good error messages.

[MrGVSV]

This sort of came up on Discord recently, albeit in regards to my work on diffing. If we end up returning a Result, we have to make sure that it's clear to the user the effects of that failure. Specifically, the user can end up with a broken/half-transformed value.

This is because we mutate self as we go along. If we fail sometime down the road, the value has not been fully applied to self.

@zicklag's suggestion to check beforehand using Reflect::reflect_ref helps, but might not account for everything (such as being given the wrong type, which is possible when using Dynamic types).

This might be okay as long as we make it explicit that running this method could leave the value in a broken/half-applied state. Alternatively, we could choose to take mut self: Box<Self> and return Result<Box<dyn Reflect>, ReflectApplyError> instead of mutating directly.

Just wanted to point this out before you dive in headfirst @feyokorenhof. Feel free to drop a message in the #reflection-dev channel on Discord if you need any help!

[feyokorenhof]

@MrGVSV thanks for the heads up!

I thought the apply function doesn't mutate self unless the types are equal. Or are you talking about being able to return an error whilst applying? Which i would also be happy to try and implement.

My understanding of Rust is very little at the moment and right now i'm kinda stuck on some (probably obvious) errors but i'm eager to learn more and really get into it :)

edit: nevermind my first question i see what you mean after looking a bit more closely at @zicklag's code. I like your idea of return a new value so the user can perform checks on it themselves. With good documentation that should be pretty solid.