bevy_reflect: Ignored field order #6511
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,9 @@ | |
| //! as opposed to an entire struct or enum. An example of such an attribute is | ||
| //! the derive helper attribute for `Reflect`, which looks like: `#[reflect(ignore)]`. | ||
|
|
||
| use crate::utility::combine_error; | ||
| use crate::REFLECT_ATTRIBUTE_NAME; | ||
| use proc_macro2::Span; | ||
| use quote::ToTokens; | ||
| use syn::spanned::Spanned; | ||
| use syn::{Attribute, Lit, Meta, NestedMeta}; | ||
|
|
@@ -69,85 +71,150 @@ pub(crate) enum DefaultBehavior { | |
| Func(syn::ExprPath), | ||
| } | ||
|
|
||
| /// Helper struct for parsing field attributes on structs, tuple structs, and enum variants. | ||
| pub(crate) struct ReflectFieldAttrParser { | ||
| /// Indicates whether the fields being parsed are part of an enum variant. | ||
| is_variant: bool, | ||
| /// The [`Span`] for the last `#[reflect(ignore)]` attribute, if any. | ||
| last_ignored: Option<Span>, | ||
| /// The [`Span`] for the last `#[reflect(skip_serializing)]` attribute, if any. | ||
| last_skipped: Option<Span>, | ||
| } | ||
|
|
||
| impl ReflectFieldAttrParser { | ||
| /// Create a new parser for struct and tuple struct fields. | ||
| pub fn new_struct() -> Self { | ||
| Self { | ||
| is_variant: false, | ||
| last_ignored: None, | ||
| last_skipped: None, | ||
| } | ||
| } | ||
|
|
||
| /// Create a new parser for enum variant struct fields. | ||
| pub fn new_enum_variant() -> Self { | ||
| Self { | ||
| is_variant: true, | ||
| last_ignored: None, | ||
| last_skipped: None, | ||
| } | ||
| } | ||
|
|
||
| /// Parse all field attributes marked "reflect" (such as `#[reflect(ignore)]`). | ||
| pub fn parse(&mut self, attrs: &[Attribute]) -> Result<ReflectFieldAttr, syn::Error> { | ||
| let mut args = ReflectFieldAttr::default(); | ||
| let mut errors: Option<syn::Error> = None; | ||
|
|
||
| let attrs = attrs | ||
| .iter() | ||
| .filter(|a| a.path.is_ident(REFLECT_ATTRIBUTE_NAME)); | ||
| for attr in attrs { | ||
| let meta = attr.parse_meta()?; | ||
| if let Err(err) = self.parse_meta(&mut args, &meta) { | ||
| combine_error(err, &mut errors); | ||
| } | ||
| } | ||
|
|
||
| self.check_ignore_order(&args, &mut errors); | ||
| self.check_skip_order(&args, &mut errors); | ||
|
|
||
| match errors { | ||
| Some(error) => Err(error), | ||
| None => Ok(args), | ||
| } | ||
| } | ||
|
|
||
| /// Recursively parses attribute metadata for things like `#[reflect(ignore)]` and `#[reflect(default = "foo")]` | ||
| fn parse_meta(&mut self, args: &mut ReflectFieldAttr, meta: &Meta) -> Result<(), syn::Error> { | ||
| match meta { | ||
| Meta::Path(path) if path.is_ident(IGNORE_SERIALIZATION_ATTR) => { | ||
| if args.ignore == ReflectIgnoreBehavior::None { | ||
| args.ignore = ReflectIgnoreBehavior::IgnoreSerialization; | ||
| self.last_skipped = Some(path.span()); | ||
| Ok(()) | ||
| } else { | ||
| Err(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) => { | ||
| if args.ignore == ReflectIgnoreBehavior::None { | ||
| args.ignore = ReflectIgnoreBehavior::IgnoreAlways; | ||
| self.last_ignored = Some(path.span()); | ||
| Ok(()) | ||
| } else { | ||
| Err(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) => { | ||
| 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( | ||
| path.span(), | ||
| format!("unknown attribute parameter: {}", path.to_token_stream()), | ||
| )) | ||
| } | ||
| Meta::List(list) if !list.path.is_ident(REFLECT_ATTRIBUTE_NAME) => { | ||
| Err(syn::Error::new(list.path.span(), "unexpected property")) | ||
| } | ||
| Meta::List(list) => { | ||
| for nested in &list.nested { | ||
| if let NestedMeta::Meta(meta) = nested { | ||
| self.parse_meta(args, meta)?; | ||
| } | ||
| } | ||
| Ok(()) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Verifies `#[reflect(ignore)]` attributes are always last in the type definition. | ||
| fn check_ignore_order(&self, args: &ReflectFieldAttr, errors: &mut Option<syn::Error>) { | ||
|
There was a problem hiding this comment. This is a logical fix to a real problem! Users deriving reflect like this already have control over their field orders, so asking them to change the order in these cases seems reasonable in most cases. However, theres always the chance that field order matters for some other reason (ex: the type is also used for binary serialization: RPC, bincode, GPU types, etc). Given that Reflect aims to be a "general purpose reflection library", I think breaking these cases will eventually be an issue, both in Bevy projects and elsewhere. There was a problem hiding this comment. Fair enough. Would it be better to put this behind a default feature? Or at least provide an opt-out attribute like There was a problem hiding this comment. If opting out breaks the scenarios we're trying to fix in this PR (FromReflect and serialization), and this PR breaks other scenarios (reflecting structs that needs specific field orders), I think thats an indicator that we need to rethink our implementation generally / come up with a solution that handles all of these scenarios correctly. Specifically: is it possible for us to support cases where reflected field indices don't line up exactly with declared field indices. Or alternatively, can we adjust our reflection apis to account for "non-existent / skipped" fields (ensuring reflected field indices always match declared field indices). If either of these are possible, what are the costs? Performance? Ergonomics? Do we need to mitigate those costs (ex: provide opt-in/out fast paths where possible)? There was a problem hiding this comment.
I just put up an alternative PR (#7575) which is more in line with this. The only downside is that it requires fields with |
||
| if args.ignore.is_active() { | ||
| if let Some(span) = self.last_ignored { | ||
| let message = if self.is_variant { | ||
| format!("fields marked with `#[reflect({IGNORE_ALL_ATTR})]` must come last in variant definition") | ||
| } else { | ||
| format!("fields marked with `#[reflect({IGNORE_ALL_ATTR})]` must come last in type definition") | ||
| }; | ||
| combine_error(syn::Error::new(span, message), errors); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Verifies `#[reflect(skip_serializing)]` attributes are always last in the type definition, | ||
| /// but before `#[reflect(ignore)]` attributes. | ||
| fn check_skip_order(&self, args: &ReflectFieldAttr, errors: &mut Option<syn::Error>) { | ||
| if args.ignore == ReflectIgnoreBehavior::None { | ||
| if let Some(span) = self.last_skipped { | ||
| let message = if self.is_variant { | ||
| format!("fields marked with `#[reflect({IGNORE_SERIALIZATION_ATTR})]` must come last in variant definition (but before any fields marked `#[reflect({IGNORE_ALL_ATTR})]`)") | ||
| } else { | ||
| format!("fields marked with `#[reflect({IGNORE_SERIALIZATION_ATTR})]` must come last in type definition (but before any fields marked `#[reflect({IGNORE_ALL_ATTR})]`)") | ||
| }; | ||
| combine_error(syn::Error::new(span, message), errors); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -290,9 +290,9 @@ mod tests { | |
| A, | ||
| B, | ||
| C { | ||
| bar: bool, | ||
| #[reflect(ignore)] | ||
| foo: f32, | ||
| }, | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: The match statements can be flattened here. Something like:
Though not sure how cleaner that is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, yeah I'm not sure how much we gain from that if we need to then duplicate the arm to handle the error: