From 3276660fb29aad501394e6266493fa4bbb009a68 Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Mon, 31 Jul 2023 13:25:37 -0700 Subject: [PATCH 1/2] Fix multiple field attributes not being respected --- .../src/field_attributes.rs | 93 +++++++++---------- .../bevy_reflect_derive/src/lib.rs | 2 +- crates/bevy_reflect/src/lib.rs | 18 +++- 3 files changed, 60 insertions(+), 53 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 e3da50088d26d..9a134c60699db 100644 --- a/crates/bevy_reflect/bevy_reflect_derive/src/field_attributes.rs +++ b/crates/bevy_reflect/bevy_reflect_derive/src/field_attributes.rs @@ -5,9 +5,8 @@ //! the derive helper attribute for `Reflect`, which looks like: `#[reflect(ignore)]`. use crate::REFLECT_ATTRIBUTE_NAME; -use quote::ToTokens; -use syn::spanned::Spanned; -use syn::{Attribute, Expr, ExprLit, Lit, Meta}; +use syn::meta::ParseNestedMeta; +use syn::{Attribute, LitStr, Token}; pub(crate) static IGNORE_SERIALIZATION_ATTR: &str = "skip_serializing"; pub(crate) static IGNORE_ALL_ATTR: &str = "ignore"; @@ -78,7 +77,8 @@ pub(crate) fn parse_field_attrs(attrs: &[Attribute]) -> Result Result Result<(), syn::Error> { - match meta { - Meta::Path(path) if path.is_ident(IGNORE_SERIALIZATION_ATTR) => { - (args.ignore == ReflectIgnoreBehavior::None) - .then(|| args.ignore = ReflectIgnoreBehavior::IgnoreSerialization) - .ok_or_else(|| syn::Error::new_spanned(path, format!("Only one of ['{IGNORE_SERIALIZATION_ATTR}','{IGNORE_ALL_ATTR}'] is allowed"))) - } - Meta::Path(path) if path.is_ident(IGNORE_ALL_ATTR) => { - (args.ignore == ReflectIgnoreBehavior::None) - .then(|| args.ignore = ReflectIgnoreBehavior::IgnoreAlways) - .ok_or_else(|| syn::Error::new_spanned(path, format!("Only one of ['{IGNORE_SERIALIZATION_ATTR}','{IGNORE_ALL_ATTR}'] is allowed"))) - } - 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) => { - if let Expr::Lit(ExprLit {lit: Lit::Str(lit_str), ..}) = &pair.value { - args.default = DefaultBehavior::Func(lit_str.parse()?); +fn parse_meta(args: &mut ReflectFieldAttr, meta: ParseNestedMeta) -> Result<(), syn::Error> { + if meta.path.is_ident(DEFAULT_ATTR) { + // Allow: + // - `#[reflect(default)]` + // - `#[reflect(default = "path::to::func")]` + match args.default { + DefaultBehavior::Required => { + if meta.input.peek(Token![=]) { + let lit = meta.value()?.parse::()?; + args.default = DefaultBehavior::Func(lit.parse()?); + } else { + args.default = DefaultBehavior::Default; + } Ok(()) } - else { - Err(syn::Error::new( - pair.span(), - format!("expected a string literal containing the name of a function, but found: {}", pair.to_token_stream()), - ))? - } - } - Meta::NameValue(pair) => { - let path = &pair.path; - Err(syn::Error::new( - path.span(), - format!("unknown attribute parameter: {}", path.to_token_stream()), - )) + _ => Err(meta.error(format!("only one of [{:?}] is allowed", [DEFAULT_ATTR]))), } - Meta::List(list) if !list.path.is_ident(REFLECT_ATTRIBUTE_NAME) => { - Err(syn::Error::new(list.path.span(), "unexpected property")) + } else if meta.path.is_ident(IGNORE_ALL_ATTR) { + // Allow: + // - `#[reflect(ignore)]` + match args.ignore { + ReflectIgnoreBehavior::None => { + args.ignore = ReflectIgnoreBehavior::IgnoreAlways; + Ok(()) + } + _ => Err(meta.error(format!( + "only one of [{:?}] is allowed", + [IGNORE_ALL_ATTR, IGNORE_SERIALIZATION_ATTR] + ))), } - Meta::List(list) => { - if let Ok(meta) = list.parse_args() { - parse_meta(args, &meta)?; + } else if meta.path.is_ident(IGNORE_SERIALIZATION_ATTR) { + // Allow: + // - `#[reflect(skip_serializing)]` + match args.ignore { + ReflectIgnoreBehavior::None => { + args.ignore = ReflectIgnoreBehavior::IgnoreSerialization; + Ok(()) } - Ok(()) + _ => Err(meta.error(format!( + "only one of [{:?}] is allowed", + [IGNORE_ALL_ATTR, IGNORE_SERIALIZATION_ATTR] + ))), } + } else { + Err(meta.error(format!( + "unknown attribute, expected {:?}", + [DEFAULT_ATTR, IGNORE_ALL_ATTR, IGNORE_SERIALIZATION_ATTR] + ))) } } diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/lib.rs b/crates/bevy_reflect/bevy_reflect_derive/src/lib.rs index 87eee59dc8d67..031395a465cd3 100644 --- a/crates/bevy_reflect/bevy_reflect_derive/src/lib.rs +++ b/crates/bevy_reflect/bevy_reflect_derive/src/lib.rs @@ -221,7 +221,7 @@ pub fn derive_reflect(input: TokenStream) -> TokenStream { /// /// By default, this attribute denotes that the field's type implements [`Default`]. /// However, it can also take in a path string to a user-defined function that will return the default value. -/// This takes the form: `#[reflect(default = "path::to::my_function)]` where `my_function` is a parameterless +/// This takes the form: `#[reflect(default = "path::to::my_function")]` where `my_function` is a parameterless /// function that must return some default value for the type. /// /// Specifying a custom default can be used to give different fields their own specialized defaults, diff --git a/crates/bevy_reflect/src/lib.rs b/crates/bevy_reflect/src/lib.rs index bb0a012888d6e..76821257ca842 100644 --- a/crates/bevy_reflect/src/lib.rs +++ b/crates/bevy_reflect/src/lib.rs @@ -772,18 +772,26 @@ mod tests { foo: String, // Use `get_bar_default()` - #[reflect(default = "get_bar_default")] #[reflect(ignore)] - bar: usize, + #[reflect(default = "get_bar_default")] + bar: NotReflect, + + // Ensure attributes can be combined + #[reflect(ignore, default = "get_bar_default")] + baz: NotReflect, } - fn get_bar_default() -> usize { - 123 + #[derive(Eq, PartialEq, Debug)] + struct NotReflect(usize); + + fn get_bar_default() -> NotReflect { + NotReflect(123) } let expected = MyStruct { foo: String::default(), - bar: 123, + bar: NotReflect(123), + baz: NotReflect(123), }; let dyn_struct = DynamicStruct::default(); From 866e1594aa817f9568f65faa2ac503d7534a201b Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Thu, 3 Aug 2023 13:04:20 -0700 Subject: [PATCH 2/2] Address PR comments --- .../src/field_attributes.rs | 50 +++++++++---------- 1 file changed, 25 insertions(+), 25 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 9a134c60699db..8ee24bd7297c4 100644 --- a/crates/bevy_reflect/bevy_reflect_derive/src/field_attributes.rs +++ b/crates/bevy_reflect/bevy_reflect_derive/src/field_attributes.rs @@ -99,44 +99,44 @@ fn parse_meta(args: &mut ReflectFieldAttr, meta: ParseNestedMeta) -> Result<(), // Allow: // - `#[reflect(default)]` // - `#[reflect(default = "path::to::func")]` - match args.default { - DefaultBehavior::Required => { - if meta.input.peek(Token![=]) { - let lit = meta.value()?.parse::()?; - args.default = DefaultBehavior::Func(lit.parse()?); - } else { - args.default = DefaultBehavior::Default; - } - Ok(()) - } - _ => Err(meta.error(format!("only one of [{:?}] is allowed", [DEFAULT_ATTR]))), + if !matches!(args.default, DefaultBehavior::Required) { + return Err(meta.error(format!("only one of [{:?}] is allowed", [DEFAULT_ATTR]))); + } + + if meta.input.peek(Token![=]) { + let lit = meta.value()?.parse::()?; + args.default = DefaultBehavior::Func(lit.parse()?); + } else { + args.default = DefaultBehavior::Default; } + + Ok(()) } else if meta.path.is_ident(IGNORE_ALL_ATTR) { // Allow: // - `#[reflect(ignore)]` - match args.ignore { - ReflectIgnoreBehavior::None => { - args.ignore = ReflectIgnoreBehavior::IgnoreAlways; - Ok(()) - } - _ => Err(meta.error(format!( + if args.ignore != ReflectIgnoreBehavior::None { + return Err(meta.error(format!( "only one of [{:?}] is allowed", [IGNORE_ALL_ATTR, IGNORE_SERIALIZATION_ATTR] - ))), + ))); } + + args.ignore = ReflectIgnoreBehavior::IgnoreAlways; + + Ok(()) } else if meta.path.is_ident(IGNORE_SERIALIZATION_ATTR) { // Allow: // - `#[reflect(skip_serializing)]` - match args.ignore { - ReflectIgnoreBehavior::None => { - args.ignore = ReflectIgnoreBehavior::IgnoreSerialization; - Ok(()) - } - _ => Err(meta.error(format!( + if args.ignore != ReflectIgnoreBehavior::None { + return Err(meta.error(format!( "only one of [{:?}] is allowed", [IGNORE_ALL_ATTR, IGNORE_SERIALIZATION_ATTR] - ))), + ))); } + + args.ignore = ReflectIgnoreBehavior::IgnoreSerialization; + + Ok(()) } else { Err(meta.error(format!( "unknown attribute, expected {:?}",