bevy_reflect: ignored fields are not ignored by `FromReflect`

[nicopap]

bevy version: main as of 1bd33ca

When deriving FromReflect on tuple structs, the field index of fields declared after a field marked with #[reflect(ignore)] are not as one would expect them.

Here is an example:

#[derive(Reflect, FromReflect)]
struct Foo(
  u32, // :0
  #[reflect(ignore)]
  f32, // :1
  String, // :2
);

let mut tuple_struct = DynamicTupleStruct::default();
tuple_struct.set_name("Foo".to_owned());
tuple_struct.insert(22_u32);
tuple_struct.insert(3432423234234_i128);
tuple_struct.insert("hello world".to_owned());

let foo = Foo::from_reflect(&tuple_struct).unwrap();

println!(
    "tuple_struct\n[0]: {:?}\n[1]: {:?}\n[2]: {:?}\n",
    tuple_struct.field(0),
    tuple_struct.field(1),
    tuple_struct.field(2),
);
println!(
    "foo\n[0]: {:?}\n[1]: {:?}\n[2]: {:?}\n",
    foo.field(0),
    foo.field(1),
    foo.field(2).
);

Prints the following:

tuple_struct
[0]: Some(22)
[1]: Some(3432423234234)
[2]: Some("hello world")

foo
[0]: Some(22)
[1]: Some("hello world")
[2]: None

The unexpected behavior is here:

let mut tuple_struct = DynamicTupleStruct::default();
tuple_struct.set_name("Foo".to_owned());
tuple_struct.insert(22_u32);
// vvv required, otherwise Foo::from_reflect returns None
tuple_struct.insert(3432423234234_i128);
tuple_struct.insert("hello world".to_owned());

let foo = Foo::from_reflect(&tuple_struct).unwrap();

You'd expect to be able to build Foo from a DynamicTupleStruct with exactly two fields, not three. The correct behavior would be to account for it in the FromReflect implementation.

[MrGVSV]

This affects everything actually, not just FromReflect. Calling TupleStruct::field_at will have a similar problem.

So this more generally relates to the behavior of field indices on structs with ignored fields as a whole.

And while it shows up most noticeably in FromReflect it could appear also outside of the reflection API (should users rely on reflection to get/set non-reflected fields by index).

[nicopap]

Calling TupleStruct::field_at will have a similar problem.

The sample example demonstrates it's not the case. In the following code .field is the method on the TupleStruct trait

println!(
    "foo\n[0]: {:?}\n[1]: {:?}\n[2]: {:?}\n",
    foo.field(0),
    foo.field(1),
    foo.field(2).
);

The output shown in the issue post demonstrates it works as expected.

[MrGVSV]

Calling TupleStruct::field_at will have a similar problem.

The sample example demonstrates it's not the case. In the following code .field is the method on the TupleStruct trait

println!(

    "foo\n[0]: {:?}\n[1]: {:?}\n[2]: {:?}\n",

    foo.field(0),

    foo.field(1),

    foo.field(2).

);

The output shown in the issue post demonstrates it works as expected.

This should print:

foo
[0]: Some(22)
[1]: Some("hello world")
[2]: None

Which correctly ignores the field at index 1.

The issue I was generalizing this to is regarding whether the reflected index should be the same as the declaration index or not (e.g. swap 1 and 2 in that print statement).

A user could take this info and try to edit a non-reflected instance to confusing results.

We could simply disregard those use cases and say, "Don't rely on reflection outside of reflection."

That's a totally fair stance I think haha, but we could also consider adding a method to convert reflected indices to declaration indices:

foo.decl_index(1); // Some(2)

Or, as suggested by others, print a warning if the ignored fields don't come last.

Just wanted to possibly open this up to other solutions if that's what we want to look into. If not, then solving it at least for FromReflect is good enough.

[MrGVSV]

Thinking on this a bit, I feel like a good start would be to implement the warning system. Making sure users know to put these fields last will help protect them from running into this issue in the first place.

With that as a temporary¹ fix, we can begin to explore other ways of dealing with this problem.

Footnotes

1. Possibly not temporary depending on if a better solution can't be found ↩

[MrGVSV]

Actually, it might not be possible to emit a warning since that API is still unstable. We could maybe consider making this a full on error.

It might make sense considering we don't actually support deserializing tuple structs with a #[reflect(skip_serializing)] marker anywhere but the end. Otherwise, it fails to deserialize and we end up with the warning:

WARN bevy_asset::asset_server: encountered an error while loading an asset: Expected integer

Or worse, it ends up putting a value into the skipped field anyways 😬.

Alternatively, we could print a warning only when (de)serializing or calling FromReflect (or whatever else breaks because of this) to let the user know that things might not work correctly. However, if a third-party crate doesn't test that their reflected types serialize properly or that it doesn't use FromReflect properly, then consumers of that crate won't be able to correct the behavior themselves (not without forking or creating a PR at least).

This would require us adding some sort of API to check if there are any misplaced ignores/skips, though, since we don't always have the registry (i.e. in FromReflect) to get that info from type data (which is where we at least store skip_serializing info).

---

I think we should go with the compile error (I wish we could do compile warnings). However, I understand and am open to the second option of just printing a warning during (de)serialization/FromReflect. I just think we should prevent this incorrect behavior for everything rather than just rely on warnings being conditionally printed out (and the first option is also just a lot simpler/cleaner to implement). Any other thoughts?