Skip to content

Commit

Permalink
bevy_reflect: Allow #[reflect(default)] on enum variant fields (#8514)
Browse files Browse the repository at this point in the history
# Objective

When using `FromReflect`, fields can be optionally left out if they are
marked with `#[reflect(default)]`. This is very handy for working with
serialized data as giant structs only need to list a subset of defined
fields in order to be constructed.

<details>
<summary>Example</summary>

Take the following struct:
```rust
#[derive(Reflect, FromReflect)]
struct Foo {
  #[reflect(default)]
  a: usize,
  #[reflect(default)]
  b: usize,
  #[reflect(default)]
  c: usize,
  #[reflect(default)]
  d: usize,
}
```

Since all the fields are default-able, we can successfully call
`FromReflect` on deserialized data like:

```rust
(
  "foo::Foo": (
    // Only set `b` and default the rest
    b: 123
  )
)
```

</details>

Unfortunately, this does not work with fields in enum variants. Marking
a variant field as `#[reflect(default)]` does nothing when calling
`FromReflect`.

## Solution

Allow enum variant fields to define a default value using
`#[reflect(default)]`.

### `#[reflect(Default)]`

One thing that structs and tuple structs can do is use their `Default`
implementation when calling `FromReflect`. Adding `#[reflect(Default)]`
to the struct or tuple struct both registers `ReflectDefault` and alters
the `FromReflect` implementation to use `Default` to generate any
missing fields.

This works well enough for structs and tuple structs, but for enums it's
not as simple. Since the `Default` implementation for an enum only
covers a single variant, it's not as intuitive as to what the behavior
will be. And (imo) it feels weird that we would be able to specify
default values in this way for one variant but not the others.

Because of this, I chose to not implement that behavior here. However,
I'm open to adding it in if anyone feels otherwise.

---

## Changelog

- Allow enum variant fields to define a default value using
`#[reflect(default)]`
  • Loading branch information
MrGVSV authored May 29, 2023
1 parent 27e1cf9 commit 6b292d4
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 22 deletions.
69 changes: 48 additions & 21 deletions crates/bevy_reflect/bevy_reflect_derive/src/enum_utility.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
use crate::fq_std::FQDefault;
use crate::derive_data::StructField;
use crate::field_attributes::DefaultBehavior;
use crate::fq_std::{FQDefault, FQOption};
use crate::{
derive_data::{EnumVariantFields, ReflectEnum},
utility::ident_or_index,
};
use proc_macro2::Ident;
use quote::{quote, ToTokens};
use syn::Member;

