From e4a5bdac93adcf3ce7324cb9471c288b7c2003e4 Mon Sep 17 00:00:00 2001 From: MrGVSV Date: Sun, 6 Mar 2022 21:44:49 -0800 Subject: [PATCH 1/5] Added `#[reflect(default)]` attribute --- .../src/field_attributes.rs | 43 ++++++++++++++++++- .../bevy_reflect_derive/src/from_reflect.rs | 36 ++++++++++++---- 2 files changed, 70 insertions(+), 9 deletions(-) diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/field_attributes.rs b/crates/bevy_reflect/bevy_reflect_derive/src/field_attributes.rs index a0f3d2b6bc3a0..c57a0bc042d44 100644 --- a/crates/bevy_reflect/bevy_reflect_derive/src/field_attributes.rs +++ b/crates/bevy_reflect/bevy_reflect_derive/src/field_attributes.rs @@ -7,15 +7,37 @@ use crate::REFLECT_ATTRIBUTE_NAME; use quote::ToTokens; use syn::spanned::Spanned; -use syn::{Attribute, Meta, NestedMeta}; +use syn::{Attribute, Lit, Meta, NestedMeta}; pub(crate) static IGNORE_ATTR: &str = "ignore"; +pub(crate) static DEFAULT_ATTR: &str = "default"; /// A container for attributes defined on a field reflected type's field. #[derive(Default)] pub(crate) struct ReflectFieldAttr { /// Determines if this field should be ignored. pub ignore: bool, + /// Sets the default behavior of this field. + pub default: DefaultBehavior, +} + +/// Controls how the default value is determined for a field. +pub(crate) enum DefaultBehavior { + /// Field is required. + Required, + /// Field can be defaulted using `Default::default()`. + Default, + /// Field can be created using the given function name. + /// + /// This assumes the function is in scope, is callable with zero arguments, + /// and returns the expected type. + Func(syn::ExprPath), +} + +impl Default for DefaultBehavior { + fn default() -> Self { + Self::Required + } } /// Parse all field attributes marked "reflect" (such as `#[reflect(ignore)]`). @@ -50,10 +72,29 @@ fn parse_meta(args: &mut ReflectFieldAttr, meta: &Meta) -> Result<(), syn::Error args.ignore = true; Ok(()) } + Meta::Path(path) if path.is_ident(DEFAULT_ATTR) => { + args.default = DefaultBehavior::Default; + Ok(()) + } Meta::Path(path) => Err(syn::Error::new( path.span(), format!("unknown attribute parameter: {}", path.to_token_stream()), )), + 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()), + )) + } + } + } Meta::NameValue(pair) => { let path = &pair.path; Err(syn::Error::new( diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs b/crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs index 8e9e9c679295b..9d8c0e1ae248c 100644 --- a/crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs +++ b/crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs @@ -1,3 +1,4 @@ +use crate::field_attributes::DefaultBehavior; use crate::ReflectDeriveData; use proc_macro::TokenStream; use proc_macro2::Span; @@ -112,8 +113,10 @@ fn get_ignored_fields(derive_data: &ReflectDeriveData, is_tuple: bool) -> Member .ignored_fields() .map(|field| { let member = get_ident(field.data, field.index, is_tuple); - let value = quote! { - Default::default() + + let value = match &field.attrs.default { + DefaultBehavior::Func(path) => quote! {#path()}, + _ => quote! {Default::default()}, }; (member, value) @@ -139,12 +142,29 @@ fn get_active_fields( let accessor = get_field_accessor(field.data, field.index, is_tuple); let ty = field.data.ty.clone(); - let value = quote! { { - <#ty as #bevy_reflect_path::FromReflect>::from_reflect( - // Accesses the field on the given dynamic struct or tuple struct - #bevy_reflect_path::#struct_type::field(#dyn_struct_name, #accessor)? - )? - }}; + let get_field = quote! { + #bevy_reflect_path::#struct_type::field(#dyn_struct_name, #accessor) + }; + + let value = match &field.attrs.default { + DefaultBehavior::Func(path) => quote! { + if let Some(field) = #get_field { + <#ty as #bevy_reflect_path::FromReflect>::from_reflect(field)? + } else { + #path() + } + }, + DefaultBehavior::Default => quote! { + if let Some(field) = #get_field { + <#ty as #bevy_reflect_path::FromReflect>::from_reflect(field)? + } else { + Default::default() + } + }, + DefaultBehavior::Required => quote! { + <#ty as #bevy_reflect_path::FromReflect>::from_reflect(#get_field?)? + }, + }; (member, value) }) From 7fcce32cad10f79a56d367fd06247c1bd126da74 Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Mon, 9 May 2022 16:58:54 -0700 Subject: [PATCH 2/5] Add FromReflect default attr tests --- crates/bevy_reflect/src/lib.rs | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/crates/bevy_reflect/src/lib.rs b/crates/bevy_reflect/src/lib.rs index b484e47738cfd..6777ac060a6a7 100644 --- a/crates/bevy_reflect/src/lib.rs +++ b/crates/bevy_reflect/src/lib.rs @@ -231,6 +231,36 @@ mod tests { assert_eq!(values, vec![1]); } + #[test] + fn from_reflect_should_use_default_attributes() { + #[derive(Reflect, FromReflect, Eq, PartialEq, Debug)] + struct MyStruct { + // Use `Default::default()` + // Note that this isn't an ignored field + #[reflect(default)] + foo: String, + + // Use `get_foo_default()` + #[reflect(default = "get_bar_default")] + #[reflect(ignore)] + bar: usize, + } + + fn get_bar_default() -> usize { + 123 + } + + let expected = MyStruct { + foo: String::default(), + bar: 123, + }; + + let dyn_struct = DynamicStruct::default(); + let my_struct = ::from_reflect(&dyn_struct); + + assert_eq!(Some(expected), my_struct); + } + #[test] fn reflect_complex_patch() { #[derive(Reflect, Eq, PartialEq, Debug, FromReflect)] From 1127f65ac7447d98c421195880044b5d87ee0526 Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Mon, 16 May 2022 23:42:06 -0700 Subject: [PATCH 3/5] Improve docs --- .../bevy_reflect/bevy_reflect_derive/src/field_attributes.rs | 3 ++- crates/bevy_reflect/bevy_reflect_derive/src/lib.rs | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/field_attributes.rs b/crates/bevy_reflect/bevy_reflect_derive/src/field_attributes.rs index c57a0bc042d44..32b9bc1e8dfbf 100644 --- a/crates/bevy_reflect/bevy_reflect_derive/src/field_attributes.rs +++ b/crates/bevy_reflect/bevy_reflect_derive/src/field_attributes.rs @@ -12,7 +12,7 @@ use syn::{Attribute, Lit, Meta, NestedMeta}; pub(crate) static IGNORE_ATTR: &str = "ignore"; pub(crate) static DEFAULT_ATTR: &str = "default"; -/// A container for attributes defined on a field reflected type's field. +/// A container for attributes defined on a reflected type's field. #[derive(Default)] pub(crate) struct ReflectFieldAttr { /// Determines if this field should be ignored. @@ -66,6 +66,7 @@ pub(crate) fn parse_field_attrs(attrs: &[Attribute]) -> Result Result<(), syn::Error> { match meta { Meta::Path(path) if path.is_ident(IGNORE_ATTR) => { diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/lib.rs b/crates/bevy_reflect/bevy_reflect_derive/src/lib.rs index 8d59def2c0f9b..7cf9fe560b004 100644 --- a/crates/bevy_reflect/bevy_reflect_derive/src/lib.rs +++ b/crates/bevy_reflect/bevy_reflect_derive/src/lib.rs @@ -61,6 +61,8 @@ pub fn derive_reflect(input: TokenStream) -> TokenStream { /// /// This macro supports the following field attributes: /// * `#[reflect(ignore)]`: Ignores the field. This requires the field to implement [`Default`]. +/// * `#[reflect(default)]`: If the field's value cannot be read, uses its [`Default`] implementation. +/// * `#[reflect(default = "some_func")]`: If the field's value cannot be read, uses the function with the given name. /// #[proc_macro_derive(FromReflect, attributes(reflect))] pub fn derive_from_reflect(input: TokenStream) -> TokenStream { From 667b903863cbfaf52ac12a6a38fb59d9ed5975e3 Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Tue, 17 May 2022 00:14:11 -0700 Subject: [PATCH 4/5] Add proper FromReflect support for #[reflect(Default)] --- .../src/container_attributes.rs | 4 ++ .../bevy_reflect_derive/src/from_reflect.rs | 49 ++++++++++++------- crates/bevy_reflect/src/lib.rs | 33 ++++++++++++- 3 files changed, 68 insertions(+), 18 deletions(-) diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs b/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs index 8e16e77626c5a..580ad0cb7b1f0 100644 --- a/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs +++ b/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs @@ -19,6 +19,10 @@ const PARTIAL_EQ_ATTR: &str = "PartialEq"; const HASH_ATTR: &str = "Hash"; const SERIALIZE_ATTR: &str = "Serialize"; +// Traits not considered "special" (i.e. use the `ReflectMyTrait` syntax) +// but useful to know exist nonetheless +pub(crate) const REFLECT_DEFAULT: &str = "ReflectDefault"; + /// A marker for trait implementations registered via the `Reflect` derive macro. #[derive(Clone)] pub(crate) enum TraitImpl { diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs b/crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs index 9d8c0e1ae248c..1827424c93d40 100644 --- a/crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs +++ b/crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs @@ -1,3 +1,4 @@ +use crate::container_attributes::REFLECT_DEFAULT; use crate::field_attributes::DefaultBehavior; use crate::ReflectDeriveData; use proc_macro::TokenStream; @@ -54,24 +55,28 @@ fn impl_struct_internal(derive_data: &ReflectDeriveData, is_tuple: bool) -> Toke }; let field_types = derive_data.active_types(); - let MemberValuePair(ignored_members, ignored_values) = - get_ignored_fields(derive_data, is_tuple); let MemberValuePair(active_members, active_values) = get_active_fields(derive_data, &ref_struct, &ref_struct_type, is_tuple); - let constructor = if derive_data.traits().contains("ReflectDefault") { + let constructor = if derive_data.traits().contains(REFLECT_DEFAULT) { quote!( let mut __this = Self::default(); #( - __this.#active_members = #active_values; + if let Some(__field) = #active_values() { + // Iff field exists -> use its value + __this.#active_members = __field; + } )* Some(__this) ) } else { + let MemberValuePair(ignored_members, ignored_values) = + get_ignored_fields(derive_data, is_tuple); + quote!( Some( Self { - #(#active_members: #active_values,)* + #(#active_members: #active_values()?,)* #(#ignored_members: #ignored_values,)* } ) @@ -107,6 +112,9 @@ fn impl_struct_internal(derive_data: &ReflectDeriveData, is_tuple: bool) -> Toke } /// Get the collection of ignored field definitions +/// +/// Each value of the `MemberValuePair` is a token stream that generates a +/// a default value for the ignored field. fn get_ignored_fields(derive_data: &ReflectDeriveData, is_tuple: bool) -> MemberValuePair { MemberValuePair::new( derive_data @@ -125,7 +133,10 @@ fn get_ignored_fields(derive_data: &ReflectDeriveData, is_tuple: bool) -> Member ) } -/// Get the collection of active field definitions +/// Get the collection of active field definitions. +/// +/// Each value of the `MemberValuePair` is a token stream that generates a +/// closure of type `fn() -> Option` where `T` is that field's type. fn get_active_fields( derive_data: &ReflectDeriveData, dyn_struct_name: &Ident, @@ -148,21 +159,25 @@ fn get_active_fields( let value = match &field.attrs.default { DefaultBehavior::Func(path) => quote! { - if let Some(field) = #get_field { - <#ty as #bevy_reflect_path::FromReflect>::from_reflect(field)? - } else { - #path() - } + (|| + if let Some(field) = #get_field { + <#ty as #bevy_reflect_path::FromReflect>::from_reflect(field) + } else { + Some(#path()) + } + ) }, DefaultBehavior::Default => quote! { - if let Some(field) = #get_field { - <#ty as #bevy_reflect_path::FromReflect>::from_reflect(field)? - } else { - Default::default() - } + (|| + if let Some(field) = #get_field { + <#ty as #bevy_reflect_path::FromReflect>::from_reflect(field) + } else { + Some(Default::default()) + } + ) }, DefaultBehavior::Required => quote! { - <#ty as #bevy_reflect_path::FromReflect>::from_reflect(#get_field?)? + (|| <#ty as #bevy_reflect_path::FromReflect>::from_reflect(#get_field?)) }, }; diff --git a/crates/bevy_reflect/src/lib.rs b/crates/bevy_reflect/src/lib.rs index 6777ac060a6a7..af53ccf3892f3 100644 --- a/crates/bevy_reflect/src/lib.rs +++ b/crates/bevy_reflect/src/lib.rs @@ -93,6 +93,7 @@ mod tests { Deserializer, }; + use super::prelude::*; use super::*; use crate as bevy_reflect; use crate::serde::{ReflectDeserializer, ReflectSerializer}; @@ -232,7 +233,7 @@ mod tests { } #[test] - fn from_reflect_should_use_default_attributes() { + fn from_reflect_should_use_default_field_attributes() { #[derive(Reflect, FromReflect, Eq, PartialEq, Debug)] struct MyStruct { // Use `Default::default()` @@ -261,6 +262,36 @@ mod tests { assert_eq!(Some(expected), my_struct); } + #[test] + fn from_reflect_should_use_default_container_attribute() { + #[derive(Reflect, FromReflect, Eq, PartialEq, Debug)] + #[reflect(Default)] + struct MyStruct { + foo: String, + #[reflect(ignore)] + bar: usize, + } + + impl Default for MyStruct { + fn default() -> Self { + Self { + foo: String::from("Hello"), + bar: 123, + } + } + } + + let expected = MyStruct { + foo: String::from("Hello"), + bar: 123, + }; + + let dyn_struct = DynamicStruct::default(); + let my_struct = ::from_reflect(&dyn_struct); + + assert_eq!(Some(expected), my_struct); + } + #[test] fn reflect_complex_patch() { #[derive(Reflect, Eq, PartialEq, Debug, FromReflect)] From d536d7f2f8e3b270db8bedd23b7f72f539feda64 Mon Sep 17 00:00:00 2001 From: Gino Valente <49806985+MrGVSV@users.noreply.github.com> Date: Mon, 30 May 2022 09:20:01 -0700 Subject: [PATCH 5/5] Apply suggestions from code review Co-authored-by: Alice Cecile --- .../bevy_reflect_derive/src/container_attributes.rs | 2 +- crates/bevy_reflect/src/lib.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs b/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs index 580ad0cb7b1f0..09a41c78c97b1 100644 --- a/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs +++ b/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs @@ -19,7 +19,7 @@ const PARTIAL_EQ_ATTR: &str = "PartialEq"; const HASH_ATTR: &str = "Hash"; const SERIALIZE_ATTR: &str = "Serialize"; -// Traits not considered "special" (i.e. use the `ReflectMyTrait` syntax) +// The traits listed below are not considered "special" (i.e. they use the `ReflectMyTrait` syntax) // but useful to know exist nonetheless pub(crate) const REFLECT_DEFAULT: &str = "ReflectDefault"; diff --git a/crates/bevy_reflect/src/lib.rs b/crates/bevy_reflect/src/lib.rs index af53ccf3892f3..6fd1cec894ec6 100644 --- a/crates/bevy_reflect/src/lib.rs +++ b/crates/bevy_reflect/src/lib.rs @@ -241,7 +241,7 @@ mod tests { #[reflect(default)] foo: String, - // Use `get_foo_default()` + // Use `get_bar_default()` #[reflect(default = "get_bar_default")] #[reflect(ignore)] bar: usize,