Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bevy_reflect: Ignored field order #6511

Closed
wants to merge 5 commits into from

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented Nov 7, 2022

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,
}

@MrGVSV MrGVSV force-pushed the reflect-ignore-order-error branch from d34e61f to babb1e8 Compare November 7, 2022 09:37
@MrGVSV MrGVSV added C-Bug An unexpected or incorrect behavior A-Reflection Runtime information about types M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed C-Bug An unexpected or incorrect behavior labels Nov 7, 2022
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +156 to +169
Meta::NameValue(pair) if pair.path.is_ident(DEFAULT_ATTR) => {
let lit = &pair.lit;
match lit {
Lit::Str(lit_str) => {
args.default = DefaultBehavior::Func(lit_str.parse()?);
Ok(())
}
err => {
Err(syn::Error::new(
err.span(),
format!("expected a string literal containing the name of a function, but found: {}", err.to_token_stream()),
))
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 MrGVSV force-pushed the reflect-ignore-order-error branch 2 times, most recently from 867f9c0 to 9443d76 Compare December 27, 2022 20:00
@MrGVSV
Copy link
Member Author

MrGVSV commented Dec 27, 2022

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

@MrGVSV MrGVSV marked this pull request as draft December 27, 2022 20:39
bors bot pushed a commit that referenced this pull request Jan 2, 2023
# Objective

There isn't really a way to test that code using bevy_reflect compiles or doesn't compile for certain scenarios. This would be especially useful for macro-centric PRs like #6511 and #6042.

## Solution

Using `bevy_ecs_compile_fail_tests` as reference, added the `bevy_reflect_compile_fail_tests` crate.

Currently, this crate contains a very simple test case. This is so that we can get the basic foundation of this crate agreed upon and merged so that more tests can be added by other PRs.

### Open Questions

- [x] Should this be added to CI? (Answer: Yes)

---

## Changelog

- Added the `bevy_reflect_compile_fail_tests` crate for testing compilation errors
@MrGVSV MrGVSV force-pushed the reflect-ignore-order-error branch from 9443d76 to f591183 Compare January 2, 2023 21:33
@MrGVSV MrGVSV marked this pull request as ready for review January 2, 2023 21:33
@MrGVSV
Copy link
Member Author

MrGVSV commented Jan 2, 2023

This should be good to go now!

@MrGVSV MrGVSV added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 2, 2023
@james7132 james7132 added this to the 0.10 milestone Jan 4, 2023
}

/// Verifies `#[reflect(ignore)]` attributes are always last in the type definition.
fn check_ignore_order(&self, args: &ReflectFieldAttr, errors: &mut Option<syn::Error>) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Member

@cart cart Jan 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliant!

@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Jan 9, 2023
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
# Objective

There isn't really a way to test that code using bevy_reflect compiles or doesn't compile for certain scenarios. This would be especially useful for macro-centric PRs like bevyengine#6511 and bevyengine#6042.

## Solution

Using `bevy_ecs_compile_fail_tests` as reference, added the `bevy_reflect_compile_fail_tests` crate.

Currently, this crate contains a very simple test case. This is so that we can get the basic foundation of this crate agreed upon and merged so that more tests can be added by other PRs.

### Open Questions

- [x] Should this be added to CI? (Answer: Yes)

---

## Changelog

- Added the `bevy_reflect_compile_fail_tests` crate for testing compilation errors
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

There isn't really a way to test that code using bevy_reflect compiles or doesn't compile for certain scenarios. This would be especially useful for macro-centric PRs like bevyengine#6511 and bevyengine#6042.

## Solution

Using `bevy_ecs_compile_fail_tests` as reference, added the `bevy_reflect_compile_fail_tests` crate.

Currently, this crate contains a very simple test case. This is so that we can get the basic foundation of this crate agreed upon and merged so that more tests can be added by other PRs.

### Open Questions

- [x] Should this be added to CI? (Answer: Yes)

---

## Changelog

- Added the `bevy_reflect_compile_fail_tests` crate for testing compilation errors
@cart cart removed this from the 0.10 milestone Feb 7, 2023
@MrGVSV
Copy link
Member Author

MrGVSV commented Mar 28, 2023

Closing in favor of #7575

@MrGVSV MrGVSV closed this Mar 28, 2023
github-merge-queue bot pushed a commit that referenced this pull request Oct 22, 2023
# Objective

Fixes #5101
Alternative to #6511

## Solution

Corrected the behavior for ignored fields in `FromReflect`, which was
previously using the incorrect field indexes.

Similarly, fields marked with `#[reflect(skip_serializing)]` no longer
break when using `FromReflect` after deserialization. This was done by
modifying `SerializationData` to store a function pointer that can later
be used to generate a default instance of the skipped field during
deserialization.

The function pointer points to a function generated by the derive macro
using the behavior designated by `#[reflect(default)]` (or just
`Default` if none provided). The entire output of the macro is now
wrapped in an [unnamed
constant](https://doc.rust-lang.org/stable/reference/items/constant-items.html#unnamed-constant)
which keeps this behavior hygienic.

#### Rationale

The biggest downside to this approach is that it requires fields marked
`#[reflect(skip_serializing)]` to provide the ability to create a
default instance— either via a `Default` impl or by specifying a custom
one. While this isn't great, I think it might be justified by the fact
that we really need to create this value when using `FromReflect` on a
deserialized object. And we need to do this _during_ deserialization
because after that (at least for tuples and tuple structs) we lose
information about which field is which: _"is the value at index 1 in
this `DynamicTupleStruct` the actual value for index 1 or is it really
the value for index 2 since index 1 is skippable...?"_

#### Alternatives

An alternative would be to store `Option<Box<dyn Reflect>>` within
`DynamicTuple` and `DynamicTupleStruct` instead of just `Box<dyn
Reflect>`. This would allow us to insert "empty"/"missing" fields during
deserialization, thus saving the positional information of the skipped
fields. However, this may require changing the API of `Tuple` and
`TupleStruct` such that they can account for their dynamic counterparts
returning `None` for a skipped field. In practice this would probably
mean exposing the `Option`-ness of the dynamics onto implementors via
methods like `Tuple::drain` or `TupleStruct::field`.

Personally, I think requiring `Default` would be better than muddying up
the API to account for these special cases. But I'm open to trying out
this other approach if the community feels that it's better.

---

## Changelog

### Public Changes

#### Fixed

- The behaviors of `#[reflect(ignore)]` and
`#[reflect(skip_serializing)]` are no longer dependent on field order

#### Changed

- Fields marked with `#[reflect(skip_serializing)]` now need to either
implement `Default` or specify a custom default function using
`#[reflect(default = "path::to::some_func")]`
- Deserializing a type with fields marked `#[reflect(skip_serializing)]`
will now include that field initialized to its specified default value
- `SerializationData::new` now takes the new `SkippedField` struct along
with the skipped field index
- Renamed `SerializationData::is_ignored_field` to
`SerializationData::is_field_skipped`

#### Added

- Added `SkippedField` struct
- Added methods `SerializationData::generate_default` and
`SerializationData::iter_skipped`

### Internal Changes

#### Changed

- Replaced `members_to_serialization_denylist` and `BitSet<u32>` with
`SerializationDataDef`
- The `Reflect` derive is more hygienic as it now outputs within an
[unnamed
constant](https://doc.rust-lang.org/stable/reference/items/constant-items.html#unnamed-constant)
- `StructField::index` has been split up into
`StructField::declaration_index` and `StructField::reflection_index`

#### Removed

- Removed `bitset` dependency

## Migration Guide

* Fields marked `#[reflect(skip_serializing)]` now must implement
`Default` or specify a custom default function with `#[reflect(default =
"path::to::some_func")]`
    ```rust
    #[derive(Reflect)]
    struct MyStruct {
      #[reflect(skip_serializing)]
      #[reflect(default = "get_foo_default")]
foo: Foo, // <- `Foo` does not impl `Default` so requires a custom
function
      #[reflect(skip_serializing)]
      bar: Bar, // <- `Bar` impls `Default`
    }
    
    #[derive(Reflect)]
    struct Foo(i32);
    
    #[derive(Reflect, Default)]
    struct Bar(i32);
    
    fn get_foo_default() -> Foo {
      Foo(123)
    }
    ```
* `SerializationData::new` has been changed to expect an iterator of
`(usize, SkippedField)` rather than one of just `usize`
    ```rust
    // BEFORE
    SerializationData::new([0, 3].into_iter());
    
    // AFTER
    SerializationData::new([
      (0, SkippedField::new(field_0_default_fn)),
      (3, SkippedField::new(field_3_default_fn)),
    ].into_iter());
    ```
* `Serialization::is_ignored_field` has been renamed to
`Serialization::is_field_skipped`
* Fields marked `#[reflect(skip_serializing)]` are now included in
deserialization output. This may affect logic that expected those fields
to be absent.
robtfm pushed a commit to robtfm/bevy that referenced this pull request Oct 23, 2023
# Objective

Fixes bevyengine#5101
Alternative to bevyengine#6511

## Solution

Corrected the behavior for ignored fields in `FromReflect`, which was
previously using the incorrect field indexes.

Similarly, fields marked with `#[reflect(skip_serializing)]` no longer
break when using `FromReflect` after deserialization. This was done by
modifying `SerializationData` to store a function pointer that can later
be used to generate a default instance of the skipped field during
deserialization.

The function pointer points to a function generated by the derive macro
using the behavior designated by `#[reflect(default)]` (or just
`Default` if none provided). The entire output of the macro is now
wrapped in an [unnamed
constant](https://doc.rust-lang.org/stable/reference/items/constant-items.html#unnamed-constant)
which keeps this behavior hygienic.

#### Rationale

The biggest downside to this approach is that it requires fields marked
`#[reflect(skip_serializing)]` to provide the ability to create a
default instance— either via a `Default` impl or by specifying a custom
one. While this isn't great, I think it might be justified by the fact
that we really need to create this value when using `FromReflect` on a
deserialized object. And we need to do this _during_ deserialization
because after that (at least for tuples and tuple structs) we lose
information about which field is which: _"is the value at index 1 in
this `DynamicTupleStruct` the actual value for index 1 or is it really
the value for index 2 since index 1 is skippable...?"_

#### Alternatives

An alternative would be to store `Option<Box<dyn Reflect>>` within
`DynamicTuple` and `DynamicTupleStruct` instead of just `Box<dyn
Reflect>`. This would allow us to insert "empty"/"missing" fields during
deserialization, thus saving the positional information of the skipped
fields. However, this may require changing the API of `Tuple` and
`TupleStruct` such that they can account for their dynamic counterparts
returning `None` for a skipped field. In practice this would probably
mean exposing the `Option`-ness of the dynamics onto implementors via
methods like `Tuple::drain` or `TupleStruct::field`.

Personally, I think requiring `Default` would be better than muddying up
the API to account for these special cases. But I'm open to trying out
this other approach if the community feels that it's better.

---

## Changelog

### Public Changes

#### Fixed

- The behaviors of `#[reflect(ignore)]` and
`#[reflect(skip_serializing)]` are no longer dependent on field order

#### Changed

- Fields marked with `#[reflect(skip_serializing)]` now need to either
implement `Default` or specify a custom default function using
`#[reflect(default = "path::to::some_func")]`
- Deserializing a type with fields marked `#[reflect(skip_serializing)]`
will now include that field initialized to its specified default value
- `SerializationData::new` now takes the new `SkippedField` struct along
with the skipped field index
- Renamed `SerializationData::is_ignored_field` to
`SerializationData::is_field_skipped`

#### Added

- Added `SkippedField` struct
- Added methods `SerializationData::generate_default` and
`SerializationData::iter_skipped`

### Internal Changes

#### Changed

- Replaced `members_to_serialization_denylist` and `BitSet<u32>` with
`SerializationDataDef`
- The `Reflect` derive is more hygienic as it now outputs within an
[unnamed
constant](https://doc.rust-lang.org/stable/reference/items/constant-items.html#unnamed-constant)
- `StructField::index` has been split up into
`StructField::declaration_index` and `StructField::reflection_index`

#### Removed

- Removed `bitset` dependency

## Migration Guide

* Fields marked `#[reflect(skip_serializing)]` now must implement
`Default` or specify a custom default function with `#[reflect(default =
"path::to::some_func")]`
    ```rust
    #[derive(Reflect)]
    struct MyStruct {
      #[reflect(skip_serializing)]
      #[reflect(default = "get_foo_default")]
foo: Foo, // <- `Foo` does not impl `Default` so requires a custom
function
      #[reflect(skip_serializing)]
      bar: Bar, // <- `Bar` impls `Default`
    }
    
    #[derive(Reflect)]
    struct Foo(i32);
    
    #[derive(Reflect, Default)]
    struct Bar(i32);
    
    fn get_foo_default() -> Foo {
      Foo(123)
    }
    ```
* `SerializationData::new` has been changed to expect an iterator of
`(usize, SkippedField)` rather than one of just `usize`
    ```rust
    // BEFORE
    SerializationData::new([0, 3].into_iter());
    
    // AFTER
    SerializationData::new([
      (0, SkippedField::new(field_0_default_fn)),
      (3, SkippedField::new(field_3_default_fn)),
    ].into_iter());
    ```
* `Serialization::is_ignored_field` has been renamed to
`Serialization::is_field_skipped`
* Fields marked `#[reflect(skip_serializing)]` are now included in
deserialization output. This may affect logic that expected those fields
to be absent.
ameknite pushed a commit to ameknite/bevy that referenced this pull request Nov 6, 2023
Fixes bevyengine#5101
Alternative to bevyengine#6511

Corrected the behavior for ignored fields in `FromReflect`, which was
previously using the incorrect field indexes.

Similarly, fields marked with `#[reflect(skip_serializing)]` no longer
break when using `FromReflect` after deserialization. This was done by
modifying `SerializationData` to store a function pointer that can later
be used to generate a default instance of the skipped field during
deserialization.

The function pointer points to a function generated by the derive macro
using the behavior designated by `#[reflect(default)]` (or just
`Default` if none provided). The entire output of the macro is now
wrapped in an [unnamed
constant](https://doc.rust-lang.org/stable/reference/items/constant-items.html#unnamed-constant)
which keeps this behavior hygienic.

The biggest downside to this approach is that it requires fields marked
`#[reflect(skip_serializing)]` to provide the ability to create a
default instance— either via a `Default` impl or by specifying a custom
one. While this isn't great, I think it might be justified by the fact
that we really need to create this value when using `FromReflect` on a
deserialized object. And we need to do this _during_ deserialization
because after that (at least for tuples and tuple structs) we lose
information about which field is which: _"is the value at index 1 in
this `DynamicTupleStruct` the actual value for index 1 or is it really
the value for index 2 since index 1 is skippable...?"_

An alternative would be to store `Option<Box<dyn Reflect>>` within
`DynamicTuple` and `DynamicTupleStruct` instead of just `Box<dyn
Reflect>`. This would allow us to insert "empty"/"missing" fields during
deserialization, thus saving the positional information of the skipped
fields. However, this may require changing the API of `Tuple` and
`TupleStruct` such that they can account for their dynamic counterparts
returning `None` for a skipped field. In practice this would probably
mean exposing the `Option`-ness of the dynamics onto implementors via
methods like `Tuple::drain` or `TupleStruct::field`.

Personally, I think requiring `Default` would be better than muddying up
the API to account for these special cases. But I'm open to trying out
this other approach if the community feels that it's better.

---

- The behaviors of `#[reflect(ignore)]` and
`#[reflect(skip_serializing)]` are no longer dependent on field order

- Fields marked with `#[reflect(skip_serializing)]` now need to either
implement `Default` or specify a custom default function using
`#[reflect(default = "path::to::some_func")]`
- Deserializing a type with fields marked `#[reflect(skip_serializing)]`
will now include that field initialized to its specified default value
- `SerializationData::new` now takes the new `SkippedField` struct along
with the skipped field index
- Renamed `SerializationData::is_ignored_field` to
`SerializationData::is_field_skipped`

- Added `SkippedField` struct
- Added methods `SerializationData::generate_default` and
`SerializationData::iter_skipped`

- Replaced `members_to_serialization_denylist` and `BitSet<u32>` with
`SerializationDataDef`
- The `Reflect` derive is more hygienic as it now outputs within an
[unnamed
constant](https://doc.rust-lang.org/stable/reference/items/constant-items.html#unnamed-constant)
- `StructField::index` has been split up into
`StructField::declaration_index` and `StructField::reflection_index`

- Removed `bitset` dependency

* Fields marked `#[reflect(skip_serializing)]` now must implement
`Default` or specify a custom default function with `#[reflect(default =
"path::to::some_func")]`
    ```rust
    #[derive(Reflect)]
    struct MyStruct {
      #[reflect(skip_serializing)]
      #[reflect(default = "get_foo_default")]
foo: Foo, // <- `Foo` does not impl `Default` so requires a custom
function
      #[reflect(skip_serializing)]
      bar: Bar, // <- `Bar` impls `Default`
    }

    #[derive(Reflect)]
    struct Foo(i32);

    #[derive(Reflect, Default)]
    struct Bar(i32);

    fn get_foo_default() -> Foo {
      Foo(123)
    }
    ```
* `SerializationData::new` has been changed to expect an iterator of
`(usize, SkippedField)` rather than one of just `usize`
    ```rust
    // BEFORE
    SerializationData::new([0, 3].into_iter());

    // AFTER
    SerializationData::new([
      (0, SkippedField::new(field_0_default_fn)),
      (3, SkippedField::new(field_3_default_fn)),
    ].into_iter());
    ```
* `Serialization::is_ignored_field` has been renamed to
`Serialization::is_field_skipped`
* Fields marked `#[reflect(skip_serializing)]` are now included in
deserialization output. This may affect logic that expected those fields
to be absent.
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

Fixes bevyengine#5101
Alternative to bevyengine#6511

## Solution

Corrected the behavior for ignored fields in `FromReflect`, which was
previously using the incorrect field indexes.

Similarly, fields marked with `#[reflect(skip_serializing)]` no longer
break when using `FromReflect` after deserialization. This was done by
modifying `SerializationData` to store a function pointer that can later
be used to generate a default instance of the skipped field during
deserialization.

The function pointer points to a function generated by the derive macro
using the behavior designated by `#[reflect(default)]` (or just
`Default` if none provided). The entire output of the macro is now
wrapped in an [unnamed
constant](https://doc.rust-lang.org/stable/reference/items/constant-items.html#unnamed-constant)
which keeps this behavior hygienic.

#### Rationale

The biggest downside to this approach is that it requires fields marked
`#[reflect(skip_serializing)]` to provide the ability to create a
default instance— either via a `Default` impl or by specifying a custom
one. While this isn't great, I think it might be justified by the fact
that we really need to create this value when using `FromReflect` on a
deserialized object. And we need to do this _during_ deserialization
because after that (at least for tuples and tuple structs) we lose
information about which field is which: _"is the value at index 1 in
this `DynamicTupleStruct` the actual value for index 1 or is it really
the value for index 2 since index 1 is skippable...?"_

#### Alternatives

An alternative would be to store `Option<Box<dyn Reflect>>` within
`DynamicTuple` and `DynamicTupleStruct` instead of just `Box<dyn
Reflect>`. This would allow us to insert "empty"/"missing" fields during
deserialization, thus saving the positional information of the skipped
fields. However, this may require changing the API of `Tuple` and
`TupleStruct` such that they can account for their dynamic counterparts
returning `None` for a skipped field. In practice this would probably
mean exposing the `Option`-ness of the dynamics onto implementors via
methods like `Tuple::drain` or `TupleStruct::field`.

Personally, I think requiring `Default` would be better than muddying up
the API to account for these special cases. But I'm open to trying out
this other approach if the community feels that it's better.

---

## Changelog

### Public Changes

#### Fixed

- The behaviors of `#[reflect(ignore)]` and
`#[reflect(skip_serializing)]` are no longer dependent on field order

#### Changed

- Fields marked with `#[reflect(skip_serializing)]` now need to either
implement `Default` or specify a custom default function using
`#[reflect(default = "path::to::some_func")]`
- Deserializing a type with fields marked `#[reflect(skip_serializing)]`
will now include that field initialized to its specified default value
- `SerializationData::new` now takes the new `SkippedField` struct along
with the skipped field index
- Renamed `SerializationData::is_ignored_field` to
`SerializationData::is_field_skipped`

#### Added

- Added `SkippedField` struct
- Added methods `SerializationData::generate_default` and
`SerializationData::iter_skipped`

### Internal Changes

#### Changed

- Replaced `members_to_serialization_denylist` and `BitSet<u32>` with
`SerializationDataDef`
- The `Reflect` derive is more hygienic as it now outputs within an
[unnamed
constant](https://doc.rust-lang.org/stable/reference/items/constant-items.html#unnamed-constant)
- `StructField::index` has been split up into
`StructField::declaration_index` and `StructField::reflection_index`

#### Removed

- Removed `bitset` dependency

## Migration Guide

* Fields marked `#[reflect(skip_serializing)]` now must implement
`Default` or specify a custom default function with `#[reflect(default =
"path::to::some_func")]`
    ```rust
    #[derive(Reflect)]
    struct MyStruct {
      #[reflect(skip_serializing)]
      #[reflect(default = "get_foo_default")]
foo: Foo, // <- `Foo` does not impl `Default` so requires a custom
function
      #[reflect(skip_serializing)]
      bar: Bar, // <- `Bar` impls `Default`
    }
    
    #[derive(Reflect)]
    struct Foo(i32);
    
    #[derive(Reflect, Default)]
    struct Bar(i32);
    
    fn get_foo_default() -> Foo {
      Foo(123)
    }
    ```
* `SerializationData::new` has been changed to expect an iterator of
`(usize, SkippedField)` rather than one of just `usize`
    ```rust
    // BEFORE
    SerializationData::new([0, 3].into_iter());
    
    // AFTER
    SerializationData::new([
      (0, SkippedField::new(field_0_default_fn)),
      (3, SkippedField::new(field_3_default_fn)),
    ].into_iter());
    ```
* `Serialization::is_ignored_field` has been renamed to
`Serialization::is_field_skipped`
* Fields marked `#[reflect(skip_serializing)]` are now included in
deserialization output. This may affect logic that expected those fields
to be absent.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide X-Controversial There is active debate or serious implications around merging this PR
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

bevy_reflect: ignored fields are not ignored by FromReflect
6 participants