bugfix: Reflection is not symmetrical

[pikerpoler]

Objective

fixing #15712

before:

• unit structs were serialized into null
• deserializing null caused an error
• empty arrays deserialized into unit structs

after:

• unit structs are serialized into null
• null deserialize into unit structs
• deserializing empty arrays cause an error

Solution

checking for the number of fields in the reflect struct and deserializing the json using deserialize_unit which handles the types correctly additionally I had to (trivially) implement visit_unit because the default implementation fails with a type error

Testing

for testing I added a main file (which is not included as a file change for obvious reasons) with the following code:

fn main() {}

mod tests {
    use std::fmt::Debug;
    use std::str::FromStr;

    use bevy::{prelude::*, reflect::serde::TypedReflectDeserializer};
    use serde::de::DeserializeSeed;

    use serde::{Deserialize, Serialize};

    #[derive(Reflect, Default, Debug, PartialEq, Serialize, Deserialize)]
    struct UnitStruct;

    fn test_equivalence<T>(w: &World, json_raw: &str, val: T)
    where
        T: Serialize + for<'de> Deserialize<'de> + Debug + Reflect + FromReflect + PartialEq,
    {
        // Json raw to json value
        let json_val = serde_json::Value::from_str(json_raw).unwrap();

        // Json value to reflect
        let type_registry = w.resource::<AppTypeRegistry>().read();
        let registration = type_registry.get(std::any::TypeId::of::<T>()).unwrap();
        let deserializer = TypedReflectDeserializer::new(registration, &type_registry);
        let reflected = deserializer.deserialize(json_val.clone()).unwrap();

        // Reflect to val
        let extracted = T::from_reflect(reflected.as_ref()).unwrap();
        assert_eq!(val, extracted);

        // Val to json value
        let json_val_deser = serde_json::to_value(&val).unwrap();

        assert_eq!(json_val, json_val_deser);
    }

    #[test]
    fn bug() {
        let mut app = App::new();
        app.register_type::<UnitStruct>();
        test_equivalence(app.world(), "null", UnitStruct);
    }
}

for this code to run, you need to add a serde_json dependancy to Aargo.toml

serde_json = { version = "1.0", features = ["preserve_order"] }

I also tested manually to check if deserializing an empty array as a unit object causes an error which it does.

println!("My super cool code.");

Migration Guide

If you serialized empty arrays into unit objects, now you need to replace them with nulls.

"[]" -> "null" json!([]) -> json!(null)

[github-actions[bot]]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[BenjaminBrienen]

after:

• deserializing empty arrays cause an error

Why is this the desired result? Is it ever possible to serialize into an empty array?

[pikerpoler]

Really well done! Can you please add a couple unit tests to demonstrate that it works and to prevent regressions?

I am working on it. right after I published the PR I realized the dev-dependencies include serde_json which should allow testing, but I'm still figuring this out.

[UkoeHB]

@pikerpoler unfortunately that fix is not strictly valid, because #[reflect(ignore)] can cause a struct to 'appear' to have zero fields to reflection when in fact it has fields.

Also, this PR closes #15712

[pikerpoler]

@pikerpoler unfortunately that fix is not strictly valid, because #[reflect(ignore)] can cause a struct to 'appear' to have zero fields to reflection when in fact it has fields.

we may solve it in a different way, using ReflectDerive::UnitStruct instead. I considered this initially, but it may require a larger refactor, and this suggestion worked for my tests:

...We can probably do that pretty easy with a simple field length check (being sure to exclude skip_serializing fields).

the section I think of changing is this: https://github.com/bevyengine/bevy/blob/efd2982a881e8756dac14e740c407b249afeefaa/crates/bevy_reflect/derive/src/remote.rs#L52

checking weather it's a UnitStruct via an enum instead of a field check is probably better anyway for readability, but It will take a bit more time to figre out and test

[pikerpoler]

after:

• deserializing empty arrays cause an error

Why is this the desired result? Is it ever possible to serialize into an empty array?

I tried to play with it a bit, and I couldn't, thought I am not sure yet if it's theoretically impossible