Skip to content

Commit

Permalink
bevy_reflect: Remove ContainerAttributes::merge (bevyengine#13303)
Browse files Browse the repository at this point in the history 
# Objective

Unblocks bevyengine#11659.

Currently the `Reflect` derive macro has to go through a merge process
for each `#[reflect]`/`#[reflet_value]` attribute encountered on a
container type.

Not only is this a bit inefficient, but it also has a soft requirement
that we can compare attributes such that an error can be thrown on
duplicates, invalid states, etc.

While working on bevyengine#11659 this proved to be challenging due to the fact
that `syn` types don't implement `PartialEq` or `Hash` without enabling
the `extra-traits` feature.

Ideally, we wouldn't have to enable another feature just to accommodate
this one use case.

## Solution

Removed `ContainerAttributes::merge`.

This was a fairly simple change as we could just have the parsing
functions take `&mut self` instead of returning `Self`.

## Testing

CI should build as there should be no user-facing change.
  • Loading branch information
@MrGVSV
MrGVSV authored May 9, 2024
1 parent 42ba9df commit 705c144
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 101 deletions.
126 changes: 35 additions & 91 deletions crates/bevy_reflect/derive/src/container_attributes.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,24 +92,6 @@ impl FromReflectAttrs {
.map(|lit| lit.value())
.unwrap_or(true)
}

/// Merges this [`FromReflectAttrs`] with another.
pub fn merge(&mut self, other: FromReflectAttrs) -> Result<(), syn::Error> {
if let Some(new) = other.auto_derive {
if let Some(existing) = &self.auto_derive {
if existing.value() != new.value() {
return Err(syn::Error::new(
new.span(),
format!("`{FROM_REFLECT_ATTR}` already set to {}", existing.value()),
));
}
} else {
self.auto_derive = Some(new);
}
}

Ok(())
}
}

/// A collection of attributes used for deriving `TypePath` via the `Reflect` derive.
Expand All @@ -133,24 +115,6 @@ impl TypePathAttrs {
.map(|lit| lit.value())
.unwrap_or(true)
}

/// Merges this [`TypePathAttrs`] with another.
pub fn merge(&mut self, other: TypePathAttrs) -> Result<(), syn::Error> {
if let Some(new) = other.auto_derive {
if let Some(existing) = &self.auto_derive {
if existing.value() != new.value() {
return Err(syn::Error::new(
new.span(),
format!("`{TYPE_PATH_ATTR}` already set to {}", existing.value()),
));
}
} else {
self.auto_derive = Some(new);
}
}

Ok(())
}
}

