Skip to content

[reflection] cleanup derive_reflect #2377

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

[reflection] cleanup derive_reflect #2377

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e9187e3
[reflection] cleanup derive_reflect
NathanSWard Jun 23, 2021
f1f0393
remove dead code
NathanSWard Jun 23, 2021
dd69ce6
simplify attr.path.get_ident() call
NathanSWard Jun 23, 2021
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
107 changes: 43 additions & 64 deletions crates/bevy_reflect/bevy_reflect_derive/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,10 @@ use syn::{
parse_macro_input,
punctuated::Punctuated,
token::{Comma, Paren, Where},
Data, DataStruct, DeriveInput, Field, Fields, Generics, Ident, Index, Member, Meta, NestedMeta,
Path,
Attribute, Data, DataStruct, DeriveInput, Field, Fields, Generics, Ident, Index, Member, Meta,
NestedMeta, Path,
};

#[derive(Default)]
struct PropAttributeArgs {
pub ignore: Option<bool>,
}
Comment on lines -20 to -23
Copy link
Contributor

Choose a reason for hiding this comment

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

This struct might exist to group future attributes, but if no other attribute is already planned I agree with the simplification


#[derive(Clone)]
enum TraitImpl {
NotImplemented,
Expand All @@ -45,11 +40,47 @@ enum DeriveType {
static REFLECT_ATTRIBUTE_NAME: &str = "reflect";
static REFLECT_VALUE_ATTRIBUTE_NAME: &str = "reflect_value";

fn active_fields(punctuated: &Punctuated<Field, Comma>) -> impl Iterator<Item = (&Field, usize)> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think naming the argument fields would be clearer than punctuated

punctuated.iter().enumerate().filter_map(|(idx, field)| {
field
.attrs
.iter()
.find(|attr| attr.path.get_ident().unwrap() == REFLECT_ATTRIBUTE_NAME)
.map(|attr| {
syn::custom_keyword!(ignore);
attr.parse_args::<Option<ignore>>()
.expect("Invalid 'property' attribute format.")
.is_none()
})
.unwrap_or(true)
.then(|| (field, idx))
})
}

fn reflect_attrs(attrs: &[Attribute]) -> (ReflectAttrs, Option<DeriveType>) {
for attribute in attrs.iter().filter_map(|attr| attr.parse_meta().ok()) {
if let Meta::List(meta_list) = attribute {
if let Some(ident) = meta_list.path.get_ident() {
if ident == REFLECT_ATTRIBUTE_NAME {
return (ReflectAttrs::from_nested_metas(&meta_list.nested), None);
} else if ident == REFLECT_VALUE_ATTRIBUTE_NAME {
return (
ReflectAttrs::from_nested_metas(&meta_list.nested),
Some(DeriveType::Value),
);
}
}
}
}

Default::default()
}

#[proc_macro_derive(Reflect, attributes(reflect, reflect_value, module))]
pub fn derive_reflect(input: TokenStream) -> TokenStream {
let ast = parse_macro_input!(input as DeriveInput);
let unit_struct_punctuated = Punctuated::new();
let (fields, mut derive_type) = match &ast.data {
let (fields, derive_type) = match &ast.data {
Data::Struct(DataStruct {
fields: Fields::Named(fields),
..
Expand All @@ -65,66 +96,14 @@ pub fn derive_reflect(input: TokenStream) -> TokenStream {
_ => (&unit_struct_punctuated, DeriveType::Value),
};

let fields_and_args = fields
.iter()
.enumerate()
.map(|(i, f)| {
(
f,
f.attrs
.iter()
.find(|a| *a.path.get_ident().as_ref().unwrap() == REFLECT_ATTRIBUTE_NAME)
.map(|a| {
syn::custom_keyword!(ignore);
let mut attribute_args = PropAttributeArgs { ignore: None };
a.parse_args_with(|input: ParseStream| {
if input.parse::<Option<ignore>>()?.is_some() {
attribute_args.ignore = Some(true);
return Ok(());
}
Ok(())
})
.expect("Invalid 'property' attribute format.");

attribute_args
}),
i,
)
})
.collect::<Vec<(&Field, Option<PropAttributeArgs>, usize)>>();
let active_fields = fields_and_args
.iter()
.filter(|(_field, attrs, _i)| {
attrs.is_none()
|| match attrs.as_ref().unwrap().ignore {
Some(ignore) => !ignore,
None => true,
}
})
.map(|(f, _attr, i)| (*f, *i))
.collect::<Vec<(&Field, usize)>>();
let active_fields = active_fields(&fields).collect::<Vec<_>>();
Copy link
Contributor

Choose a reason for hiding this comment

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

This merges two steps into one which is good, but I will probably need to undo it in #1395 in order to also get the list of inactive fields (see https://github.com/bevyengine/bevy/pull/1395/files#diff-786606f1541ef395577b541509d2834381796a7f9859e6aaeef68dcf551928bcR211-R220 ). That's my problem I guess :-)


let (reflect_attrs, modified_derive_type) = reflect_attrs(&ast.attrs);
let derive_type = modified_derive_type.unwrap_or(derive_type);

let bevy_reflect_path = BevyManifest::default().get_path("bevy_reflect");
let type_name = &ast.ident;

let mut reflect_attrs = ReflectAttrs::default();
for attribute in ast.attrs.iter().filter_map(|attr| attr.parse_meta().ok()) {
let meta_list = if let Meta::List(meta_list) = attribute {
meta_list
} else {
continue;
};

if let Some(ident) = meta_list.path.get_ident() {
if ident == REFLECT_ATTRIBUTE_NAME {
reflect_attrs = ReflectAttrs::from_nested_metas(&meta_list.nested);
} else if ident == REFLECT_VALUE_ATTRIBUTE_NAME {
derive_type = DeriveType::Value;
reflect_attrs = ReflectAttrs::from_nested_metas(&meta_list.nested);
}
}
}

let registration_data = &reflect_attrs.data;
let get_type_registration_impl = impl_get_type_registration(
type_name,
Expand Down