Error on bad #[reflect(ignore)] ordering

crates/bevy_pbr/src/pbr_material.rs

@@ -177,26 +177,6 @@ pub struct StandardMaterial {
  /// which can be done via `cull_mode`.
  pub double_sided: bool,
 
- /// Whether to cull the "front", "back" or neither side of a mesh.
- /// If set to `None`, the two sides of the mesh are visible.
- ///
- /// Defaults to `Some(Face::Back)`.
- /// In bevy, the order of declaration of a triangle's vertices
- /// in [`Mesh`] defines the triangle's front face.
- ///
- /// When a triangle is in a viewport,
- /// if its vertices appear counter-clockwise from the viewport's perspective,
- /// then the viewport is seeing the triangle's front face.
- /// Conversly, if the vertices appear clockwise, you are seeing the back face.
- ///
- /// In short, in bevy, front faces winds counter-clockwise.
- ///
- /// Your 3D editing software should manage all of that.
- ///
- /// [`Mesh`]: bevy_render::mesh::Mesh
- // TODO: include this in reflection somehow (maybe via remote types like serde https://serde.rs/remote-derive.html)
- #[reflect(ignore)]
- pub cull_mode: Option<Face>,
 
  /// Whether to apply only the base color to this material.
  ///
@@ -225,6 +205,26 @@ pub struct StandardMaterial {
  ///
  /// [z-fighting]: https://en.wikipedia.org/wiki/Z-fighting
  pub depth_bias: f32,
+ /// Whether to cull the "front", "back" or neither side of a mesh.
+ /// If set to `None`, the two sides of the mesh are visible.
+ ///
+ /// Defaults to `Some(Face::Back)`.
+ /// In bevy, the order of declaration of a triangle's vertices
+ /// in [`Mesh`] defines the triangle's front face.
+ ///
+ /// When a triangle is in a viewport,
+ /// if its vertices appear counter-clockwise from the viewport's perspective,
+ /// then the viewport is seeing the triangle's front face.
+ /// Conversly, if the vertices appear clockwise, you are seeing the back face.
+ ///
+ /// In short, in bevy, front faces winds counter-clockwise.
+ ///
+ /// Your 3D editing software should manage all of that.
+ ///
+ /// [`Mesh`]: bevy_render::mesh::Mesh
+ // TODO: include this in reflection somehow (maybe via remote types like serde https://serde.rs/remote-derive.html)
+ #[reflect(ignore)]
+ pub cull_mode: Option<Face>,
 }
 
 impl Default for StandardMaterial {

crates/bevy_reflect/bevy_reflect_derive/src/derive_data.rs

@@ -1,5 +1,5 @@
 use crate::container_attributes::ReflectTraits;
-use crate::field_attributes::{parse_field_attrs, ReflectFieldAttr};
+use crate::field_attributes::{ReflectFieldAttr, ReflectFieldAttrParser};
 use crate::utility::members_to_serialization_denylist;
 use bit_set::BitSet;
 use quote::quote;
@@ -100,9 +100,6 @@ pub(crate) struct EnumVariant<'a> {
  pub data: &'a Variant,
  /// The fields within this variant.
  pub fields: EnumVariantFields<'a>,
- /// The reflection-based attributes on the variant.
- #[allow(dead_code)]
- pub attrs: ReflectFieldAttr,
  /// The index of this variant within the enum.
  #[allow(dead_code)]
  pub index: usize,
@@ -195,7 +192,8 @@ impl<'a> ReflectDerive<'a> {
 
  return match &input.data {
  Data::Struct(data) => {
- let fields = Self::collect_struct_fields(&data.fields)?;
+ let mut field_parser = ReflectFieldAttrParser::new_struct();
+ let fields = Self::collect_struct_fields(&data.fields, &mut field_parser)?;
  let reflect_struct = ReflectStruct {
  meta,
  serialization_denylist: members_to_serialization_denylist(
@@ -223,12 +221,15 @@ impl<'a> ReflectDerive<'a> {
  };
  }
 
- fn collect_struct_fields(fields: &'a Fields) -> Result<Vec<StructField<'a>>, syn::Error> {
+ fn collect_struct_fields(
+ fields: &'a Fields,
+ field_parser: &mut ReflectFieldAttrParser,
+ ) -> Result<Vec<StructField<'a>>, syn::Error> {
  let sifter: utility::ResultSifter<StructField<'a>> = fields
  .iter()
  .enumerate()
  .map(|(index, field)| -> Result<StructField, syn::Error> {
- let attrs = parse_field_attrs(&field.attrs)?;
+ let attrs = field_parser.parse(&field.attrs)?;
  Ok(StructField {
  index,
  attrs,
@@ -252,7 +253,8 @@ impl<'a> ReflectDerive<'a> {
  .iter()
  .enumerate()
  .map(|(index, variant)| -> Result<EnumVariant, syn::Error> {
- let fields = Self::collect_struct_fields(&variant.fields)?;
+ let mut field_parser = ReflectFieldAttrParser::new_enum_variant();
+ let fields = Self::collect_struct_fields(&variant.fields, &mut field_parser)?;
 
  let fields = match variant.fields {
  Fields::Named(..) => EnumVariantFields::Named(fields),
@@ -261,7 +263,6 @@ impl<'a> ReflectDerive<'a> {
  };
  Ok(EnumVariant {
  fields,
- attrs: parse_field_attrs(&variant.attrs)?,
  data: variant,
  index,
  #[cfg(feature = "documentation")]

crates/bevy_reflect/bevy_reflect_derive/src/field_attributes.rs

@@ -5,6 +5,7 @@
 //! the derive helper attribute for `Reflect`, which looks like: `#[reflect(ignore)]`.
 
 use crate::REFLECT_ATTRIBUTE_NAME;
+use proc_macro2::Span;
 use quote::ToTokens;
 use syn::spanned::Spanned;
 use syn::{Attribute, Lit, Meta, NestedMeta};
@@ -69,85 +70,158 @@ pub(crate) enum DefaultBehavior {
  Func(syn::ExprPath),
 }
 
-/// Parse all field attributes marked "reflect" (such as `#[reflect(ignore)]`).
-pub(crate) fn parse_field_attrs(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) = parse_meta(&mut args, &meta) {
- if let Some(ref mut error) = errors {
- error.combine(err);
- } else {
- errors = Some(err);
- }
+/// 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,
  }
  }
 
- if let Some(error) = errors {
- Err(error)
- } else {
- Ok(args)
+ /// 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,
+ }
  }
-}
 
-/// Recursively parses attribute metadata for things like `#[reflect(ignore)]` and `#[reflect(default = "foo")]`
-fn parse_meta(args: &mut ReflectFieldAttr, meta: &Meta) -> 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")))
+ /// 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) {
+ Self::combine_error(err, &mut errors);
+ }
  }
- Meta::Path(path) if path.is_ident(DEFAULT_ATTR) => {
- args.default = DefaultBehavior::Default;
- Ok(())
+
+ self.check_ignore_order(&args, &mut errors);
+ self.check_skip_order(&args, &mut errors);
+
+ if let Some(error) = errors {
+ Err(error)
+ } else {
+ Ok(args)
  }
-  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()?);
+ }
+
+ /// 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")))
  }
- 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::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::NameValue(pair) => {
- let path = &pair.path;
- Err(syn::Error::new(
+ 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(())
+ }
  }
- Meta::List(list) if !list.path.is_ident(REFLECT_ATTRIBUTE_NAME) => {
- Err(syn::Error::new(list.path.span(), "unexpected property"))
+ }
+
+ /// Verifies `#[reflect(ignore)]` attributes are always last in the type definition.
+ fn check_ignore_order(&self, args: &ReflectFieldAttr, errors: &mut Option<syn::Error>) {
+ if args.ignore.is_active() {
+ if let Some(span) = self.last_ignored {
+ if self.is_variant {
+ Self::combine_error(syn::Error::new(span, format!("fields marked with `#[reflect({IGNORE_ALL_ATTR})]` must come last in variant definition")), errors);
+ } else {
+ Self::combine_error(syn::Error::new(span, format!("fields marked with `#[reflect({IGNORE_ALL_ATTR})]` must come last in type definition")), errors);
+ }
+ }
  }
- Meta::List(list) => {
- for nested in &list.nested {
- if let NestedMeta::Meta(meta) = nested {
- parse_meta(args, meta)?;
+ }
+
+ /// 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 {
+ if self.is_variant {
+ Self::combine_error(syn::Error::new(span, format!("fields marked with `#[reflect({IGNORE_SERIALIZATION_ATTR})]` must come last in variant definition (but before any fields marked `#[reflect({IGNORE_ALL_ATTR})]`)")), errors);
+ } else {
+ Self::combine_error(syn::Error::new(span, format!("fields marked with `#[reflect({IGNORE_SERIALIZATION_ATTR})]` must come last in type definition (but before any fields marked `#[reflect({IGNORE_ALL_ATTR})]`)")), errors);
  }
  }
- Ok(())
+ }
+ }
+
+ /// Set or combine the given error into an optionally existing error.
+ fn combine_error(err: syn::Error, errors: &mut Option<syn::Error>) {
+ if let Some(error) = errors {
+ error.combine(err);
+ } else {
+ *errors = Some(err);
  }
  }
 }

crates/bevy_reflect/bevy_reflect_derive/src/utility.rs

@@ -108,11 +108,11 @@ impl<T> ResultSifter<T> {
 /// pub struct HelloWorld {
 /// reflected_field: u32 // index: 0
 ///
-/// #[reflect(ignore)]
-/// non_reflected_field: u32 // index: N/A (not 1!)
-///
 /// #[reflect(skip_serializing)]
 /// non_serialized_field: u32 // index: 1
+///
+/// #[reflect(ignore)]
+/// non_reflected_field: u32 // index: N/A (not 2!)
 /// }
 /// ```
 /// Would convert to the `0b01` bitset (i.e second field is NOT serialized)

crates/bevy_reflect/src/enums/mod.rs

@@ -290,9 +290,9 @@ mod tests {
  A,
  B,
  C {
+ bar: bool,
  #[reflect(ignore)]
  foo: f32,
- bar: bool,
  },
  }