/// Contains all data needed to construct all variants within an enum.
pub(crate) struct EnumVariantConstructors {
Expand All @@ -30,7 +33,7 @@ pub(crate) fn get_variant_constructors(
let name = ident.to_string();
let variant_constructor = reflect_enum.get_unit(ident);

let fields = match &variant.fields {
let fields: &[StructField] = match &variant.fields {
EnumVariantFields::Unit => &[],
EnumVariantFields::Named(fields) | EnumVariantFields::Unnamed(fields) => {
fields.as_slice()
Expand All @@ -39,35 +42,59 @@ pub(crate) fn get_variant_constructors(
let mut reflect_index: usize = 0;
let constructor_fields = fields.iter().enumerate().map(|(declare_index, field)| {
let field_ident = ident_or_index(field.data.ident.as_ref(), declare_index);
let field_ty = &field.data.ty;

let field_value = if field.attrs.ignore.is_ignored() {
quote! { #FQDefault::default() }
match &field.attrs.default {
DefaultBehavior::Func(path) => quote! { #path() },
_ => quote! { #FQDefault::default() }
}
} else {
let error_repr = field.data.ident.as_ref().map_or_else(
|| format!("at index {reflect_index}"),
|name| format!("`{name}`"),
);
let unwrapper = if can_panic {
let type_err_message = format!(
"the field {error_repr} should be of type `{}`",
field.data.ty.to_token_stream()
);
quote!(.expect(#type_err_message))
let (resolve_error, resolve_missing) = if can_panic {
let field_ref_str = match &field_ident {
Member::Named(ident) => format!("the field `{ident}`"),
Member::Unnamed(index) => format!("the field at index {}", index.index)
};
let ty = field.data.ty.to_token_stream();

let on_error = format!("{field_ref_str} should be of type `{ty}`");
let on_missing = format!("{field_ref_str} is required but could not be found");

(quote!(.expect(#on_error)), quote!(.expect(#on_missing)))
} else {
quote!(?)
(quote!(?), quote!(?))
};

let field_accessor = match &field.data.ident {
Some(ident) => {
let name = ident.to_string();
quote!(.field(#name))
quote!(#ref_value.field(#name))
}
None => quote!(.field_at(#reflect_index)),
None => quote!(#ref_value.field_at(#reflect_index)),
};
reflect_index += 1;
let missing_field_err_message = format!("the field {error_repr} was not declared");
let accessor = quote!(#field_accessor .expect(#missing_field_err_message));
quote! {
#bevy_reflect_path::FromReflect::from_reflect(#ref_value #accessor)
#unwrapper

match &field.attrs.default {
DefaultBehavior::Func(path) => quote! {
if let #FQOption::Some(field) = #field_accessor {
<#field_ty as #bevy_reflect_path::FromReflect>::from_reflect(field)
#resolve_error
} else {
#path()
}
},
DefaultBehavior::Default => quote! {
if let #FQOption::Some(field) = #field_accessor {
<#field_ty as #bevy_reflect_path::FromReflect>::from_reflect(field)
#resolve_error
} else {
#FQDefault::default()
}
},
DefaultBehavior::Required => quote! {
<#field_ty as #bevy_reflect_path::FromReflect>::from_reflect(#field_accessor #resolve_missing)
#resolve_error
},
}
};
quote! { #field_ident : #field_value }
Expand Down
3 changes: 2 additions & 1 deletion crates/bevy_reflect/bevy_reflect_derive/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ pub(crate) static REFLECT_VALUE_ATTRIBUTE_NAME: &str = "reflect_value";
/// to improve performance and/or robustness.
/// An example of where this is used is in the [`FromReflect`] derive macro,
/// where adding this attribute will cause the `FromReflect` implementation to create
/// a base value using its [`Default`] implementation avoiding issues with ignored fields.
/// a base value using its [`Default`] implementation avoiding issues with ignored fields
/// (for structs and tuple structs only).
///
/// ## `#[reflect_value]`
///
Expand Down
33 changes: 33 additions & 0 deletions crates/bevy_reflect/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -751,6 +751,39 @@ mod tests {
assert_eq!(Some(expected), my_struct);
}

#[test]
fn from_reflect_should_use_default_variant_field_attributes() {
#[derive(Reflect, FromReflect, Eq, PartialEq, Debug)]
enum MyEnum {
Foo(#[reflect(default)] String),
Bar {
#[reflect(default = "get_baz_default")]
#[reflect(ignore)]
baz: usize,
},
}

fn get_baz_default() -> usize {
123
}

let expected = MyEnum::Foo(String::default());

let dyn_enum = DynamicEnum::new("Foo", DynamicTuple::default());
let my_enum = <MyEnum as FromReflect>::from_reflect(&dyn_enum);

assert_eq!(Some(expected), my_enum);

let expected = MyEnum::Bar {
baz: get_baz_default(),
};

let dyn_enum = DynamicEnum::new("Bar", DynamicStruct::default());
let my_enum = <MyEnum as FromReflect>::from_reflect(&dyn_enum);

assert_eq!(Some(expected), my_enum);
}

#[test]
fn from_reflect_should_use_default_container_attribute() {
#[derive(Reflect, FromReflect, Eq, PartialEq, Debug)]
Expand Down

0 comments on commit 6b292d4

Please sign in to comment.