Skip to content

Commit

Permalink
bevy_reflect_derive: Tidying up the code (bevyengine#4712)
Browse files Browse the repository at this point in the history
# Objective

The `bevy_reflect_derive` crate is not the cleanest or easiest to follow/maintain. The `lib.rs` file is especially difficult with over 1000 lines of code written in a confusing order. This is just a result of growth within the crate and it would be nice to clean it up for future work.

## Solution

Split `bevy_reflect_derive` into many more submodules. The submodules include:

* `container_attributes` - Code relating to container attributes
* `derive_data` - Code relating to reflection-based derive metadata
* `field_attributes` - Code relating to field attributes
* `impls` - Code containing actual reflection implementations
* `reflect_value` - Code relating to reflection-based value metadata
* `registration` - Code relating to type registration
* `utility` - General-purpose utility functions

This leaves the `lib.rs` file to contain only the public macros, making it much easier to digest (and fewer than 200 lines).

By breaking up the code into smaller modules, we make it easier for future contributors to find the code they're looking for or identify which module best fits their own additions.

### Metadata Structs

This cleanup also adds two big metadata structs: `ReflectFieldAttr` and `ReflectDeriveData`. The former is used to store all attributes for a struct field (if any). The latter is used to store all metadata for struct-based derive inputs.

Both significantly reduce code duplication and make editing these macros much simpler. The tradeoff is that we may collect more metadata than needed. However, this is usually a small thing (such as checking for attributes when they're not really needed or creating a `ReflectFieldAttr` for every field regardless of whether they actually have an attribute).

We could try to remove these tradeoffs and squeeze some more performance out, but doing so might come at the cost of developer experience. Personally, I think it's much nicer to create a `ReflectFieldAttr` for every field since it means I don't have to do two `Option` checks. Others may disagree, though, and so we can discuss changing this either in this PR or in a future one.

### Out of Scope

_Some_ documentation has been added or improved, but ultimately good docs are probably best saved for a dedicated PR.

## 🔍 Focus Points (for reviewers)

I know it's a lot to sift through, so here is a list of **key points for reviewers**:

- The following files contain code that was mostly just relocated:
  - `reflect_value.rs`
  - `registration.rs`
- `container_attributes.rs` was also mostly moved but features some general cleanup (reducing nesting, removing hardcoded strings, etc.) and lots of doc comments
- Most impl logic was moved from `lib.rs` to `impls.rs`, but they have been significantly modified to use the new `ReflectDeriveData` metadata struct in order to reduce duplication.
- `derive_data.rs` and `field_attributes.rs` contain almost entirely new code and should probably be given the most attention.
- Likewise, `from_reflect.rs` saw major changes using `ReflectDeriveData` so it should also be given focus.
- There was no change to the `lib.rs` exports so the end-user API should be the same.

## Prior Work

This task was initially tackled by @NathanSWard in bevyengine#2377 (which was closed in favor of this PR), so hats off to them for beating me to the punch by nearly a year!

---

## Changelog

* **[INTERNAL]** Split `bevy_reflect_derive` into smaller submodules
* **[INTERNAL]** Add `ReflectFieldAttr`
* **[INTERNAL]** Add `ReflectDeriveData`
* Add `BevyManifest::get_path_direct()` method (`bevy_macro_utils`)


Co-authored-by: MrGVSV <[email protected]>
  • Loading branch information
2 people authored and ItsDoot committed Feb 1, 2023
1 parent a15b3cf commit f610dca
Show file tree
Hide file tree
Showing 13 changed files with 1,257 additions and 1,118 deletions.
16 changes: 16 additions & 0 deletions crates/bevy_macro_utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,22 @@ impl BevyManifest {
deps.and_then(find_in_deps)
.or_else(|| deps_dev.and_then(find_in_deps))
}

/// Returns the path for the crate with the given name.
///
/// This is a convenience method for constructing a [manifest] and
/// calling the [`get_path`] method.
///
/// This method should only be used where you just need the path and can't
/// cache the [manifest]. If caching is possible, it's recommended to create
/// the [manifest] yourself and use the [`get_path`] method.
///
/// [`get_path`]: Self::get_path
/// [manifest]: Self
pub fn get_path_direct(name: &str) -> syn::Path {
Self::default().get_path(name)
}

pub fn get_path(&self, name: &str) -> syn::Path {
self.maybe_get_path(name)
.unwrap_or_else(|| Self::parse_str(name))
Expand Down
234 changes: 234 additions & 0 deletions crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,234 @@
//! Contains code related to container attributes for reflected types.
//!
//! A container attribute is an attribute which applies to an entire struct or enum
//! as opposed to a particular field or variant. An example of such an attribute is
//! the derive helper attribute for `Reflect`, which looks like:
//! `#[reflect(PartialEq, Default, ...)]` and `#[reflect_value(PartialEq, Default, ...)]`.

use crate::utility;
use proc_macro2::Ident;
use quote::quote;
use syn::parse::{Parse, ParseStream};
use syn::punctuated::Punctuated;
use syn::token::Comma;
use syn::{Meta, NestedMeta, Path};

// The "special" trait idents that are used internally for reflection.
// Received via attributes like `#[reflect(PartialEq, Hash, ...)]`
const PARTIAL_EQ_ATTR: &str = "PartialEq";
const HASH_ATTR: &str = "Hash";
const SERIALIZE_ATTR: &str = "Serialize";

/// A marker for trait implementations registered via the `Reflect` derive macro.
#[derive(Clone)]
pub(crate) enum TraitImpl {
/// The trait is not registered as implemented.
NotImplemented,
/// The trait is registered as implemented.
Implemented,

// TODO: This can be made to use `ExprPath` instead of `Ident`, allowing for fully qualified paths to be used
/// The trait is registered with a custom function rather than an actual implementation.
Custom(Ident),
}

impl Default for TraitImpl {
fn default() -> Self {
Self::NotImplemented
}
}

/// A collection of traits that have been registered for a reflected type.
///
/// This keeps track of a few traits that are utilized internally for reflection
/// (we'll call these traits _special traits_ within this context), but it
/// will also keep track of all registered traits. Traits are registered as part of the
/// `Reflect` derive macro using the helper attribute: `#[reflect(...)]`.
///
/// The list of special traits are as follows:
/// * `Hash`
/// * `PartialEq`
/// * `Serialize`
///
/// When registering a trait, there are a few things to keep in mind:
/// * Traits must have a valid `Reflect{}` struct in scope. For example, `Default`
/// needs `bevy_reflect::prelude::ReflectDefault` in scope.
/// * Traits must be single path identifiers. This means you _must_ use `Default`
/// instead of `std::default::Default` (otherwise it will try to register `Reflectstd`!)
/// * A custom function may be supplied in place of an actual implementation
/// for the special traits (but still follows the same single-path identifier
/// rules as normal).
///
/// # Example
///
/// Registering the `Default` implementation:
///
/// ```ignore
/// // Import ReflectDefault so it's accessible by the derive macro
/// use bevy_reflect::prelude::ReflectDefault.
///
/// #[derive(Reflect, Default)]
/// #[reflect(Default)]
/// struct Foo;
/// ```
///
/// Registering the `Hash` implementation:
///
/// ```ignore
/// // `Hash` is a "special trait" and does not need (nor have) a ReflectHash struct
///
/// #[derive(Reflect, Hash)]
/// #[reflect(Hash)]
/// struct Foo;
/// ```
///
/// Registering the `Hash` implementation using a custom function:
///
/// ```ignore
/// // This function acts as our `Hash` implementation and
/// // corresponds to the `Reflect::reflect_hash` method.
/// fn get_hash(foo: &Foo) -> Option<u64> {
/// Some(123)
/// }
///
/// #[derive(Reflect)]
/// // Register the custom `Hash` function
/// #[reflect(Hash(get_hash))]
/// struct Foo;
/// ```
///
/// > __Note:__ Registering a custom function only works for special traits.
///
#[derive(Default)]
pub(crate) struct ReflectTraits {
hash: TraitImpl,
partial_eq: TraitImpl,
serialize: TraitImpl,
idents: Vec<Ident>,
}

impl ReflectTraits {
/// Create a new [`ReflectTraits`] instance from a set of nested metas.
pub fn from_nested_metas(nested_metas: &Punctuated<NestedMeta, Comma>) -> Self {
let mut traits = ReflectTraits::default();
for nested_meta in nested_metas.iter() {
match nested_meta {
// Handles `#[reflect( Hash, Default, ... )]`
NestedMeta::Meta(Meta::Path(path)) => {
// Get the first ident in the path (hopefully the path only contains one and not `std::hash::Hash`)
let ident = if let Some(segment) = path.segments.iter().next() {
segment.ident.to_string()
} else {
continue;
};

match ident.as_str() {
PARTIAL_EQ_ATTR => traits.partial_eq = TraitImpl::Implemented,
HASH_ATTR => traits.hash = TraitImpl::Implemented,
SERIALIZE_ATTR => traits.serialize = TraitImpl::Implemented,
// We only track reflected idents for traits not considered special
_ => traits.idents.push(utility::get_reflect_ident(&ident)),
}
}
// Handles `#[reflect( Hash(custom_hash_fn) )]`
NestedMeta::Meta(Meta::List(list)) => {
// Get the first ident in the path (hopefully the path only contains one and not `std::hash::Hash`)
let ident = if let Some(segment) = list.path.segments.iter().next() {
segment.ident.to_string()
} else {
continue;
};

let list_meta = list.nested.iter().next();
if let Some(NestedMeta::Meta(Meta::Path(path))) = list_meta {
if let Some(segment) = path.segments.iter().next() {
// This should be the ident of the custom function
let trait_func_ident = TraitImpl::Custom(segment.ident.clone());
match ident.as_str() {
PARTIAL_EQ_ATTR => traits.partial_eq = trait_func_ident,
HASH_ATTR => traits.hash = trait_func_ident,
SERIALIZE_ATTR => traits.serialize = trait_func_ident,
_ => {}
}
}
}
}
_ => {}
}
}

traits
}

/// Returns true if the given reflected trait name (i.e. `ReflectDefault` for `Default`)
/// is registered for this type.
pub fn contains(&self, name: &str) -> bool {
self.idents.iter().any(|ident| ident == name)
}

/// The list of reflected traits by their reflected ident (i.e. `ReflectDefault` for `Default`).
pub fn idents(&self) -> &[Ident] {
&self.idents
}

/// Returns the logic for `Reflect::reflect_hash` as a `TokenStream`.
///
/// If `Hash` was not registered, returns `None`.
pub fn get_hash_impl(&self, path: &Path) -> Option<proc_macro2::TokenStream> {
match &self.hash {
TraitImpl::Implemented => Some(quote! {
use std::hash::{Hash, Hasher};
let mut hasher = #path::ReflectHasher::default();
Hash::hash(&std::any::Any::type_id(self), &mut hasher);
Hash::hash(self, &mut hasher);
Some(hasher.finish())
}),
TraitImpl::Custom(impl_fn) => Some(quote! {
Some(#impl_fn(self))
}),
TraitImpl::NotImplemented => None,
}
}

/// Returns the logic for `Reflect::reflect_partial_eq` as a `TokenStream`.
///
/// If `PartialEq` was not registered, returns `None`.
pub fn get_partial_eq_impl(&self) -> Option<proc_macro2::TokenStream> {
match &self.partial_eq {
TraitImpl::Implemented => Some(quote! {
let value = value.any();
if let Some(value) = value.downcast_ref::<Self>() {
Some(std::cmp::PartialEq::eq(self, value))
} else {
Some(false)
}
}),
TraitImpl::Custom(impl_fn) => Some(quote! {
Some(#impl_fn(self, value))
}),
TraitImpl::NotImplemented => None,
}
}

/// Returns the logic for `Reflect::serializable` as a `TokenStream`.
///
/// If `Serialize` was not registered, returns `None`.
pub fn get_serialize_impl(&self, path: &Path) -> Option<proc_macro2::TokenStream> {
match &self.serialize {
TraitImpl::Implemented => Some(quote! {
Some(#path::serde::Serializable::Borrowed(self))
}),
TraitImpl::Custom(impl_fn) => Some(quote! {
Some(#impl_fn(self))
}),
TraitImpl::NotImplemented => None,
}
}
}

impl Parse for ReflectTraits {
fn parse(input: ParseStream) -> syn::Result<Self> {
let result = Punctuated::<NestedMeta, Comma>::parse_terminated(input)?;
Ok(ReflectTraits::from_nested_metas(&result))
}
}
Loading

0 comments on commit f610dca

Please sign in to comment.