bevy_reflect: Fix ignored/skipped field order (bevyengine#7575)

# 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.

crates/bevy_reflect/bevy_reflect_derive/Cargo.toml

@@ -23,4 +23,3 @@ syn = { version = "2.0", features = ["full"] }
 proc-macro2 = "1.0"
 quote = "1.0"
 uuid = { version = "1.1", features = ["v4"] }
-bit-set = "0.5.2" 

crates/bevy_reflect/bevy_reflect_derive/src/derive_data.rs

@@ -1,11 +1,11 @@
 use crate::container_attributes::{FromReflectAttrs, ReflectTraits};
 use crate::field_attributes::{parse_field_attrs, ReflectFieldAttr};
 use crate::type_path::parse_path_no_leading_colon;
-use crate::utility::{members_to_serialization_denylist, StringExpr, WhereClauseOptions};
-use bit_set::BitSet;
+use crate::utility::{StringExpr, WhereClauseOptions};
 use quote::{quote, ToTokens};
 use syn::token::Comma;
 
+use crate::serialization::SerializationDataDef;
 use crate::{
     utility, REFLECT_ATTRIBUTE_NAME, REFLECT_VALUE_ATTRIBUTE_NAME, TYPE_NAME_ATTRIBUTE_NAME,
     TYPE_PATH_ATTRIBUTE_NAME,
@@ -65,7 +65,7 @@ pub(crate) struct ReflectMeta<'a> {
 /// ```
 pub(crate) struct ReflectStruct<'a> {
     meta: ReflectMeta<'a>,
-    serialization_denylist: BitSet<u32>,
+    serialization_data: Option<SerializationDataDef>,
     fields: Vec<StructField<'a>>,
 }
 
@@ -95,7 +95,14 @@ pub(crate) struct StructField<'a> {
     /// The reflection-based attributes on the field.
     pub attrs: ReflectFieldAttr,
     /// The index of this field within the struct.
-    pub index: usize,
+    pub declaration_index: usize,
+    /// The index of this field as seen by the reflection API.
+    ///
+    /// This index accounts for the removal of [ignored] fields.
+    /// It will only be `Some(index)` when the field is not ignored.
+    ///
+    /// [ignored]: crate::field_attributes::ReflectIgnoreBehavior::IgnoreAlways
+    pub reflection_index: Option<usize>,
     /// The documentation for this field, if any
     #[cfg(feature = "documentation")]
     pub doc: crate::documentation::Documentation,
@@ -272,9 +279,7 @@ impl<'a> ReflectDerive<'a> {
                 let fields = Self::collect_struct_fields(&data.fields)?;
                 let reflect_struct = ReflectStruct {
                     meta,
-                    serialization_denylist: members_to_serialization_denylist(
-                        fields.iter().map(|v| v.attrs.ignore),
-                    ),
+                    serialization_data: SerializationDataDef::new(&fields)?,
                     fields,
                 };
 
@@ -308,19 +313,31 @@ impl<'a> ReflectDerive<'a> {
     }
 
     fn collect_struct_fields(fields: &'a Fields) -> Result<Vec<StructField<'a>>, syn::Error> {
+        let mut active_index = 0;
         let sifter: utility::ResultSifter<StructField<'a>> = fields
             .iter()
             .enumerate()
-            .map(|(index, field)| -> Result<StructField, syn::Error> {
-                let attrs = parse_field_attrs(&field.attrs)?;
-                Ok(StructField {
-                    index,
-                    attrs,
-                    data: field,
-                    #[cfg(feature = "documentation")]
-                    doc: crate::documentation::Documentation::from_attributes(&field.attrs),
-                })
-            })
+            .map(
+                |(declaration_index, field)| -> Result<StructField, syn::Error> {
+                    let attrs = parse_field_attrs(&field.attrs)?;
+
+                    let reflection_index = if attrs.ignore.is_ignored() {
+                        None
+                    } else {
+                        active_index += 1;
+                        Some(active_index - 1)
+                    };
+
+                    Ok(StructField {
+                        declaration_index,
+                        reflection_index,
+                        attrs,
+                        data: field,
+                        #[cfg(feature = "documentation")]
+                        doc: crate::documentation::Documentation::from_attributes(&field.attrs),
+                    })
+                },
+            )
             .fold(
                 utility::ResultSifter::default(),
                 utility::ResultSifter::fold,
@@ -420,12 +437,9 @@ impl<'a> ReflectStruct<'a> {
         &self.meta
     }
 
-    /// Access the data about which fields should be ignored during serialization.
-    ///
-    /// The returned bitset is a collection of indices obtained from the [`members_to_serialization_denylist`] function.
-    #[allow(dead_code)]
-    pub fn serialization_denylist(&self) -> &BitSet<u32> {
-        &self.serialization_denylist
+    /// Returns the [`SerializationDataDef`] for this struct.
+    pub fn serialization_data(&self) -> Option<&SerializationDataDef> {
+        self.serialization_data.as_ref()
     }
 
     /// Returns the `GetTypeRegistration` impl as a `TokenStream`.
@@ -438,7 +452,7 @@ impl<'a> ReflectStruct<'a> {
         crate::registration::impl_get_type_registration(
             self.meta(),
             where_clause_options,
-            Some(&self.serialization_denylist),
+            self.serialization_data(),
         )
     }
 

crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs

@@ -189,7 +189,7 @@ fn get_ignored_fields(reflect_struct: &ReflectStruct) -> MemberValuePair {
         reflect_struct
             .ignored_fields()
             .map(|field| {
-                let member = ident_or_index(field.data.ident.as_ref(), field.index);
+                let member = ident_or_index(field.data.ident.as_ref(), field.declaration_index);
 
                 let value = match &field.attrs.default {
                     DefaultBehavior::Func(path) => quote! {#path()},
@@ -218,8 +218,12 @@ fn get_active_fields(
         reflect_struct
             .active_fields()
             .map(|field| {
-                let member = ident_or_index(field.data.ident.as_ref(), field.index);
-                let accessor = get_field_accessor(field.data, field.index, is_tuple);
+                let member = ident_or_index(field.data.ident.as_ref(), field.declaration_index);
+                let accessor = get_field_accessor(
+                    field.data,
+                    field.reflection_index.expect("field should be active"),
+                    is_tuple,
+                );
                 let ty = field.data.ty.clone();
 
                 let get_field = quote! {

crates/bevy_reflect/bevy_reflect_derive/src/impls/enums.rs

@@ -346,7 +346,11 @@ fn generate_impls(reflect_enum: &ReflectEnum, ref_index: &Ident, ref_name: &Iden
                     // Ignored field
                     continue;
                 }
-                constructor_argument.push(generate_for_field(reflect_idx, field.index, field));
+                constructor_argument.push(generate_for_field(
+                    reflect_idx,
+                    field.declaration_index,
+                    field,
+                ));
                 reflect_idx += 1;
             }
             constructor_argument

crates/bevy_reflect/bevy_reflect_derive/src/impls/structs.rs

@@ -19,12 +19,12 @@ pub(crate) fn impl_struct(reflect_struct: &ReflectStruct) -> proc_macro2::TokenS
                 .ident
                 .as_ref()
                 .map(|i| i.to_string())
-                .unwrap_or_else(|| field.index.to_string())
+                .unwrap_or_else(|| field.declaration_index.to_string())
         })
         .collect::<Vec<String>>();
     let field_idents = reflect_struct
         .active_fields()
-        .map(|field| ident_or_index(field.data.ident.as_ref(), field.index))
+        .map(|field| ident_or_index(field.data.ident.as_ref(), field.declaration_index))
         .collect::<Vec<_>>();
     let field_types = reflect_struct.active_types();
     let field_count = field_idents.len();

crates/bevy_reflect/bevy_reflect_derive/src/impls/tuple_structs.rs

@@ -14,7 +14,7 @@ pub(crate) fn impl_tuple_struct(reflect_struct: &ReflectStruct) -> proc_macro2::
 
     let field_idents = reflect_struct
         .active_fields()
-        .map(|field| Member::Unnamed(Index::from(field.index)))
+        .map(|field| Member::Unnamed(Index::from(field.declaration_index)))
         .collect::<Vec<_>>();
     let field_types = reflect_struct.active_types();
     let field_count = field_idents.len();

crates/bevy_reflect/bevy_reflect_derive/src/lib.rs

@@ -24,6 +24,7 @@ mod from_reflect;
 mod impls;
 mod reflect_value;
 mod registration;
+mod serialization;
 mod trait_reflection;
 mod type_path;
 mod type_uuid;
@@ -201,8 +202,10 @@ pub fn derive_reflect(input: TokenStream) -> TokenStream {
     };
 
     TokenStream::from(quote! {
-        #reflect_impls
-        #from_reflect_impl
+        const _: () = {
+            #reflect_impls
+            #from_reflect_impl
+        };
     })
 }
 
@@ -241,15 +244,20 @@ pub fn derive_from_reflect(input: TokenStream) -> TokenStream {
         Err(err) => return err.into_compile_error().into(),
     };
 
-    match derive_data {
+    let from_reflect_impl = match derive_data {
         ReflectDerive::Struct(struct_data) | ReflectDerive::UnitStruct(struct_data) => {
             from_reflect::impl_struct(&struct_data)
         }
         ReflectDerive::TupleStruct(struct_data) => from_reflect::impl_tuple_struct(&struct_data),
         ReflectDerive::Enum(meta) => from_reflect::impl_enum(&meta),
         ReflectDerive::Value(meta) => from_reflect::impl_value(&meta),
-    }
-    .into()
+    };
+
+    TokenStream::from(quote! {
+        const _: () = {
+            #from_reflect_impl
+        };
+    })
 }
 
 /// Derives the `TypePath` trait, providing a stable alternative to [`std::any::type_name`].
@@ -275,21 +283,31 @@ pub fn derive_type_path(input: TokenStream) -> TokenStream {
         Err(err) => return err.into_compile_error().into(),
     };
 
-    impls::impl_type_path(
+    let type_path_impl = impls::impl_type_path(
         derive_data.meta(),
         // Use `WhereClauseOptions::new_value` here so we don't enforce reflection bounds
         &WhereClauseOptions::new_value(derive_data.meta()),
-    )
-    .into()
+    );
+
+    TokenStream::from(quote! {
+        const _: () = {
+            #type_path_impl
+        };
+    })
 }
 
 // From https://github.com/randomPoison/type-uuid
 #[proc_macro_derive(TypeUuid, attributes(uuid))]
 pub fn derive_type_uuid(input: TokenStream) -> TokenStream {
     let input = parse_macro_input!(input as DeriveInput);
-    type_uuid::type_uuid_derive(input)
-        .unwrap_or_else(syn::Error::into_compile_error)
-        .into()
+    let uuid_impl =
+        type_uuid::type_uuid_derive(input).unwrap_or_else(syn::Error::into_compile_error);
+
+    TokenStream::from(quote! {
+        const _: () = {
+            #uuid_impl
+        };
+    })
 }
 
 /// A macro that automatically generates type data for traits, which their implementors can then register.
@@ -401,8 +419,10 @@ pub fn impl_reflect_value(input: TokenStream) -> TokenStream {
     let from_reflect_impl = from_reflect::impl_value(&meta);
 
     TokenStream::from(quote! {
-        #reflect_impls
-        #from_reflect_impl
+        const _: () = {
+            #reflect_impls
+            #from_reflect_impl
+        };
     })
 }
 
@@ -446,7 +466,7 @@ pub fn impl_reflect_struct(input: TokenStream) -> TokenStream {
         Err(err) => return err.into_compile_error().into(),
     };
 
-    match derive_data {
+    let output = match derive_data {
         ReflectDerive::Struct(struct_data) => {
             if !struct_data.meta().type_path().has_custom_path() {
                 return syn::Error::new(
@@ -460,27 +480,30 @@ pub fn impl_reflect_struct(input: TokenStream) -> TokenStream {
             let impl_struct = impls::impl_struct(&struct_data);
             let impl_from_struct = from_reflect::impl_struct(&struct_data);
 
-            TokenStream::from(quote! {
+            quote! {
                 #impl_struct
                 #impl_from_struct
-            })
+            }
         }
         ReflectDerive::TupleStruct(..) => syn::Error::new(
             ast.span(),
             "impl_reflect_struct does not support tuple structs",
         )
-        .into_compile_error()
-        .into(),
+        .into_compile_error(),
         ReflectDerive::UnitStruct(..) => syn::Error::new(
             ast.span(),
             "impl_reflect_struct does not support unit structs",
         )
-        .into_compile_error()
-        .into(),
+        .into_compile_error(),
         _ => syn::Error::new(ast.span(), "impl_reflect_struct only supports structs")
-            .into_compile_error()
-            .into(),
-    }
+            .into_compile_error(),
+    };
+
+    TokenStream::from(quote! {
+        const _: () = {
+            #output
+        };
+    })
 }
 
 /// A macro used to generate a `FromReflect` trait implementation for the given type.
@@ -521,7 +544,14 @@ pub fn impl_from_reflect_value(input: TokenStream) -> TokenStream {
         }
     };
 
-    from_reflect::impl_value(&ReflectMeta::new(type_path, def.traits.unwrap_or_default())).into()
+    let from_reflect_impl =
+        from_reflect::impl_value(&ReflectMeta::new(type_path, def.traits.unwrap_or_default()));
+
+    TokenStream::from(quote! {
+        const _: () = {
+            #from_reflect_impl
+        };
+    })
 }
 
 /// A replacement for [deriving `TypePath`] for use on foreign types.
@@ -583,12 +613,24 @@ pub fn impl_type_path(input: TokenStream) -> TokenStream {
 
     let meta = ReflectMeta::new(type_path, ReflectTraits::default());
 
-    impls::impl_type_path(&meta, &WhereClauseOptions::new_value(&meta)).into()
+    let type_path_impl = impls::impl_type_path(&meta, &WhereClauseOptions::new_value(&meta));
+
+    TokenStream::from(quote! {
+        const _: () = {
+            #type_path_impl
+        };
+    })
 }
 
 /// Derives `TypeUuid` for the given type. This is used internally to implement `TypeUuid` on foreign types, such as those in the std. This macro should be used in the format of `<[Generic Params]> [Type (Path)], [Uuid (String Literal)]`.
 #[proc_macro]
 pub fn impl_type_uuid(input: TokenStream) -> TokenStream {
     let def = parse_macro_input!(input as type_uuid::TypeUuidDef);
-    type_uuid::gen_impl_type_uuid(def).into()
+    let uuid_impl = type_uuid::gen_impl_type_uuid(def);
+
+    TokenStream::from(quote! {
+        const _: () = {
+            #uuid_impl
+        };
+    })
 }