/// A collection of traits that have been registered for a reflected type.
Expand Down Expand Up @@ -231,23 +195,29 @@ impl ContainerAttributes {
///
/// # Example
/// - `Hash, Debug(custom_debug), MyTrait`
pub fn parse_terminated(input: ParseStream, trait_: ReflectTraitToImpl) -> syn::Result<Self> {
let mut this = Self::default();

pub fn parse_terminated(
&mut self,
input: ParseStream,
trait_: ReflectTraitToImpl,
) -> syn::Result<()> {
terminated_parser(Token![,], |stream| {
this.parse_container_attribute(stream, trait_)
self.parse_container_attribute(stream, trait_)
})(input)?;

Ok(this)
Ok(())
}

/// Parse the contents of a `#[reflect(...)]` attribute into a [`ContainerAttributes`] instance.
///
/// # Example
/// - `#[reflect(Hash, Debug(custom_debug), MyTrait)]`
/// - `#[reflect(no_field_bounds)]`
pub fn parse_meta_list(meta: &MetaList, trait_: ReflectTraitToImpl) -> syn::Result<Self> {
meta.parse_args_with(|stream: ParseStream| Self::parse_terminated(stream, trait_))
pub fn parse_meta_list(
&mut self,
meta: &MetaList,
trait_: ReflectTraitToImpl,
) -> syn::Result<()> {
meta.parse_args_with(|stream: ParseStream| self.parse_terminated(stream, trait_))
}

/// Parse a single container attribute.
Expand Down Expand Up @@ -392,7 +362,7 @@ impl ContainerAttributes {
trait_: ReflectTraitToImpl,
) -> syn::Result<()> {
let pair = input.parse::<MetaNameValue>()?;
let value = extract_bool(&pair.value, |lit| {
let extracted_bool = extract_bool(&pair.value, |lit| {
// Override `lit` if this is a `FromReflect` derive.
// This typically means a user is opting out of the default implementation
// from the `Reflect` derive and using the `FromReflect` derive directly instead.
Expand All @@ -401,7 +371,16 @@ impl ContainerAttributes {
.unwrap_or_else(|| lit.clone())
})?;

self.from_reflect_attrs.auto_derive = Some(value);
if let Some(existing) = &self.from_reflect_attrs.auto_derive {
if existing.value() != extracted_bool.value() {
return Err(syn::Error::new(
extracted_bool.span(),
format!("`{FROM_REFLECT_ATTR}` already set to {}", existing.value()),
));
}
} else {
self.from_reflect_attrs.auto_derive = Some(extracted_bool);
}

Ok(())
}
Expand All @@ -416,7 +395,7 @@ impl ContainerAttributes {
trait_: ReflectTraitToImpl,
) -> syn::Result<()> {
let pair = input.parse::<MetaNameValue>()?;
let value = extract_bool(&pair.value, |lit| {
let extracted_bool = extract_bool(&pair.value, |lit| {
// Override `lit` if this is a `FromReflect` derive.
// This typically means a user is opting out of the default implementation
// from the `Reflect` derive and using the `FromReflect` derive directly instead.
Expand All @@ -425,7 +404,16 @@ impl ContainerAttributes {
.unwrap_or_else(|| lit.clone())
})?;

self.type_path_attrs.auto_derive = Some(value);
if let Some(existing) = &self.type_path_attrs.auto_derive {
if existing.value() != extracted_bool.value() {
return Err(syn::Error::new(
extracted_bool.span(),
format!("`{TYPE_PATH_ATTR}` already set to {}", existing.value()),
));
}
} else {
self.type_path_attrs.auto_derive = Some(extracted_bool);
}

Ok(())
}
Expand Down Expand Up @@ -530,50 +518,6 @@ impl ContainerAttributes {
pub fn no_field_bounds(&self) -> bool {
self.no_field_bounds
}

/// Merges the trait implementations of this [`ContainerAttributes`] with another one.
///
/// An error is returned if the two [`ContainerAttributes`] have conflicting implementations.
pub fn merge(&mut self, other: ContainerAttributes) -> Result<(), syn::Error> {
// Destructuring is used to help ensure that all fields are merged
let Self {
debug,
hash,
partial_eq,
from_reflect_attrs,
type_path_attrs,
custom_where,
no_field_bounds,
idents,
} = self;

debug.merge(other.debug)?;
hash.merge(other.hash)?;
partial_eq.merge(other.partial_eq)?;
from_reflect_attrs.merge(other.from_reflect_attrs)?;
type_path_attrs.merge(other.type_path_attrs)?;

Self::merge_custom_where(custom_where, other.custom_where);

*no_field_bounds |= other.no_field_bounds;

for ident in other.idents {
add_unique_ident(idents, ident)?;
}
Ok(())
}

fn merge_custom_where(this: &mut Option<WhereClause>, other: Option<WhereClause>) {
match (this, other) {
(Some(this), Some(other)) => {
this.predicates.extend(other.predicates);
}
(this @ None, Some(other)) => {
*this = Some(other);
}
_ => {}
}
}
}

/// Adds an identifier to a vector of identifiers if it is not already present.
Expand Down
14 changes: 5 additions & 9 deletions crates/bevy_reflect/derive/src/derive_data.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ impl<'a> ReflectDerive<'a> {
input: &'a DeriveInput,
provenance: ReflectProvenance,
) -> Result<Self, syn::Error> {
let mut traits = ContainerAttributes::default();
let mut container_attributes = ContainerAttributes::default();
// Should indicate whether `#[reflect_value]` was used.
let mut reflect_mode = None;
// Should indicate whether `#[type_path = "..."]` was used.
Expand All @@ -205,9 +205,7 @@ impl<'a> ReflectDerive<'a> {
}

reflect_mode = Some(ReflectMode::Normal);
let new_traits =
ContainerAttributes::parse_meta_list(meta_list, provenance.trait_)?;
traits.merge(new_traits)?;
container_attributes.parse_meta_list(meta_list, provenance.trait_)?;
}
Meta::List(meta_list) if meta_list.path.is_ident(REFLECT_VALUE_ATTRIBUTE_NAME) => {
if !matches!(reflect_mode, None | Some(ReflectMode::Value)) {
Expand All @@ -218,9 +216,7 @@ impl<'a> ReflectDerive<'a> {
}

reflect_mode = Some(ReflectMode::Value);
let new_traits =
ContainerAttributes::parse_meta_list(meta_list, provenance.trait_)?;
traits.merge(new_traits)?;
container_attributes.parse_meta_list(meta_list, provenance.trait_)?;
}
Meta::Path(path) if path.is_ident(REFLECT_VALUE_ATTRIBUTE_NAME) => {
if !matches!(reflect_mode, None | Some(ReflectMode::Value)) {
Expand Down Expand Up @@ -296,7 +292,7 @@ impl<'a> ReflectDerive<'a> {
generics: &input.generics,
};

let meta = ReflectMeta::new(type_path, traits);
let meta = ReflectMeta::new(type_path, container_attributes);

if provenance.source == ReflectImplSource::ImplRemoteType
&& meta.type_path_attrs().should_auto_derive()
Expand Down Expand Up @@ -439,7 +435,7 @@ impl<'a> ReflectMeta<'a> {
Self { docs, ..self }
}

/// The registered reflect traits on this struct.
/// The registered reflect attributes on this struct.
pub fn attrs(&self) -> &ContainerAttributes {
&self.attrs
}
Expand Down
6 changes: 5 additions & 1 deletion crates/bevy_reflect/derive/src/reflect_value.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,11 @@ impl ReflectValueDef {
if input.peek(Paren) {
let content;
parenthesized!(content in input);
traits = Some(ContainerAttributes::parse_terminated(&content, trait_)?);
traits = Some({
let mut attrs = ContainerAttributes::default();
attrs.parse_terminated(&content, trait_)?;
attrs
});
}
Ok(ReflectValueDef {
attrs,
Expand Down

0 comments on commit 705c144

Please sign in to comment.