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: Fix ignored/skipped field order #7575

Merged
merged 5 commits into from
Oct 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion crates/bevy_reflect/bevy_reflect_derive/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
62 changes: 38 additions & 24 deletions crates/bevy_reflect/bevy_reflect_derive/src/derive_data.rs
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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>>,
}

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
};

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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`.
Expand All @@ -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(),
)
}

Expand Down
10 changes: 7 additions & 3 deletions crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()},
Expand Down Expand Up @@ -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! {
Expand Down
6 changes: 5 additions & 1 deletion crates/bevy_reflect/bevy_reflect_derive/src/impls/enums.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_reflect/bevy_reflect_derive/src/impls/structs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
94 changes: 68 additions & 26 deletions crates/bevy_reflect/bevy_reflect_derive/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
};
MrGVSV marked this conversation as resolved.
Show resolved Hide resolved
})
}

Expand Down Expand Up @@ -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`].
Expand All @@ -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.
Expand Down Expand Up @@ -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
};
})
}

Expand Down Expand Up @@ -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(
Expand All @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
};
})
}
Loading
Loading