bevy_reflect: Ignored field order

[MrGVSV]

Objective

Fixes #5101

Solution

Make it so that placing #[reflect(ignore)] or #[reflect(skip_serializing)] on a field that is not at the end of the type definition results in a compile error.

Rationale

Invalid ordering is a common and annoying footgun that can be very difficult to debug— or at least very frustrating. Ignoring a field in the middle of your struct can throw off the index you get back from Struct::index_of and friends. This is made even worse for things like tuple structs, where doing so breaks both FromReflect and serialization.

It should not be so easy to completely break reflection when using the derive macro. Thus, I think it warrants an error so we prevent users from entering this state in the first place.

Alternatives

One alternative, would be to output a compiler warning. Unfortunately, that API is still unstable.

We could also print out a warning at runtime whenever we rely on ordering. However, as discussed in this comment, we run the risk of some things not being caught (namely by third-party crates). Doing this is also just more cumbersome and prone to error.

Lastly, we could potentially store some sort of mapping that gives us the correct declaration index for a given index. This might work, but makes things like FromReflect a bit more difficult to implement. It also means we need to add a public API to access this information, which may be confusing/unintuitive. That being said, this is probably the most viable alternative out of the three.

---

Changelog

• #[reflect(ignore)] and #[reflect(skip_serializing)] attributes now result in a compile error if placed in an invalid order

Migration Guide

Fields marked with #[reflect(ignore)] must now be placed at the end of their respective type definition. This applies to all structs, tuple structs, and enum variants.

// OLD:
#[derive(Reflect)]
struct MyStruct {
  #[reflect(ignore)] // <- ERROR!
  _ignore_me: i32,
  foo: i32,
  bar: i32,
}

// NEW:
#[derive(Reflect)]
struct MyStruct {
  foo: i32,
  bar: i32,
  #[reflect(ignore)] // <- OK!
  _ignore_me: i32,
}

Additionally, #[reflect(skip_serializing)] must also be placed at the end of the type definition. However, it must also come before #[reflect(ignore)].

// OLD:
#[derive(Reflect)]
struct MyStruct {
  #[reflect(ignore)] // <- ERROR!
  _ignore_me: i32,
  #[reflect(skip_serializing)] // <-ERROR!
  _skip_me: i32,
  foo: i32,
}

// NEW:
#[derive(Reflect)]
struct MyStruct {
  foo: i32,
  #[reflect(skip_serializing)] // <- OK!
  _skip_me: i32,
  #[reflect(ignore)] // <- OK!
  _ignore_me: i32,
}

[nicopap]

LGTM, would be better with compilation failure test on ignore attribute used not-at-the-end.

[nicopap]

Nit: The match statements can be flattened here. Something like:

Meta::NameValue(MetaNameValue { lit: Lit::Str(lit_str), path, .. } if path.is…

Though not sure how cleaner that is.

[MrGVSV]

Hm, yeah I'm not sure how much we gain from that if we need to then duplicate the arm to handle the error:

// Handle good value:
Meta::NameValue(MetaNameValue { lit: Lit::Str(lit_str), path, .. } if path.is…
// Handle bad value:
Meta::NameValue(MetaNameValue { lit, path, .. } if path.is…

[MrGVSV]

Blocked on #7041. Moving to draft until that PR is merged.

[MrGVSV]

This should be good to go now!

[cart]

This is a logical fix to a real problem! Users deriving reflect like this already have control over their field orders, so asking them to change the order in these cases seems reasonable in most cases. However, theres always the chance that field order matters for some other reason (ex: the type is also used for binary serialization: RPC, bincode, GPU types, etc).

Given that Reflect aims to be a "general purpose reflection library", I think breaking these cases will eventually be an issue, both in Bevy projects and elsewhere.

[MrGVSV]

Fair enough. Would it be better to put this behind a default feature? Or at least provide an opt-out attribute like #[reflect(allow_ignore_order)] (or some other better name lol)?

[cart]

If opting out breaks the scenarios we're trying to fix in this PR (FromReflect and serialization), and this PR breaks other scenarios (reflecting structs that needs specific field orders), I think thats an indicator that we need to rethink our implementation generally / come up with a solution that handles all of these scenarios correctly.

Specifically: is it possible for us to support cases where reflected field indices don't line up exactly with declared field indices. Or alternatively, can we adjust our reflection apis to account for "non-existent / skipped" fields (ensuring reflected field indices always match declared field indices). If either of these are possible, what are the costs? Performance? Ergonomics? Do we need to mitigate those costs (ex: provide opt-in/out fast paths where possible)?

[MrGVSV]

Specifically: is it possible for us to support cases where reflected field indices don't line up exactly with declared field indices. Or alternatively, can we adjust our reflection apis to account for "non-existent / skipped" fields (ensuring reflected field indices always match declared field indices).

I just put up an alternative PR (#7575) which is more in line with this. The only downside is that it requires fields with #[reflect(skip_serializing)] to provide some way of generating a default instance (either with a Default impl or with a custom function). As noted in the PR, though, I think it makes sense to add such a requirement.

[cart]

Brilliant!

[MrGVSV]

Closing in favor of #7575